Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Jacob Champion <[email-masked]>
Date: Mon, 20 Jan 2025 16:40:36 -0800

On Mon, Jan 20, 2025 at 2:10 PM Daniel Gustafsson <[email-masked]> wrote:
> +  /*
> +   * The mechanism should have set up the necessary callbacks; all we
> +   * need to do is signal the caller.
> +   */
> +  *async = true;
> +  return STATUS_OK;
> Is it worth adding assertions here to ensure that everything has been set up
> properly to help when adding a new mechanism in the future?

Yeah, I think that'd be helpful.

> +   /* Done. Tear down the async implementation. */
> +   conn->cleanup_async_auth(conn);
> +   conn->cleanup_async_auth = NULL;
> +   Assert(conn->altsock == PGINVALID_SOCKET);
> In pqDropConnection() we set ->altsock to NULL

(I assume you mean PGINVALID_SOCKET?)

> just to be sure rather than
> assert that cleanup has done so.  Shouldn't we be consistent in the
> expectation and set to NULL here as well?

I'm not opposed; I just figured that the following code might be a bit
confusing:

    Assert(conn->altsock == PGINVALID_SOCKET);
    conn->altsock = PGINVALID_SOCKET;

But I can add a comment to the assignment to try to explain. I don't
know what the likelihood of landing code that trips that assertion is,
but an explicit assignment would at least stop problems from
cascading.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 20 Jan 2025 16:43:41 -0800

> but an explicit assignment would at least stop problems from

On second thought, I can just fail the connection if this happens.

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 21 Jan 2025 10:29:22 +0100

> On 21 Jan 2025, at 01:40, Jacob Champion <[email-masked]> wrote:

> (I assume you mean PGINVALID_SOCKET?)

Doh, yes.

> but an explicit assignment would at least stop problems from

It is weird, but stopping the escalation of a problem seems important.

> On 21 Jan 2025, at 01:43, Jacob Champion <[email-masked]> wrote:
> 

> On second thought, I can just fail the connection if this happens.

Yeah, I think that's the best option here.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Tue, 21 Jan 2025 08:46:43 -0800

> Yeah, I think that's the best option here.

Done that way in v43.

--Jacob
1:  1f38ec8039b = 1:  5d474397364 Move PG_MAX_AUTH_TOKEN_LENGTH to libpq/auth.h
2:  9f87ffea1c7 = 2:  20452d21e0b require_auth: prepare for multiple SASL mechanisms
3:  bda684d19cc ! 3:  f0afefb80d6 libpq: handle asynchronous actions during SASL
    @@ src/interfaces/libpq/fe-auth.c: pg_SASL_init(PGconn *conn, int payloadlen)
     +		/*
     +		 * The mechanism should have set up the necessary callbacks; all we
     +		 * need to do is signal the caller.
    ++		 *
    ++		 * In non-assertion builds, this postcondition is enforced at time of
    ++		 * use in PQconnectPoll().
     +		 */
    ++		Assert(conn->async_auth);
    ++		Assert(conn->cleanup_async_auth);
    ++
     +		*async = true;
     +		return STATUS_OK;
     +	}
    @@ src/interfaces/libpq/fe-connect.c: keep_going:						/* We will come back to here
     +				if (!conn->async_auth || !conn->cleanup_async_auth)
     +				{
     +					/* programmer error; should not happen */
    -+					libpq_append_conn_error(conn, "async authentication has no handler");
    ++					libpq_append_conn_error(conn,
    ++											"internal error: async authentication has no handler");
     +					goto error_return;
     +				}
     +
    @@ src/interfaces/libpq/fe-connect.c: keep_going:						/* We will come back to here
     +					/* Done. Tear down the async implementation. */
     +					conn->cleanup_async_auth(conn);
     +					conn->cleanup_async_auth = NULL;
    -+					Assert(conn->altsock == PGINVALID_SOCKET);
    ++
    ++					/*
    ++					 * Cleanup must unset altsock, both as an indication that
    ++					 * it's been released, and to stop pqSocketCheck from
    ++					 * looking at the wrong socket after async auth is done.
    ++					 */
    ++					if (conn->altsock != PGINVALID_SOCKET)
    ++					{
    ++						Assert(false);
    ++						libpq_append_conn_error(conn,
    ++												"internal error: async cleanup did not release polling socket");
    ++						goto error_return;
    ++					}
     +
     +					/*
     +					 * Reenter the authentication exchange with the server. We
    @@ src/interfaces/libpq/fe-connect.c: keep_going:						/* We will come back to here
     +				 * Caller needs to poll some more. conn->async_auth() should
     +				 * have assigned an altsock to poll on.
     +				 */
    -+				Assert(conn->altsock != PGINVALID_SOCKET);
    ++				if (conn->altsock == PGINVALID_SOCKET)
    ++				{
    ++					Assert(false);
    ++					libpq_append_conn_error(conn,
    ++											"internal error: async authentication did not set a socket for polling");
    ++					goto error_return;
    ++				}
    ++
     +				return status;
     +			}
     +
4:  3dc6dd3433c = 4:  711ca3f1efc Add OAUTHBEARER SASL mechanism
5:  97e0a2aae26 = 5:  66ef3b4b687 XXX fix libcurl link error
6:  db0167009b9 = 6:  4df1bc59638 DO NOT MERGE: Add pytest suite for OAuth

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 27 Jan 2025 23:49:50 +0100

> On 21 Jan 2025, at 17:46, Jacob Champion <[email-masked]> wrote:

> Done that way in v43.

I've spent some time staring at, and testing, 0001, 0002 and 0003 with the
intent of getting them in to pave the way for the end goal of getting 0004 in.
In general I would say they are ready, I only have a small nitpick on 0002:

+	conn->allowed_sasl_mechs[0] = &pg_scram_mech;
I'm not a huge fan of this hardcoding in fill_allowed_sasl_mechs().  It's true
that we only have one as of this patch, but we might as well plan a little for
the future maintainability. I took a quick stab in the attached.

On top of that I just re-arranged a comment to, IMHO, better match the style in
the rest of the file.

Unless there are objections I aim at committing these patches reasonably soon
to lower the barrier for getting OAuth support committed.

--
Daniel Gustafsson

commit 73f6e943711f9c158a0f1b32fcc78f210d767083
Author: Daniel Gustafsson <[email-masked]>
Date:   Mon Jan 27 15:13:58 2025 +0100

    nitpickerying

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85ebf9f6d87..e390e428284 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -396,6 +396,25 @@ static const PQEnvironmentOption EnvironmentOptions[] =
 	}
 };
 
+typedef struct SupportedSASLMech
+{
+	const char *name;
+	const pg_fe_sasl_mech *mech;
+} SupportedSASLMech;
+
+#define SASL_MECHANISM_COUNT 1
+
+static SupportedSASLMech supported_sasl_mech[] =
+{
+	{
+		"SCRAM", &pg_scram_mech
+	},
+	{
+		NULL, NULL
+	}
+};
+
+
 /* The connection URI must start with either of the following designators: */
 static const char uri_designator[] = "postgresql://";
 static const char short_uri_designator[] = "postgres://";
@@ -512,8 +531,8 @@ pqDropConnection(PGconn *conn, bool flushInput)
 		conn->cleanup_async_auth = NULL;
 	}
 	conn->async_auth = NULL;
-	conn->altsock = PGINVALID_SOCKET;	/* cleanup_async_auth() should have
-										 * done this, but make sure. */
+	/* cleanup_async_auth() should have done this, but make sure */
+	conn->altsock = PGINVALID_SOCKET;
 #ifdef ENABLE_GSS
 	{
 		OM_uint32	min_s;
@@ -1144,14 +1163,14 @@ fill_allowed_sasl_mechs(PGconn *conn)
 	 *
 	 * To add a new mechanism to require_auth,
 	 * - update the length of conn->allowed_sasl_mechs,
-	 * - add the new pg_fe_sasl_mech pointer to this function, and
 	 * - handle the new mechanism name in the require_auth portion of
 	 *   pqConnectOptions2(), below.
 	 */
-	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == 1,
-					 "fill_allowed_sasl_mechs() must be updated when resizing conn->allowed_sasl_mechs[]");
+	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == SASL_MECHANISM_COUNT,
+					 "conn->allowed_sasl_mechs[] is not sufficiently large for holding all supported SASL mechanisms");
 
