v29-0004-Review-comments-2024-09-11.patch

From 37259ae39f6ee4ddad2373bc729ee3560db42a85 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 11 Sep 2024 15:21:40 +0200
Subject: [PATCH v29 4/4] Review comments 2024-09-11

---
 configure                                     |  5 +
 configure.ac                                  |  4 +
 doc/src/sgml/oauth-validators.sgml            | 91 ++++++++++++++++++-
 src/backend/libpq/auth-oauth.c                |  3 +-
 src/interfaces/libpq/fe-auth-oauth-curl.c     | 81 +++++++++--------
 src/test/modules/oauth_validator/Makefile     | 18 +++-
 .../oauth_validator/expected/validator.out    |  6 --
 .../modules/oauth_validator/sql/validator.sql |  1 -
 src/test/modules/oauth_validator/validator.c  |  6 +-
 src/test/perl/PostgreSQL/Test/OAuthServer.pm  | 10 +-
 10 files changed, 163 insertions(+), 62 deletions(-)
 delete mode 100644 src/test/modules/oauth_validator/expected/validator.out
 delete mode 100644 src/test/modules/oauth_validator/sql/validator.sql

diff --git a/configure b/configure
index bf95091074..8bfc3c2215 100755
--- a/configure
+++ b/configure
@@ -8533,6 +8533,11 @@ $as_echo "#define USE_OAUTH 1" >>confdefs.h
 
 $as_echo "#define USE_OAUTH_CURL 1" >>confdefs.h
 
+  # OAuth requires python for testing
+  if test "$with_python" != yes; then
+    { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: *** OAuth support tests requires --with-python to run" >&5
+$as_echo "$as_me: WARNING: *** OAuth support tests requires --with-python to run" >&2;}
+  fi
 elif test x"$with_oauth" != x"no"; then
   as_fn_error $? "--with-oauth must specify curl" "$LINENO" 5
 fi
diff --git a/configure.ac b/configure.ac
index 0faa566579..7e3a74527a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -932,6 +932,10 @@ fi
 if test x"$with_oauth" = x"curl"; then
   AC_DEFINE([USE_OAUTH], 1, [Define to 1 to build with OAuth 2.0 support. (--with-oauth)])
   AC_DEFINE([USE_OAUTH_CURL], 1, [Define to 1 to use libcurl for OAuth support.])
+  # OAuth requires python for testing
+  if test "$with_python" != yes; then
+    AC_MSG_WARN([*** OAuth support tests requires --with-python to run])
+  fi
 elif test x"$with_oauth" != x"no"; then
   AC_MSG_ERROR([--with-oauth must specify curl])
 fi
diff --git a/doc/src/sgml/oauth-validators.sgml b/doc/src/sgml/oauth-validators.sgml
index 3c7884baf9..75cd9fc557 100644
--- a/doc/src/sgml/oauth-validators.sgml
+++ b/doc/src/sgml/oauth-validators.sgml
@@ -2,8 +2,95 @@
 
 <chapter id="oauth-validators">
  <title>Implementing OAuth Validator Modules</title>
-
+ <indexterm zone="oauth-validators">
+  <primary>OAuth Validators</primary>
+ </indexterm>
+ <para>
+  <productname>PostgreSQL</productname> provides infrastructure for creating
+  custom modules to perform server-side validation of OAuth tokens.
+ </para>
  <para>
-  TODO
+  OAuth validation modules must at least consist of an initialization function
+  (see <xref linkend="oauth-validator-init"/>) and the required callback for
+  performing validation (see <xref linkend="oauth-validator-callback-validate"/>).
  </para>
