From 5d5694934a6abcc285024b12632f6e0f8bd74f43 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 28 Mar 2024 21:59:02 +0100 Subject: [PATCH v27 4/5] 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 | 66 +++++++++++++++-------- 2 files changed, 57 insertions(+), 31 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 d9c9fc6cf9..0e52218422 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 * @@ -1334,7 +1336,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; @@ -1356,9 +1363,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; } @@ -1675,7 +1692,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); @@ -1817,32 +1839,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; } /* -- 2.34.1