-	conn->allowed_sasl_mechs[0] = &pg_scram_mech;
+	for (int i = 0; i < SASL_MECHANISM_COUNT; i++)
+		conn->allowed_sasl_mechs[i] = supported_sasl_mech[i].mech;
 }
 
 /*
@@ -1506,8 +1525,7 @@ pqConnectOptions2(PGconn *conn)
 			 * Next group: SASL mechanisms. All of these use the same request
 			 * codes, so the list of allowed mechanisms is tracked separately.
 			 *
-			 * fill_allowed_sasl_mechs() must be updated when adding a new
-			 * mechanism here!
+			 * supported_sasl_mech must contain all mechanism handled here.
 			 */
 			else if (strcmp(method, "scram-sha-256") == 0)
 			{
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index a2644a2e653..89d738efdc1 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2801,6 +2801,7 @@ Subscription
 SubscriptionInfo
 SubscriptionRelState
 SummarizerReadLocalXLogPrivate
+SupportedSASLMech
 SupportRequestCost
 SupportRequestIndexCondition
 SupportRequestOptimizeWindowClause

---

From: Jacob Champion <[email-masked]>
Date: Mon, 27 Jan 2025 16:59:36 -0800

> the future maintainability. I took a quick stab in the attached.

Okay. I've folded that in and simplified it some, to remove the unused
names and just store the mechanism pointers without a wrapper struct;
see what you think.

> to lower the barrier for getting OAuth support committed.

Thanks!

--

v44 tackles threadsafety for older versions of Curl. If we can't prove
that the installed libcurl is threadsafe at configure time, we'll wrap
our one-time initialization in the pg_g_threadlock. Otherwise, we
won't bother with locking, but we will bail out loudly if our
threadsafety code has not been compiled in and libcurl has been
downgraded to a version/build that can't do that itself. Documentation
has been added for clients, to detail when they need to worry about
PQregisterThreadLock(), in the same way they already do with Kerberos.

While I was playing with that, I noticed that the Autoconf side of
things was not correctly picking up pkg-config variables, and my local
environment had masked the bug. I've added code to handle
CFLAGS/LDFLAGS in the same way that e.g. libxml is handled.

libpq no longer requires the authorization server to advertise support
for the device_code grant type. Entra ID doesn't appear to add that to
any of the openid-configurations it publishes, which was the primary
impetus for the change. Note that if a provider claims to support a
device_authorization_endpoint but then rejects a device_code grant,
we're not going to know what spec they're implementing anyway, so this
check likely doesn't give us any particular advantage. I've removed it
with an explanatory comment.

A description of the OAUTHBEARER handshake has been added to our
protocol docs, and I've added a comment to the new GUC in the sample
file. I've also added slightly nicer error messages in the case that
either OAuth endpoint isn't secured by HTTPS.

Thanks,
--Jacob
1:  5d474397364 = 1:  258b8dbb770 Move PG_MAX_AUTH_TOKEN_LENGTH to libpq/auth.h
2:  20452d21e0b ! 2:  ec960cf363d require_auth: prepare for multiple SASL mechanisms
    @@ src/interfaces/libpq/fe-auth.c: pg_SASL_init(PGconn *conn, int payloadlen)
      	{
     
      ## src/interfaces/libpq/fe-connect.c ##
    +@@ src/interfaces/libpq/fe-connect.c: static const PQEnvironmentOption EnvironmentOptions[] =
    + 	}
    + };
    + 
    ++static const pg_fe_sasl_mech *supported_sasl_mechs[] =
    ++{
    ++	&pg_scram_mech,
    ++};
    ++#define SASL_MECHANISM_COUNT lengthof(supported_sasl_mechs)
    ++
    + /* The connection URI must start with either of the following designators: */
    + static const char uri_designator[] = "postgresql://";
    + static const char short_uri_designator[] = "postgres://";
     @@ src/interfaces/libpq/fe-connect.c: libpq_prng_init(PGconn *conn)
      	pg_prng_seed(&conn->prng_state, rseed);
      }
    @@ src/interfaces/libpq/fe-connect.c: libpq_prng_init(PGconn *conn)
     +	 * rely on the compile-time assertion here to keep us honest.
     +	 *
     +	 * To add a new mechanism to require_auth,
    ++	 * - add it to supported_sasl_mechs,
     +	 * - update the length of conn->allowed_sasl_mechs,
    -+	 * - add the new pg_fe_sasl_mech pointer to this function, and
     +	 * - handle the new mechanism name in the require_auth portion of
     +	 *   pqConnectOptions2(), below.
     +	 */
    -+	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == 1,
    -+					 "fill_allowed_sasl_mechs() must be updated when resizing conn->allowed_sasl_mechs[]");
    ++	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == SASL_MECHANISM_COUNT,
    ++					 "conn->allowed_sasl_mechs[] is not sufficiently large for holding all supported SASL mechanisms");
     +
    -+	conn->allowed_sasl_mechs[0] = &pg_scram_mech;
    ++	for (int i = 0; i < SASL_MECHANISM_COUNT; i++)
    ++		conn->allowed_sasl_mechs[i] = supported_sasl_mechs[i];
     +}
     +
     +/*
    @@ src/interfaces/libpq/fe-connect.c: pqConnectOptions2(PGconn *conn)
     +			 * Next group: SASL mechanisms. All of these use the same request
     +			 * codes, so the list of allowed mechanisms is tracked separately.
     +			 *
    -+			 * fill_allowed_sasl_mechs() must be updated when adding a new
    -+			 * mechanism here!
    ++			 * supported_sasl_mechs must contain all mechanisms handled here.
     +			 */
      			else if (strcmp(method, "scram-sha-256") == 0)
      			{
    @@ src/interfaces/libpq/fe-connect.c: pqConnectOptions2(PGconn *conn)
     +					i = index_of_allowed_sasl_mech(conn, mech);
     +					if (i < 0)
     +						goto duplicate;
    - 
    --				conn->allowed_auth_methods &= ~bits;
    ++
     +					conn->allowed_sasl_mechs[i] = NULL;
     +				}
     +				else
    @@ src/interfaces/libpq/fe-connect.c: pqConnectOptions2(PGconn *conn)
     +					i = index_of_allowed_sasl_mech(conn, mech);
     +					if (i >= 0)
     +						goto duplicate;
    -+
    + 
    +-				conn->allowed_auth_methods &= ~bits;
     +					i = index_of_allowed_sasl_mech(conn, NULL);
     +					if (i < 0)
     +					{
3:  f0afefb80d6 ! 3:  9725788086c libpq: handle asynchronous actions during SASL
    @@ src/interfaces/libpq/fe-connect.c: pqDropConnection(PGconn *conn, bool flushInpu
     +		conn->cleanup_async_auth = NULL;
     +	}
     +	conn->async_auth = NULL;
    -+	conn->altsock = PGINVALID_SOCKET;	/* cleanup_async_auth() should have
    -+										 * done this, but make sure. */
    ++	/* cleanup_async_auth() should have done this, but make sure */
    ++	conn->altsock = PGINVALID_SOCKET;
      #ifdef ENABLE_GSS
      	{
      		OM_uint32	min_s;
4:  711ca3f1efc ! 4:  a260d9436f0 Add OAUTHBEARER SASL mechanism
    @@ .cirrus.tasks.yml: task:
        ###
        # Test that code can be built with gcc/clang without warnings
     
    + ## config/programs.m4 ##
    +@@ config/programs.m4: AC_DEFUN([PGAC_CHECK_STRIP],
    +   AC_SUBST(STRIP_STATIC_LIB)
    +   AC_SUBST(STRIP_SHARED_LIB)
    + ])# PGAC_CHECK_STRIP
    ++
    ++
    ++
    ++# PGAC_CHECK_LIBCURL
    ++# ------------------
    ++# Check for required libraries and headers, and test to see whether the current
    ++# installation of libcurl is threadsafe.
    ++
    ++AC_DEFUN([PGAC_CHECK_LIBCURL],
    ++[
    ++  AC_CHECK_HEADER(curl/curl.h, [],
    ++				  [AC_MSG_ERROR([header file <curl/curl.h> is required for --with-libcurl])])
    ++  AC_CHECK_LIB(curl, curl_multi_init, [],
    ++			   [AC_MSG_ERROR([library 'curl' does not provide curl_multi_init])])
    ++
    ++  # Check to see whether the current platform supports threadsafe Curl
    ++  # initialization.
    ++  AC_CACHE_CHECK([for curl_global_init thread safety], [pgac_cv__libcurl_threadsafe_init],
    ++  [AC_RUN_IFELSE([AC_LANG_PROGRAM([
    ++#include <curl/curl.h>
    ++],[
    ++    curl_version_info_data *info;
    ++
    ++    if (curl_global_init(CURL_GLOBAL_ALL))
    ++        return -1;
    ++
    ++    info = curl_version_info(CURLVERSION_NOW);
    ++#ifdef CURL_VERSION_THREADSAFE
    ++    if (info->features & CURL_VERSION_THREADSAFE)
    ++        return 0;
    ++#endif
    ++
    ++    return 1;
    ++])],
    ++  [pgac_cv__libcurl_threadsafe_init=yes],
    ++  [pgac_cv__libcurl_threadsafe_init=no],
    ++  [pgac_cv__libcurl_threadsafe_init=unknown])])
    ++  if test x"$pgac_cv__libcurl_threadsafe_init" = xyes ; then
    ++    AC_DEFINE(HAVE_THREADSAFE_CURL_GLOBAL_INIT, 1,
    ++              [Define to 1 if curl_global_init() is guaranteed to be threadsafe.])
    ++  fi
    ++])# PGAC_CHECK_LIBCURL
    +
      ## configure ##
     @@ configure: XML2_LIBS
      XML2_CFLAGS
    @@ configure: fi
     +
     +fi
     +
    ++  # We only care about -I, -D, and -L switches;
    ++  # note that -lcurl will be added by PGAC_CHECK_LIBCURL below.
    ++  for pgac_option in $LIBCURL_CFLAGS; do
    ++    case $pgac_option in
    ++      -I*|-D*) CPPFLAGS="$CPPFLAGS $pgac_option";;
    ++    esac
    ++  done
    ++  for pgac_option in $LIBCURL_LIBS; do
    ++    case $pgac_option in
    ++      -L*) LDFLAGS="$LDFLAGS $pgac_option";;
    ++    esac
    ++  done
    ++
     +  # OAuth requires python for testing
     +  if test "$with_python" != yes; then
     +    { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: *** OAuth support tests require --with-python to run" >&5
    @@ configure: fi
     +# during gss_acquire_cred(). This is possibly related to Curl's Heimdal
     +# dependency on that platform?
     +if test "$with_libcurl" = yes ; then
    ++
    ++  ac_fn_c_check_header_mongrel "$LINENO" "curl/curl.h" "ac_cv_header_curl_curl_h" "$ac_includes_default"
    ++if test "x$ac_cv_header_curl_curl_h" = xyes; then :
    ++
    ++else
    ++  as_fn_error $? "header file <curl/curl.h> is required for --with-libcurl" "$LINENO" 5
    ++fi
    ++
    ++
     +  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for curl_multi_init in -lcurl" >&5
     +$as_echo_n "checking for curl_multi_init in -lcurl... " >&6; }
     +if ${ac_cv_lib_curl_curl_multi_init+:} false; then :
    @@ configure: fi
     +  LIBS="-lcurl $LIBS"
     +
     +else
    -+  as_fn_error $? "library 'curl' is required for --with-libcurl" "$LINENO" 5
    ++  as_fn_error $? "library 'curl' does not provide curl_multi_init" "$LINENO" 5
     +fi
     +
    ++
    ++  # Check to see whether the current platform supports threadsafe Curl
    ++  # initialization.
    ++  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for curl_global_init thread safety" >&5
    ++$as_echo_n "checking for curl_global_init thread safety... " >&6; }
    ++if ${pgac_cv__libcurl_threadsafe_init+:} false; then :
    ++  $as_echo_n "(cached) " >&6
    ++else
    ++  if test "$cross_compiling" = yes; then :
    ++  pgac_cv__libcurl_threadsafe_init=unknown
    ++else
    ++  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
    ++/* end confdefs.h.  */
    ++
    ++#include <curl/curl.h>
    ++
    ++int
    ++main ()
    ++{
    ++
    ++    curl_version_info_data *info;
    ++
    ++    if (curl_global_init(CURL_GLOBAL_ALL))
    ++        return -1;
    ++
    ++    info = curl_version_info(CURLVERSION_NOW);
    ++#ifdef CURL_VERSION_THREADSAFE
    ++    if (info->features & CURL_VERSION_THREADSAFE)
    ++        return 0;
    ++#endif
    ++
    ++    return 1;
    ++
    ++  ;
    ++  return 0;
    ++}
    ++_ACEOF
    ++if ac_fn_c_try_run "$LINENO"; then :
    ++  pgac_cv__libcurl_threadsafe_init=yes
    ++else
    ++  pgac_cv__libcurl_threadsafe_init=no
    ++fi
    ++rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
    ++  conftest.$ac_objext conftest.beam conftest.$ac_ext
    ++fi
    ++
    ++fi
    ++{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__libcurl_threadsafe_init" >&5
    ++$as_echo "$pgac_cv__libcurl_threadsafe_init" >&6; }
    ++  if test x"$pgac_cv__libcurl_threadsafe_init" = xyes ; then
    ++
    ++$as_echo "#define HAVE_THREADSAFE_CURL_GLOBAL_INIT 1" >>confdefs.h
    ++
    ++  fi
    ++
     +fi
     +
      if test "$with_gssapi" = yes ; then
        if test "$PORTNAME" != "win32"; then
          { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing gss_store_cred_into" >&5
    -@@ configure: fi
    - 
    - done
    - 
    -+fi
    -+
    -+if test "$with_libcurl" = yes; then
    -+  ac_fn_c_check_header_mongrel "$LINENO" "curl/curl.h" "ac_cv_header_curl_curl_h" "$ac_includes_default"
    -+if test "x$ac_cv_header_curl_curl_h" = xyes; then :
    -+
    -+else
    -+  as_fn_error $? "header file <curl/curl.h> is required for --with-libcurl" "$LINENO" 5
    -+fi
    -+
    -+
    - fi
    - 
    - if test "$PORTNAME" = "win32" ; then
     
      ## configure.ac ##
     @@ configure.ac: fi
    @@ configure.ac: fi
     +  # to explicitly set TLS 1.3 ciphersuites).
     +  PKG_CHECK_MODULES(LIBCURL, [libcurl >= 7.61.0])
     +
    ++  # We only care about -I, -D, and -L switches;
    ++  # note that -lcurl will be added by PGAC_CHECK_LIBCURL below.
    ++  for pgac_option in $LIBCURL_CFLAGS; do
    ++    case $pgac_option in
    ++      -I*|-D*) CPPFLAGS="$CPPFLAGS $pgac_option";;
    ++    esac
    ++  done
    ++  for pgac_option in $LIBCURL_LIBS; do
    ++    case $pgac_option in
    ++      -L*) LDFLAGS="$LDFLAGS $pgac_option";;
    ++    esac
    ++  done
    ++
     +  # OAuth requires python for testing
     +  if test "$with_python" != yes; then
     +    AC_MSG_WARN([*** OAuth support tests require --with-python to run])
    @@ configure.ac: failure.  It is possible the compiler isn't looking in the proper
     +# during gss_acquire_cred(). This is possibly related to Curl's Heimdal
     +# dependency on that platform?
     +if test "$with_libcurl" = yes ; then
    -+  AC_CHECK_LIB(curl, curl_multi_init, [], [AC_MSG_ERROR([library 'curl' is required for --with-libcurl])])
    ++  PGAC_CHECK_LIBCURL
     +fi
     +
      if test "$with_gssapi" = yes ; then
        if test "$PORTNAME" != "win32"; then
          AC_SEARCH_LIBS(gss_store_cred_into, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'], [],
    -@@ configure.ac: elif test "$with_uuid" = ossp ; then
    -       [AC_MSG_ERROR([header file <ossp/uuid.h> or <uuid.h> is required for OSSP UUID])])])
    - fi
    - 
    -+if test "$with_libcurl" = yes; then
    -+  AC_CHECK_HEADER(curl/curl.h, [], [AC_MSG_ERROR([header file <curl/curl.h> is required for --with-libcurl])])
    -+fi
    -+
    - if test "$PORTNAME" = "win32" ; then
    -    AC_CHECK_HEADERS(crtdefs.h)
    - fi
     
      ## doc/src/sgml/client-auth.sgml ##
     @@ doc/src/sgml/client-auth.sgml: include_dir         <replaceable>directory</replaceable>
    @@ doc/src/sgml/libpq.sgml: void PQinitSSL(int do_ssl);
      
       <sect1 id="libpq-threading">
        <title>Behavior in Threaded Programs</title>
    +@@ doc/src/sgml/libpq.sgml: int PQisthreadsafe();
    +    <application>libpq</application> source code for a way to do cooperative
    +    locking between <application>libpq</application> and your application.
    +   </para>
    ++
    ++  <para>
    ++   Similarly, if you are using Curl inside your application,
    ++   <emphasis>and</emphasis> you do not already
    ++   <ulink url="https://curl.se/libcurl/c/curl_global_init.html">initialize
    ++   libcurl globally</ulink> before starting new threads, you will need to
    ++   cooperatively lock (again via <function>PQregisterThreadLock</function>)
    ++   around any code that may initialize libcurl. This restriction is lifted for
    ++   more recent versions of Curl that are built to support threadsafe
    ++   initialization; those builds can be identified by the advertisement of a
    ++   <literal>threadsafe</literal> feature in their version metadata.
    ++  </para>
    +  </sect1>
    + 
    + 
     
      ## doc/src/sgml/oauth-validators.sgml (new) ##
     @@
    @@ doc/src/sgml/postgres.sgml: break is not needed in a wider output rendering.
       </part>

    + ## doc/src/sgml/protocol.sgml ##
    +@@ doc/src/sgml/protocol.sgml: SELCT 1/0;<!-- this typo is intentional -->
    + 
    +   <para>
    +    <firstterm>SASL</firstterm> is a framework for authentication in connection-oriented
    +-   protocols. At the moment, <productname>PostgreSQL</productname> implements two SASL
    +-   authentication mechanisms, SCRAM-SHA-256 and SCRAM-SHA-256-PLUS. More
    +-   might be added in the future. The below steps illustrate how SASL
    +-   authentication is performed in general, while the next subsection gives
    +-   more details on SCRAM-SHA-256 and SCRAM-SHA-256-PLUS.
    ++   protocols. At the moment, <productname>PostgreSQL</productname> implements three
    ++   SASL authentication mechanisms: SCRAM-SHA-256, SCRAM-SHA-256-PLUS, and
    ++   OAUTHBEARER. More might be added in the future. The below steps illustrate how SASL
    ++   authentication is performed in general, while the next subsections give
    ++   more details on particular mechanisms.
    +   </para>
    + 
    +   <procedure>
    +@@ doc/src/sgml/protocol.sgml: SELCT 1/0;<!-- this typo is intentional -->
    +    <step id="sasl-auth-end">
    +     <para>
    +      Finally, when the authentication exchange is completed successfully, the
    +-     server sends an AuthenticationSASLFinal message, followed
    ++     server sends an optional AuthenticationSASLFinal message, followed
    +      immediately by an AuthenticationOk message. The AuthenticationSASLFinal
    +      contains additional server-to-client data, whose content is particular to the
    +      selected authentication mechanism. If the authentication mechanism doesn't
    +@@ doc/src/sgml/protocol.sgml: SELCT 1/0;<!-- this typo is intentional -->
    +    <title>SCRAM-SHA-256 Authentication</title>
    + 
    +    <para>
    +-    The implemented SASL mechanisms at the moment
    +-    are <literal>SCRAM-SHA-256</literal> and its variant with channel
    +-    binding <literal>SCRAM-SHA-256-PLUS</literal>. They are described in
    ++    <literal>SCRAM-SHA-256</literal>, and its variant with channel
    ++    binding <literal>SCRAM-SHA-256-PLUS</literal>, are password-based
    ++    authentication mechanisms. They are described in
    +     detail in <ulink url="https://datatracker.ietf.org/doc/html/rfc7677">RFC 7677</ulink>
    +     and <ulink url="https://datatracker.ietf.org/doc/html/rfc5802">RFC 5802</ulink>.
    +    </para>
    +@@ doc/src/sgml/protocol.sgml: SELCT 1/0;<!-- this typo is intentional -->
    +     </step>
    +    </procedure>
    +   </sect2>
    ++
    ++  <sect2 id="sasl-oauthbearer">
    ++   <title>OAUTHBEARER Authentication</title>
    ++
    ++   <para>
    ++    <literal>OAUTHBEARER</literal> is a token-based mechanism for federated
    ++    authentication. It is described in detail in
    ++    <ulink url="https://datatracker.ietf.org/doc/html/rfc7628">RFC 7628</ulink>.
    ++   </para>
    ++
    ++   <para>
    ++    A typical exchange differs depending on whether or not the client already
    ++    has a bearer token cached for the current user. If it does not, the exchange
    ++    will take place over two connections: the first "discovery" connection to
    ++    obtain OAuth metadata from the server, and the second connection to send
    ++    the token after the client has obtained it. (libpq does not currently
    ++    implement a caching method as part of its builtin flow, so it uses the
    ++    two-connection exchange.)
    ++   </para>
    ++
    ++   <para>
    ++    This mechanism is client-initiated, like SCRAM. The client initial response
    ++    consists of the standard "GS2" header used by SCRAM, followed by a list of
    ++    <literal>key=value</literal> pairs. The only key currently supported by
    ++    the server is <literal>auth</literal>, which contains the bearer token.
    ++    <literal>OAUTHBEARER</literal> additionally specifies three optional
    ++    components of the client initial response (the <literal>authzid</literal> of
    ++    the GS2 header, and the <structfield>host</structfield> and
    ++    <structfield>port</structfield> keys) which are currently ignored by the
    ++    server.
    ++   </para>
    ++
    ++   <para>
    ++    <literal>OAUTHBEARER</literal> does not support channel binding, and there
    ++    is no "OAUTHBEARER-PLUS" mechanism. This mechanism does not make use of
    ++    server data during a successful authentication, so the
    ++    AuthenticationSASLFinal message is not used in the exchange.
    ++   </para>
    ++
    ++   <procedure>
    ++    <title>Example</title>
    ++    <step>
    ++     <para>
    ++      During the first exchange, the server sends an AuthenticationSASL message
    ++      with the <literal>OAUTHBEARER</literal> mechanism advertised.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      The client responds by sending a SASLInitialResponse message which
    ++      indicates the <literal>OAUTHBEARER</literal> mechanism. Assuming the
    ++      client does not already have a valid bearer token for the current user,
    ++      the <structfield>auth</structfield> field is empty, indicating a discovery
    ++      connection.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      Server sends an AuthenticationSASLContinue message containing an error
    ++      <literal>status</literal> alongside a well-known URI and scopes that the
    ++      client should use to conduct an OAuth flow.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      Client sends a SASLResponse message containing the empty set (a single
    ++      <literal>0x01</literal> byte) to finish its half of the discovery
    ++      exchange.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      Server sends an ErrorMessage to fail the first exchange.
    ++     </para>
    ++     <para>
    ++      At this point, the client conducts one of many possible OAuth flows to
    ++      obtain a bearer token, using any metadata that it has been configured with
    ++      in addition to that provided by the server. (This description is left
    ++      deliberately vague; <literal>OAUTHBEARER</literal> does not specify or
    ++      mandate any particular method for obtaining a token.)
    ++     </para>
    ++     <para>
    ++      Once it has a token, the client reconnects to the server for the final
    ++      exchange:
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      The server once again sends an AuthenticationSASL message with the
    ++      <literal>OAUTHBEARER</literal> mechanism advertised.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      The client responds by sending a SASLInitialResponse message, but this
    ++      time the <structfield>auth</structfield> field in the message contains the
    ++      bearer token that was obtained during the client flow.
    ++     </para>
    ++    </step>
    ++
    ++    <step>
    ++     <para>
    ++      The server validates the token according to the instructions of the
    ++      token provider. If the client is authorized to connect, it sends an
    ++      AuthenticationOk message to end the SASL exchange.
    ++     </para>
    ++    </step>
    ++   </procedure>
    ++  </sect2>
    +  </sect1>
    + 
    +  <sect1 id="protocol-replication">
    +
      ## doc/src/sgml/regress.sgml ##
     @@ doc/src/sgml/regress.sgml: make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance libpq_encryption'
            </para>
    @@ meson.build: endif
     +  libcurl = dependency('libcurl', version: '>= 7.61.0', required: libcurlopt)
     +  if libcurl.found()
     +    cdata.set('USE_LIBCURL', 1)
    ++
    ++    # Check to see whether the current platform supports threadsafe Curl
    ++    # initialization.
    ++    libcurl_threadsafe_init = false
    ++
    ++    if not meson.is_cross_build()
    ++      r = cc.run('''
    ++        #include <curl/curl.h>
    ++
    ++        int main(void)
    ++        {
    ++            curl_version_info_data *info;
    ++
    ++            if (curl_global_init(CURL_GLOBAL_ALL))
    ++                return -1;
    ++
    ++            info = curl_version_info(CURLVERSION_NOW);
    ++        #ifdef CURL_VERSION_THREADSAFE
    ++            if (info->features & CURL_VERSION_THREADSAFE)
    ++                return 0;
    ++        #endif
    ++
    ++            return 1;
    ++        }''',
    ++        name: 'test for curl_global_init thread safety',
    ++        dependencies: libcurl,
    ++      )
    ++
    ++      assert(r.compiled())
    ++      if r.returncode() == 0
    ++        libcurl_threadsafe_init = true
    ++        message('curl_global_init is threadsafe')
    ++      elif r.returncode() == 1
    ++        message('curl_global_init is not threadsafe')
    ++      else
    ++        message('curl_global_init failed; assuming not threadsafe')
    ++      endif
    ++    endif
    ++
    ++    if libcurl_threadsafe_init
    ++      cdata.set('HAVE_THREADSAFE_CURL_GLOBAL_INIT', 1)
    ++    endif
     +  endif
    ++
     +else
     +  libcurl = not_found_dep
     +endif
    @@ src/backend/utils/misc/postgresql.conf.sample
      #ssl_passphrase_command_supports_reload = off
      
     +# OAuth
    -+#oauth_validator_libraries = ''
    ++#oauth_validator_libraries = ''	# comma-separated list of trusted validator modules
     +
      
      #------------------------------------------------------------------------------
    @@ src/include/pg_config.h.in
      /* Define to 1 if you have the `ldap' library (-lldap). */
      #undef HAVE_LIBLDAP
      
    +@@
    + /* Define to 1 if you have the <termios.h> header file. */
    + #undef HAVE_TERMIOS_H
    + 
    ++/* Define to 1 if curl_global_init() is guaranteed to be threadsafe. */
    ++#undef HAVE_THREADSAFE_CURL_GLOBAL_INIT
    ++
    + /* Define to 1 if your compiler understands `typeof' or something similar. */
    + #undef HAVE_TYPEOF
    + 
     @@
      /* Define to 1 to build with LDAP support. (--with-ldap) */
      #undef USE_LDAP
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	return true;
     +}
     +
    ++#define HTTPS_SCHEME "https://"
     +#define OAUTH_GRANT_TYPE_DEVICE_CODE "urn:ietf:params:oauth:grant-type:device_code"
     +
     +/*
     + * Ensure that the provider supports the Device Authorization flow (i.e. it
    -+ * accepts the device_code grant type and provides an authorization endpoint).
    ++ * provides an authorization endpoint, and both the token and authorization
    ++ * endpoint URLs seem reasonable).
     + */
     +static bool
     +check_for_device_flow(struct async_ctx *actx)
     +{
     +	const struct provider *provider = &actx->provider;
    -+	const struct curl_slist *grant;
    -+	bool		device_grant_found = false;
     +
     +	Assert(provider->issuer);	/* ensured by parse_provider() */
    -+
    -+	/*------
    -+	 * First, sanity checks for discovery contents that are OPTIONAL in the
    -+	 * spec but required for our flow:
    -+	 * - the issuer must support the device_code grant
    -+	 * - the issuer must have actually given us a
    -+	 *   device_authorization_endpoint
    -+	 */
    -+
    -+	grant = provider->grant_types_supported;
    -+	while (grant)
    -+	{
    -+		if (strcmp(grant->data, OAUTH_GRANT_TYPE_DEVICE_CODE) == 0)
    -+		{
    -+			device_grant_found = true;
    -+			break;
    -+		}
    -+
    -+		grant = grant->next;
    -+	}
    -+
    -+	if (!device_grant_found)
    -+	{
    -+		actx_error(actx, "issuer \"%s\" does not support device code grants",
    -+				   provider->issuer);
    -+		return false;
    -+	}
    ++	Assert(provider->token_endpoint);	/* ensured by parse_provider() */
     +
     +	if (!provider->device_authorization_endpoint)
     +	{
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		return false;
     +	}
     +
    -+	/* TODO: check that the endpoint uses HTTPS */
    ++	/*
    ++	 * The original implementation checked that OAUTH_GRANT_TYPE_DEVICE_CODE
    ++	 * was present in the discovery document's grant_types_supported list. MS
    ++	 * Entra does not advertise this grant type, though, and since it doesn't
    ++	 * make sense to stand up a device_authorization_endpoint without also
    ++	 * accepting device codes at the token_endpoint, that's the only thing we
    ++	 * currently require.
    ++	 */
    ++
    ++	/*
    ++	 * Although libcurl will fail later if the URL contains an unsupported
    ++	 * scheme, that error message is going to be a bit opaque. This is a
    ++	 * decent time to bail out if we're not using HTTPS for the endpoints
    ++	 * we'll use for the flow.
    ++	 */
    ++	if (!actx->debugging)
    ++	{
    ++		if (pg_strncasecmp(provider->device_authorization_endpoint,
    ++						   HTTPS_SCHEME, strlen(HTTPS_SCHEME)) != 0)
    ++		{
    ++			actx_error(actx,
    ++					   "device authorization endpoint \"%s\" must use HTTPS",
    ++					   provider->device_authorization_endpoint);
    ++			return false;
    ++		}
    ++
    ++		if (pg_strncasecmp(provider->token_endpoint,
    ++						   HTTPS_SCHEME, strlen(HTTPS_SCHEME)) != 0)
    ++		{
    ++			actx_error(actx,
    ++					   "token endpoint \"%s\" must use HTTPS",
    ++					   provider->token_endpoint);
    ++			return false;
    ++		}
    ++	}
     +
     +	return true;
     +}
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	return true;
     +}
     +
    ++/*
    ++ * Calls curl_global_init() in a thread-safe way.
    ++ *
    ++ * libcurl has stringent requirements for the thread context in which you call
    ++ * curl_global_init(), because it's going to try initializing a bunch of other
    ++ * libraries (OpenSSL, Winsock, etc). Recent versions of libcurl have improved
    ++ * the thread-safety situation, but there's a chicken-and-egg problem at
    ++ * runtime: you can't check the thread safety until you've initialized libcurl,
    ++ * which you can't do from within a thread unless you know it's thread-safe...
    ++ *
    ++ * Returns true if initialization was successful. Successful or not, this
    ++ * function will not try to reinitialize Curl on successive calls.
    ++ */
    ++static bool
    ++initialize_curl(PGconn *conn)
    ++{
    ++	/*
    ++	 * Don't let the compiler play tricks with this variable. In the
    ++	 * HAVE_THREADSAFE_CURL_GLOBAL_INIT case, we don't care if two threads
    ++	 * enter simultaneously, but we do care if this gets set transiently to
    ++	 * PG_BOOL_YES/NO in cases where that's not the final answer.
    ++	 */
    ++	static volatile PGTernaryBool init_successful = PG_BOOL_UNKNOWN;
    ++#if HAVE_THREADSAFE_CURL_GLOBAL_INIT
    ++	curl_version_info_data *info;
    ++#endif
    ++
    ++#if !HAVE_THREADSAFE_CURL_GLOBAL_INIT
    ++
    ++	/*
    ++	 * Lock around the whole function. If a libpq client performs its own work
    ++	 * with libcurl, it must either ensure that Curl is initialized safely
    ++	 * before calling us (in which case our call will be a no-op), or else it
    ++	 * must guard its own calls to curl_global_init() with a registered
    ++	 * threadlock handler. See PQregisterThreadLock().
    ++	 */
    ++	pglock_thread();
    ++#endif
    ++
    ++	/*
    ++	 * Skip initialization if we've already done it. (Curl tracks the number
    ++	 * of calls; there's no point in incrementing the counter every time we
    ++	 * connect.)
    ++	 */
    ++	if (init_successful == PG_BOOL_YES)
    ++		goto done;
    ++	else if (init_successful == PG_BOOL_NO)
    ++	{
    ++		libpq_append_conn_error(conn,
    ++								"curl_global_init previously failed during OAuth setup");
    ++		goto done;
    ++	}
    ++
    ++	/*
    ++	 * We know we've already initialized Winsock by this point (see
    ++	 * pqMakeEmptyPGconn()), so we should be able to safely skip that bit. But
    ++	 * we have to tell libcurl to initialize everything else, because other
    ++	 * pieces of our client executable may already be using libcurl for their
    ++	 * own purposes. If we initialize libcurl with only a subset of its
    ++	 * features, we could break those other clients nondeterministically, and
    ++	 * that would probably be a nightmare to debug.
    ++	 *
    ++	 * If some other part of the program has already called this, it's a
    ++	 * no-op.
    ++	 */
    ++	if (curl_global_init(CURL_GLOBAL_ALL & ~CURL_GLOBAL_WIN32) != CURLE_OK)
    ++	{
    ++		libpq_append_conn_error(conn,
    ++								"curl_global_init failed during OAuth setup");
    ++		init_successful = PG_BOOL_NO;
    ++		goto done;
    ++	}
    ++
    ++#if HAVE_THREADSAFE_CURL_GLOBAL_INIT
    ++
    ++	/*
    ++	 * If we determined at configure time that the Curl installation is
    ++	 * threadsafe, our job here is much easier. We simply initialize above
    ++	 * without any locking (concurrent or duplicated calls are fine in that
    ++	 * situation), then double-check to make sure the runtime setting agrees,
    ++	 * to try to catch silent downgrades.
    ++	 */
    ++	info = curl_version_info(CURLVERSION_NOW);
    ++	if (!(info->features & CURL_VERSION_THREADSAFE))
    ++	{
    ++		/*
    ++		 * In a downgrade situation, the damage is already done. Curl global
    ++		 * state may be corrupted. Be noisy.
    ++		 */
    ++		libpq_append_conn_error(conn, "libcurl is no longer threadsafe\n"
    ++								"\tCurl initialization was reported threadsafe when libpq\n"
    ++								"\twas compiled, but the currently installed version of\n"
    ++								"\tlibcurl reports that it is not. Recompile libpq against\n"
    ++								"\tthe installed version of libcurl.");
    ++		init_successful = PG_BOOL_NO;
    ++		goto done;
    ++	}
    ++#endif
    ++
    ++	init_successful = PG_BOOL_YES;
    ++
    ++done:
    ++#if !HAVE_THREADSAFE_CURL_GLOBAL_INIT
    ++	pgunlock_thread();
    ++#endif
    ++	return (init_successful == PG_BOOL_YES);
    ++}
     +
     +/*
     + * The core nonblocking libcurl implementation. This will be called several
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	fe_oauth_state *state = conn->sasl_state;
     +	struct async_ctx *actx;
     +
    -+	/*
    -+	 * XXX This is not safe. libcurl has stringent requirements for the thread
    -+	 * context in which you call curl_global_init(), because it's going to try
    -+	 * initializing a bunch of other libraries (OpenSSL, Winsock...). And we
    -+	 * probably need to consider both the TLS backend libcurl is compiled
    -+	 * against and what the user has asked us to do via PQinit[Open]SSL.
    -+	 *
    -+	 * Recent versions of libcurl have improved the thread-safety situation,
    -+	 * but you apparently can't check at compile time whether the
    -+	 * implementation is thread-safe, and there's a chicken-and-egg problem
    -+	 * where you can't check the thread safety until you've initialized
    -+	 * libcurl, which you can't do before you've made sure it's thread-safe...
    -+	 *
    -+	 * We know we've already initialized Winsock by this point, so we should
    -+	 * be able to safely skip that bit. But we have to tell libcurl to
    -+	 * initialize everything else, because other pieces of our client
    -+	 * executable may already be using libcurl for their own purposes. If we
    -+	 * initialize libcurl first, with only a subset of its features, we could
    -+	 * break those other clients nondeterministically, and that would probably
    -+	 * be a nightmare to debug.
    -+	 */
    -+	curl_global_init(CURL_GLOBAL_ALL
    -+					 & ~CURL_GLOBAL_WIN32); /* we already initialized Winsock */
    ++	if (!initialize_curl(conn))
    ++		return PGRES_POLLING_FAILED;
     +
     +	if (!state->async_ctx)
     +	{
    @@ src/interfaces/libpq/fe-connect.c: static const internalPQconninfoOption PQconni
      	/* Terminating entry --- MUST BE LAST */
      	{NULL, NULL, NULL, NULL,
      	NULL, NULL, 0}
    +@@ src/interfaces/libpq/fe-connect.c: static const PQEnvironmentOption EnvironmentOptions[] =
    + static const pg_fe_sasl_mech *supported_sasl_mechs[] =
    + {
    + 	&pg_scram_mech,
    ++	&pg_oauth_mech,
    + };
    + #define SASL_MECHANISM_COUNT lengthof(supported_sasl_mechs)
    + 
     @@ src/interfaces/libpq/fe-connect.c: pqDropServerData(PGconn *conn)
      	conn->write_failed = false;
      	free(conn->write_err_msg);
    @@ src/interfaces/libpq/fe-connect.c: static inline void
      	 * linked list, conn->allowed_sasl_mechs is an array of static length. We
      	 * rely on the compile-time assertion here to keep us honest.
      	 *
    -@@ src/interfaces/libpq/fe-connect.c: fill_allowed_sasl_mechs(PGconn *conn)
    - 	 * - handle the new mechanism name in the require_auth portion of
    - 	 *   pqConnectOptions2(), below.
    - 	 */
    --	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == 1,
    -+	StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == 2,
    - 					 "fill_allowed_sasl_mechs() must be updated when resizing conn->allowed_sasl_mechs[]");
    - 
    - 	conn->allowed_sasl_mechs[0] = &pg_scram_mech;
    -+	conn->allowed_sasl_mechs[1] = &pg_oauth_mech;
    - }
    - 
    - /*
     @@ src/interfaces/libpq/fe-connect.c: pqConnectOptions2(PGconn *conn)
      			{
      				mech = &pg_scram_mech;
5:  66ef3b4b687 = 5:  035a3832b40 XXX fix libcurl link error
6:  4df1bc59638 ! 6:  5e360725bf9 DO NOT MERGE: Add pytest suite for OAuth
    @@ src/test/python/client/test_oauth.py (new)
     +                {
     +                    "issuer": "{issuer}",
     +                    "token_endpoint": "https://256.256.256.256/token",
    -+                    "device_authorization_endpoint": "https://256.256.256.256/dev",
    -+                },
    -+            ),
    -+            r'cannot run OAuth device authorization: issuer "https://.*" does not support device code grants',
    -+            id="missing device code grants",
    -+        ),
    -+        pytest.param(
    -+            (
    -+                200,
    -+                {
    -+                    "issuer": "{issuer}",
    -+                    "token_endpoint": "https://256.256.256.256/token",
     +                    "grant_types_supported": [
     +                        "urn:ietf:params:oauth:grant-type:device_code"
     +                    ],

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 31 Jan 2025 17:23:36 +0100

> On 28 Jan 2025, at 01:59, Jacob Champion <[email-masked]> wrote:

After staring at the patchset even more I committed patches 0001 and 0002 today
as a preparatory step for getting OAuth in.  I will work on the 0003 (which is
now 0001) next.

Attached is a v45 which is v44 without the now committer patches to keep the
CFBot happy.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 6 Feb 2025 23:02:43 +0100

> On 31 Jan 2025, at 17:23, Daniel Gustafsson <[email-masked]> wrote:
> 

After more staring and commitmessage tweaking I pushed the v45-0001 patch and
it has so far built green in a number of BF animals (as well as in CI).

This to pave the way for the main OAUTHBEARER patch in this set, which I hope
we can reach a final version of very soon.

Attached is a v46 which is v45 minus the now committed patch. 

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 6 Feb 2025 21:48:19 -0800

> Attached is a v46 which is v45 minus the now committed patch.

Thank you! Attached is v47, which creeps ever closer to the finish line.

For ease of review, v47-0001 is identical to v46-0001. The new changes
are split into separate fixup! commits which I'll squash in the next
round. They're ordered roughly in order of increasing complexity:

- 0002 removes and/or rewrites TODO comments that I do not plan to implement.
- 0003 makes the kqueue implementation register a one-shot timer
rather than a repeating timer, to match the epoll implementation.

- 0004 fixes a bug in backend cleanup:

I noticed that there was a "private state cookie changed" error in
some of the test logs, but none of the tests had actually failed.
Changing that to a PANIC revealed that before_shmem_exit() is too late
to run the cleanup function, since the state allocation has already
been released. I've swapped that out for a reset callback.

- 0005 warns at configure time if libcurl doesn't have a nonblocking
DNS implementation.
- 0006 augments bare Asserts during client-side JSON parsing with code
that will fail gracefully in production builds as well.
- 0007 escapes binary data during the printing of libcurl debug
output. (If you're having a bad enough day to need the debug spray,
you're probably not in the mood for the sound of a hundred BELs.)
- 0008 parses and passes through the expires_in and optional
verification_uri_complete fields from the device endpoint to any
custom user prompt. (We do not use them ourselves, at the moment. But
after seeing some nice demos of RHEL/PAM/sssd support for device flow
QR codes at FOSDEM, I think we're definitely going to want to make
those available to devs.)

- 0009 is gold-plating for the OAUTH_STEP_WAIT_INTERVAL state:

If PQconnectPoll client calls us early while we're waiting for the
ping interval to expire, we will immediately send the next request
even if we should be waiting. That bothers me a bit, because if our
implementation gets a tempban from an OAuth provider because one of
our clients accidentally implemented a busy-loop, I think we're likely
to get the blame. Ideally we should kick back up to the caller and
tell them to wait longer, instead.

Checking to see if the timer has expired is easy enough for
epoll/timerfd, but I wasn't able to find an easy way to do that with a
single kqueue. Instead, I split the kqueue in two and treat the second
one as the timer. (If it becomes readable, the timer has expired.)
There is an additional advantage in that I get to remove some `#ifdef
HAVE_SYS_EPOLL_H` sections; the two implementations are closer in
spirit now.

Thanks,
--Jacob
 1:  ac5b3e053a3 =  1:  6747b7cc795 Add OAUTHBEARER SASL mechanism
 -:  ----------- >  2:  483129c1ca9 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  3:  75d98784ded fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  4:  fd60ceb4c84 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  5:  595362ef2c1 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  6:  f73c042adc9 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  7:  298839b69f0 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  8:  1cf48a8f835 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  9:  27135876559 fixup! Add OAUTHBEARER SASL mechanism
 2:  4190ef1bac7 = 10:  d8c1f298080 XXX fix libcurl link error
 3:  11cf045bd21 ! 11:  dbf305d0489 DO NOT MERGE: Add pytest suite for OAuth
    @@ src/test/python/client/test_oauth.py (new)
     +    _fields_ = [
     +        ("verification_uri", ctypes.c_char_p),
     +        ("user_code", ctypes.c_char_p),
    ++        ("verification_uri_complete", ctypes.c_char_p),
    ++        ("expires_in", ctypes.c_int),
     +    ]
     +
     +
    @@ src/test/python/client/test_oauth.py (new)
     +        assert call.type == PQAUTHDATA_PROMPT_OAUTH_DEVICE
     +        assert call.verification_uri.decode() == verification_url
     +        assert call.user_code.decode() == user_code
    ++        assert call.verification_uri_complete is None
    ++        assert call.expires_in == 5
     +
     +    if not success:
     +        # The client should not try to connect again.

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 7 Feb 2025 21:12:34 +0100

> On 7 Feb 2025, at 06:48, Jacob Champion <[email-masked]> wrote:
> 

Great, thanks!  Below are a few quick comments after a first read-through and

> - 0005 warns at configure time if libcurl doesn't have a nonblocking

Is it really enough to do this at build time?  A very small percentage of users
running this will also be building their own libpq so the warning is lost on
them.  That being said, I'm not entirely sure what else we could do (bleeping a
warning every time is clearly not userfriendly) so maybe this is a TODO in the
code?

> that will fail gracefully in production builds as well.

+  oauth_json_set_error(ctx,       /* don't bother translating */
With the project style format for translator comments this should be:

	+  /* translator: xxx */
	+  oauth_json_set_error(ctx,

> you're probably not in the mood for the sound of a hundred BELs.)

Does it make sense to prefix the printing in debug_callback() with some header
stating that the following data is debug output from curl and not postgres?  I
have a feeling I'm stockholm syndromed by knowing the internals so I'm not sure
if that would be helpful to someone not knowing the implementation details.

> those available to devs.)