+
+ <sect1 id="oauth-validator-init">
+  <title>Initialization Functions</title>
+  <indexterm zone="oauth-validator-init">
+   <primary>_PG_oauth_validator_module_init</primary>
+  </indexterm>
+  <para>
+   An OAuth validator module is loaded by dynamically loading a shared library
+   with the <xref linkend="guc-oauth-validator_library"/>'s name as the library
+   base name. The normal library search path is used to locate the library. To
+   provide the validator callbacks and to indicate that the library is an OAuth
+   validator module a function named
+   <function>_PG_oauth_validator_module_init</function> must be provided. The
+   return value of the function must be a pointer to a struct of type
+   <structname>OAuthValidatorCallbacks</structname> which contains all that
+   libpq need to perform token validation using the module. The returned
+   pointer must be of server lifetime, which is typically achieved by defining
+   it as a <literal>static const</literal> variable in global scope.
+<programlisting>
+typedef struct OAuthValidatorCallbacks
+{
+    ValidatorStartupCB startup_cb;
+    ValidatorShutdownCB shutdown_cb;
+    ValidatorValidateCB validate_cb;
+} OAuthValidatorCallbacks;
+
+typedef const OAuthValidatorCallbacks *(*OAuthValidatorModuleInit) (void);
+</programlisting>
+
+   Only the <function>validate_cb</function> callback is required, the others
+   are optional.
+  </para>
+ </sect1>
+
+ <sect1 id="oauth-validator-callbacks">
+  <title>OAuth Validator Callbacks</title>
+  <para>
+   OAuth validator modules implement their functionality by defining a set of
+   callbacks, libpq will call them as required to process the authentication
+   request from the user.
+  </para>
+
+  <sect2 id="oauth-validator-callback-startup">
+   <title>Startup Callback</title>
+   <para>
+    The <function>startup_cb</function> callback is executed directly after
+    loading the module. This callback can be used to set up local state and
+    perform additional initialization if required. If the validator module
+    has state it can use <structfield>state->private_data</structfield> to
+    store it.
+
+<programlisting>
+typedef void (*ValidatorStartupCB) (ValidatorModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
+  <sect2 id="oauth-validator-callback-validate">
+   <title>Validate Callback</title>
+   <para>
+<programlisting>
+typedef ValidatorModuleResult *(*ValidatorValidateCB) (ValidatorModuleState *state, const char *token, const char *role);
+</programlisting>
+   </para>
+  </sect2>
+
+  <sect2 id="oauth-validator-callback-shutdown">
+   <title>Shutdown Callback</title>
+   <para>
+    The <function>shutdown_cb</function> callback is executed when the backend
+    process associated with the connection exits. If the validator module has
+    any state, this callback should free it to avoid resource leaks.
+<programlisting>
+typedef void (*ValidatorShutdownCB) (ValidatorModuleState *state);
+</programlisting>
+   </para>
+  </sect2>
+
+ </sect1>
 </chapter>
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index ec1418c3fc..f2f8fe81e2 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -442,7 +442,8 @@ parse_kvpairs_for_auth(char **input)
 			 errmsg("malformed OAUTHBEARER message"),
 			 errdetail("Message did not contain a final terminator.")));
 
-	return NULL;				/* unreachable */
+	pg_unreachable();
+	return NULL;
 }
 
 static void
diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c
index 0e52218422..c79329e98a 100644
--- a/src/interfaces/libpq/fe-auth-oauth-curl.c
+++ b/src/interfaces/libpq/fe-auth-oauth-curl.c
@@ -622,8 +622,8 @@ check_content_type(struct async_ctx *actx, const char *type)
 	}
 
 	/*
-	 * We need to perform a length limited comparison and not compare the whole
-	 * string.
+	 * We need to perform a length limited comparison and not compare the
+	 * whole string.
 	 */
 	if (pg_strncasecmp(content_type, type, type_len) != 0)
 		goto fail;
@@ -645,7 +645,7 @@ check_content_type(struct async_ctx *actx, const char *type)
 			case ';':
 				return true;	/* success! */
 
-			/* HTTP optional whitespace allows only spaces and htabs. */
+				/* HTTP optional whitespace allows only spaces and htabs. */
 			case ' ':
 			case '\t':
 				break;
