From d163d2ca0a7581fcde16c93e1b87143d956e3e4f Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 28 Mar 2024 21:59:02 +0100 Subject: [PATCH v24 6/8] Review comments Fixes and tidy-ups following a review of v21, a few items are (listed in no specific order): * Implement a version check for libcurl in autoconf, the equivalent check for Meson is still a TODO. [ed: moved to an earlier commit] * Address a few TODOs in the code * libpq JSON support memory management fixups [ed: moved to an earlier commit] --- src/backend/libpq/auth-oauth.c | 22 ++-- src/interfaces/libpq/fe-auth-oauth-curl.c | 117 ++++++++++++++-------- src/interfaces/libpq/fe-auth-oauth.c | 7 +- 3 files changed, 92 insertions(+), 54 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 2a0d74a079..ec1418c3fc 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -533,7 +533,9 @@ validate_token_format(const char *header) if (!header || strlen(header) <= 7) { ereport(COMMERROR, - (errmsg("malformed OAuth bearer token 1"))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is less than 8 bytes.")); return NULL; } @@ -551,9 +553,9 @@ validate_token_format(const char *header) if (!*token) { ereport(COMMERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("malformed OAuth bearer token 2"), - errdetail("Bearer token is empty."))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is empty.")); return NULL; } @@ -573,9 +575,9 @@ validate_token_format(const char *header) * of someone's password into the logs. */ ereport(COMMERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("malformed OAuth bearer token 3"), - errdetail("Bearer token is not in the correct format."))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is not in the correct format.")); return NULL; } @@ -617,10 +619,10 @@ validate(Port *port, const char *auth) /* Make sure the validator authenticated the user. */ if (ret->authn_id == NULL || ret->authn_id[0] == '\0') { - /* TODO: use logdetail; reduce message duplication */ ereport(LOG, - (errmsg("OAuth bearer authentication failed for user \"%s\": validator provided no identity", - port->user_name))); + errmsg("OAuth bearer authentication failed for user \"%s\"", + port->user_name), + errdetail_log("Validator provided no identity")); return false; } diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c index c9866c222a..944f450fec 100644 --- a/src/interfaces/libpq/fe-auth-oauth-curl.c +++ b/src/interfaces/libpq/fe-auth-oauth-curl.c @@ -31,6 +31,8 @@ #include "libpq-int.h" #include "mb/pg_wchar.h" +#define MAX_OAUTH_RESPONSE_SIZE (1024 * 1024) + /* * Parsed JSON Representations * @@ -207,6 +209,8 @@ struct async_ctx struct device_authz authz; bool user_prompted; /* have we already sent the authz prompt? */ + + int running; }; /* @@ -681,7 +685,11 @@ parse_oauth_json(struct async_ctx *actx, const struct json_field *fields) return false; } - makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true); + if (!makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true)) + { + actx_error(actx, "out of memory"); + return false; + } ctx.errbuf = &actx->errbuf; ctx.fields = fields; @@ -1225,7 +1233,12 @@ setup_curl_handles(struct async_ctx *actx) * pretty strict when it comes to provider behavior, so we have to check * what comes back anyway.) */ - actx->headers = curl_slist_append(actx->headers, "Accept:"); /* TODO: check result */ + actx->headers = curl_slist_append(actx->headers, "Accept:"); + if (actx->headers == NULL) + { + actx_error(actx, "out of memory"); + return false; + } CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers, return false); return true; @@ -1247,9 +1260,19 @@ append_data(char *buf, size_t size, size_t nmemb, void *userdata) PQExpBuffer resp = userdata; size_t len = size * nmemb; - /* TODO: cap the maximum size */ + /* In case we receive data over the threshold, abort the transfer */ + if ((resp->len + len) > MAX_OAUTH_RESPONSE_SIZE) + return 0; + + /* The data passed from libcurl is not null-terminated */ appendBinaryPQExpBuffer(resp, buf, len); - /* TODO: check for broken buffer */ + + /* + * Signal an error in order to abort the transfer in case we ran out of + * memory in accepting the data. + */ + if (PQExpBufferBroken(resp)) + return 0; return len; } @@ -1266,7 +1289,6 @@ static bool start_request(struct async_ctx *actx) { CURLMcode err; - int running; resetPQExpBuffer(&actx->work_data); CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false); @@ -1280,7 +1302,7 @@ start_request(struct async_ctx *actx) return false; } - err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &running); + err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &actx->running); if (err) { actx_error(actx, "asynchronous HTTP request failed: %s", @@ -1289,19 +1311,11 @@ start_request(struct async_ctx *actx) } /* - * Sanity check. - * - * TODO: even though this is nominally an asynchronous process, there are - * apparently operations that can synchronously fail by this point, such - * as connections to closed local ports. Maybe we need to let this case - * fall through to drive_request instead, or else perform a - * curl_multi_info_read immediately. + * Even though this is nominally an asynchronous process, there are some + * operations that can synchronously fail by this point like connections + * to closed local ports. Fall through and leave the sanity check for the + * next state consuming actx. */ - if (running != 1) - { - actx_error(actx, "failed to queue HTTP request"); - return false; - } return true; } @@ -1314,12 +1328,18 @@ static PostgresPollingStatusType drive_request(struct async_ctx *actx) { CURLMcode err; - int running; CURLMsg *msg; int msgs_left; bool done; - err = curl_multi_socket_all(actx->curlm, &running); + /* Sanity check the previous operation */ + if (actx->running != 1) + { + actx_error(actx, "failed to queue HTTP request"); + return false; + } + + err = curl_multi_socket_all(actx->curlm, &actx->running); if (err) { actx_error(actx, "asynchronous HTTP request failed: %s", @@ -1327,7 +1347,7 @@ drive_request(struct async_ctx *actx) return PGRES_POLLING_FAILED; } - if (running) + if (actx->running) { /* We'll come back again. */ return PGRES_POLLING_READING; @@ -1541,7 +1561,12 @@ start_device_authz(struct async_ctx *actx, PGconn *conn) appendPQExpBuffer(work_buffer, "client_id=%s", conn->oauth_client_id); if (conn->oauth_scope) appendPQExpBuffer(work_buffer, "&scope=%s", conn->oauth_scope); - /* TODO check for broken buffer */ + + if (PQExpBufferBroken(work_buffer)) + { + actx_error(actx, "out of memory"); + return false; + } /* Make our request. */ CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri, return false); @@ -1683,32 +1708,34 @@ finish_token_request(struct async_ctx *actx, struct token *tok) CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false); /* - * Per RFC 6749, Section 5, a successful response uses 200 OK. An error - * response uses either 400 Bad Request or 401 Unauthorized. - * - * TODO: there are references online to 403 appearing in the wild... + * Per RFC 6749, Section 5, a successful response uses 200 OK. */ - if (response_code != 200 - && response_code != 400 - /* && response_code != 401 TODO */ ) + if (response_code == 200) { - actx_error(actx, "unexpected response code %ld", response_code); - return false; + actx->errctx = "failed to parse access token response"; + if (!parse_access_token(actx, tok)) + return false; /* error message already set */ + + return true; } /* - * Pull the fields we care about from the document. + * An error response uses either 400 Bad Request or 401 Unauthorized. + * There are references online to implementations using 403 for error + * return which would violate the specification. For now we stick to the + * specification but we might have to revisit this. */ - if (response_code == 200) + if (response_code == 400 || response_code == 401) { - actx->errctx = "failed to parse access token response"; - if (!parse_access_token(actx, tok)) - return false; /* error message already set */ + if (!parse_token_error(actx, &tok->err)) + return false; + + return true; } - else if (!parse_token_error(actx, &tok->err)) - return false; - return true; + /* Any other response codes are considered invalid */ + actx_error(actx, "unexpected response code %ld", response_code); + return false; } @@ -1926,16 +1953,20 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) * errors; anything else and we bail. */ err = &tok.err; - if (!err->error || (strcmp(err->error, "authorization_pending") - && strcmp(err->error, "slow_down"))) + if (!err->error) + { + actx_error(actx, "unknown error"); + goto error_return; + } + + if (strcmp(err->error, "authorization_pending") != 0 && + strcmp(err->error, "slow_down") != 0) { - /* TODO handle !err->error */ if (err->error_description) appendPQExpBuffer(&actx->errbuf, "%s ", err->error_description); appendPQExpBuffer(&actx->errbuf, "(%s)", err->error); - goto error_return; } diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index f943a31cc0..61de9ac451 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -247,7 +247,12 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen) return false; } - makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true); + if (!makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true)) + { + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("out of memory")); + return false; + } initPQExpBuffer(&ctx.errbuf); sem.semstate = &ctx; -- 2.34.1