Aha, cool!  I was a bit surprised to not find a definition of expires_in in RFC
8628, as in what happens if -1 is passed?  8628 seems to broadly speaking fall
into the category of "just dont do the wrong thing and all will be fine" =/.
Another question that comes to mind is how the reciever should interpret the
information since it doesn't know when the device_code/user_code was generated
so it doesn't know how much of expires_in which has already passed.  (Which is
not something for us to solve, just a general observation.)

+   even if they can't use the non-textual method. Review the RFC's
+   <ulink url="https://datatracker.ietf.org/doc/html/rfc8628#section-3.3.1">notes
+   on non-textual verification</ulink>.
To align more with the rest of the documentation I think something along these
lines is better: "For more information, see section 3.3.1 in <ulink ..>RFC
8628</ulink>.

      Assert(cnt == 1);
-     return 1;                       /* don't fall through in release builds */
+     return 0;
While not introduced in this fixup patch, reading it again now I sort of think
we should make that Assert(false) to clearly indicate that we don't think the
assertion will ever pass, we're just asking to error out since we already know
the failure condition holds.

> - 0009 is gold-plating for the OAUTH_STEP_WAIT_INTERVAL state:

+  actx_error(actx, "failed to create timer kqueue: %m");
Maybe we should add a translator note explaining that kqueue should not be
translated since it's very easy to mistake it for "queue".  Doing it on the
first string including kqueue should be enough I suppose.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Feb 2025 17:56:18 -0800

> warning every time is clearly not userfriendly) so maybe this is a TODO in the

I've added a TODO back. At the moment, I don't have any good ideas; if
the user isn't building libpq, they're not going to be able to take
action on the warning anyway, and for many use cases they're probably
not going to care.

