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