@@ -817,8 +817,8 @@ parse_device_authz(struct async_ctx *actx, struct device_authz *authz)
 		{"verification_uri", JSON_TOKEN_STRING, {&authz->verification_uri}, REQUIRED},
 
 		/*
-		 * Some services (Google, Azure) spell verification_uri differently. We
-		 * accept either.
+		 * Some services (Google, Azure) spell verification_uri differently.
+		 * We accept either.
 		 */
 		{"verification_url", JSON_TOKEN_STRING, {&authz->verification_uri}, REQUIRED},
 
@@ -1165,11 +1165,11 @@ register_timer(CURLM *curlm, long timeout, void *ctx)
 	struct async_ctx *actx = ctx;
 
 	/*
-	 * TODO: maybe just signal drive_request() to immediately call back in
-	 * the (timeout == 0) case?
+	 * TODO: maybe just signal drive_request() to immediately call back in the
+	 * (timeout == 0) case?
 	 */
 	if (!set_timer(actx, timeout))
-		return -1; /* actx_error already called */
+		return -1;				/* actx_error already called */
 
 	return 0;
 }
@@ -1184,7 +1184,7 @@ static int
 debug_callback(CURL *handle, curl_infotype type, char *data, size_t size,
 			   void *clientp)
 {
-	const char * const end = data + size;
+	const char *const end = data + size;
 	const char *prefix;
 
 	/* Prefixes are modeled off of the default libcurl debug output. */
@@ -1194,12 +1194,12 @@ debug_callback(CURL *handle, curl_infotype type, char *data, size_t size,
 			prefix = "*";
 			break;
 
-		case CURLINFO_HEADER_IN: /* fall through */
+		case CURLINFO_HEADER_IN:	/* fall through */
 		case CURLINFO_DATA_IN:
 			prefix = "<";
 			break;
 
-		case CURLINFO_HEADER_OUT: /* fall through */
+		case CURLINFO_HEADER_OUT:	/* fall through */
 		case CURLINFO_DATA_OUT:
 			prefix = ">";
 			break;
@@ -1296,8 +1296,8 @@ setup_curl_handles(struct async_ctx *actx)
 	{
 		/*
 		 * Set a callback for retrieving error information from libcurl, the
-		 * function only takes effect when CURLOPT_VERBOSE has been set so make
-		 * sure the order is kept.
+		 * function only takes effect when CURLOPT_VERBOSE has been set so
+		 * make sure the order is kept.
 		 */
 		CHECK_SETOPT(actx, CURLOPT_DEBUGFUNCTION, debug_callback, return false);
 		CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false);
@@ -1309,17 +1309,17 @@ setup_curl_handles(struct async_ctx *actx)
 	 * Only HTTPS is allowed. (Debug mode additionally allows HTTP; this is
 	 * intended for testing only.)
 	 *
-	 * There's a bit of unfortunate complexity around the choice of CURLoption.
-	 * CURLOPT_PROTOCOLS is deprecated in modern Curls, but its replacement
-	 * didn't show up until relatively recently.
+	 * There's a bit of unfortunate complexity around the choice of
+	 * CURLoption. CURLOPT_PROTOCOLS is deprecated in modern Curls, but its
+	 * replacement didn't show up until relatively recently.
 	 */
 	{
 #if CURL_AT_LEAST_VERSION(7, 85, 0)
-		const CURLoption popt = CURLOPT_PROTOCOLS_STR;
+		const		CURLoption popt = CURLOPT_PROTOCOLS_STR;
 		const char *protos = "https";
-		const char * const unsafe = "https,http";
+		const char *const unsafe = "https,http";
 #else
-		const CURLoption popt = CURLOPT_PROTOCOLS;
+		const		CURLoption popt = CURLOPT_PROTOCOLS;
 		long		protos = CURLPROTO_HTTPS;
 		const long	unsafe = CURLPROTO_HTTPS | CURLPROTO_HTTP;
 #endif
@@ -1408,8 +1408,8 @@ start_request(struct async_ctx *actx)
 	}
 
 	/*
-	 * actx->running tracks the number of running handles, so we can immediately
-	 * call back if no waiting is needed.
+	 * actx->running tracks the number of running handles, so we can
+	 * immediately call back if no waiting is needed.
 	 *
 	 * Even though this is nominally an asynchronous process, there are some
 	 * operations that can synchronously fail by this point (e.g. connections
@@ -1452,23 +1452,23 @@ drive_request(struct async_ctx *actx)
 		/*
 		 * There's an async request in progress. Pump the multi handle.
 		 *
-		 * TODO: curl_multi_socket_all() is deprecated, presumably because it's
-		 * inefficient and pointless if your event loop has already handed you
-		 * the exact sockets that are ready. But that's not our use case --
-		 * our client has no way to tell us which sockets are ready. (They don't
-		 * even know there are sockets to begin with.)
+		 * TODO: curl_multi_socket_all() is deprecated, presumably because
+		 * it's inefficient and pointless if your event loop has already
+		 * handed you the exact sockets that are ready. But that's not our use
+		 * case -- our client has no way to tell us which sockets are ready.
+		 * (They don't even know there are sockets to begin with.)
 		 *
 		 * We can grab the list of triggered events from the multiplexer
 		 * ourselves, but that's effectively what curl_multi_socket_all() is
 		 * going to do... so it appears to be exactly the API we need.
 		 *
 		 * Ignore the deprecation for now. This needs a followup on
-		 * curl-library@, to make sure we're not shooting ourselves in the foot
-		 * in some other way.
+		 * curl-library@, to make sure we're not shooting ourselves in the
+		 * foot in some other way.
 		 */
 		CURL_IGNORE_DEPRECATION(
-			err = curl_multi_socket_all(actx->curlm, &actx->running);
-		)
+								err = curl_multi_socket_all(actx->curlm, &actx->running);
+			)
 
 		if (err)
 		{
@@ -1915,8 +1915,8 @@ handle_token_response(struct async_ctx *actx, char **token)
 	}
 
 	/*
-	 * authorization_pending and slow_down are the only
-	 * acceptable errors; anything else and we bail.
+	 * authorization_pending and slow_down are the only acceptable errors;
+	 * anything else and we bail.
 	 */
 	err = &tok.err;
 	if (strcmp(err->error, "authorization_pending") != 0 &&
@@ -1930,8 +1930,8 @@ handle_token_response(struct async_ctx *actx, char **token)
 	}
 
 	/*
-	 * A slow_down error requires us to permanently increase
-	 * our retry interval by five seconds. RFC 8628, Sec. 3.5.
+	 * A slow_down error requires us to permanently increase our retry
+	 * interval by five seconds. RFC 8628, Sec. 3.5.
 	 */
 	if (strcmp(err->error, "slow_down") == 0)
 	{
@@ -2102,8 +2102,8 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 
 		/*
 		 * Each case here must ensure that actx->running is set while we're
-		 * waiting on some asynchronous work. Most cases rely on start_request()
-		 * to do that for them.
+		 * waiting on some asynchronous work. Most cases rely on
+		 * start_request() to do that for them.
 		 */
 		switch (actx->step)
 		{
@@ -2160,7 +2160,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 				}
 
 				if (state->token)
-					break; /* done! */
+					break;		/* done! */
 
 				/*
 				 * Wait for the required interval before issuing the next
@@ -2170,10 +2170,11 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
 					goto error_return;
 
 #ifdef HAVE_SYS_EPOLL_H
+
 				/*
-				 * No Curl requests are running, so we can simplify by
-				 * having the client wait directly on the timerfd rather
-				 * than the multiplexer. (This isn't possible for kqueue.)
+				 * No Curl requests are running, so we can simplify by having
+				 * the client wait directly on the timerfd rather than the
+				 * multiplexer. (This isn't possible for kqueue.)
 				 */
 				*altsock = actx->timerfd;
 #endif
diff --git a/src/test/modules/oauth_validator/Makefile b/src/test/modules/oauth_validator/Makefile
index 655ce75796..f5028f2e52 100644
--- a/src/test/modules/oauth_validator/Makefile
+++ b/src/test/modules/oauth_validator/Makefile
@@ -1,5 +1,13 @@
-export PYTHON
-export with_oauth
+#-------------------------------------------------------------------------
+#
+# Makefile for src/test/modules/oauth_validator
+#
+# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/modules/oauth_validator/Makefile
+#
+#-------------------------------------------------------------------------
 
 MODULES = validator
 PGFILEDESC = "validator - test OAuth validator module"
@@ -8,8 +16,6 @@ NO_INSTALLCHECK = 1
 
 TAP_TESTS = 1
 
-REGRESS = validator
-
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -19,4 +25,8 @@ subdir = src/test/modules/oauth_validator
 top_builddir = ../../../..
 include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
+
+export PYTHON
+export with_oauth
+
 endif
diff --git a/src/test/modules/oauth_validator/expected/validator.out b/src/test/modules/oauth_validator/expected/validator.out
deleted file mode 100644
index 360caa2cb3..0000000000
--- a/src/test/modules/oauth_validator/expected/validator.out
+++ /dev/null
@@ -1,6 +0,0 @@
-SELECT 1;
- ?column? 
-----------
-        1
-(1 row)
-
diff --git a/src/test/modules/oauth_validator/sql/validator.sql b/src/test/modules/oauth_validator/sql/validator.sql
deleted file mode 100644
index e0ac49d1ec..0000000000
--- a/src/test/modules/oauth_validator/sql/validator.sql
+++ /dev/null
@@ -1 +0,0 @@
-SELECT 1;
diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c
index 7b4dc9c494..c41dd07132 100644
--- a/src/test/modules/oauth_validator/validator.c
+++ b/src/test/modules/oauth_validator/validator.c
@@ -22,9 +22,9 @@ PG_MODULE_MAGIC;
 
 static void validator_startup(ValidatorModuleState *state);
 static void validator_shutdown(ValidatorModuleState *state);
-static ValidatorModuleResult * validate_token(ValidatorModuleState *state,
-											  const char *token,
-											  const char *role);
+static ValidatorModuleResult *validate_token(ValidatorModuleState *state,
+											 const char *token,
+											 const char *role);
 
 static const OAuthValidatorCallbacks validator_callbacks = {
 	.startup_cb = validator_startup,
diff --git a/src/test/perl/PostgreSQL/Test/OAuthServer.pm b/src/test/perl/PostgreSQL/Test/OAuthServer.pm
index abdff5a3c3..7bf4e4a03c 100644
--- a/src/test/perl/PostgreSQL/Test/OAuthServer.pm
+++ b/src/test/perl/PostgreSQL/Test/OAuthServer.pm
@@ -34,25 +34,25 @@ sub run
 	my $port;
 
 	my $pid = open(my $read_fh, "-|", $ENV{PYTHON}, "t/oauth_server.py")
-		or die "failed to start OAuth server: $!";
+	  or die "failed to start OAuth server: $!";
 
-	read($read_fh, $port, 7) // die "failed to read port number: $!";
+	read($read_fh, $port, 7) or die "failed to read port number: $!";
 	chomp $port;
 	die "server did not advertise a valid port"
-		unless Scalar::Util::looks_like_number($port);
+	  unless Scalar::Util::looks_like_number($port);
 
 	$self->{'pid'} = $pid;
 	$self->{'port'} = $port;
 	$self->{'child'} = $read_fh;
 
-	diag("OAuth provider (PID $pid) is listening on port $port\n");
+	note("OAuth provider (PID $pid) is listening on port $port\n");
 }
 
 sub stop
 {
 	my $self = shift;
 
-	diag("Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n");
+	note("Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n");
 
 	kill(15, $self->{'pid'});
 	$self->{'pid'} = undef;
-- 
2.39.3 (Apple Git-146)