>         +  oauth_json_set_error(ctx,

This comment was just meant to draw attention to the lack of
libpq_gettext(). Does it still need a translator note if we don't run
it through translation?

> if that would be helpful to someone not knowing the implementation details.

Seems reasonable; I've added "[libcurl]" to the front.

> into the category of "just dont do the wrong thing and all will be fine" =/.

Yup. :(

> not something for us to solve, just a general observation.)

And even if we passed the Date header value through from the server,
that'd do the wrong thing if the clocks are off. I think for now,
expires_in is likely to be a best-effort UX, in the vein of "hey,
maybe type faster if you don't want a timeout."

> lines is better: "For more information, see section 3.3.1 in <ulink ..>RFC

Done.

> the failure condition holds.

Done.

> first string including kqueue should be enough I suppose.

Done.

--

v48 is attached.

- 0001 contains all of the v47 fixups squashed into v47-0001.
- 0002 contains all the above feedback and rewrites two more commented TODOs.
- 0003 completes a couple of summary paragraphs in the documentation,
and makes it clear that the builtin flow is not currently supported on
Windows.
- 0004 gets a missed pgperltidy and explicitly skips unsupported tests
on Windows.
- 0005 cowardly pulls the MAX_OAUTH_RESPONSE_SIZE down to 256k.

- 0006 gives us additional levers to pull in the event that API or ABI
changes must be backported for security reasons:

Daniel and I talked at FOSDEM about wanting to have additional
guardrails on the server-side validator API. Ideally, we'd wait for
major version boundaries to change APIs, as per usual. But if any bugs
come to light that affect the security of the system, we may want to
have more control over the boundary between the server and the
validator. So I've added two features to the API.

The first is a magic number embedded in the OAuthValidatorCallbacks
struct. Should it ever be necessary to force a recompilation of
validator modules, that number can be bumped in an emergency to allow
the server to reject modules with an older ABI (or otherwise treat
them differently).

The second is state->sversion, added to the ValidatorModuleState
struct, which contains the PG_VERSION_NUM. This currently has no use,
but if there's ever a situation where the ValidatorModule* structs
need to gain new members within a stable release line, this would let
module developers make sense of the situation. It also provides an
easy way for modules to enforce a minimum minor version, for example
if there's a critical security bug in older versions that they'd
rather not deal with.

By adding these fields in addition to the existing module magic
machinery, we've probably doomed them to be unused cruft. But that
seems better than the reverse situation.

Thanks!

--Jacob
 1:  6747b7cc795 !  1:  cf86e3bfbbc Add OAUTHBEARER SASL mechanism
    @@ config/programs.m4: AC_DEFUN([PGAC_CHECK_STRIP],
     +    AC_DEFINE(HAVE_THREADSAFE_CURL_GLOBAL_INIT, 1,
     +              [Define to 1 if curl_global_init() is guaranteed to be threadsafe.])
     +  fi
    ++
    ++  # Warn if a thread-friendly DNS resolver isn't built.
    ++  AC_CACHE_CHECK([for curl support for asynchronous DNS], [pgac_cv__libcurl_async_dns],
    ++  [AC_RUN_IFELSE([AC_LANG_PROGRAM([
    ++#include <curl/curl.h>
    ++],[
    ++    curl_version_info_data *info;
    ++
    ++    if (curl_global_init(CURL_GLOBAL_ALL))
    ++        return -1;
    ++
    ++    info = curl_version_info(CURLVERSION_NOW);
    ++    return (info->features & CURL_VERSION_ASYNCHDNS) ? 0 : 1;
    ++])],
    ++  [pgac_cv__libcurl_async_dns=yes],
    ++  [pgac_cv__libcurl_async_dns=no],
    ++  [pgac_cv__libcurl_async_dns=unknown])])
    ++  if test x"$pgac_cv__libcurl_async_dns" != xyes ; then
    ++    AC_MSG_WARN([
    ++*** The installed version of libcurl does not support asynchronous DNS
    ++*** lookups. Connection timeouts will not be honored during DNS resolution,
    ++*** which may lead to hangs in client programs.])
    ++  fi
     +])# PGAC_CHECK_LIBCURL
     
      ## configure ##
    @@ configure: fi
     +
     +  fi
     +
    ++  # Warn if a thread-friendly DNS resolver isn't built.
    ++  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for curl support for asynchronous DNS" >&5
    ++$as_echo_n "checking for curl support for asynchronous DNS... " >&6; }
    ++if ${pgac_cv__libcurl_async_dns+:} false; then :
    ++  $as_echo_n "(cached) " >&6
    ++else
    ++  if test "$cross_compiling" = yes; then :
    ++  pgac_cv__libcurl_async_dns=unknown
    ++else
    ++  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
    ++/* end confdefs.h.  */
    ++
    ++#include <curl/curl.h>
    ++
    ++int
    ++main ()
    ++{
    ++
    ++    curl_version_info_data *info;
    ++
    ++    if (curl_global_init(CURL_GLOBAL_ALL))
    ++        return -1;
    ++
    ++    info = curl_version_info(CURLVERSION_NOW);
    ++    return (info->features & CURL_VERSION_ASYNCHDNS) ? 0 : 1;
    ++
    ++  ;
    ++  return 0;
    ++}
    ++_ACEOF
    ++if ac_fn_c_try_run "$LINENO"; then :
    ++  pgac_cv__libcurl_async_dns=yes
    ++else
    ++  pgac_cv__libcurl_async_dns=no
    ++fi
    ++rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
    ++  conftest.$ac_objext conftest.beam conftest.$ac_ext
    ++fi
    ++
    ++fi
    ++{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__libcurl_async_dns" >&5
    ++$as_echo "$pgac_cv__libcurl_async_dns" >&6; }
    ++  if test x"$pgac_cv__libcurl_async_dns" != xyes ; then
    ++    { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
    ++*** The installed version of libcurl does not support asynchronous DNS
    ++*** lookups. Connection timeouts will not be honored during DNS resolution,
    ++*** which may lead to hangs in client programs." >&5
    ++$as_echo "$as_me: WARNING:
    ++*** The installed version of libcurl does not support asynchronous DNS
    ++*** lookups. Connection timeouts will not be honored during DNS resolution,
    ++*** which may lead to hangs in client programs." >&2;}
    ++  fi
    ++
     +fi
     +
      if test "$with_gssapi" = yes ; then
    @@ doc/src/sgml/libpq.sgml: void PQinitSSL(int do_ssl);
     +{
     +    const char *verification_uri;   /* verification URI to visit */
     +    const char *user_code;          /* user code to enter */
    ++    const char *verification_uri_complete;  /* optional combination of URI and
    ++                                             * code, or NULL */
    ++    int         expires_in;         /* seconds until user code expires */
     +} PGpromptOAuthDevice;
     +</synopsis>
     +        </para>
    @@ doc/src/sgml/libpq.sgml: void PQinitSSL(int do_ssl);
     +         <link linkend="libpq-oauth-authdata-oauth-bearer-token">custom OAuth
     +         flow</link>, this authdata type will not be used.
     +        </para>
    ++        <para>
    ++         If a non-NULL <structfield>verification_uri_complete</structfield> is
    ++         provided, it may optionally be used for non-textual verification (for
    ++         example, by displaying a QR code). The URL and user code should still
    ++         be displayed to the end user in this case, because the code will be
    ++         manually confirmed by the provider, and the URL lets users continue
    ++         even if they can't use the non-textual method. Review the RFC's
    ++         <ulink url="https://datatracker.ietf.org/doc/html/rfc8628#section-3.3.1">notes
    ++         on non-textual verification</ulink>.
    ++        </para>
     +       </listitem>
     +      </varlistentry>
     +
    @@ meson.build: endif
     +    if libcurl_threadsafe_init
     +      cdata.set('HAVE_THREADSAFE_CURL_GLOBAL_INIT', 1)
     +    endif
    ++
    ++    # Warn if a thread-friendly DNS resolver isn't built.
    ++    libcurl_async_dns = false
    ++
    ++    if not meson.is_cross_build()
    ++      r = cc.run('''
    ++        #include <curl/curl.h>
    ++
    ++        int main(void)
    ++        {
    ++            curl_version_info_data *info;
    ++
    ++            if (curl_global_init(CURL_GLOBAL_ALL))
    ++                return -1;
    ++
    ++            info = curl_version_info(CURLVERSION_NOW);
    ++            return (info->features & CURL_VERSION_ASYNCHDNS) ? 0 : 1;
    ++        }''',
    ++        name: 'test for curl support for asynchronous DNS',
    ++        dependencies: libcurl,
    ++      )
    ++
    ++      assert(r.compiled())
    ++      if r.returncode() == 0
    ++        libcurl_async_dns = true
    ++      endif
    ++    endif
    ++
    ++    if not libcurl_async_dns
    ++      warning('''
    ++*** The installed version of libcurl does not support asynchronous DNS
    ++*** lookups. Connection timeouts will not be honored during DNS resolution,
    ++*** which may lead to hangs in client programs.''')
    ++    endif
     +  endif
     +
     +else
    @@ src/backend/libpq/auth-oauth.c (new)
     +						   char **output, int *outputlen, const char **logdetail);
     +
     +static void load_validator_library(const char *libname);
    -+static void shutdown_validator_library(int code, Datum arg);
    ++static void shutdown_validator_library(void *arg);
     +
     +static ValidatorModuleState *validator_module_state;
     +static const OAuthValidatorCallbacks *ValidatorCallbacks;
    @@ src/backend/libpq/auth-oauth.c (new)
     +load_validator_library(const char *libname)
     +{
     +	OAuthValidatorModuleInit validator_init;
    ++	MemoryContextCallback *mcb;
     +
     +	Assert(libname && *libname);
     +
    @@ src/backend/libpq/auth-oauth.c (new)
     +	if (ValidatorCallbacks->startup_cb != NULL)
     +		ValidatorCallbacks->startup_cb(validator_module_state);
     +
    -+	before_shmem_exit(shutdown_validator_library, 0);
    ++	/* Shut down the library before cleaning up its state. */
    ++	mcb = palloc0(sizeof(*mcb));
    ++	mcb->func = shutdown_validator_library;
    ++
    ++	MemoryContextRegisterResetCallback(CurrentMemoryContext, mcb);
     +}
     +
     +/*
     + * Call the validator module's shutdown callback, if one is provided. This is
    -+ * invoked via before_shmem_exit().
    ++ * invoked during memory context reset.
     + */
     +static void
    -+shutdown_validator_library(int code, Datum arg)
    ++shutdown_validator_library(void *arg)
     +{
     +	if (ValidatorCallbacks->shutdown_cb != NULL)
     +		ValidatorCallbacks->shutdown_cb(validator_module_state);
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	char	   *device_code;
     +	char	   *user_code;
     +	char	   *verification_uri;
    ++	char	   *verification_uri_complete;
    ++	char	   *expires_in_str;
     +	char	   *interval_str;
     +
     +	/* Fields below are parsed from the corresponding string above. */
    ++	int			expires_in;
     +	int			interval;
     +};
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	free(authz->device_code);
     +	free(authz->user_code);
     +	free(authz->verification_uri);
    ++	free(authz->verification_uri_complete);
    ++	free(authz->expires_in_str);
     +	free(authz->interval_str);
     +}
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +{
     +	enum OAuthStep step;		/* where are we in the flow? */
     +
    -+#ifdef HAVE_SYS_EPOLL_H
    -+	int			timerfd;		/* a timerfd for signaling async timeouts */
    -+#endif
    ++	int			timerfd;		/* descriptor for signaling async timeouts */
     +	pgsocket	mux;			/* the multiplexer socket containing all
     +								 * descriptors tracked by libcurl, plus the
     +								 * timerfd */
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +
     +	if (actx->mux != PGINVALID_SOCKET)
     +		close(actx->mux);
    -+#ifdef HAVE_SYS_EPOLL_H
     +	if (actx->timerfd >= 0)
     +		close(actx->timerfd);
    -+#endif
     +
     +	free(actx);
     +}
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		/*
     +		 * We should never start parsing a new field while a previous one is
     +		 * still active.
    -+		 *
    -+		 * TODO: this code relies on assertions too much. We need to exit
    -+		 * sanely on internal logic errors, to avoid turning bugs into
    -+		 * vulnerabilities.
     +		 */
    -+		Assert(!ctx->active);
    ++		if (ctx->active)
    ++		{
    ++			Assert(false);
    ++			oauth_parse_set_error(ctx,
    ++								  "internal error: started field '%s' before field '%s' was finished",
    ++								  name, ctx->active->name);
    ++			return JSON_SEM_ACTION_FAILED;
    ++		}
     +
     +		while (field->name)
     +		{
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	struct oauth_parse *ctx = state;
     +
     +	--ctx->nested;
    -+	if (!ctx->nested)
    -+		Assert(!ctx->active);	/* all fields should be fully processed */
    ++
    ++	/*
    ++	 * All fields should be fully processed by the end of the top-level
    ++	 * object.
    ++	 */
    ++	if (!ctx->nested && ctx->active)
    ++	{
    ++		Assert(false);
    ++		oauth_parse_set_error(ctx,
    ++							  "internal error: field '%s' still active at end of object",
    ++							  ctx->active->name);
    ++		return JSON_SEM_ACTION_FAILED;
    ++	}
     +
     +	return JSON_SUCCESS;
     +}
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	if (ctx->active)
     +	{
     +		/*
    -+		 * This assumes that no target arrays can contain other arrays, which
    -+		 * we check in the array_start callback.
    ++		 * Clear the target (which should be an array inside the top-level
    ++		 * object). For this to be safe, no target arrays can contain other
    ++		 * arrays; we check for that in the array_start callback.
     +		 */
    -+		Assert(ctx->nested == 2);
    -+		Assert(ctx->active->type == JSON_TOKEN_ARRAY_START);
    ++		if (ctx->nested != 2 || ctx->active->type != JSON_TOKEN_ARRAY_START)
    ++		{
    ++			Assert(false);
    ++			oauth_parse_set_error(ctx,
    ++								  "internal error: found unexpected array end while parsing field '%s'",
    ++								  ctx->active->name);
    ++			return JSON_SEM_ACTION_FAILED;
    ++		}
     +
     +		ctx->active = NULL;
     +	}
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +
     +		if (field->type != JSON_TOKEN_ARRAY_START)
     +		{
    -+			Assert(ctx->nested == 1);
    -+			Assert(!*field->target.scalar);
    ++			/* Ensure that we're parsing the top-level keys... */
    ++			if (ctx->nested != 1)
    ++			{
    ++				Assert(false);
    ++				oauth_parse_set_error(ctx,
    ++									  "internal error: scalar target found at nesting level %d",
    ++									  ctx->nested);
    ++				return JSON_SEM_ACTION_FAILED;
    ++			}
    ++
    ++			/* ...and that a result has not already been set. */
    ++			if (*field->target.scalar)
    ++			{
    ++				Assert(false);
    ++				oauth_parse_set_error(ctx,
    ++									  "internal error: scalar field '%s' would be assigned twice",
    ++									  ctx->active->name);
    ++				return JSON_SEM_ACTION_FAILED;
    ++			}
     +
     +			*field->target.scalar = strdup(token);
     +			if (!*field->target.scalar)
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		{
     +			struct curl_slist *temp;
     +
    -+			Assert(ctx->nested == 2);
    ++			/* The target array should be inside the top-level object. */
    ++			if (ctx->nested != 2)
    ++			{
    ++				Assert(false);
    ++				oauth_parse_set_error(ctx,
    ++									  "internal error: array member found at nesting level %d",
    ++									  ctx->nested);
    ++				return JSON_SEM_ACTION_FAILED;
    ++			}
     +
     +			/* Note that curl_slist_append() makes a copy of the token. */
     +			temp = curl_slist_append(*field->target.array, token);
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +}
     +
     +/*
    ++ * Parses a valid JSON number into a double. The input must have come from
    ++ * pg_parse_json(), so that we know the lexer has validated it; there's no
    ++ * in-band signal for invalid formats.
    ++ */
    ++static double
    ++parse_json_number(const char *s)
    ++{
    ++	double		parsed;
    ++	int			cnt;
    ++
    ++	/*
    ++	 * The JSON lexer has already validated the number, which is stricter than
    ++	 * the %f format, so we should be good to use sscanf().
    ++	 */
    ++	cnt = sscanf(s, "%lf", &parsed);
    ++
    ++	if (cnt != 1)
    ++	{
    ++		/*
    ++		 * Either the lexer screwed up or our assumption above isn't true, and
    ++		 * either way a developer needs to take a look.
    ++		 */
    ++		Assert(cnt == 1);
    ++		return 0;
    ++	}
    ++
    ++	return parsed;
    ++}
    ++
    ++/*
     + * Parses the "interval" JSON number, corresponding to the number of seconds to
     + * wait between token endpoint requests.
     + *
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     + * the result at a minimum of one. (Zero-second intervals would result in an
     + * expensive network polling loop.) Tests may remove the lower bound with
     + * PGOAUTHDEBUG, for improved performance.
    -+ *
    -+ * TODO: maybe clamp the upper bound too, based on the libpq timeout and/or the
    -+ * code expiration time?
     + */
     +static int
     +parse_interval(struct async_ctx *actx, const char *interval_str)
     +{
     +	double		parsed;
    -+	int			cnt;
    -+
    -+	/*
    -+	 * The JSON lexer has already validated the number, which is stricter than
    -+	 * the %f format, so we should be good to use sscanf().
    -+	 */
    -+	cnt = sscanf(interval_str, "%lf", &parsed);
    -+
    -+	if (cnt != 1)
    -+	{
    -+		/*
    -+		 * Either the lexer screwed up or our assumption above isn't true, and
    -+		 * either way a developer needs to take a look.
    -+		 */
    -+		Assert(cnt == 1);
    -+		return 1;				/* don't fall through in release builds */
    -+	}
     +
    ++	parsed = parse_json_number(interval_str);
     +	parsed = ceil(parsed);
     +
     +	if (parsed < 1)
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +}
     +
     +/*
    ++ * Parses the "expires_in" JSON number, corresponding to the number of seconds
    ++ * remaining in the lifetime of the device code request.
    ++ *
    ++ * Similar to parse_interval, but we have even fewer requirements for reasonable
    ++ * values since we don't use the expiration time directly (it's passed to the
    ++ * PQAUTHDATA_PROMPT_OAUTH_DEVICE hook, in case the application wants to do
    ++ * something with it). We simply round and clamp to int range.
    ++ */
    ++static int
    ++parse_expires_in(struct async_ctx *actx, const char *expires_in_str)
    ++{
    ++	double		parsed;
    ++
    ++	parsed = parse_json_number(expires_in_str);
    ++	parsed = round(parsed);
    ++
    ++	if (INT_MAX <= parsed)
    ++		return INT_MAX;
    ++	else if (parsed <= INT_MIN)
    ++		return INT_MIN;
    ++
    ++	return parsed;
    ++}
    ++
    ++/*
     + * Parses the Device Authorization Response (RFC 8628, Sec. 3.2).
     + */
     +static bool
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		{"device_code", JSON_TOKEN_STRING, {&authz->device_code}, REQUIRED},
     +		{"user_code", JSON_TOKEN_STRING, {&authz->user_code}, REQUIRED},
     +		{"verification_uri", JSON_TOKEN_STRING, {&authz->verification_uri}, REQUIRED},
    ++		{"expires_in", JSON_TOKEN_NUMBER, {&authz->expires_in_str}, REQUIRED},
     +
     +		/*
     +		 * Some services (Google, Azure) spell verification_uri differently.
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		 */
     +		{"verification_url", JSON_TOKEN_STRING, {&authz->verification_uri}, REQUIRED},
     +
    -+		/*
    -+		 * The following fields are technically REQUIRED, but we don't use
    -+		 * them anywhere yet:
    -+		 *
    -+		 * - expires_in
    -+		 */
    -+
    ++		{"verification_uri_complete", JSON_TOKEN_STRING, {&authz->verification_uri_complete}, OPTIONAL},
     +		{"interval", JSON_TOKEN_NUMBER, {&authz->interval_str}, OPTIONAL},
     +
     +		{0},
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		authz->interval = 5;
     +	}
     +
    ++	Assert(authz->expires_in_str);	/* ensured by parse_oauth_json() */
    ++	authz->expires_in = parse_expires_in(actx, authz->expires_in_str);
    ++
     +	return true;
     +}
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		{"access_token", JSON_TOKEN_STRING, {&tok->access_token}, REQUIRED},
     +		{"token_type", JSON_TOKEN_STRING, {&tok->token_type}, REQUIRED},
     +
    -+		/*
    -+		 * The following fields are technically REQUIRED, but we don't use
    -+		 * them anywhere yet:
    ++		/*---
    ++		 * We currently have no use for the following OPTIONAL fields:
     +		 *
    -+		 * - scope (only required if different than requested -- TODO check)
    ++		 * - expires_in: This will be important for maintaining a token cache,
    ++		 *               but we do not yet implement one.
    ++		 *
    ++		 * - refresh_token: Ditto.
    ++		 *
    ++		 * - scope: This is only sent when the authorization server sees fit to
    ++		 *          change our scope request. It's not clear what we should do
    ++		 *          about this; either it's been done as a matter of policy, or
    ++		 *          the user has explicitly denied part of the authorization,
    ++		 *          and either way the server-side validator is in a better
    ++		 *          place to complain if the change isn't acceptable.
     +		 */
     +
     +		{0},
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     + * select() on instead of the Postgres socket during OAuth negotiation.
     + *
     + * This is just an epoll set or kqueue abstracting multiple other descriptors.
    -+ * A timerfd is always part of the set when using epoll; it's just disabled
    -+ * when we're not using it.
    ++ * For epoll, the timerfd is always part of the set; it's just disabled when
    ++ * we're not using it. For kqueue, the "timerfd" is actually a second kqueue
    ++ * instance which is only added to the set when needed.
     + */
     +static bool
     +setup_multiplexer(struct async_ctx *actx)
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		return false;
     +	}
     +
    ++	/*
    ++	 * Originally, we set EVFILT_TIMER directly on the top-level multiplexer.
    ++	 * This makes it difficult to implement timer_expired(), though, so now we
    ++	 * set EVFILT_TIMER on a separate actx->timerfd, which is chained to
    ++	 * actx->mux while the timer is active.
    ++	 */
    ++	actx->timerfd = kqueue();
    ++	if (actx->timerfd < 0)
    ++	{
    ++		actx_error(actx, "failed to create timer kqueue: %m");
    ++		return false;
    ++	}
    ++
     +	return true;
     +#endif
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +
     +/*
     + * Enables or disables the timer in the multiplexer set. The timeout value is
    -+ * in milliseconds (negative values disable the timer). Rather than continually
    -+ * adding and removing the timer, we keep it in the set at all times and just
    -+ * disarm it when it's not needed.
    ++ * in milliseconds (negative values disable the timer).
    ++ *
    ++ * For epoll, rather than continually adding and removing the timer, we keep it
    ++ * in the set at all times and just disarm it when it's not needed. For kqueue,
    ++ * the timer is removed completely when disabled to prevent stale timeouts from
    ++ * remaining in the queue.
     + */
     +static bool
     +set_timer(struct async_ctx *actx, long timeout)
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		actx_error(actx, "setting timerfd to %ld: %m", timeout);
     +		return false;
     +	}
    ++
    ++	return true;
     +#endif
     +#ifdef HAVE_SYS_EVENT_H
     +	struct kevent ev;
     +
    -+	EV_SET(&ev, 1, EVFILT_TIMER, timeout < 0 ? EV_DELETE : EV_ADD,
    ++	/* Enable/disable the timer itself. */
    ++	EV_SET(&ev, 1, EVFILT_TIMER, timeout < 0 ? EV_DELETE : (EV_ADD | EV_ONESHOT),
     +		   0, timeout, 0);
    -+	if (kevent(actx->mux, &ev, 1, NULL, 0, NULL) < 0 && errno != ENOENT)
    ++	if (kevent(actx->timerfd, &ev, 1, NULL, 0, NULL) < 0 && errno != ENOENT)
     +	{
     +		actx_error(actx, "setting kqueue timer to %ld: %m", timeout);
     +		return false;
     +	}
    -+#endif
    ++
    ++	/*
    ++	 * Add/remove the timer to/from the mux. (In contrast with epoll, if we
    ++	 * allowed the timer to remain registered here after being disabled, the
    ++	 * mux queue would retain any previous stale timeout notifications and
    ++	 * remain readable.)
    ++	 */
    ++	EV_SET(&ev, actx->timerfd, EVFILT_READ, timeout < 0 ? EV_DELETE : EV_ADD,
    ++		   0, 0, 0);
    ++	if (kevent(actx->mux, &ev, 1, NULL, 0, NULL) < 0 && errno != ENOENT)
    ++	{
    ++		actx_error(actx, "could not update timer on kqueue: %m");
    ++		return false;
    ++	}
     +
     +	return true;
    ++#endif
    ++
    ++	actx_error(actx, "libpq does not support timers on this platform");
    ++	return false;
    ++}
    ++
    ++/*
    ++ * Returns 1 if the timeout in the multiplexer set has expired since the last
    ++ * call to set_timer(), 0 if the timer is still running, or -1 (with an
    ++ * actx_error() report) if the timer cannot be queried.
    ++ */
    ++static int
    ++timer_expired(struct async_ctx *actx)
    ++{
    ++#if HAVE_SYS_EPOLL_H
    ++	struct itimerspec spec = {0};
    ++
    ++	if (timerfd_gettime(actx->timerfd, &spec) < 0)
    ++	{
    ++		actx_error(actx, "getting timerfd value: %m");
    ++		return -1;
    ++	}
    ++
    ++	/*
    ++	 * This implementation assumes we're using single-shot timers. If you
    ++	 * change to using intervals, you'll need to reimplement this function
    ++	 * too, possibly with the read() or select() interfaces for timerfd.
    ++	 */
    ++	Assert(spec.it_interval.tv_sec == 0
    ++		   && spec.it_interval.tv_nsec == 0);
    ++
    ++	/* If the remaining time to expiration is zero, we're done. */
    ++	return (spec.it_value.tv_sec == 0
    ++			&& spec.it_value.tv_nsec == 0);
    ++#endif
    ++#ifdef HAVE_SYS_EVENT_H
    ++	int			res;
    ++
    ++	/* Is the timer queue ready? */
    ++	res = PQsocketPoll(actx->timerfd, 1 /* forRead */ , 0, 0);
    ++	if (res < 0)
    ++	{
    ++		actx_error(actx, "checking kqueue for timeout: %m");
    ++		return -1;
    ++	}
    ++
    ++	return (res > 0);
    ++#endif
    ++
    ++	actx_error(actx, "libpq does not support timers on this platform");
    ++	return -1;
     +}
     +
     +/*
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	struct async_ctx *actx = ctx;
     +
     +	/*
    -+	 * TODO: maybe just signal drive_request() to immediately call back in the
    -+	 * (timeout == 0) case?
    ++	 * There might be an optimization opportunity here: if timeout == 0, we
    ++	 * could signal drive_request to immediately call
    ++	 * curl_multi_socket_action, rather than returning all the way up the
    ++	 * stack only to come right back. But it's not clear that the additional
    ++	 * code complexity is worth it.
     +	 */
     +	if (!set_timer(actx, timeout))
     +		return -1;				/* actx_error already called */
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +debug_callback(CURL *handle, curl_infotype type, char *data, size_t size,
     +			   void *clientp)
     +{
    -+	const char *const end = data + size;
     +	const char *prefix;
    ++	bool		printed_prefix = false;
    ++	PQExpBufferData buf;
     +
     +	/* Prefixes are modeled off of the default libcurl debug output. */
     +	switch (type)
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +			return 0;
     +	}
     +
    ++	initPQExpBuffer(&buf);
    ++
     +	/*
     +	 * Split the output into lines for readability; sometimes multiple headers
    -+	 * are included in a single call.
    ++	 * are included in a single call. We also don't allow unprintable ASCII
    ++	 * through without a basic <XX> escape.
     +	 */
    -+	while (data < end)
    ++	for (int i = 0; i < size; i++)
     +	{
    -+		size_t		len = end - data;
    -+		char	   *eol = memchr(data, '\n', len);
    ++		char		c = data[i];
     +
    -+		if (eol)
    -+			len = eol - data + 1;
    ++		if (!printed_prefix)
    ++		{
    ++			appendPQExpBuffer(&buf, "%s ", prefix);
    ++			printed_prefix = true;
    ++		}
     +
    -+		/* TODO: handle unprintables */
    -+		fprintf(stderr, "%s %.*s%s", prefix, (int) len, data,
    -+				eol ? "" : "\n");
    ++		if (c >= 0x20 && c <= 0x7E)
    ++			appendPQExpBufferChar(&buf, c);
    ++		else if ((type == CURLINFO_HEADER_IN
    ++				  || type == CURLINFO_HEADER_OUT
    ++				  || type == CURLINFO_TEXT)
    ++				 && (c == '\r' || c == '\n'))
    ++		{
    ++			/*
    ++			 * Don't bother emitting <0D><0A> for headers and text; it's not
    ++			 * helpful noise.
    ++			 */
    ++		}
    ++		else
    ++			appendPQExpBuffer(&buf, "<%02X>", c);
     +
    -+		data += len;
    ++		if (c == '\n')
    ++		{
    ++			appendPQExpBufferChar(&buf, c);
    ++			printed_prefix = false;
    ++		}
     +	}
     +
    ++	if (printed_prefix)
    ++		appendPQExpBufferChar(&buf, '\n');	/* finish the line */
    ++
    ++	fprintf(stderr, "%s", buf.data);
    ++	termPQExpBuffer(&buf);
     +	return 0;
     +}
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +static bool
     +setup_curl_handles(struct async_ctx *actx)
     +{
    -+	curl_version_info_data *curl_info;
    -+
     +	/*
     +	 * Create our multi handle. This encapsulates the entire conversation with
     +	 * libcurl for this connection.
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	}
     +
     +	/*
    -+	 * Extract information about the libcurl we are linked against.
    -+	 */
    -+	curl_info = curl_version_info(CURLVERSION_NOW);
    -+
    -+	/*
     +	 * The multi handle tells us what to wait on using two callbacks. These
     +	 * will manipulate actx->mux as needed.
     +	 */
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	 * Multi-threaded applications must set CURLOPT_NOSIGNAL. This requires us
     +	 * to handle the possibility of SIGPIPE ourselves using pq_block_sigpipe;
     +	 * see pg_fe_run_oauth_flow().
    ++	 *
    ++	 * NB: If libcurl is not built against a friendly DNS resolver (c-ares or
    ++	 * threaded), setting this option prevents DNS lookups from timing out
    ++	 * correctly. We warn about this situation at configure time.
     +	 */
     +	CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L, return false);
    -+	if (!curl_info->ares_num)
    -+	{
    -+		/* No alternative resolver, TODO: warn about timeouts */
    -+	}
     +
     +	if (actx->debugging)
     +	{
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		CHECK_SETOPT(actx, popt, protos, return false);
     +	}
     +
    -+	/* TODO: would anyone use this in "real" situations, or just testing? */
    ++	/*
    ++	 * If we're in debug mode, allow the developer to change the trusted CA
    ++	 * list. For now, this is not something we expose outside of the UNSAFE
    ++	 * mode, because it's not clear that it's useful in production: both libpq
    ++	 * and the user's browser must trust the same authorization servers for
    ++	 * the flow to work at all, so any changes to the roots are likely to be
    ++	 * done system-wide.
    ++	 */
     +	if (actx->debugging)
     +	{
     +		const char *env;
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	 *    of the authorization server where the authorization request was
     +	 *    sent to. This comparison MUST use simple string comparison as defined
     +	 *    in Section 6.2.1 of [RFC3986].
    -+	 *
    -+	 * TODO: Encoding support?
     +	 */
     +	if (strcmp(conn->oauth_issuer_id, provider->issuer) != 0)
     +	{
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +	PGpromptOAuthDevice prompt = {
     +		.verification_uri = actx->authz.verification_uri,
     +		.user_code = actx->authz.user_code,
    -+		/* TODO: optional fields */
    ++		.verification_uri_complete = actx->authz.verification_uri_complete,
    ++		.expires_in = actx->authz.expires_in,
     +	};
     +
     +	res = PQauthDataHook(PQAUTHDATA_PROMPT_OAUTH_DEVICE, conn, &prompt);
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +		}
     +
     +		actx->mux = PGINVALID_SOCKET;
    -+#ifdef HAVE_SYS_EPOLL_H
     +		actx->timerfd = -1;
    -+#endif
     +
     +		/* Should we enable unsafe features? */
     +		actx->debugging = oauth_unsafe_debugging_enabled();
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +						/* not done yet */
     +						return status;
     +					}
    ++
    ++					break;
     +				}
     +
     +			case OAUTH_STEP_WAIT_INTERVAL:
    -+				/* TODO check that the timer has expired */
    ++
    ++				/*
    ++				 * The client application is supposed to wait until our timer
    ++				 * expires before calling PQconnectPoll() again, but that
    ++				 * might not happen. To avoid sending a token request early,
    ++				 * check the timer before continuing.
    ++				 */
    ++				if (!timer_expired(actx))
    ++				{
    ++					conn->altsock = actx->timerfd;
    ++					return PGRES_POLLING_READING;
    ++				}
    ++
    ++				/* Disable the expired timer. */
    ++				if (!set_timer(actx, -1))
    ++					goto error_return;
    ++
     +				break;
     +		}
     +
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c (new)
     +				if (!set_timer(actx, actx->authz.interval * 1000))
     +					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.)
    ++				 * multiplexer.
     +				 */
     +				conn->altsock = actx->timerfd;
    -+#endif
     +
     +				actx->step = OAUTH_STEP_WAIT_INTERVAL;
     +				actx->running = 1;
    @@ src/interfaces/libpq/fe-auth-oauth.c (new)
     +{
     +	struct json_ctx *ctx = state;
     +
    ++	/* Only top-level keys are considered. */
     +	if (ctx->nested == 1)
     +	{
     +		if (strcmp(name, ERROR_STATUS_FIELD) == 0)
    @@ src/interfaces/libpq/fe-auth-oauth.c (new)
     +
     +	if (ctx->target_field)
     +	{
    -+		Assert(ctx->nested == 1);
    ++		if (ctx->nested != 1)
    ++		{
    ++			/*
    ++			 * ctx->target_field should not have been set for nested keys.
    ++			 * Assert and don't continue any further for production builds.
    ++			 */
    ++			Assert(false);
    ++			oauth_json_set_error(ctx,	/* don't bother translating */
    ++								 "internal error: target scalar found at nesting level %d during OAUTHBEARER parsing",
    ++								 ctx->nested);
    ++			return JSON_SEM_ACTION_FAILED;
    ++		}
     +
     +		/*
     +		 * We don't allow duplicate field names; error out if the target has
    @@ src/interfaces/libpq/libpq-fe.h: extern int	PQenv2encoding(void);
     +{
     +	const char *verification_uri;	/* verification URI to visit */
     +	const char *user_code;		/* user code to enter */
    ++	const char *verification_uri_complete;	/* optional combination of URI and
    ++											 * code, or NULL */
    ++	int			expires_in;		/* seconds until user code expires */
     +} PGpromptOAuthDevice;
     +
     +/* for PGoauthBearerRequest.async() */
    @@ src/test/modules/oauth_validator/oauth_hook_client.c (new)
     +	printf(" --expected-uri URI		fail if received configuration link does not match URI\n");
     +	printf(" --misbehave=MODE		have the hook fail required postconditions\n"
     +		   "						(MODEs: no-hook, fail-async, no-token, no-socket)\n");
    -+	printf(" --no-hook				don't install OAuth hooks (connection will fail)\n");
    ++	printf(" --no-hook				don't install OAuth hooks\n");
     +	printf(" --hang-forever			don't ever return a token (combine with connect_timeout)\n");
     +	printf(" --token TOKEN			use the provided TOKEN value\n");
    ++	printf(" --stress-async			busy-loop on PQconnectPoll rather than polling\n");
     +}
     +
     +/* --options */
     +static bool no_hook = false;
     +static bool hang_forever = false;
    ++static bool stress_async = false;
     +static const char *expected_uri = NULL;
     +static const char *expected_scope = NULL;
     +static const char *misbehave_mode = NULL;
    @@ src/test/modules/oauth_validator/oauth_hook_client.c (new)
     +		{"token", required_argument, NULL, 1003},
     +		{"hang-forever", no_argument, NULL, 1004},
     +		{"misbehave", required_argument, NULL, 1005},
    ++		{"stress-async", no_argument, NULL, 1006},
     +		{0}
     +	};
     +
    @@ src/test/modules/oauth_validator/oauth_hook_client.c (new)
     +				misbehave_mode = optarg;
     +				break;
     +
    ++			case 1006:			/* --stress-async */
    ++				stress_async = true;
    ++				break;
    ++
     +			default:
     +				usage(argv);
     +				return 1;
    @@ src/test/modules/oauth_validator/oauth_hook_client.c (new)
     +	PQsetAuthDataHook(handle_auth_data);
     +
     +	/* Connect. (All the actual work is in the hook.) */
    -+	conn = PQconnectdb(conninfo);
    ++	if (stress_async)
    ++	{
    ++		/*
    ++		 * Perform an asynchronous connection, busy-looping on PQconnectPoll()
    ++		 * without actually waiting on socket events. This stresses code paths
    ++		 * that rely on asynchronous work to be done before continuing with
    ++		 * the next step in the flow.
    ++		 */
    ++		PostgresPollingStatusType res;
    ++
    ++		conn = PQconnectStart(conninfo);
    ++
    ++		do
    ++		{
    ++			res = PQconnectPoll(conn);
    ++		} while (res != PGRES_POLLING_FAILED && res != PGRES_POLLING_OK);
    ++	}
    ++	else
    ++	{
    ++		/* Perform a standard synchronous connection. */
    ++		conn = PQconnectdb(conninfo);
    ++	}
    ++
     +	if (PQstatus(conn) != CONNECTION_OK)
     +	{
    -+		fprintf(stderr, "Connection to database failed: %s\n",
    ++		fprintf(stderr, "connection to database failed: %s\n",
     +				PQerrorMessage(conn));
     +		PQfinish(conn);
     +		return 1;
    @@ src/test/modules/oauth_validator/t/001_server.pl (new)
     +	  qr/failed to obtain access token: mutual TLS required for client \(invalid_client\)/
     +);
     +
    ++# Stress test: make sure our builtin flow operates correctly even if the client
    ++# application isn't respecting PGRES_POLLING_READING/WRITING signals returned
    ++# from PQconnectPoll().
    ++$base_connstr =
    ++  "$common_connstr port=" . $node->port . " host=" . $node->host;
    ++my @cmd = (
    ++	"oauth_hook_client", "--no-hook", "--stress-async",
    ++	connstr(stage => 'all', retries => 1, interval => 1));
    ++
    ++note "running '" . join("' '", @cmd) . "'";
    ++my ($stdout, $stderr) = run_command(\@cmd);
    ++
    ++like($stdout, qr/connection succeeded/, "stress-async: stdout matches");
    ++unlike($stderr, qr/connection to database failed/, "stress-async: stderr matches");
    ++
     +#
     +# This section of tests reconfigures the validator module itself, rather than
     +# the OAuth server.
    @@ src/test/modules/oauth_validator/t/oauth_server.py (new)
     +            "device_code": "postgres",
     +            "user_code": "postgresuser",
     +            self._uri_spelling: uri,
    -+            "expires-in": 5,
    ++            "expires_in": 5,
     +            **self._response_padding,
     +        }
     +
    @@ src/test/modules/oauth_validator/validator.c (new)
     +{
     +	/* Check to make sure our private state still exists. */
     +	if (state->private_data != PRIVATE_COOKIE)
    -+		elog(ERROR, "oauth_validator: private state cookie changed to %p in shutdown",
    ++		elog(PANIC, "oauth_validator: private state cookie changed to %p in shutdown",
     +			 state->private_data);
     +}
     +
 2:  483129c1ca9 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 3:  75d98784ded <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 4:  fd60ceb4c84 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 5:  595362ef2c1 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 6:  f73c042adc9 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 7:  298839b69f0 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 8:  1cf48a8f835 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 9:  27135876559 <  -:  ----------- fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  2:  9171989a75e fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  3:  1bd03e1de10 fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  4:  0929bfbc5fc fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  5:  be882ef6eae fixup! Add OAUTHBEARER SASL mechanism
 -:  ----------- >  6:  954341052b4 fixup! Add OAUTHBEARER SASL mechanism
10:  d8c1f298080 =  7:  d88a2938e7e XXX fix libcurl link error
11:  dbf305d0489 !  8:  44e5cbc8ad1 DO NOT MERGE: Add pytest suite for OAuth
    @@ src/test/python/client/test_oauth.py (new)
     +            id="invalid request without description",
     +        ),
     +        pytest.param(
    -+            (400, {"error": "invalid_request", "padding": "x" * 1024 * 1024}),
    ++            (400, {"error": "invalid_request", "padding": "x" * 256 * 1024}),
     +            r"failed to obtain device authorization: response is too large",
     +            id="gigantic authz response",
     +        ),
    @@ src/test/python/client/test_oauth.py (new)
     +            id="access denied without description",
     +        ),
     +        pytest.param(
    -+            (400, {"error": "access_denied", "padding": "x" * 1024 * 1024}),
    ++            (400, {"error": "access_denied", "padding": "x" * 256 * 1024}),
     +            r"failed to obtain access token: response is too large",
     +            id="gigantic token response",
     +        ),
    @@ src/test/python/client/test_oauth.py (new)
     +                        "urn:ietf:params:oauth:grant-type:device_code"
     +                    ],
     +                    "device_authorization_endpoint": "https://256.256.256.256/dev",
    -+                    "filler": "x" * 1024 * 1024,
    ++                    "filler": "x" * 256 * 1024,
     +                },
     +            ),
     +            r"failed to fetch OpenID discovery document: response is too large",
    @@ src/test/python/server/oauthtest.c (new)
     +
     +static void test_startup(ValidatorModuleState *state);
     +static void test_shutdown(ValidatorModuleState *state);
    -+static ValidatorModuleResult *test_validate(ValidatorModuleState *state,
    -+											const char *token,
    -+											const char *role);
    ++static bool test_validate(const ValidatorModuleState *state,
    ++						  const char *token,
    ++						  const char *role,
    ++						  ValidatorModuleResult *result);
     +
     +static const OAuthValidatorCallbacks callbacks = {
    ++	PG_OAUTH_VALIDATOR_MAGIC,
    ++
     +	.startup_cb = test_startup,
     +	.shutdown_cb = test_shutdown,
     +	.validate_cb = test_validate,
    @@ src/test/python/server/oauthtest.c (new)
     +{
     +}
     +
    -+static ValidatorModuleResult *
    -+test_validate(ValidatorModuleState *state, const char *token, const char *role)
    ++static bool
    ++test_validate(const ValidatorModuleState *state,
    ++			  const char *token, const char *role,
    ++			  ValidatorModuleResult *res)
     +{
    -+	ValidatorModuleResult *res;
    -+
    -+	res = palloc0(sizeof(ValidatorModuleResult));	/* TODO: palloc context? */
    -+
     +	if (reflect_role)
     +	{
     +		res->authorized = true;
    -+		res->authn_id = pstrdup(role);	/* TODO: constify? */
    ++		res->authn_id = pstrdup(role);
     +	}
     +	else
     +	{
     +		if (*expected_bearer && strcmp(token, expected_bearer) == 0)
     +			res->authorized = true;
     +		if (set_authn_id)
    -+			res->authn_id = authn_id;
    ++			res->authn_id = pstrdup(authn_id);
     +	}
     +
    -+	return res;
    ++	return true;
     +}
     
      ## src/test/python/server/test_oauth.py (new) ##

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 12 Feb 2025 15:55:23 +0100

On 08.02.25 02:56, Jacob Champion wrote:
> On Fri, Feb 7, 2025 at 12:12 PM Daniel Gustafsson<[email-masked]> wrote:

This just depends on how people have built their libcurl, right?

Do we have any information whether the async-dns-free build is a common 
configuration?

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 12 Feb 2025 15:59:11 +0100

> libpq_gettext(). Does it still need a translator note if we don't run

No, that wouldn't have any effect.

I think you can just remove that comment.  It's pretty established that 
internal errors don't need translation, so it would be understood from 
looking at the code.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 13 Feb 2025 13:23:01 -0800

> Do we have any information whether the async-dns-free build is a common

I don't think the annual Curl survey covers that, unfortunately.

> internal errors don't need translation, so it would be understood from

Okay, will do.

Thanks,
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 13 Feb 2025 23:56:29 +0100

> On 8 Feb 2025, at 02:56, Jacob Champion <[email-masked]> wrote:

Thanks for the new version!

> - 0004 gets a missed pgperltidy and explicitly skips unsupported tests

+if ($Config{osname} eq 'MSWin32')
We can get away with nog importing Config at all since Test::Utils export a
symbol for this already in $windows_os.  Fixed in my attached.

> validator. So I've added two features to the API.

I think what you added there is quite sufficient for handling the worst case
that ideally should never happen.  Even though I can't see us breaking this
given the code being trivial, I also don't feel like realizing after the fact
when we need it that it was subtly broken, so I added a test validator which
use the wrong ABI version.

I have now read the entire patch cover-to-cover twice to try and catch any
rough or sharp edges.  Unsurprisingly given the number of revisions this patch
has gone through, and the number of hours that have been put into it, there
isn't much to be found.  Most of my findings below are well and truly in the
nit- pickery category (and my favourite, paranoia-induced defensive
programming).  There are no architectural flaws that I can detect, and cross-
referencing with the RFC's I don't see anything mixed up in spec compliance.

To make it easier for you to see what I mean I have implemented most of the
comments and attached as a fixup patch, from which you can cherry-pick hunks
you agree with.  Those I didn't implement should be marked as such below.

As we discussed off-list I took the liberty of squashing the previous fixup
patches into a single one, and squashed your fixes for my comments against v47
into 0001.  All of my proposals are in 0004.

Some comments:

+        The system which hosts the protected resources which are
The repetition of "which ..  which" reads a bit off to me, I propose to
simplify as "The system hosting then.." instead.

+        The organization, product vendor, or other entity which develops and/or
+        administers the OAuth servers and clients for a given application.
Since we define terminology here, shouldn't this be "OAuth resource servers"?

+        <productname>PostgreSQL</productname> does not provide an authorization
+        server; it's obtained from the OAuth provider.
The "obtained from" part makes it sound like you need to get some server
software to run this with PostgreSQL.  How about "; it is the responsibility
of.."?

+         <xref linkend="libpq-connect-oauth-issuer"/> setting. No variations in
+         case or format are permitted.
While not wrong in any way, I think it would be clearer to write "formatting"
here since that's really what we are talking about no?

+         Build with libcurl support for OAuth 2.0 client flows.
+         This requires the <productname>curl</productname> package to be
+         installed.  Building with this will check for the required header files
I don't think we need to document that curl need to be installed since it's
likely already on the system of anyone reading this.  I do however think we
should state the minimum required version.

+        setting in <link linkend="auth-oauth">the server's HBA configuration.</link>
Nitpickery: I prefer to have the period outside the <link> markup.

+        the connection will fail. This is required to prevent a class of "mix-up
+        attacks" on OAuth clients.
Since mix-up attacks aren't very well documented I think we should aid the
readers by linking to the OAuth WG announcement of this class of attacks.  From
there readers can find the original paper but I think linking directly to that
is less helpful than the mailinglist post.

+   running a client via SSH. Client applications may implement their own flows
For consistency with the rest of the docs we should wrap SSH with an
<application> tag.

+   You will then log into your OAuth provider, which will ask whether you want
A think third person here reads more like the rest of the docs.

+        which <application>libpq</application> will call when when action is
"when when", I think this should really be "when an"?

+       sprays HTTP traffic (containing several critical secrets) to standard
+       error during the OAuth flow
This might not be readily understandable by non-native speakers.

+   Similarly, if you are using Curl inside your application,
Should use <productname> markup.

+   more recent versions of Curl that are built to support threadsafe
s/threadsafe/thread-safe/g for documentation consistency (ditto in other places
where used as a adjective and not code identifier).

+  itself; validator modules provide the glue between the server and the OAuth
s/glue/integration layer/ to avoid confusing readers not used to english idioms.

+   Since a misbehaving validator might let unauthorized users into the database,
+   correct implementation is critical. See
"Don't make any bugs" isn't very helpful advice =) Expanded on it slightly.

+   An OAuth validator module is loaded by dynamically loading one of the shared
The double use of load in "loaded .. loading", rewording to try and simplify.

+    The server has ensured that the token is well-formed syntactically, but no
"server" is an overloaded nomenclature here, perhaps using libpq instead to
clearly indicate that it's postgres and not an OAuth server.

+    The connection will only proceed if the module sets
+    <structfield>authorized</structfield> to <literal>true</literal>.  To
In other places we use the structure name as well and not just the member,
adding that here to be consistent.

+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
Off-by-one

+ * - RFC 7628: https://tools.ietf.org/html/rfc7628
Replacing all tools.ietf.org mentions with datatracker.ietf.org to save a few
redirects.

+                       p++;
+                       if (*p != ',')
If the SASL exchange, are we certain that a rogue client cannot inject a
message which trips us past the end of string?  Should we be doublecheck when
advancing p across the message?

+sanitize_char(char c)
+{
+       static char buf[5];
With the multithreading work on the horizon we should probably avoid static
variables like these to not create work for our future selves?  The code isn't
as neat when passing in a buffer/length but it avoids the need for a static or
threadlocal variable. Or am I overthinking this?

+       initStringInfo(&issuer);
+       appendStringInfoString(&issuer, ctx->issuer);
The double StringInfoData variables in generate_error_response to be able to
JSON escape the issuer is a bit of an eye-sore, a version of
escape_json_with_len which also took an offset into the buffer on where to
start maybe?  Nothing that is urgent to address now (and I have not changed
anything here), but I'll keep it at the back of my head.

+       ereport(LOG, errmsg("internal error in OAuth validator module"));
I wonder if this should be using WARNING instead?  It's really something that
should trigger alarm bells going off.  I've also added an errcode for easier
fleet analysis.

In load_validator_library we don't explicitly verify that the required callback
is defined in the returned structure, which seems like a cheap enough belts and
suspenders level check.

+       if (parsed < 1)
+               return actx->debugging ? 0 : 1;
Is 1 second a sane lower bound on interval for all situations?  I'm starting to
wonder if we should be more conservative here, or even make it configurable in
some way? The default if not set of 5 seconds is quite a lot higher than 1.

+       if (INT_MAX <= parsed)
I think it's closer to project to style to keep the variable on the left side
in such comparisons, so changed these.

+       parsed = parse_json_number(expires_in_str);
+       parsed = round(parsed);
Shouldn't we floor() the value here to ensure we never report an expiration
time longer than the actual expiration?

+       * Some services (Google, Azure) spell verification_uri differently.
I did another round of documentation reading and couldn't find any provider
which also use "verification_url_complete".  However, since it looks so similar
to verification_url it seems worthwhile to add a comment to save readers from
the same rabbithole.

register_socket() doesn't have an error catch for the case when neither epoll
nor kqeue is supported.  Shouldn't it set actx_error() here as well?  (Not done
in my review patch.)

+       if (actx->curl_err[0])
+       {
+               size_t          len;
+
+               appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err);
Should this also qualify that the error comes from outside of postgres?
Something like "(libcurl:%s)" to match?  I haven't changed this in the attached
since I'm still on the fence, but I'm leanings that we probably should.
Thoughts?

-   * We only support one mechanism at the moment, so rather than deal with a
+   * We only support two mechanisms at the moment, so rather than deal with a
While there's nothing incorrect about this comment, I have a feeling we won't
support more mechanisms than we can justify having a simple array for anytime
soon =)

