From b2cece52ba68a83b6ecdbda6c5c45d4aeb0b66ac Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 1 Dec 2025 15:07:26 -0800 Subject: [PATCH 4/7] libpq: Introduce PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 For the libpq-oauth module to eventually make use of the PGoauthBearerRequest API, it needs some additional functionality: the derived Issuer ID for the authorization server needs to be provided by libpq, and error messages need to be built without relying on PGconn internals. These features seem useful for application hooks, too, so that they don't each have to reinvent the wheel. The original plan was for additions to PGoauthBearerRequest to be made without a version bump to the PGauthData type, and that applications would simply check a LIBPQ_HAS_* macro at compile time to decide whether they could use the new features. That theoretically works for applications linked against libpq, since it's not safe to downgrade libpq from the version you've compiled against. That strategy won't work for plugins, though, due to a complication first noticed during the libpq-oauth module split: it's normal for a plugin on disk to be *newer* than the libpq that's loading it, because you might have upgraded your installation while an application was running. (Put another way: a plugin architecture causes the compile-time and run-time dependency arrows to point in opposite directions, so plugins won't be able to rely on the LIBPQ_HAS_* macros to determine what APIs are available to them.) (TODO: Are there implications for our use of RTLD_NOW at dlopen() time? Can this be improved?) Instead, add a new struct which extends the "v1" PGoauthBearerRequest. When an application hook receives PQAUTHDATA_OAUTH_BEARER_TOKEN_V2, it may safely downcast the request pointer it receives in its callbacks to make use of the new functionality. libpq will first try the new version, then fall back to the old before giving up. TODO: Error handling introduces a dependency on pqexpbuffer.h, which is technically considered internal... but we export the ABI for psql, so is it really internal at this point? TODO: Could we just add to the end of PGoauthBearerRequest, and tell users not to use the additional fields if they have version 1? --- doc/src/sgml/libpq.sgml | 87 +++++++++++++++ src/tools/pgindent/typedefs.list | 1 + src/interfaces/libpq/libpq-fe.h | 32 +++++- src/interfaces/libpq/fe-auth-oauth.c | 104 +++++++++++++----- .../oauth_validator/oauth_hook_client.c | 97 +++++++++++++++- .../modules/oauth_validator/t/002_client.pl | 38 ++++++- 6 files changed, 324 insertions(+), 35 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 45eadc4de7e..baf8ef4d3d0 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -10350,6 +10350,7 @@ PQauthDataHook_type PQgetAuthDataHook(void); PQAUTHDATA_PROMPT_OAUTH_DEVICE + Available in PostgreSQL 18 and later. Replaces the default user prompt during the builtin device authorization client flow. data points to @@ -10402,6 +10403,7 @@ typedef struct _PGpromptOAuthDevice PQAUTHDATA_OAUTH_BEARER_TOKEN + Available in PostgreSQL 18 and later. Adds a custom implementation of a flow, replacing the builtin flow if it is installed. @@ -10409,6 +10411,13 @@ typedef struct _PGpromptOAuthDevice user/issuer/scope combination, if one is available without blocking, or else set up an asynchronous callback to retrieve one. + + + For PostgreSQL releases 19 and later, + applications should prefer + PQAUTHDATA_OAUTH_BEARER_TOKEN_V2. + + data points to an instance of PGoauthBearerRequest, which should be filled in @@ -10500,6 +10509,84 @@ typedef struct PGoauthBearerRequest + + + + PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 + + PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 + PQAUTHDATA_OAUTH_BEARER_TOKEN + + + + Available in PostgreSQL 19 and later. + + Provides all the functionality of + PQAUTHDATA_OAUTH_BEARER_TOKEN, + and adds the ability to set custom error messages and inspect the + OAuth issuer ID that the client expects to use. + + + data points to an instance + of PGoauthBearerRequestV2, which should be filled in + by the implementation: + +typedef struct +{ + PGoauthBearerRequest v1; /* see the PGoauthBearerRequest struct, above */ + + /* Hook inputs (constant across all calls) */ + const char *issuer; /* the issuer identifier (RFC 9207) in use */ + + /* Hook outputs */ + + /* An initialized, empty buffer for reporting errors when the flow fails. */ + struct PQExpBufferData *error; +} PGoauthBearerRequestV2; + + + + When a PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 hook is in + use, libpq additionally guarantees that the + request pointer that is provided to the + v1.async and v1.cleanup + callbacks may be safely cast to (PGoauthBearerRequestV2 *). + Implementations must otherwise follow the v1 API, as described above, + using the v1 struct member. + + + + Casting to (PGoauthBearerRequestV2 *) is + only safe when the hook type is + PQAUTHDATA_OAUTH_BEARER_TOKEN_V2. Applications may + crash or misbehave if a hook attempts to use v2 features with the v1 + hook type. + + + + In addition to the functionality of the version 1 API, the v2 struct + provides an additional input and output for the hook: + + + issuer contains the issuer identifier, as + defined in RFC 9207, + that is in use for the current connection. This identifier is + determined by . + To avoid mix-up attacks, custom flows should ensure that any discovery + metadata provided by the authorization server matches this issuer ID. + + + error contains an empty + PQExpBuffer that can be used to construct a custom + error message when a flow fails. The message will be included as part + of PQerrorMessage(). + Hooks must not free or reassign this buffer; it is managed by + libpq. The API for manipulating the error + buffer is provided in "internal/pqexpbuffer.h". + + + + diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index c1ad80a418d..be8fe0ac22b 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1906,6 +1906,7 @@ PGdataValue PGlobjfuncs PGnotify PGoauthBearerRequest +PGoauthBearerRequestV2 PGpipelineStatus PGpromptOAuthDevice PGresAttDesc diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index 29ee0c8a4fd..8f839785fa1 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -63,6 +63,10 @@ extern "C" /* Indicates presence of the PQAUTHDATA_PROMPT_OAUTH_DEVICE authdata hook */ #define LIBPQ_HAS_PROMPT_OAUTH_DEVICE 1 +/* Features added in PostgreSQL v19: */ +/* Indicates presence of the PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 authdata hook */ +#define LIBPQ_HAS_OAUTH_BEARER_TOKEN_V2 1 + /* * Option flags for PQcopyResult */ @@ -193,7 +197,9 @@ typedef enum { PQAUTHDATA_PROMPT_OAUTH_DEVICE, /* user must visit a device-authorization * URL */ - PQAUTHDATA_OAUTH_BEARER_TOKEN, /* server requests an OAuth Bearer token */ + PQAUTHDATA_OAUTH_BEARER_TOKEN, /* server requests an OAuth Bearer token + * (prefer v2, below, instead) */ + PQAUTHDATA_OAUTH_BEARER_TOKEN_V2, /* newest API for OAuth Bearer tokens */ } PGauthData; /* PGconn encapsulates a connection to the backend. @@ -729,6 +735,7 @@ extern int PQenv2encoding(void); /* === in fe-auth.c === */ +/* Authdata for PQAUTHDATA_PROMPT_OAUTH_DEVICE */ typedef struct _PGpromptOAuthDevice { const char *verification_uri; /* verification URI to visit */ @@ -745,6 +752,7 @@ typedef struct _PGpromptOAuthDevice #define PQ_SOCKTYPE int #endif +/* Authdata for PQAUTHDATA_OAUTH_BEARER_TOKEN */ typedef struct PGoauthBearerRequest { /* Hook inputs (constant across all calls) */ @@ -800,6 +808,28 @@ typedef struct PGoauthBearerRequest #undef PQ_SOCKTYPE +struct PQExpBufferData; /* see pqexpbuffer.h */ + +/* Authdata for PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 */ +typedef struct +{ + PGoauthBearerRequest v1; /* see the PGoauthBearerRequest struct, above */ + + /* Hook inputs (constant across all calls) */ + const char *issuer; /* the issuer identifier (RFC 9207) in use, as + * derived from the connection's oauth_issuer */ + + /* Hook outputs */ + + /* + * An initialized, empty buffer for reporting errors when the flow fails. + * Implementations may use the API in pqexpbuffer.h to build a custom + * error description. It will be included in the connection's + * PQerrorMessage() output. + */ + struct PQExpBufferData *error; +} PGoauthBearerRequestV2; + extern char *PQencryptPassword(const char *passwd, const char *user); extern char *PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user, const char *algorithm); extern PGresult *PQchangePassword(PGconn *conn, const char *user, const char *passwd); diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index d146c5f567c..265ec43035a 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -668,6 +668,32 @@ cleanup: return success; } +/* + * Helper for handling user flow failures. If the implementation put anything + * into request->error, it's added to conn->errorMessage here. + */ +static void +report_user_flow_error(PGconn *conn, const PGoauthBearerRequestV2 *request) +{ + const char *errmsg = NULL; + + if (PQExpBufferBroken(request->error)) + errmsg = libpq_gettext("out of memory"); + else if (request->error->len) + errmsg = request->error->data; + + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("user-defined OAuth flow failed")); + + if (errmsg) + { + appendPQExpBufferStr(&conn->errorMessage, ": "); + appendPQExpBufferStr(&conn->errorMessage, errmsg); + } + + appendPQExpBufferChar(&conn->errorMessage, '\n'); +} + /* * Callback implementation of conn->async_auth() for a user-defined OAuth flow. * Delegates the retrieval of the token to the application's async callback. @@ -680,20 +706,23 @@ static PostgresPollingStatusType run_user_oauth_flow(PGconn *conn) { fe_oauth_state *state = conn->sasl_state; - PGoauthBearerRequest *request = state->async_ctx; + PGoauthBearerRequestV2 *request = state->async_ctx; PostgresPollingStatusType status; - if (!request->async) + if (!request->v1.async) { libpq_append_conn_error(conn, "user-defined OAuth flow provided neither a token nor an async callback"); return PGRES_POLLING_FAILED; } - status = request->async(conn, request, &conn->altsock); + status = request->v1.async(conn, + (PGoauthBearerRequest *) request, + &conn->altsock); + if (status == PGRES_POLLING_FAILED) { - libpq_append_conn_error(conn, "user-defined OAuth flow failed"); + report_user_flow_error(conn, request); return status; } else if (status == PGRES_POLLING_OK) @@ -703,14 +732,14 @@ run_user_oauth_flow(PGconn *conn) * onto the original string, since it may not be safe for us to free() * it.) */ - if (!request->token) + if (!request->v1.token) { libpq_append_conn_error(conn, "user-defined OAuth flow did not provide a token"); return PGRES_POLLING_FAILED; } - conn->oauth_token = strdup(request->token); + conn->oauth_token = strdup(request->v1.token); if (!conn->oauth_token) { libpq_append_conn_error(conn, "out of memory"); @@ -732,21 +761,23 @@ run_user_oauth_flow(PGconn *conn) } /* - * Cleanup callback for the async user flow. Delegates most of its job to the - * user-provided cleanup implementation, then disconnects the altsock. + * Cleanup callback for the async user flow. Delegates most of its job to + * PGoauthBearerRequestV2.cleanup(), then disconnects the altsock and frees the + * request itself. */ static void cleanup_user_oauth_flow(PGconn *conn) { fe_oauth_state *state = conn->sasl_state; - PGoauthBearerRequest *request = state->async_ctx; + PGoauthBearerRequestV2 *request = state->async_ctx; Assert(request); - if (request->cleanup) - request->cleanup(conn, request); + if (request->v1.cleanup) + request->v1.cleanup(conn, (PGoauthBearerRequest *) request); conn->altsock = PGINVALID_SOCKET; + destroyPQExpBuffer(request->error); free(request); state->async_ctx = NULL; } @@ -968,8 +999,8 @@ use_builtin_flow(PGconn *conn, fe_oauth_state *state) * token for presentation to the server. * * If the application has registered a custom flow handler using - * PQAUTHDATA_OAUTH_BEARER_TOKEN, it may either return a token immediately (e.g. - * if it has one cached for immediate use), or set up for a series of + * PQAUTHDATA_OAUTH_BEARER_TOKEN[_V2], it may either return a token immediately + * (e.g. if it has one cached for immediate use), or set up for a series of * asynchronous callbacks which will be managed by run_user_oauth_flow(). * * If the default handler is used instead, a Device Authorization flow is used @@ -983,27 +1014,44 @@ static bool setup_token_request(PGconn *conn, fe_oauth_state *state) { int res; - PGoauthBearerRequest request = { - .openid_configuration = conn->oauth_discovery_uri, - .scope = conn->oauth_scope, + PGoauthBearerRequestV2 request = { + .v1 = { + .openid_configuration = conn->oauth_discovery_uri, + .scope = conn->oauth_scope, + }, + .issuer = conn->oauth_issuer_id, + .error = createPQExpBuffer(), }; - Assert(request.openid_configuration); + Assert(request.v1.openid_configuration); + Assert(request.issuer); + + if (PQExpBufferBroken(request.error)) + { + libpq_append_conn_error(conn, "out of memory"); + return false; + } + + /* + * The client may have overridden the OAuth flow. Try the v2 hook first, + * then fall back to the v1 implementation. + */ + res = PQauthDataHook(PQAUTHDATA_OAUTH_BEARER_TOKEN_V2, conn, &request); + if (res == 0) + res = PQauthDataHook(PQAUTHDATA_OAUTH_BEARER_TOKEN, conn, &request); - /* The client may have overridden the OAuth flow. */ - res = PQauthDataHook(PQAUTHDATA_OAUTH_BEARER_TOKEN, conn, &request); if (res > 0) { - PGoauthBearerRequest *request_copy; + PGoauthBearerRequestV2 *request_copy; - if (request.token) + if (request.v1.token) { /* * We already have a token, so copy it into the conn. (We can't * hold onto the original string, since it may not be safe for us * to free() it.) */ - conn->oauth_token = strdup(request.token); + conn->oauth_token = strdup(request.v1.token); if (!conn->oauth_token) { libpq_append_conn_error(conn, "out of memory"); @@ -1011,8 +1059,9 @@ setup_token_request(PGconn *conn, fe_oauth_state *state) } /* short-circuit */ - if (request.cleanup) - request.cleanup(conn, &request); + if (request.v1.cleanup) + request.v1.cleanup(conn, (PGoauthBearerRequest *) &request); + destroyPQExpBuffer(request.error); return true; } @@ -1031,7 +1080,7 @@ setup_token_request(PGconn *conn, fe_oauth_state *state) } else if (res < 0) { - libpq_append_conn_error(conn, "user-defined OAuth flow failed"); + report_user_flow_error(conn, &request); goto fail; } else if (!use_builtin_flow(conn, state)) @@ -1043,8 +1092,9 @@ setup_token_request(PGconn *conn, fe_oauth_state *state) return true; fail: - if (request.cleanup) - request.cleanup(conn, &request); + if (request.v1.cleanup) + request.v1.cleanup(conn, (PGoauthBearerRequest *) &request); + destroyPQExpBuffer(request.error); return false; } diff --git a/src/test/modules/oauth_validator/oauth_hook_client.c b/src/test/modules/oauth_validator/oauth_hook_client.c index 15d0cf938a8..b1a3b014079 100644 --- a/src/test/modules/oauth_validator/oauth_hook_client.c +++ b/src/test/modules/oauth_validator/oauth_hook_client.c @@ -20,6 +20,7 @@ #include "getopt_long.h" #include "libpq-fe.h" +#include "pqexpbuffer.h" static int handle_auth_data(PGauthData type, PGconn *conn, void *data); static PostgresPollingStatusType async_cb(PGconn *conn, @@ -36,13 +37,16 @@ usage(char *argv[]) printf("recognized flags:\n"); printf(" -h, --help show this message\n"); + printf(" -v VERSION select the hook API version (default 2)\n"); printf(" --expected-scope SCOPE fail if received scopes do not match SCOPE\n"); printf(" --expected-uri URI fail if received configuration link does not match URI\n"); + printf(" --expected-issuer ISS fail if received issuer does not match ISS (v2 only)\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\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(" --error ERRMSG fail instead, with the given ERRMSG (v2 only)\n"); printf(" --stress-async busy-loop on PQconnectPoll rather than polling\n"); } @@ -51,9 +55,12 @@ 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_issuer = NULL; static const char *expected_scope = NULL; static const char *misbehave_mode = NULL; static char *token = NULL; +static char *errmsg = NULL; +static int hook_version = PQAUTHDATA_OAUTH_BEARER_TOKEN_V2; int main(int argc, char *argv[]) @@ -68,6 +75,8 @@ main(int argc, char *argv[]) {"hang-forever", no_argument, NULL, 1004}, {"misbehave", required_argument, NULL, 1005}, {"stress-async", no_argument, NULL, 1006}, + {"expected-issuer", required_argument, NULL, 1007}, + {"error", required_argument, NULL, 1008}, {0} }; @@ -75,7 +84,7 @@ main(int argc, char *argv[]) PGconn *conn; int c; - while ((c = getopt_long(argc, argv, "h", long_options, NULL)) != -1) + while ((c = getopt_long(argc, argv, "hv:", long_options, NULL)) != -1) { switch (c) { @@ -83,6 +92,18 @@ main(int argc, char *argv[]) usage(argv); return 0; + case 'v': + if (strcmp(optarg, "1") == 0) + hook_version = PQAUTHDATA_OAUTH_BEARER_TOKEN; + else if (strcmp(optarg, "2") == 0) + hook_version = PQAUTHDATA_OAUTH_BEARER_TOKEN_V2; + else + { + usage(argv); + return 1; + } + break; + case 1000: /* --expected-scope */ expected_scope = optarg; break; @@ -111,6 +132,14 @@ main(int argc, char *argv[]) stress_async = true; break; + case 1007: /* --expected-issuer */ + expected_issuer = optarg; + break; + + case 1008: /* --error */ + errmsg = optarg; + break; + default: usage(argv); return 1; @@ -167,16 +196,24 @@ main(int argc, char *argv[]) /* * PQauthDataHook implementation. Replaces the default client flow by handling - * PQAUTHDATA_OAUTH_BEARER_TOKEN. + * PQAUTHDATA_OAUTH_BEARER_TOKEN[_V2]. */ static int handle_auth_data(PGauthData type, PGconn *conn, void *data) { - PGoauthBearerRequest *req = data; + PGoauthBearerRequest *req; + PGoauthBearerRequestV2 *req2 = NULL; + + Assert(hook_version == PQAUTHDATA_OAUTH_BEARER_TOKEN || + hook_version == PQAUTHDATA_OAUTH_BEARER_TOKEN_V2); - if (no_hook || (type != PQAUTHDATA_OAUTH_BEARER_TOKEN)) + if (no_hook || type != hook_version) return 0; + req = data; + if (type == PQAUTHDATA_OAUTH_BEARER_TOKEN_V2) + req2 = data; + if (hang_forever) { /* Start asynchronous processing. */ @@ -221,6 +258,44 @@ handle_auth_data(PGauthData type, PGconn *conn, void *data) } } + if (expected_issuer) + { + if (!req2) + { + fprintf(stderr, "--expected-issuer cannot be combined with -v1\n"); + return -1; + } + + if (!req2->issuer) + { + fprintf(stderr, "expected issuer \"%s\", got NULL\n", expected_issuer); + return -1; + } + + if (strcmp(expected_issuer, req2->issuer) != 0) + { + fprintf(stderr, "expected issuer \"%s\", got \"%s\"\n", expected_issuer, req2->issuer); + return -1; + } + } + + if (errmsg) + { + if (token) + { + fprintf(stderr, "--error cannot be combined with --token\n"); + return -1; + } + else if (!req2) + { + fprintf(stderr, "--error cannot be combined with -v1\n"); + return -1; + } + + appendPQExpBufferStr(req2->error, errmsg); + return -1; + } + req->token = token; return 1; } @@ -273,6 +348,20 @@ misbehave_cb(PGconn *conn, PGoauthBearerRequest *req, pgsocket *altsock) if (strcmp(misbehave_mode, "fail-async") == 0) { /* Just fail "normally". */ + if (errmsg) + { + PGoauthBearerRequestV2 *req2; + + if (hook_version == PQAUTHDATA_OAUTH_BEARER_TOKEN) + { + fprintf(stderr, "--error cannot be combined with -v1\n"); + exit(1); + } + + req2 = (PGoauthBearerRequestV2 *) req; + appendPQExpBufferStr(req2->error, errmsg); + } + return PGRES_POLLING_FAILED; } else if (strcmp(misbehave_mode, "no-token") == 0) diff --git a/src/test/modules/oauth_validator/t/002_client.pl b/src/test/modules/oauth_validator/t/002_client.pl index e6c91fc911c..0cee72ef58e 100644 --- a/src/test/modules/oauth_validator/t/002_client.pl +++ b/src/test/modules/oauth_validator/t/002_client.pl @@ -78,9 +78,9 @@ sub test my $log_start = -s $node->logfile; my ($stdout, $stderr) = run_command(\@cmd); - if (defined($params{expected_stdout})) + if ($params{expect_success}) { - like($stdout, $params{expected_stdout}, "$test_name: stdout matches"); + like($stdout, qr/connection succeeded/, "$test_name: stdout matches"); } if (defined($params{expected_stderr})) @@ -110,11 +110,24 @@ test( flags => [ "--token", "my-token", "--expected-uri", "$issuer/.well-known/openid-configuration", + "--expected-issuer", "$issuer", "--expected-scope", $scope, ], - expected_stdout => qr/connection succeeded/, + expect_success => 1, log_like => [qr/oauth_validator: token="my-token", role="$user"/]); +# Make sure the v1 hook continues to work. */ +test( + "v1 synchronous hook can provide a token", + flags => [ + "-v1", + "--token" => "my-token-v1", + "--expected-uri" => "$issuer/.well-known/openid-configuration", + "--expected-scope" => $scope, + ], + expect_success => 1, + log_like => [qr/oauth_validator: token="my-token-v1", role="$user"/]); + if ($ENV{with_libcurl} ne 'yes') { # libpq should help users out if no OAuth support is built in. @@ -126,6 +139,15 @@ if ($ENV{with_libcurl} ne 'yes') ); } +# v2 synchronous flows should be able to set custom error messages. +test( + "basic synchronous hook can set error messages", + flags => [ + "--error" => "a custom error message", + ], + expected_stderr => + qr/user-defined OAuth flow failed: a custom error message/); + # connect_timeout should work if the flow doesn't respond. $common_connstr = "$common_connstr connect_timeout=1"; test( @@ -165,4 +187,14 @@ foreach my $c (@cases) expected_stderr => $c->{'expected_error'}); } +# v2 async flows should be able to set error messages, too. +test( + "asynchronous hook can set error messages", + flags => [ + "--misbehave" => "fail-async", + "--error" => "async error message", + ], + expected_stderr => + qr/user-defined OAuth flow failed: async error message/); + done_testing(); -- 2.34.1