v24-0006-Review-comments.patch

From d163d2ca0a7581fcde16c93e1b87143d956e3e4f Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
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