Sorry for the wall of text.

In general, I feel that this is getting very close to its final form wrt being
a committable patch, and assuming we don't find anything structurally unsound
in the coming days I don't see a blocker for getting this into v18 before the
final commitfest.  If anyone disagrees with this I'd love for that be brought
up.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 14 Feb 2025 00:01:07 +0100

> On 13 Feb 2025, at 22:23, Jacob Champion <[email-masked]> wrote:
> 

We should be able to get a decent idea by inspecting the packaging scripts for
the major distributions I think.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 14 Feb 2025 17:14:16 -0800

> into 0001.  All of my proposals are in 0004.

Great! I have attached v50; 0001 has almost all of v49 squashed in,
0002 has my changes on top (includes a pgindent/pgperltidy), and 0003
holds the only part I don't like (see below). 0004 contains the
FreeBSD hack that I suspect we'll need to merge in until Cirrus images
are updated.

> Since we define terminology here, shouldn't this be "OAuth resource servers"?

The resource server is Postgres for our purposes; I've changed it to
"authorization servers".

> "Don't make any bugs" isn't very helpful advice =) Expanded on it slightly.

Hmm, I think the overloading of "validate" in the replacement text
could be confusing. I guess my point is less "don't write bugs" and
more "a bug here has extreme impact"? I've taken another shot at it;
see what you think.

> clearly indicate that it's postgres and not an OAuth server.

I've replaced this with "PostgreSQL" to match up with Peter's earlier
feedback (we were using "libpq" to describe the backend and he wanted
to avoid that).

> threadlocal variable. Or am I overthinking this?

This is the only part of the feedback patch that I'm not a fan of,
mostly because it begins to diverge heavily from the SCRAM code it
copied from. I don't disagree with the goal of getting rid of the
static buffers, but I would like to see them modified at the same time
so that we can refactor easily if/when a third SASL mechanism shows
up. (Maybe with a psprintf() rather than buffers?)

> advancing p across the message?

The existing != checks will bail out if they get to the end of the
string. It relies on byte-at-a-time advancement for safety, as well as
the SASL code higher in the stack that ensures that the input buffer
is always null terminated. (SCRAM relies on that too.) If we ever
jumped farther than a byte, we'd need stronger checks, but at the
moment I don't think this change helps us.

> is defined in the returned structure, which seems like a cheap enough belts and

Yeah, there's a later check at time of use, but it's not as
user-friendly. I've adjusted the new error message to make it a bit
closer to the logical plugin wording.

> some way? The default if not set of 5 seconds is quite a lot higher than 1.

Mmm, maybe it should be made configurable, but one second seems like a
long time from a CPU perspective. Maybe it would be applicable to
embedded clients? But only if some provider out there actually starts
using smaller intervals than their clients can stand... Should we wait
to hear from someone who is interested in configuring it?

> time longer than the actual expiration?

Sounds reasonable. Done in 0002.

Done.

> since I'm still on the fence, but I'm leanings that we probably should.

Done. More context is probably better than less here.

> support more mechanisms than we can justify having a simple array for anytime

Yeah. My goal was mostly to justify the use of the (unusual)
static-length list to future readers.

Thank you so much for the reviews!

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 17 Feb 2025 13:03:36 +0100

> On 15 Feb 2025, at 02:14, Jacob Champion <[email-masked]> wrote:

> more "a bug here has extreme impact"? I've taken another shot at it;

I'm not sure we're at the right wording still.  I have a feeling this topic is
worth a longer paragraph describing the potential severity of various error
conditions, but I don't think that needs to go in now, we can iterate on that
over time as well.

> feedback (we were using "libpq" to describe the backend and he wanted

Ah yes, much better.

> up. (Maybe with a psprintf() rather than buffers?)

Fair enough, I can get behind that.

> moment I don't think this change helps us.

Thanks for clarifying.

> closer to the logical plugin wording.

-    errmsg("%s module \"%s\" must define the symbol %s",
+    errmsg("%s module \"%s\" must provide a %s callback",

My rationale for picking the former message was that it's same as we have
earlier in the file, so it didn't add more translator work for (ideally) rarely
used errors.

That being said, I agree that should probably align these messages with the
counterparts for archive modules and logical plugins, which currently use the
following:

    errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")
    errmsg("archive modules must register an archive callback")
    elog(ERROR, "output plugins have to declare the _PG_output_plugin_init symbol");
    elog(ERROR, "output plugins have to register a begin callback");

It's a bit surprising to me that we use elog() for output plugins, while these
errors should be rare they can be triggered by third-party code so it seems
more appropriate to use ereport() IMHO.  Given that these are so similar we
should be able to reduce translator burden by providing more or less just two
messages.

Since this will be reaching into other parts of the code, it should be its own
patch though, so for now let's go with what you proposed and we can revisit this.

> to hear from someone who is interested in configuring it?

I indeed think we should await feedback, making it configurable isn't exactly
free so I hesitate to do it if nobody wants it.

The attached v51 squashes your commits together, discarding the changes
discussed here, and takes a stab at a commit message for these as this is
getting very close to be able to go in.  There are no additional changes.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 17 Feb 2025 10:15:27 -0800

> getting very close to be able to go in.  There are no additional changes.

Awesome, thank you! It's been a little bit since I've re-run my
fuzzers, and a new Valgrind run would be a good idea, so I will just
keep throwing tests at it and review the new commit messages while
they run.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 17 Feb 2025 15:51:23 -0800

On Mon, Feb 17, 2025 at 10:15 AM Jacob Champion
<[email-masked]> wrote:
> It's been a little bit since I've re-run my
> keep throwing tests at it

Fuzzers are happy so far.

Valgrind did find something! A mistake I made during parameter
discovery: setup_oauth_parameters() ensures that conn->oauth_issuer_id
is always set using the "issuer" connection option, but during the
second connection, I reassigned the pointer for it (and
conn->oauth_discovery_uri) and leaked the previous allocations.

v52-0002 fixes that. I've taken the opportunity to document that those
two parameters are designed to be unchangeable for the connection once
they've been assigned.

--

Reviews for the commit message:

> postgres cannot ship with one built-in.

s/postgres/Postgres/. Maybe a softening to "does not" ship with one?

> Each pg_hba entry can
> specify one, or more, validators or be left blank for the validator
> installed as default.

Each pg_hba entry can specify only one of the DBA-blessed validators, not more.

> This adds a requirement on libucurl

s/libucurl/libcurl/

And as discussed offlist, we should note that the builtin device flow
is not currently supported on Windows.

Thanks!
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 19 Feb 2025 15:13:35 +0100

> On 18 Feb 2025, at 00:51, Jacob Champion <[email-masked]> wrote:
> 

Nice.

> Reviews for the commit message:

All proposed changes applied.

The attached rebased has your 0002 fix as well as some minor tweaks like a few
small whitespace changes from a pgperltidy run and a copyright date fix which
still said 2024.

Unless something shows up I plan to commit this sometime tomorrow to allow it
ample time in the tree before the freeze.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Wed, 19 Feb 2025 11:54:24 -0800

> small whitespace changes from a pgperltidy run and a copyright date fix which

LGTM.

Thanks!
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 20 Feb 2025 17:29:58 +0100

> On 19 Feb 2025, at 15:13, Daniel Gustafsson <[email-masked]> wrote:

> ample time in the tree before the freeze.

I spent a few more hours staring at this, and ran it through a number of CI and
local builds, without anything showing up.  Pushed to master with the first set
of buildfarm animals showing green builds.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 20 Feb 2025 09:28:36 -0800

> of buildfarm animals showing green builds.

Thank you!! And _huge thanks_ to everyone who's reviewed and provided
feedback. I'm going to start working with Andrew on getting the new
tests going in the buildfarm.

If you've been reading along and would like to get started with OAuth
validators and flows, but don't know where to start, please reach out.
The proof of the new APIs will be in the using, and the best time to
tell me if you hate those APIs is now :D

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Thu, 20 Feb 2025 15:07:21 -0500

> of buildfarm animals showing green builds.

After doing an in-tree "make check", I see

$ git status
On branch master
Your branch is up to date with 'origin/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        src/test/modules/oauth_validator/oauth_hook_client

Looks like we're short a .gitignore entry.  (It does appear that
"make clean" cleans it up, at least.)

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Thu, 20 Feb 2025 12:21:45 -0800

> "make clean" cleans it up, at least.)

So we are! Sorry about that. The attached patch gets in-tree builds
clean for me again.

Thanks,
--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 20 Feb 2025 21:37:13 +0100

> On 20 Feb 2025, at 21:21, Jacob Champion <[email-masked]> wrote:
> 

Fixed, thanks for the report!

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Fri, 21 Feb 2025 12:18:57 -0500

Hi,

> tests going in the buildfarm.

+1

One question about interruptability. The docs say:

+    <varlistentry>
+     <term>Interruptibility</term>
+     <listitem>
+      <para>
+       Modules must remain interruptible by signals so that the server can
+       correctly handle authentication timeouts and shutdown signals from
+       <application>pg_ctl</application>. For example, a module receiving
+       <symbol>EINTR</symbol>/<symbol>EAGAIN</symbol> from a blocking call
+       should call <function>CHECK_FOR_INTERRUPTS()</function> before retrying.
+       The same should be done during any long-running loops. Failure to follow
+       this guidance may result in unresponsive backend sessions.
+      </para>
+     </listitem>
+    </varlistentry>

Is EINTR checking really sufficient?

I don't think we can generally rely on all blocking system calls to be
interruptible by signals on all platforms?

And, probably worse, isn't relying on getting EINTR rather racy, due to the
chance of the signal arriving between CHECK_FOR_INTERRUPTS() and the blocking
system call?

Afaict the only real way to do safely across platforms is to never call
blocking functions, e.g. by using non-blocking sockets and waiting for
readiness using latches.

And a second one about supporting !CURL_VERSION_ASYNCHDNS:

Is it a good idea to support that? We e.g. rely on libpq connections made by
the backend to be interruptible. Admittedly that's, I think, already not
bulletproof, due to libpq's DNS lookups going through libc if connection
string contains a name that needs to be looked up, but this seems to make that
a bit worse? With the connection string the DNS lookups can at least be
avoided, not that I think we properly document that...

Greetings,

Andres

---

From: Tom Lane <[email-masked]>
Date: Sun, 23 Feb 2025 11:49:21 -0500

> of buildfarm animals showing green builds.

Coverity has a nit-pick about this:

/srv/coverity/git/pgsql-git/postgresql/src/interfaces/libpq/fe-auth-oauth.c: 784 in setup_token_request()
778     		if (!request_copy)
779     		{
780     			libpq_append_conn_error(conn, "out of memory");
781     			goto fail;
782     		}
783     
>>>     CID 1643156:  High impact quality  (WRITE_CONST_FIELD)
>>>     A write to an aggregate overwrites a const-qualified field within the aggregate.
784     		memcpy(request_copy, &request, sizeof(request));
785     
786     		conn->async_auth = run_user_oauth_flow;
787     		conn->cleanup_async_auth = cleanup_user_oauth_flow;
788     		state->async_ctx = request_copy;
789     	}

This is evidently because of the fields declared const:

	/* Hook inputs (constant across all calls) */
	const char *const openid_configuration; /* OIDC discovery URI */
	const char *const scope;	/* required scope(s), or NULL */

IMO, the set of cases where it's legitimate to mark individual struct
fields as const is negligibly small, and this doesn't seem to be one
of them.  It's not obvious to me where/how PGoauthBearerRequest
structs are supposed to be constructed, but I find it hard to believe
that they will all spring full-grown from the forehead of Zeus.
Nonetheless, this declaration requires exactly that.

(I'm kind of surprised that we're not getting similar bleats from
any buildfarm animals, but so far I don't see any.)

BTW, as another nitpicky style matter: why do PGoauthBearerRequest
etc. spell their struct tag names differently from their typedef names
(that is, with/without an underscore)?  That is not our project style
anywhere else, and I'm failing to detect a good reason to do it here.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Sun, 23 Feb 2025 18:08:37 +0100

> On 23 Feb 2025, at 17:49, Tom Lane <[email-masked]> wrote:
> 
> of them.

Thanks for the report, will fix.

> anywhere else, and I'm failing to detect a good reason to do it here.

Indeed it isn't, the only explanation is that I missed it. Will fix.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 24 Feb 2025 00:45:32 +0100

> Indeed it isn't, the only explanation is that I missed it. Will fix.

The attached diff passes CI and works for me, will revisit in the morning.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 24 Feb 2025 10:00:25 +0100

> On 24 Feb 2025, at 00:45, Daniel Gustafsson <[email-masked]> wrote:

> The attached diff passes CI and works for me, will revisit in the morning.

Dave reported (on Discord) that the OPTIONAL macro collided with windef.h, so
attached is a small fix for that as well (even though we don't support Windows
here right now there is little point in clashing since we don't need that
particular macro name so rename anyways).

--
Daniel Gustafsson

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 24 Feb 2025 09:41:12 -0500

On 2025-02-20 Th 11:29 AM, Daniel Gustafsson wrote:
>> On 19 Feb 2025, at 15:13, Daniel Gustafsson<[email-masked]> wrote:

I notice that 001_server.pl contains this:

if ($ENV{with_python} ne 'yes')
{
     plan skip_all => 'OAuth tests require --with-python to run';
}

and various other things that insist on this. But I think all we should 
need is for Python to be present, whether or not we are building plpython.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 24 Feb 2025 15:48:13 +0100

> On 24 Feb 2025, at 15:41, Andrew Dunstan <[email-masked]> wrote:

> and various other things that insist on this. But I think all we should need is for Python to be present, whether or not we are building plpython.

Agreed.  Right now the --with-python check is what runs PGAC_PATH_PYTHON but
maybe there is value in separating this going forward into a) have python; b)
have python and build plpython?

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Mon, 24 Feb 2025 10:10:14 -0500

Hi,

> have python and build plpython?

FWIW, For other things we need in tests, e.g. gzip, we don't actually use a
configure test.  I think we could do the same for the python commandline
binary.

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Mon, 24 Feb 2025 09:39:52 -0800

> interruptible by signals on all platforms?

Probably not; I wasn't sure how much detail to put in here after "must
remain interruptible."

> chance of the signal arriving between CHECK_FOR_INTERRUPTS() and the blocking

That is worse, and to be honest I hadn't ever thought about that race
condition before your email. So wait... how do people actually rely on
EINTR in production-grade client software? pselect/ppoll exist,
clearly, but they're used so rarely IME. (Have we all just been
subconsciously trained to mash Ctrl-C until the program finally stops?
I'm honestly kind of horrified by this revelation.)

Anyway, yes, this documentation clearly needs to be strengthened.

> a bit worse?

A bit. The same for Kerberos, IIRC. Is the current configure warning
not strong enough to imply that the packager is on shaky ground? (I
patterned that off of the LDAP crash warning, which seemed much worse
to me. :D)

We can always declare non-support, I suppose, but anyone who doesn't
care about that problem (say, because they just want to use
single-connection psql) is then stuck hacking around it.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 24 Feb 2025 09:39:57 -0800

> Nonetheless, this declaration requires exactly that.

As read-only inputs to the client API, they're not meant to be changed
for the lifetime of the struct (and the lifetime of the client flow).
The only place to initialize such a struct is directly above this
code.

> any buildfarm animals, but so far I don't see any.)

Is there a reason for compilers to complain? memcpy's the way I know
of to put a const-member struct on the heap, but maybe there are other
ways that don't annoy Coverity?

If the cost of this warning is too high, removing the const
declarations isn't the end of the world. But we use unconstify and
other type-punning copies in so many other places that this didn't
seem all that bad for the goal of helping out the client writer.

> anywhere else, and I'm failing to detect a good reason to do it here.

This underscore pattern was copied directly from PQconninfoOption and
PQprintOpt.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 24 Feb 2025 09:43:41 -0800

On Mon, Feb 24, 2025 at 1:00 AM Daniel Gustafsson <[email-masked]> wrote:
> Dave reported (on Discord) that the OPTIONAL macro collided with windef.h,

Ugh.

> so

No objections to those patches, from inspection. (See note to Tom,
above, but I won't die on the const hill.)

Thanks!
--Jacob

---

From: Andres Freund <[email-masked]>
Date: Mon, 24 Feb 2025 15:30:49 -0500

Hi,

> I'm honestly kind of horrified by this revelation.)

If you need to handle the race it you need to combine it with something
additional, e.g. the so called "self pipe trick".  Which e.g. the latch / wait
event set code does.

> not strong enough to imply that the packager is on shaky ground?

I don't think it's strong enough.

> (I patterned that off of the LDAP crash warning, which seemed much worse to
> me. :D)

I don't think that's a comparable case, because there were in-production uses
of PG+ldap that (kind of) worked. Whereas we start on a green field here.

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Mon, 24 Feb 2025 14:02:04 -0800

> additional, e.g. the so called "self pipe trick".  Which e.g. the latch / wait

Right; I'm just used to that trick being deployed in massively
parallel async event engines rather than linear synchronous code
waiting on a single descriptor. I'm still a bit in disbelief, to be
honest. I'll get over it. Thank you for the note!

> of PG+ldap that (kind of) worked. Whereas we start on a green field here.

Fair enough. I'll work on a patch to disallow it; best case, no one
ever complains, and we've pruned an entire configuration from the list
of things to worry about.

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 25 Feb 2025 09:22:40 -0800

> of things to worry about.

Here goes:

- 0001 fails configuration if the AsynchDNS feature is not built into libcurl.
- 0002 removes EINTR references from the validator documentation and
instead points authors towards our internal Wait APIs.
- 0003 is an optional followup to the const changes from upthread:
there's no need to memcpy() now, and anyone reading the code without
the history might wonder why I chose such a convoluted way to copy a
struct. :D

WDYT?

--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Sat, 1 Mar 2025 02:43:52 +1300

Hi,

If you trigger the new optional NetBSD CI task, the oauthvalidator
tests implode[1].  Apparently that OS's kevent() doesn't like zero
relative timeouts for EVFILT_TIMER[2].  I see that you worked around
the same problem for Linux timerfd already by rounding 0 up to 1, so
we could just do the same here, and it passes with the attached.  A
cute alternative, not tested, might be to put NOTE_ABSTIME into fflag
if timeout == 0 (then it's an absolute time in the past, which should
fire immediately).

But I'm curious, how hard would it be to do this ↓ instead and not
have that problem on any OS?

     * There might be an optimization opportunity here: if timeout == 0, we
     * could signal drive_request to immediately call
     * curl_multi_socket_action, rather than returning all the way up the
     * stack only to come right back. But it's not clear that the additional
     * code complexity is worth it.

[1] https://cirrus-ci.com/task/6354435774873600
[2] https://github.com/NetBSD/src/blob/67c7c4658e77aa4534b6aac8c041d77097c5e722/sys/kern/kern_event.c#L1375
From 7deb153caf552c9bcb380f88eddbca94be33a0c2 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Sat, 1 Mar 2025 00:27:01 +1300
Subject: [PATCH] Fix OAUTH on NetBSD.

NetBSD's EVFILT_TIMER doesn't like zero relative timeouts and fails with
EINVAL.  Steal the workaround from the same problem on Linux from a few
lines up: round 0 up to 1.

This could be seen on the optional NetBSD CI task, but not on the NetBSD
build farm animals because they aren't finding curl and testing this
code.
---
 src/interfaces/libpq/fe-auth-oauth-curl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c
index ae339579f88..6e60a81574d 100644
--- a/src/interfaces/libpq/fe-auth-oauth-curl.c
+++ b/src/interfaces/libpq/fe-auth-oauth-curl.c
@@ -1363,6 +1363,16 @@ set_timer(struct async_ctx *actx, long timeout)
 #ifdef HAVE_SYS_EVENT_H
 	struct kevent ev;
 
+#ifdef __NetBSD__
+
+	/*
+	 * Work around NetBSD's rejection of zero timeouts (EINVAL), a bit like
+	 * timerfd above.
+	 */
+	if (timeout == 0)
+		timeout = 1;
+#endif
+
 	/* Enable/disable the timer itself. */
 	EV_SET(&ev, 1, EVFILT_TIMER, timeout < 0 ? EV_DELETE : (EV_ADD | EV_ONESHOT),
 		   0, timeout, 0);
-- 
2.48.1

---

From: Jacob Champion <[email-masked]>
Date: Fri, 28 Feb 2025 09:37:29 -0800

On Fri, Feb 28, 2025 at 5:44 AM Thomas Munro <[email-masked]> wrote:
> tests implode[1].

Oh, thank you for reporting that. I need to pay more attention to the
BSD CI thread.

> Apparently that OS's kevent() doesn't like zero
> we could just do the same here, and it passes with the attached.

Just from inspection, that looks good to me. I'll look into running
the new BSD tasks on the other patches I posted above, too.

Should we maybe consider just doing that across the board, and put up
with the inefficiency? Admittedly 1ms is a lot more dead time than
1ns...

> A

That could work. I think I need to stare at the man pages more if we
go that direction, since (IIUC) NOTE_ABSTIME changes up some other
default behavior for the timer.

>      * code complexity is worth it.

I'm not sure if it's hard, so much as it is confusing. My first
attempt at it a while back gave me the feeling that I wouldn't
remember how it worked in a few months.

Here are the things that I think we would have to consider, at minimum:
1. Every call to curl_multi_socket_action/all now has to look for the
new "time out immediately" flag after returning.
2. If it is set, and if actx->running has been set to zero, we have to
decide what that means conceptually. (Is that an impossible case? Or
do we ignore the timeout and assume the request is done/failed?)
3. Otherwise, we need to clear the flag and immediately call
curl_multi_socket_action(CURL_SOCKET_TIMEOUT), and repeat until that
flag is no longer set. That feels brittle to me, because if there's
some misunderstanding in our code or some strange corner case in an
old version of Curl on some platform, and we keep getting a timeout of
zero, we'll hit an infinite loop. (The current behavior instead
returns control to the top level every time, and gives
curl_multi_socket_all() a chance to right the ship by checking the
status of all the outstanding sockets.)
4. Even if 2 and 3 are just FUD, there's another potential call to
set_timer(0) in OAUTH_STEP_TOKEN_REQUEST. The interval can only be
zero in debug mode, so if we add new code for that case alone, the
tests will be mostly exercising a non-production code path.

I prefer your patch, personally.

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 28 Feb 2025 13:57:03 -0800

> the new BSD tasks on the other patches I posted above, too.

After your patch gets us past the zero timeout bug, NetBSD next runs into

    failed to fetch OpenID discovery document: Unsupported protocol
(libcurl: Received HTTP/0.9 when not allowed)'

...but only for a single test (nonempty oauth_client_secret), which is
very strange. And it doesn't always fail during the same HTTP request.
I'll look into it.

--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Sat, 1 Mar 2025 12:38:37 +1300

> with the inefficiency? Admittedly 1ms is a lot more dead time than

Last time I checked, NetBSD is still using scheduler ticks (100hz
periodic interrupt) for all this kind of stuff so it's even worse than
that :-)

> I prefer your patch, personally.

Cool, I'll commit it shortly unless someone else comes up with a better idea.

---

From: Thomas Munro <[email-masked]>
Date: Sat, 1 Mar 2025 13:36:29 +1300

> very strange. And it doesn't always fail during the same HTTP request.

In case it's relevant, it was green for me, but I also ran it in
combination with my 3x-go-faster patch on that other thread. . o O {
Timing/race stuff?  Normally the build farm shakes that stuff out a
bit more reliably than CI, but I doubt libcurl is set up on many
animals... }

---

From: Jacob Champion <[email-masked]>
Date: Mon, 3 Mar 2025 16:07:35 -0800

> bit more reliably than CI, but I doubt libcurl is set up on many

That does help, thanks. Luckily, I can still sometimes reproduce with
that patch, which should speed things up nicely.

Commenting out the failing test causes the next test to fail with
basically the same error, so there's something stateful going on.
There are some suspicious messages that occasionally show up right
before the failure:

    # [libcurl] * IPv6: ::1
    # [libcurl] * IPv4: 127.0.0.1
    # [libcurl] *   Trying [::1]:65269...
    # [libcurl] * getsockname() failed with errno 22: Invalid argument
    # [libcurl] * connect to ::1 port 65269 from ::1 port 65270
failed: Connection refused
    # [libcurl] *   Trying 127.0.0.1:65269...
    # [libcurl] * Connected to localhost (127.0.0.1) port 65269

Later, Curl reconnects via IPv6 -- this time succeeding -- but then
the response gets mangled in some way. I assume headers are being
truncated, based on Curl's complaint about "HTTP/0.9".

The NetBSD man pages say that EINVAL is returned when the socket is
already shut down, suggesting some sort of bad interaction between
Curl and the test authorization server (and/or the OS?). I wonder if
my test server doesn't handle dual-stack setups correctly. I'll see if
I can get ktruss working on either side.

Thanks,
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Tue, 4 Mar 2025 17:10:49 +1300

>     # [libcurl] * getsockname() failed with errno 22: Invalid argument

Weird.

> truncated, based on Curl's complaint about "HTTP/0.9".

And weirder.  With no evidence, I kinda wonder if that part could be a
bug in curl, if it gets a failure in an unexpected place like that and
gets confused, but let's start with the error...

> I can get ktruss working on either side.

POSIX says that about getsockname() too, as does the macOS man page,
but not those of FreeBSD, OpenBSD or Linux, but I don't think that's
the issue.  (From a quick peek: FreeBSD (in_getsockaddr) asserts that
sotoinpcb(so) is not NULL so it's always able to cough up the address
from the protocol control block object, while NetBSD (tcp_sockaddr)
and macOS/XNU (in6_getsockaddr) check for NULL and return EINVAL, so
at a wild guess, there may some path somewhere that knows the socket
is shutdown in both directions and all data has been drained so it can
be destroyed, and POSIX doesn't require sockets in this state to be
able to tell you their address so that's all OK?)

It's also unspecified if you haven't called connect() or bind() (well,
POSIX actually says that the address it gives you is unspecified, not
that the error is unspecified...).

I tried on a NetBSD 9 Vagrant box I had lying around, and ... ahh:

 28729      1 psql     write(0x2, 0x7f7fffddf3d0, 0x24) = 36
       "[libcurl] *   Trying [::1]:64244...\n"
 28729      1 psql     setsockopt(0x9, 0x6, 0x1, 0x7f7fffde015c, 0x4) = 0
 28729      1 psql     setsockopt(0x9, 0xffff, 0x800, 0x7f7fffde015c, 0x4) = 0
 26362      1 perl     __select50                  = 1
 28729      1 psql     connect                     Err#36 EINPROGRESS
 26362      1 perl     read                        = 36
 28729      1 psql     getsockname(0x9, 0x7f7fffde0240,
0x7f7fffde023c) Err#22 EINVAL

Other times it succeeds:

 28729      1 psql     connect                     Err#36 EINPROGRESS
 28729      1 psql     getsockname                 = 0

I think that is telling us that a non-blocking socket can be in a
state that is not yet connected enough even to tell you its local
address?  That is, connect() returns without having allocated a local
address, and does that part asynchronously too?  I don't know what to
think about that yet...

https://stackoverflow.com/questions/25333547/is-it-safe-to-call-getsockname-while-a-nonblocking-stream-socket-is-connecting

---

From: Jacob Champion <[email-masked]>
Date: Tue, 4 Mar 2025 09:08:00 -0800

On Mon, Mar 3, 2025 at 4:07 PM Jacob Champion
<[email-masked]> wrote:
> I wonder if
> my test server doesn't handle dual-stack setups correctly.

Spoilers: it's this.

> I'll see if

ktruss shows absolutely no syscall activity on the authorization
server during the failing test, because Curl's talking to something
else. sockstat confirms that I completely forgot to listen on IPv6 in
the test server. Dual stack sockets only work from the IPv6
direction...

There must be some law of conservation of weirdness, where the
strangest failure modes have the most boring explanations. I'll work
on a fix.

> address, and does that part asynchronously too?  I don't know what to

That is also really good to know, though. So that EINVAL message
might, in the end, be completely unrelated to the bug? (Curl doesn't
worry about the error, looks like, just prints it to the debug
stream.)

Thanks!
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Wed, 5 Mar 2025 11:37:27 +1300

> strangest failure modes have the most boring explanations. I'll work

Heh, wow, that was confusing :-)  Actually I'm still confused (why
passing sometimes then?) but I'm sure all will become clear with your
patch...

> worry about the error, looks like, just prints it to the debug

Yeah.  I wonder if it only happens if the connection is doomed to fail
already, or something.  But I don't plan to dig further if it's
harmless (maybe curl shouldn't really do that, IDK).

---

From: Jacob Champion <[email-masked]>
Date: Tue, 4 Mar 2025 14:44:48 -0800

On Tue, Mar 4, 2025 at 2:38 PM Thomas Munro <[email-masked]> wrote:
> passing sometimes then?)

Curl doesn't mind if the IPv6 connection fails outright; it'll use the
IPv4 in that case. But if something else ephemeral pops up on IPv6 and
starts speaking something that's not HTTP, that's a problem.

> but I'm sure all will become clear with your
> patch...

Maybe. My first attempt gets all the BSDs green except macOS -- which
now fails in a completely different test, haha... -_-

I'll keep you posted.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 6 Mar 2025 12:57:24 -0800

> now fails in a completely different test, haha... -_-

Small update: there is not one bug, but three that interact. ಠ_ಠ

1) The test server advertises an issuer of `https://localhost:<port>`,
but it doesn't listen on all localhost interfaces. When Curl tries to
contact the issuer on IPv6, its Happy Eyeballs handling usually falls
back to IPv4 after discovering that IPv6 is nonfunctional, but
occasionally it contacts something that was temporarily listening
there instead.

Since I don't really want to write a bunch of IPv6 fallback code for
the test server -- this should be testing OAuth, not finding all the
ways that buildfarm OSes can expose dual stack sockets -- I changed
the issuer to be IPv4-only. When I did this, the interval timing tests
immediately failed on macOS.

2) macOS's EVFILT_TIMER implementation seems to be different from the
other BSDs. On Mac, when you re-add a timer to a kqueue, any existing
timer-fired events for it are not cleared out and the kqueue might
remain readable. This breaks a postcondition of our set_timer()
function, which is that new timeouts are supposed to completely
replace previous timeouts.

With a dual stack issuer, the Happy Eyeballs timeouts would be
routinely cleared out by libcurl, setting up a clean slate for the
next call to set_timer(). But with an IPv4-only issuer, libcurl didn't
need to clear out the timeouts (they'd already fired), which meant
that our call to set the ping interval was ineffective.

3) There is a related performance bug on other platforms. If a Curl
timeout happens partway through a request (so libcurl won't clear it),
the timer-expired event will stay set and CPU will be burned to spin
pointlessly on drive_request(). This is much easier to notice after
taking Happy Eyeballs out of the picture. It doesn't cause logical
failures -- Curl basically discards the unnecessary calls -- but it's
definitely unintended.

--

Problem 1 is a simple patch. I am working on a fix for Problem 2, but
I got stuck trying to get a "perfect" solution working yesterday...
Since this is a partial(?) blocker for getting NetBSD going, I'm going
to pivot to an ugly-but-simple approach today.

I plan to defer working on Problem 3, which should just be a
performance bug, until the tests are green again. And I would like to
eventually add some stronger unit tests for the timer behavior, to
catch other potential OS-specific problems in the future.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 6 Mar 2025 16:35:43 -0800

> to pivot to an ugly-but-simple approach today.

Attached:
- 0001 fixes IPv6 failures,
- 0002 fixes set_timer() on Mac, and
- 0003-0005 are the existing followup patches from upthread.

Thanks!
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Fri, 7 Mar 2025 18:12:50 +1300

> replace previous timeouts.

I don't see that behaviour on my Mac with a simple program, and that
seems like it couldn't possibly be intended.  Hmm...  <browses source
code painfully> I wonder if this atomic generation scheme has a hole
in it, under concurrency...

https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1661

The code on the other OSes just dequeues it when reprogramming the
timer, which involves a lock and no doubt a few more cycles, and is
clearly not quite as exciting but ...

---

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Mar 2025 09:30:52 -0800

On Thu, Mar 6, 2025 at 9:13 PM Thomas Munro <[email-masked]> wrote:
> seems like it couldn't possibly be intended.

What version of macOS?

Just to make sure I'm not chasing ghosts, I've attached my test
program. Here are my CI results for running it:

= FreeBSD =

[      6 us] timer is set
[   1039 us] kqueue is readable
[   1050 us] timer is reset
[   1052 us] kqueue is not readable

= NetBSD =

[      3 us] timer is set
[  14993 us] kqueue is readable
[  15000 us] timer is reset
[  15002 us] kqueue is not readable

= OpenBSD =

[     24 us] timer is set
[  19660 us] kqueue is readable
[  19709 us] timer is reset
[  19712 us] kqueue is not readable

= macOS Sonoma =

[      4 us] timer is set
[   1282 us] kqueue is readable
[   1286 us] timer is reset
[   1287 us] kqueue is still readable

--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Sat, 8 Mar 2025 10:51:41 +1300

> program. Here are my CI results for running it:

Ah, right, yeah I see that here too.  I thought you were saying that
kevent() could report an already triggered alarm even though we'd
replaced it (it doesn') but of course you meant poll(kq) as libpq
does.

I believe I know exactly why: kqueues are considered readable (by
poll/select/other kqueues) if there are any events queued[1].  Apple's
EVFILT_TIMER implementation is doing that trick[2] where it leaves
them queued, but filt_timerprocess() filters them out if its own
private _FIRED flag isn't set, so kevent() itself won't wake up or
return them.  That trick doesn't survive nesting.  I think I would
call that a bug.  (I think I would keep the atomic CAS piece -- it
means you don't have to drain the timer callout synchronously when
reprogramming it which is a cool trick, but I think they overshot when
they left the knote queued.)

Maybe just do the delete-and-add in one call?

    EV_SET(&ev[0], 1, EVFILT_TIMER, EV_DELETE, 0, 0, 0);
    EV_SET(&ev[1], 1, EVFILT_TIMER, EV_ADD | EV_ONESHOT, 0, timeout, 0);
    if (kevent(kq, &ev[0], 2, NULL, 0, NULL) < 0)

[1] https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1078
[2] https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1661

---

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Mar 2025 15:02:50 -0800

On Fri, Mar 7, 2025 at 1:52 PM Thomas Munro <[email-masked]> wrote:
> call that a bug.

Bleh. Thank you for the analysis!

>     if (kevent(kq, &ev[0], 2, NULL, 0, NULL) < 0)

I think that requires me to copy the EV_RECEIPT handling from
register_socket(), to make sure an ENOENT is correctly ignored on
delete but doesn't mask failures from the addition. Do you prefer that
to the separate calls? (Or, better yet, is it easier than I'm making
it?)

Thanks!
--Jacob

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Mon, 17 Mar 2025 14:36:46 +0300

Hi,

I just wanted to report that the 'oauth_validator/t/001_server.pl'
test failed on FreeBSD in one of my local CI runs [1]. I looked at the
thread but could not find the same error report; if this is already
known, please excuse me.

HEAD was at 3943f5cff6 and there were no other changes. Sharing the
failure here for visibility:

[11:09:56.548] stderr:
[11:09:56.548] #   Failed test 'stress-async: stdout matches'
[11:09:56.548] #   at
/tmp/cirrus-ci-build/src/test/modules/oauth_validator/t/001_server.pl
line 409.
[11:09:56.548] #                   ''
[11:09:56.548] #     doesn't match '(?^:connection succeeded)'
[11:09:56.548] #   Failed test 'stress-async: stderr matches'
[11:09:56.548] #   at
/tmp/cirrus-ci-build/src/test/modules/oauth_validator/t/001_server.pl
line 410.
[11:09:56.548] #                   '[libcurl] * Host localhost:39251
was resolved.
[11:09:56.548] # [libcurl] * IPv6: ::1
[11:09:56.548] # [libcurl] * IPv4: 127.0.0.1
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Immediate connect fail for ::1: Connection refused
[11:09:56.548] # [libcurl] *   Trying 127.0.0.1:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (127.0.0.1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > GET
/param/.well-known/openid-configuration HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] * Request completely sent off
[11:09:56.548] # [libcurl] * HTTP 1.0, assume close after body
[11:09:56.548] # [libcurl] < HTTP/1.0 200 OK
[11:09:56.548] # [libcurl] < Server: BaseHTTP/0.6 Python/3.11.11
[11:09:56.548] # [libcurl] < Date: Mon, 17 Mar 2025 11:09:55 GMT
[11:09:56.548] # [libcurl] < Content-Type: application/json
[11:09:56.548] # [libcurl] < Content-Length: 400
[11:09:56.548] # [libcurl] <
[11:09:56.548] # [libcurl] < {"issuer":
"http://localhost:39251/param", "token_endpoint":
"http://localhost:39251/param/token", "device_authorization_endpoint":
"http://localhost:39251/param/authorize", "response_types_supported":
["token"], "subject_types_supported": ["public"],
"id_token_signing_alg_values_supported": ["RS256"],
"grant_types_supported": ["authorization_code",
"urn:ietf:params:oauth:grant-type:device_code"]}
[11:09:56.548] # [libcurl] * shutting down connection #0
[11:09:56.548] # [libcurl] * Hostname localhost was found in DNS cache
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Immediate connect fail for ::1: Connection refused
[11:09:56.548] # [libcurl] *   Trying 127.0.0.1:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (127.0.0.1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > POST /param/authorize HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] > Content-Length: 92
[11:09:56.548] # [libcurl] > Content-Type: application/x-www-form-urlencoded
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] >
scope=openid+postgres&client_id=eyJpbnRlcnZhbCI6MSwicmV0cmllcyI6MSwic3RhZ2UiOiJhbGwifQ%3D%3D
[11:09:56.548] # [libcurl] * upload completely sent off: 92 bytes
[11:09:56.548] # [libcurl] * HTTP 1.0, assume close after body
[11:09:56.548] # [libcurl] < HTTP/1.0 200 OK
[11:09:56.548] # [libcurl] < Server: BaseHTTP/0.6 Python/3.11.11
[11:09:56.548] # [libcurl] < Date: Mon, 17 Mar 2025 11:09:55 GMT
[11:09:56.548] # [libcurl] < Content-Type: application/json
[11:09:56.548] # [libcurl] < Content-Length: 132
[11:09:56.548] # [libcurl] <
[11:09:56.548] # [libcurl] < {"device_code": "postgres", "user_code":
"postgresuser", "verification_uri": "https://example.com/",
"expires_in": 5, "interval": 1}
[11:09:56.548] # [libcurl] * shutting down connection #1
[11:09:56.548] # [libcurl] * Hostname localhost was found in DNS cache
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (::1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > POST /param/token HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] > Content-Length: 157
[11:09:56.548] # [libcurl] > Content-Type: application/x-www-form-urlencoded
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] >
device_code=postgres&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Adevice_code&client_id=eyJpbnRlcnZhbCI6MSwicmV0cmllcyI6MSwic3RhZ2UiOiJhbGwifQ%3D%3D
[11:09:56.548] # [libcurl] * upload completely sent off: 157 bytes
[11:09:56.548] # [libcurl] * Received HTTP/0.9 when not allowed
[11:09:56.548] # [libcurl] * closing connection #2
[11:09:56.548] # connection to database failed: connection to server
on socket "/tmp/xZKYtq40nL/.s.PGSQL.21400" failed: failed to obtain
access token: Unsupported protocol (libcurl: Received HTTP/0.9 when
not allowed)
[11:09:56.548] # '
[11:09:56.548] #           matches '(?^:connection to database failed)'
[11:09:56.548] # Looks like you failed 2 tests of 121.
[11:09:56.548]
[11:09:56.548] (test program exited with status code 2)

[1] https://cirrus-ci.com/task/4621590844932096

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Jacob Champion <[email-masked]>
Date: Mon, 17 Mar 2025 08:08:30 -0700

> [11:09:56.548] # [libcurl] * Connected to localhost (::1) port 39251

Curl should not have connected to ::1 (the test server isn't listening
on IPv6). Whatever is talking on that port doesn't understand HTTP,
and we later fail with the "HTTP/0.9" error -- a slightly confusing
way to describe a protocol violation.

0001 will fix that. I think we should get that and 0002 in, ASAP. (And
the others.) Thomas has shown me a side quest to get rid of the second
kqueue instance, but so far that is not bearing fruit and we shouldn't
wait on it.

Thanks again!
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Wed, 19 Mar 2025 17:09:21 +1300

On Tue, Mar 18, 2025 at 4:08 AM Jacob Champion
<[email-masked]> wrote:
> the others.)

All pushed (wasn't sure if Daniel was going to but once I got tangled
up in all that kqueue stuff he probably quite reasonably assumed that
I would :-)).

> Thomas has shown me a side quest to get rid of the second

Cool, thanks for looking into it anyway.  (Feel free to post a
nonworking patch with a got-stuck-here-because problem statement...)

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 00:17:15 -0400

Thomas Munro <[email-masked]> writes:
> All pushed

You may have noticed it already, but indri reports that this
printf-like call isn't right:

fe-auth-oauth-curl.c:1392:49: error: data argument not used by format string [-Werror,-Wformat-extra-args]
 1392 |                 actx_error(actx, "deleting kqueue timer: %m", timeout);
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
fe-auth-oauth-curl.c:324:59: note: expanded from macro 'actx_error'
  324 |         appendPQExpBuffer(&(ACTX)->errbuf, libpq_gettext(FMT), ##__VA_ARGS__)
      |                                                          ~~~     ^

"timeout" isn't being used anymore.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Wed, 19 Mar 2025 17:34:18 +1300

>  1392 |                 actx_error(actx, "deleting kqueue timer: %m", timeout);

Yeah.  Thanks, fixed.

Now I'm wondering about teaching CI to fail on compiler warnings, ie
not just the special warnings task but also in the Mac etc builds.
The reason it doesn't is because it's sort of annoying to stop the
main tests because of a format string snafu, but we must be able to
put a new step at the end after all tests that scans the build logs
for warning and then raises the alarm...

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 00:57:29 -0400

BTW, I was pretty seriously disheartened just now to realize that
this feature was implemented by making libpq depend on libcurl.
I'd misread the relevant commit messages to say that libcurl was
just being used as test infrastructure; but nope, it's a genuine
build and runtime dependency.  I wonder how much big-picture
thinking went into that.  I can see at least two objections:

* This represents a pretty large expansion of dependency footprint,
not just for us but for the umpteen hundred packages that depend on
libpq.  libcurl alone maybe wouldn't be so bad, but have you looked
at libcurl's dependencies?  On RHEL8,

$ ldd /usr/lib64/libcurl.so.4.5.0
        linux-vdso.so.1 (0x00007fffd3075000)
        libnghttp2.so.14 => /lib64/libnghttp2.so.14 (0x00007f992097a000)
        libidn2.so.0 => /lib64/libidn2.so.0 (0x00007f992075c000)
        libssh.so.4 => /lib64/libssh.so.4 (0x00007f99204ec000)
        libpsl.so.5 => /lib64/libpsl.so.5 (0x00007f99202db000)
        libssl.so.1.1 => /lib64/libssl.so.1.1 (0x00007f9920046000)
        libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x00007f991fb5b000)
        libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f991f906000)
        libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f991f61b000)
        libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f991f404000)
        libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f991f200000)
        libldap-2.4.so.2 => /lib64/libldap-2.4.so.2 (0x00007f991efb1000)
        liblber-2.4.so.2 => /lib64/liblber-2.4.so.2 (0x00007f991eda1000)
        libbrotlidec.so.1 => /lib64/libbrotlidec.so.1 (0x00007f991eb94000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f991e97c000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f991e75c000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f991e386000)
        libunistring.so.2 => /lib64/libunistring.so.2 (0x00007f991e005000)
        librt.so.1 => /lib64/librt.so.1 (0x00007f991ddfd000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f9920e30000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f991dbf9000)
        libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f991d9e8000)
        libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f991d7e4000)
        libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f991d5cc000)
        libsasl2.so.3 => /lib64/libsasl2.so.3 (0x00007f991d3ae000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f991d02c000)
        libbrotlicommon.so.1 => /lib64/libbrotlicommon.so.1 (0x00007f991ce0b000)
        libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f991cbe0000)
        libcrypt.so.1 => /lib64/libcrypt.so.1 (0x00007f991c9b7000)
        libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f991c733000)

* Given libcurl's very squishy portfolio:

  libcurl is a free and easy-to-use client-side URL transfer library, supporting
  FTP, FTPS, HTTP, HTTPS, SCP, SFTP, TFTP, TELNET, DICT, LDAP, LDAPS, FILE, IMAP,
  SMTP, POP3 and RTSP. libcurl supports SSL certificates, HTTP POST, HTTP PUT,
  FTP uploading, HTTP form based upload, proxies, cookies, user+password
  authentication (Basic, Digest, NTLM, Negotiate, Kerberos4), file transfer
  resume, http proxy tunneling and more.

it's not exactly hard to imagine them growing a desire to handle
"postgresql://" URLs, which they would surely do by invoking libpq.
Then we'll have circular build dependencies and circular runtime
dependencies, not to mention inter-library recursion at runtime.

This is not quite a hill that I wish to die on, but I will
flatly predict that we will regret this.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 09:31:42 -0400

> flatly predict that we will regret this.

I regularly see curl security fixes in my Debian updates, so there is a
security issue that any serious curl bug could also make Postgres
vulnerable.  I might be willing to die on that hill.

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 19 Mar 2025 14:38:08 +0100

> On 19 Mar 2025, at 05:57, Tom Lane <[email-masked]> wrote:
> 
> thinking went into that.

A considerable amount. 

libcurl is not a dependency for OAuth support in libpq, the support was
designed to be exensible such that clients can hook in their own flow
implementations.  This part does not require libcurl.  It is however a
dependency for the RFC 8628 implementation which is included when building with
--with-libcurl, this in order to ship something which can be used out of the
box (for actual connections *and* testing) without clients being forced to
provide their own implementation.

This obviously means that the RFC8628 part could be moved to contrib/, but I
fear we wouldn't make life easier for packagers by doing that.

>  ...
> "postgresql://" URLs,

While there is no guarantee that such a pull request wont be submitted,
speaking as a (admittedly not very active at the moment) libcurl maintainer I
consider it highly unlikely that it would be accepted.  A postgres connnection
does not fit into what libcurl/curl is and wants to be.

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Wed, 19 Mar 2025 09:46:50 -0400

Hi,

> for warning and then raises the alarm...

The best way would probably be to tee the output of the build to a log file
and then have a script at the end to check for errors in that.

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 09:11:43 +1300

> fear we wouldn't make life easier for packagers by doing that.

How feasible/fragile/weird would it be to dlopen() it on demand?
Looks like it'd take ~20 function pointers:

                 U curl_easy_cleanup
                 U curl_easy_escape
                 U curl_easy_getinfo
                 U curl_easy_init
                 U curl_easy_setopt
                 U curl_easy_strerror
                 U curl_free
                 U curl_global_init
                 U curl_multi_add_handle
                 U curl_multi_cleanup
                 U curl_multi_info_read
                 U curl_multi_init
                 U curl_multi_remove_handle
                 U curl_multi_setopt
                 U curl_multi_socket_action
                 U curl_multi_socket_all
                 U curl_multi_strerror
                 U curl_slist_append
                 U curl_slist_free_all
                 U curl_version_info

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 17:03:59 -0400

> How feasible/fragile/weird would it be to dlopen() it on demand?

FWIW, that would not really move the needle one bit so far as
my worries are concerned.  What I'm unhappy about is the very
sizable expansion of our build dependency footprint as well
as the sizable expansion of the 'package requires' footprint.
The fact that the new dependencies are mostly indirect doesn't
soften that blow at all.

To address that (without finding some less kitchen-sink-y OAuth
implementation to depend on), we'd need to shove the whole thing
into a separately-built, separately-installable package.

What I expect is likely to happen is that packagers will try to do
that themselves to avoid the dependency bloat.  AFAICT our current
setup will make that quite painful for them, and in any case I
don't believe it's work we should make them do.  If they fail to
do that, the burden of the extra dependencies will fall on end
users.  Either way, it's not going to make us look good.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:02:57 +1300

> users.  Either way, it's not going to make us look good.

It would increase the build dependencies, assuming a package
maintainer wants to enable as many features as possible, but it would
*not* increase the 'package requires' footprint, merely the 'package
suggests' footprint (as Debian calls it), and it's up to the user
whether they install suggested extra packages, no?

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:14:29 +1300

> > > How feasible/fragile/weird would it be to dlopen() it on demand?

. o O { There may also be security reasons to reject the idea, would
need to look into that... }

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 18:19:23 -0400

> whether they install suggested extra packages, no?

Maybe I'm confused, but what I saw was a hard dependency on libcurl,
as well as several of its dependencies:

$ ./configure --with-libcurl
...
$ make
...
$ ldd src/interfaces/libpq/libpq.so.5.18 
        linux-vdso.so.1 (0x00007ffc145fe000)
        libcurl.so.4 => /lib64/libcurl.so.4 (0x00007f2c2fa36000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f2c2f95b000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f2c2f600000)
        libnghttp2.so.14 => /lib64/libnghttp2.so.14 (0x00007f2c2f931000)
        libidn2.so.0 => /lib64/libidn2.so.0 (0x00007f2c2f910000)
        libssh.so.4 => /lib64/libssh.so.4 (0x00007f2c2f89b000)
        libpsl.so.5 => /lib64/libpsl.so.5 (0x00007f2c2f885000)
        libssl.so.3 => /lib64/libssl.so.3 (0x00007f2c2f51a000)
        libcrypto.so.3 => /lib64/libcrypto.so.3 (0x00007f2c2f000000)
        libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f2c2f82f000)
        libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f2c2ef26000)
        libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f2c2f816000)
        libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f2c2f80d000)
        libldap.so.2 => /lib64/libldap.so.2 (0x00007f2c2eebf000)
        liblber.so.2 => /lib64/liblber.so.2 (0x00007f2c2eead000)
        libbrotlidec.so.1 => /lib64/libbrotlidec.so.1 (0x00007f2c2ee9f000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f2c2ee85000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f2c2fb43000)
        libunistring.so.2 => /lib64/libunistring.so.2 (0x00007f2c2ed00000)
        libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f2c2ecef000)
        libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f2c2ece8000)
        libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f2c2ecd4000)
        libevent-2.1.so.7 => /lib64/libevent-2.1.so.7 (0x00007f2c2ec7b000)
        libsasl2.so.3 => /lib64/libsasl2.so.3 (0x00007f2c2ec5b000)
        libbrotlicommon.so.1 => /lib64/libbrotlicommon.so.1 (0x00007f2c2ec38000)
        libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f2c2ec0b000)
        libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f2c2ebd1000)
        libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f2c2eb35000)

I don't think that will be satisfied by 'package suggests'.
Even if it somehow manages to load, the result of trying to
use OAuth would be a segfault rather than any useful message.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:28:50 +1300

> use OAuth would be a segfault rather than any useful message.

I was imagining that it would just error out if you try to use that
stuff and it fails to open libcurl.  Then it's up to end users: if
they want to use libpq + OAuth, they have to install both libpq5 and
libcurl packages, and if they don't their connections will just fail,
presumably with some error message explaining why.  Or something like
that...

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 19:11:28 -0400

> presumably with some error message explaining why.  Or something like

Am I understanding that curl is being used just to honor the RFC and it
is only for testing?  That seems like a small reason to add such a
dependency.

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 19:59:28 -0400

> fear we wouldn't make life easier for packagers by doing that.

I see it now ---- without having RFC 8628 built into the server, clients
have to implement it.  Do we know what percentage would need to do that?
The spec:

	https://datatracker.ietf.org/doc/html/rfc8628

Do we think packagers will use the --with-libcurl configure option?

It does kind of make sense for curl to handle OAUTH since curl has to
simulate a browser.  I assume we can't call a shell to invoke curl from
the command line.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 20 Mar 2025 13:33:26 -0700

Hi all,

With the understanding that the patchset is no longer just "my" baby...

= Dependencies =

I like seeing risk/reward discussions. I agonized over the choice of
HTTP dependency, and I transitioned from an "easier" OAuth library
over to Curl early on because of the same tradeoffs.

That said... Tom, I think the dependency list you've presented is not
quite fair, because it doesn't show what libpq's dependency list was
before adding Curl. From your email, and my local Rocky 9 install, I
think these are the net-new dependencies that we (and packagers) need
to worry about:

        libcurl.so.4
        libnghttp2.so.14
        libidn2.so.0
        libssh.so.4
        libpsl.so.5
        libunistring.so.2
        libbrotlidec.so.1
        libbrotlicommon.so.1
        libz.so.1

That's more than I'd like, to be perfectly honest. I'm least happy
about libssh, because we're not using SFTP but we have to pay for it.
And the Deb-alikes add librtmp, which I'm not thrilled about either.

The rest are, IMO, natural dependencies of a mature HTTP client: the
HTTP/1 and HTTP/2 engines, Punycode, the Public Suffix List, UTF
handling, and common response compression types. Those are kind of
part and parcel of communicating on the web. (If we find an HTTP
client that does all those things itself, awesome, but then we have to
ask how well they did it.)

So one question for the collective is -- putting Curl itself aside --
is having a basic-but-usable OAuth flow, out of the box, worth the
costs of a generic HTTP client? A non-trivial footprint *will* be
there, whether it's one library or several, whether we delay-load it
or not, whether we have the unused SFTP/RTMP dependencies or not. But
we could still find ways to reduce that cost for people who aren't
using it, if necessary.

= Asides =

I would also like to point out: End users opt into this by
preregistering a client ID with an OAuth issuer ID, then providing
that pair of IDs in the connection string. We will not just start
crawling the web because a server tells us to. I don't want to
downplay the additional risk of having it in the address space, but
the design goal is that vulnerabilities in the HTTP logic should not
affect users who have not explicitly consented to the use of OAuth.

There were some other questions/statements made upthread that I want
to clarify too:

On Wed, Mar 19, 2025 at 4:11 PM Bruce Momjian <[email-masked]> wrote:
> is only for testing?

No. (I see you found it later, but to state clearly for the record:
it's not just for testing.) libcurl is used for the Device
Authorization flow implementation. You don't have to use Device
Authorization to use OAuth, but we don't provide any alternative flows
in-tree; you'd have to use the libpq API to insert your own flow.

> I see it now ---- without having RFC 8628 built into the server,

(libpq, not the server. We do not ship server-side plugins at all, yet.)

> clients

For version 1 of the feature, Device Authorization is the only option
for our utilities (psql et al). I can't really speculate on
percentages; it depends on what percentage want to use OAuth and don't
like (or can't use) our builtin flow. Obviously the percentage goes up
to 100% if we don't provide one. Plus we lose significant testability,
plus no one can use it from psql.

> Do we think packagers will use the --with-libcurl configure option?

Well, hopefully, yes. The tradeoffs of the builtin flow were chosen
explicitly so that existing clients could use it with minimal-to-no
code changes.

Thanks!
--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 20 Mar 2025 16:50:57 -0400

> we could still find ways to reduce that cost for people who aren't

One observation is that security scanning tools are going to see the
curl dependency and look at any CSVs related to them and ask us, whether
they are using OAUTH or not.

---

From: Tom Lane <[email-masked]>
Date: Thu, 20 Mar 2025 17:08:54 -0400

Bruce Momjian <[email-masked]> writes:
>> costs of a generic HTTP client?

> they are using OAUTH or not.

Yes.  Also, none of this has addressed my complaint about the extent
of the build and install dependencies.  Yes, simply not selecting
--with-libcurl removes the problem ... but most packagers are under
very heavy pressure to enable all features of a package.

---

From: Andres Freund <[email-masked]>
Date: Thu, 20 Mar 2025 19:26:49 -0400

Hi,

> very heavy pressure to enable all features of a package.

How about we provide the current libpq.so without linking to curl and also a
libpq-oauth.so that has curl support? If we do it right libpq-oauth.so would
itself link to libpq.so, making libpq-oauth.so a fairly small library.

That way packagers can split libpq-oauth.so into a separate package, while
still just building once.

That'd be a bit of work on the buildsystem side, but it seems doable.

> From what's been said here, only a small minority of users are likely
> to have any interest in this feature.  So my answer to "is it worth
> the cost" is no, and would be no even if I had a lower estimate of
> the costs.

I think this is likely going to be rather widely used, way more widely than
e.g. kerberos or ldap support in libpq. My understanding is that there's a
fair bit of pressure in lots of companies to centralize authentication towards
centralized systems, even for server applications.

> I don't have any problem with making a solution available to those
> users who want it --- but I really do NOT want this to be part of
> stock libpq nor done as part of the core Postgres build.  I do not
> think that the costs of that have been fully accounted for, especially
> not the fact that almost all of those costs fall on people other than
> us.

I am on board with not having it as part of stock libpq, but I don't see what
we gain by not building it as part of postgres (if the dependencies are
available, of course).

Greetings,

Andres Freund

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 21 Mar 2025 08:40:45 -0400

> That'd be a bit of work on the buildsystem side, but it seems doable.

That certainly seems worth exploring.

> centralized systems, even for server applications.

Indeed. There is still work to do on OAUTH2 but the demand you mention 
is just going to keep increasing.

> we gain by not building it as part of postgres (if the dependencies are

+1.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 21 Mar 2025 19:02:02 +0100

> On 21 Mar 2025, at 13:40, Andrew Dunstan <[email-masked]> wrote:

> That certainly seems worth exploring.

This is being worked on.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Wed, 26 Mar 2025 12:09:14 -0700

> > That certainly seems worth exploring.

Attached is a proof of concept, with code from Daniel and myself,
which passes the CI as a starting point.

Roughly speaking, some things to debate are
- the module API itself
- how much to duplicate from libpq vs how much to export
- is this even what you had in mind

libpq-oauth.so is dlopen'd when needed. If it's not found or it
doesn't have the right symbols, builtin OAuth will not happen. Right
now we have an SO version of 1; maybe we want to remove the SO version
entirely to better indicate that it shouldn't be linked?

Two symbols are exported for the async authentication callbacks. Since
the module understands PGconn internals, maybe we could simplify this
to a single callback that manipulates the connection directly.

To keep the diff small to start, the current patch probably exports
too much. I think appendPQExpBufferVA makes sense, considering we
export much of the PQExpBuffer API already, but I imagine we won't
want to expose the pg_g_threadlock symbol. (libpq could maybe push
that pointer into the libpq-oauth module at load time, instead of
having the module pull it.) And we could probably go either way with
the PQauthDataHook; I prefer having a getter and setter for future
flexibility, but it would be simpler to just export the hook directly.

The following functions are duplicated from libpq:
- libpq_block_sigpipe
- libpq_reset_sigpipe
- libpq_binddomain
- libpq_[n]gettext
- libpq_append_conn_error
- oauth_unsafe_debugging_enabled

Those don't seem too bad to me, though maybe there's a good way to
deduplicate. But i18n needs further work. It builds right now, but I
don't think it works yet.

WDYT?

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Mon, 31 Mar 2025 16:06:49 +0200

Re: Andres Freund
> > very heavy pressure to enable all features of a package.

And this feature is kind of only useful if it's available anywhere. If
only half of your clients are able to use SSO, you'd probably stick
with passwords anyway. So it needs to be enabled by default.

> still just building once.

That's definitely a good plan. The blast radius of build dependencies
isn't really a problem, the install/run-time is.

Perhaps we could do the same with libldap and libgssapi? (Though
admittedly I have never seen any complaints or nagging questions from
security people about these.)

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Thu, 3 Apr 2025 11:02:03 -0700

> up in all that kqueue stuff he probably quite reasonably assumed that

Attached are two more followups, separate from the libcurl split:
- 0001 is a patch originally proposed at [1]. Christoph pointed out
that the build fails on a platform that tries to enable Curl without
having either epoll() or kqueue(), due to a silly mistake I made in
the #ifdefs.
- 0002 should fix some timeouts in 002_client.pl reported by Andres on
Discord. I allowed a short connect_timeout to propagate into tests
which should not have it.

(The goal of 0001 is to get things building for now. After I finish
splitting the implementation into its own module, it'll make more
sense to simply not build that module on platforms that can't
implement a useful flow.)

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2B%3D4898tXuTvb2LstorRo9JsAnBcn8LE%3DqrgVPiPW8ZfCw%40mail.gmail.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 3 Apr 2025 21:50:16 +0200

> On 3 Apr 2025, at 20:02, Jacob Champion <[email-masked]> wrote:
> 

Thanks, both LGTM so pushed.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 12 Jun 2025 12:58:44 -0700

> catch other potential OS-specific problems in the future.

To follow up on this: I had intended to send a patch fixing the timer
bug this week, but after fixing it, the performance problem did not
disappear. Turns out: other file descriptors can get stuck open on
BSD, depending on how complicated Curl wants to make the order of
operations, and the existing tests aren't always enough to expose it.
(It also depends on the Curl version installed.)

I will split this off into its own thread soon, because this
megathread is just too big, but I wanted to make a note here and file
an open item. As part of that, I have a set of more rigorous unit
tests for the libcurl-libpq interaction that I'm working on, since the
external view of "the flow worked/didn't work" is not enough to
indicate internal health.

--Jacob

---

From: Ivan Kush <[email-masked]>
Date: Fri, 20 Jun 2025 13:08:35 +0300

Hello!

This patch fixes CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl 
support in configure

Custom parameters and paths to libcurl were mistakenly excluded from 
CPPFLAGS, LDFLAGS, and LIBS, although AsyncDNS check was OK.

For example, the command `pkg-config --libs libcurl` gives 
`-L/usr/local/lib -lcurl`. LDFLAGS will not contain `-L/usr/local/lib`.

This patch fixes such behaviour.

Test case:

I've tested custom Postgres in an old Debian based Linux distro. This 
distro contains old libcurl (< 7.61, package libcurl3) that was compiled 
with symbols CURL_OPENSSL_3. So I've installed newer version of 
libcurlssl, package libcurl4-openssl-dev, that contains symbols 
CURL_OPENSSL_4 and compiled my libcurl version > 7.61.

After compilation during testing some Postgres shared libraries or 
binaries that was linked with libcurl showed an error "version 
CURL_OPENSSL_3 not found (required by …/libcurl.so.4)"

-- 
Best wishes,
Ivan Kush
Tantor Labs LLC
From 8a24c24f85c40e2aa0c40afc8f9cd7a19afa66c3 Mon Sep 17 00:00:00 2001
From: Ivan Kush <[email-masked]>
Date: Fri, 20 Jun 2025 12:16:47 +0300
Subject: [PATCH] oauth: Fix CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl support

Custom parameters and paths to libcurl were mistakenly excluded from CPPFLAGS,
LDFLAGS, and LIBS.
For example, the command `pkg-config --libs libcurl` gives
`-L/usr/local/lib -lcurl`. LDFLAGS will not contain `-L/usr/local/lib`

This patch fixes this.

Author: Ivan Kush <[email-masked]>
Author: Lev Nikolaev <[email-masked]>
Reviewed-by:
Discussion:
---
 config/programs.m4 | 7 +++----
 configure          | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/config/programs.m4 b/config/programs.m4
index 0ad1e58b48d..2556e469323 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -348,9 +348,8 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
 *** The installed version of libcurl does not support asynchronous DNS
 *** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
 *** to use it with libpq.])
+    CPPFLAGS=$pgac_save_CPPFLAGS
+    LDFLAGS=$pgac_save_LDFLAGS
+    LIBS=$pgac_save_LIBS
   fi
-
-  CPPFLAGS=$pgac_save_CPPFLAGS
-  LDFLAGS=$pgac_save_LDFLAGS
-  LIBS=$pgac_save_LIBS
 ])# PGAC_CHECK_LIBCURL
diff --git a/configure b/configure
index 4f15347cc95..46a011d1d1b 100755
--- a/configure
+++ b/configure
@@ -12883,12 +12883,10 @@ $as_echo "$pgac_cv__libcurl_async_dns" >&6; }
 *** The installed version of libcurl does not support asynchronous DNS
 *** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
 *** to use it with libpq." "$LINENO" 5
+    CPPFLAGS=$pgac_save_CPPFLAGS
+    LDFLAGS=$pgac_save_LDFLAGS
+    LIBS=$pgac_save_LIBS
   fi
-
-  CPPFLAGS=$pgac_save_CPPFLAGS
-  LDFLAGS=$pgac_save_LDFLAGS
-  LIBS=$pgac_save_LIBS
-
 fi

2.34.1

---

From: Jacob Champion <[email-masked]>
Date: Mon, 23 Jun 2025 08:32:26 -0700

> This patch fixes CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl

Hi Ivan, thanks for the report! Your patch puts new logic directly
after an AC_MSG_ERROR() call, so any effect has to come from the fact
that we're no longer restoring the old compiler and linker flags.
That's not what we want -- Curl needs to be isolated from the rest of
the build.

> CURL_OPENSSL_3 not found (required by …/libcurl.so.4)"

What's your configure line? You need to make sure that your custom
libcurl is used at configure-time, compile-time, and run-time.

And which binaries are complaining? The only thing that should ever be
linked against libcurl is libpq-oauth-18.so.

Thanks,
--Jacob

---

From: Ivan Kush <[email-masked]>
Date: Wed, 2 Jul 2025 15:45:30 +0300

Thanks for the clarification! I thought linker flags should be installed 
globally for all compilation targets.

Another question:

Why don't we set LIBS in the configure in "checking for curl_multi_init" 
using LIBCURL_LIBS or LIBCURL_LDFLAGS?
https://github.com/postgres/postgres/blob/master/configure#L12734

Like this:
     LIBS="$(LIBCURL_LDFLAGS) $(LIBCURL_LDLIBS)"

And set LIBS with -lcurl.

As I understand we need to check the properties of libcurl we are 
compiling with?
It may be some local libcurl from /opt/my_libcurl. So LIBCURL_... may 
contain a flag like -L/opt/my_libcurl
Without these LIBCURL... variables we will check a system libcurl, not 
our local.

I mean why don't we set LIBS

current *configure*

$as_echo_n "checking for curl_multi_init in -lcurl... " >&6; }
....
else
   ac_check_lib_save_LIBS=$LIBS
LIBS="-lcurl  $LIBS"
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h.  */

https://github.com/postgres/postgres/blob/master/configure#L12734

For example, I've logged flags after this code sample and they don't 
contain -L/opt/my_libcurl

     IVK configure:13648: CFLAGS=-Wall -Wmissing-prototypes 
-Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels 
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing 
-fwrapv -fexcess-precision=standard -pipe -O2
     IVK configure:13649: LDFLAGS=-Wl,-z,relro -Wl,-z,now -flto=auto 
-ffat-lto-objects -L/usr/lib/llvm-10/lib -L/usr/local/lib/zstd
     IVK configure:13650: LIBS=-lcurl  -lz -lreadline -lpthread -lrt 
-ldl -lm
     IVK configure:13651: LDLIBS=

-- 
Best wishes,
Ivan Kush
Tantor Labs LLC

---

From: Jacob Champion <[email-masked]>
Date: Wed, 2 Jul 2025 07:46:40 -0700

> globally for all compilation targets.

Not for libcurl, since the libpq-oauth module split.

> [...]

Ah, that's definitely a bug. I've tested alternate PKG_CONFIG_PATHs,
but I haven't regularly tested on systems that have no system libcurl
at all. So those header and lib checks need to be moved after the use
of LIBCURL_CPPFLAGS and LIBCURL_LDFLAGS to prevent a false failure.
Otherwise they're only useful for the LIBCURL_LDLIBS assignment.

I wonder if I should just get rid of those to better match the Meson
implementation... but the error messages from the checks will likely
be nicer than compilation failures during the later test programs. Hm.

(Thanks for the report!)

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 13:36:26 -0400

> Ah, that's definitely a bug.

I just ran into a vaguely-related failure: on RHEL8, building
with --with-libcurl leads to failures during check-world:

../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlopen'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlclose'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlerror'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlsym'
collect2: error: ld returned 1 exit status

Per "man dlopen", you have to link with libdl to use these functions
on this platform.  (Curiously, although RHEL9 still says that in the
documentation, it doesn't seem to actually need -ldl.)  I was able
to resolve this by adding -ldl in libpq's Makefile:

-SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)
+SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -ldl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)

It doesn't look like the Meson support needs such explicit tracking of
required libraries, but perhaps I'm missing something?  I'm not able
to test that directly for lack of a usable ninja version on this
platform.

Apologies for not noticing this sooner.  I don't think I'd tried
--with-libcurl since the changes to split out libpq-oauth.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 9 Jul 2025 13:46:29 -0400

Hi,

On 2025-07-09 13:36:26 -0400, Tom Lane wrote:
> required libraries, but perhaps I'm missing something?

It should be fine, -ldl is added to "os_deps" if needed, and os_deps is used
for all code in pg.

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 10:59:47 -0700

> +SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -ldl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)

Hmm, okay. That analysis and fix look good to me. (It looks like none
of the RHEL animals are testing with Curl yet, and locally I was using
Rocky 9...)

I'll work up a patch to send through the CI. I can't currently test
RHEL8 easily -- Rocky 8 is incompatible with my Macbook? -- which I
will need to rectify eventually, but I can't this week.

Thanks!
--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 14:13:08 -0400

> will need to rectify eventually, but I can't this week.

No need, I already tested locally.  Will push shortly.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 11:39:41 -0700

> No need, I already tested locally.  Will push shortly.

Thank you very much!

Here is a draft patch for Ivan's reported issue; I still need to put
it through its paces with some more unusual setups, but I want to get
cfbot on it.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 15:07:07 -0400

> it through its paces with some more unusual setups, but I want to get

I'm confused about why this moves up the temporary changes of
CPPFLAGS and LDFLAGS, but not LIBS?  Maybe that's actually correct,
but it looks strange (and perhaps deserves a comment about why).

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 12:28:52 -0700

> but it looks strange (and perhaps deserves a comment about why).

Yeah, that's fair. It's because LIBCURL_LDLIBS isn't set until that
AC_CHECK_LIB test is run, and the test needs LIBCURL_LDFLAGS to be in
force.

(Upthread, I was idly wondering if those AC_CHECKs should just be
removed -- after all, PKG_CHECK_MODULES just told us where Curl was --
but I'm nervous that this might make more niche use cases like
cross-compilation harder to use in practice?)

Does the attached help clarify?

Thanks,
--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 15:42:30 -0400

> Does the attached help clarify?

Yes, thanks.

> cross-compilation harder to use in practice?)

Nah, let's keep them.  We do document for at least some libraries
how to manually specify the include and link options without
depending on pkg-config.  If someone tries that with libcurl,
it'd be good to have sanity checks on the results.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 16:54:02 -0700

> it'd be good to have sanity checks on the results.

Sounds good, thanks for the review!

> it through its paces with some more unusual setups, but I want to get

On HEAD, Rocky 9 fails to build with a custom Curl PKG_CONFIG_PATH and
no libcurl-devel installed. With this patch, that build now succeeds,
and it still succeeds after libcurl-devel is reinstalled, with the
compiler tests continuing to use the custom libcurl and not the
system's.

So I'll give Ivan a little time in case he'd like to test/review
again, but otherwise I plan to push it this week.

Thanks,
--Jacob

---

From: [email-masked]
Date: Thu, 10 Jul 2025 17:41:07 +0300

I agree with the patch. Works in my OSes

---

From: Jacob Champion <[email-masked]>
Date: Fri, 11 Jul 2025 10:33:20 -0700

> I agree with the patch. Works in my OSes

Thanks Ivan! Committed.

--Jacob