From 84c6893325491fc860bcb903741e894b60478f9a Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 28 Mar 2024 21:59:02 +0100 Subject: [PATCH v23 6/6] Review comments Fixes and tidy-ups following a review of v21, a few items are (listed in no specific order): * Implement a version check for libcurl in autoconf, the equivalent check for Meson is still a TODO. * Address a few TODOs in the code * libpq JSON support memory management fixups [ed: these have been moved to an earlier commit] --- config/programs.m4 | 21 ++ configure | 34 +++ configure.ac | 1 + src/backend/libpq/auth-oauth.c | 28 +- src/include/common/oauth-common.h | 2 +- src/interfaces/libpq/Makefile | 4 +- src/interfaces/libpq/fe-auth-oauth-curl.c | 275 +++++++++++-------- src/interfaces/libpq/fe-auth-oauth.c | 9 +- src/interfaces/libpq/fe-auth-oauth.h | 2 +- src/test/modules/oauth_validator/validator.c | 2 +- src/test/perl/PostgreSQL/Test/OAuthServer.pm | 10 +- src/tools/pgindent/typedefs.list | 1 + 12 files changed, 256 insertions(+), 133 deletions(-) diff --git a/config/programs.m4 b/config/programs.m4 index 490ec9fe9f..157da7eec5 100644 --- a/config/programs.m4 +++ b/config/programs.m4 @@ -142,7 +142,28 @@ if test "$pgac_cv_ldap_safe" != yes; then *** also uses LDAP will crash on exit.]) fi]) +# PGAC_CHECK_LIBCURL +# ------------------ +# Check for libcurl 8.4.0 or higher since earlier versions can be compiled +# with a codepatch containing exit(), and PostgreSQL does not allow any lib +# linked to libpq which can call exit. +# PGAC_CHECK_LIBCURL +AC_DEFUN([PGAC_CHECK_LIBCURL], +[AC_CACHE_CHECK([for compatible libcurl], [pgac_cv_check_libcurl], +[AC_COMPILE_IFELSE([AC_LANG_PROGRAM( +[#include +#if LIBCURL_VERSION_MAJOR <= 8 && LIBCURL_VERSION_MINOR < 4 +choke me +#endif], [])], +[pgac_cv_check_libcurl=yes], +[pgac_cv_check_libcurl=no])]) + +if test "$pgac_cv_check_libcurl" != yes; then + AC_MSG_ERROR([ +*** The installed version of libcurl is too old to use with PostgreSQL. +*** libcurl version 8.4.0 or later is required.]) +fi]) # PGAC_CHECK_READLINE # ------------------- diff --git a/configure b/configure index 864bb9b7c3..1a785006b9 100755 --- a/configure +++ b/configure @@ -13137,6 +13137,40 @@ else as_fn_error $? "library 'curl' is required for --with-oauth=curl" "$LINENO" 5 fi + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for compatible libcurl" >&5 +$as_echo_n "checking for compatible libcurl... " >&6; } +if ${pgac_cv_check_libcurl+:} false; then : + $as_echo_n "(cached) " >&6 +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +#if LIBCURL_VERSION_MAJOR <= 8 && LIBCURL_VERSION_MINOR < 4 +choke me +#endif +int +main () +{ + + ; + return 0; +} +_ACEOF +if ac_fn_c_try_compile "$LINENO"; then : + pgac_cv_check_libcurl=yes +else + pgac_cv_check_libcurl=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_check_libcurl" >&5 +$as_echo "$pgac_cv_check_libcurl" >&6; } + +if test "$pgac_cv_check_libcurl" != yes; then + as_fn_error $? " +*** The installed version of libcurl is too old to use with PostgreSQL. +*** libcurl version 8.4.0 or later is required." "$LINENO" 5 +fi fi # for contrib/sepgsql diff --git a/configure.ac b/configure.ac index 6aceed898c..cdc6bea660 100644 --- a/configure.ac +++ b/configure.ac @@ -1445,6 +1445,7 @@ AC_SUBST(LDAP_LIBS_BE) if test "$with_oauth" = curl ; then AC_CHECK_LIB(curl, curl_multi_init, [], [AC_MSG_ERROR([library 'curl' is required for --with-oauth=curl])]) + PGAC_CHECK_LIBCURL fi # for contrib/sepgsql diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 024f304e4d..ec1418c3fc 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -476,9 +476,9 @@ generate_error_response(struct oauth_ctx *ctx, char **output, int *outputlen) initStringInfo(&buf); /* - * TODO: note that escaping here should be belt-and-suspenders, since - * escapable characters aren't valid in either the issuer URI or the scope - * list, but the HBA doesn't enforce that yet. + * Escaping the string here is belt-and-suspenders defensive programming + * since escapable characters aren't valid in either the issuer URI or the + * scope list, but the HBA doesn't enforce that yet. */ appendStringInfoString(&buf, "{ \"status\": \"invalid_token\", "); @@ -533,7 +533,9 @@ validate_token_format(const char *header) if (!header || strlen(header) <= 7) { ereport(COMMERROR, - (errmsg("malformed OAuth bearer token 1"))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is less than 8 bytes.")); return NULL; } @@ -551,9 +553,9 @@ validate_token_format(const char *header) if (!*token) { ereport(COMMERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("malformed OAuth bearer token 2"), - errdetail("Bearer token is empty."))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is empty.")); return NULL; } @@ -573,9 +575,9 @@ validate_token_format(const char *header) * of someone's password into the logs. */ ereport(COMMERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("malformed OAuth bearer token 3"), - errdetail("Bearer token is not in the correct format."))); + errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("malformed OAuth bearer token"), + errdetail_log("Bearer token is not in the correct format.")); return NULL; } @@ -617,10 +619,10 @@ validate(Port *port, const char *auth) /* Make sure the validator authenticated the user. */ if (ret->authn_id == NULL || ret->authn_id[0] == '\0') { - /* TODO: use logdetail; reduce message duplication */ ereport(LOG, - (errmsg("OAuth bearer authentication failed for user \"%s\": validator provided no identity", - port->user_name))); + errmsg("OAuth bearer authentication failed for user \"%s\"", + port->user_name), + errdetail_log("Validator provided no identity")); return false; } diff --git a/src/include/common/oauth-common.h b/src/include/common/oauth-common.h index 5ff3488bfb..8fe5626778 100644 --- a/src/include/common/oauth-common.h +++ b/src/include/common/oauth-common.h @@ -3,7 +3,7 @@ * oauth-common.h * Declarations for helper functions used for OAuth/OIDC authentication * - * Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * * src/include/common/oauth-common.h diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 0e0369cb63..9a290782f2 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -116,6 +116,8 @@ backend_src = $(top_srcdir)/src/backend # which seems to insert references to that even in pure C code. Excluding # __tsan_func_exit is necessary when using ThreadSanitizer data race detector # which use this function for instrumentation of function exit. +# libcurl registers an exit handler in the memory debugging code when running +# with LeakSanitizer. # Skip the test when profiling, as gcc may insert exit() calls for that. # Also skip the test on platforms where libpq infrastructure may be provided # by statically-linked libraries, as we can't expect them to honor this @@ -123,7 +125,7 @@ backend_src = $(top_srcdir)/src/backend libpq-refs-stamp: $(shlib) ifneq ($(enable_coverage), yes) ifeq (,$(filter solaris,$(PORTNAME))) - @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \ + @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit -e _atexit | grep exit; then \ echo 'libpq must not be calling any function which invokes exit'; exit 1; \ fi endif diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c index 0504f96e4e..9dd8454cac 100644 --- a/src/interfaces/libpq/fe-auth-oauth-curl.c +++ b/src/interfaces/libpq/fe-auth-oauth-curl.c @@ -3,7 +3,7 @@ * fe-auth-oauth-curl.c * The libcurl implementation of OAuth/OIDC authentication. * - * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION @@ -31,6 +31,8 @@ #include "libpq-int.h" #include "mb/pg_wchar.h" +#define MAX_OAUTH_RESPONSE_SIZE (1024 * 1024) + /* * Parsed JSON Representations * @@ -143,7 +145,7 @@ free_token(struct token *tok) /* States for the overall async machine. */ typedef enum { - OAUTH_STEP_INIT, + OAUTH_STEP_INIT = 0, OAUTH_STEP_DISCOVERY, OAUTH_STEP_DEVICE_AUTHORIZATION, OAUTH_STEP_TOKEN_REQUEST, @@ -151,8 +153,9 @@ typedef enum } OAuthStep; /* - * The async_ctx holds onto state that needs to persist across multiple calls to - * pg_fe_run_oauth_flow(). Almost everything interacts with this in some way. + * The async_ctx holds onto state that needs to persist across multiple calls + * to pg_fe_run_oauth_flow(). Almost everything interacts with this in some + * way. */ struct async_ctx { @@ -162,9 +165,10 @@ struct async_ctx int timerfd; /* a timerfd for signaling async timeouts */ #endif pgsocket mux; /* the multiplexer socket containing all - * descriptors tracked by cURL, plus the + * descriptors tracked by libcurl, plus the * timerfd */ - CURLM *curlm; /* top-level multi handle for cURL operations */ + CURLM *curlm; /* top-level multi handle for libcurl + * operations */ CURL *curl; /* the (single) easy handle for serial * requests */ @@ -183,7 +187,7 @@ struct async_ctx * actx_error[_str] to manipulate this. This must be filled * with something useful on an error. * - * - curl_err: an optional static error buffer used by cURL to put + * - curl_err: an optional static error buffer used by libcurl to put * detailed information about failures. Unfortunately * untranslatable. * @@ -195,7 +199,7 @@ struct async_ctx */ const char *errctx; /* not freed; must point to static allocation */ PQExpBufferData errbuf; - char curl_err[CURL_ERROR_SIZE]; + PQExpBufferData curl_err; /* * These documents need to survive over multiple calls, and are therefore @@ -205,6 +209,8 @@ struct async_ctx struct device_authz authz; bool user_prompted; /* have we already sent the authz prompt? */ + + int running; }; /* @@ -238,7 +244,7 @@ free_curl_async_ctx(PGconn *conn, void *ctx) if (err) libpq_append_conn_error(conn, - "cURL easy handle removal failed: %s", + "libcurl easy handle removal failed: %s", curl_multi_strerror(err)); } @@ -258,7 +264,7 @@ free_curl_async_ctx(PGconn *conn, void *ctx) if (err) libpq_append_conn_error(conn, - "cURL multi handle cleanup failed: %s", + "libcurl multi handle cleanup failed: %s", curl_multi_strerror(err)); } @@ -292,8 +298,8 @@ free_curl_async_ctx(PGconn *conn, void *ctx) appendPQExpBufferStr(&(ACTX)->errbuf, S) /* - * Macros for getting and setting state for the connection's two cURL handles, - * so you don't have to write out the error handling every time. + * Macros for getting and setting state for the connection's two libcurl + * handles, so you don't have to write out the error handling every time. */ #define CHECK_MSETOPT(ACTX, OPT, VAL, FAILACTION) \ @@ -622,19 +628,28 @@ parse_oauth_json(struct async_ctx *actx, const struct json_field *fields) actx_error(actx, "no content type was provided"); goto cleanup; } - else if (strcasecmp(content_type, "application/json") != 0) + + /* + * We only check the media-type and not the parameters, so we need to + * perform a length limited comparison and not compare the whole string. + */ + if (pg_strncasecmp(content_type, "application/json", strlen("application/json")) != 0) { - actx_error(actx, "unexpected content type \"%s\"", content_type); - goto cleanup; + actx_error(actx, "unexpected content type: \"%s\"", content_type); + return false; } if (strlen(resp->data) != resp->len) { actx_error(actx, "response contains embedded NULLs"); - goto cleanup; + return false; } - makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true); + if (!makeJsonLexContextCstringLen(&lex, resp->data, resp->len, PG_UTF8, true)) + { + actx_error(actx, "out of memory"); + return false; + } ctx.errbuf = &actx->errbuf; ctx.fields = fields; @@ -787,7 +802,11 @@ parse_device_authz(struct async_ctx *actx, struct device_authz *authz) authz->interval = parse_interval(authz->interval_str); else { - /* TODO: handle default interval of 5 seconds */ + /* + * RFC 8628 specify 5 seconds as the default value if the server + * doesn't provide an interval. + */ + authz->interval = 5; } return true; @@ -838,7 +857,7 @@ parse_access_token(struct async_ctx *actx, struct token *tok) } /* - * cURL Multi Setup/Callbacks + * libcurl Multi Setup/Callbacks */ /* @@ -894,7 +913,7 @@ setup_multiplexer(struct async_ctx *actx) /* * Adds and removes sockets from the multiplexer set, as directed by the - * cURL multi handle. + * libcurl multi handle. */ static int register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx, @@ -925,7 +944,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx, break; default: - actx_error(actx, "unknown cURL socket operation (%d)", what); + actx_error(actx, "unknown libcurl socket operation: %d", what); return -1; } @@ -997,7 +1016,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx, break; default: - actx_error(actx, "unknown cURL socket operation (%d)", what); + actx_error(actx, "unknown libcurl socket operation: %d", what); return -1; } @@ -1018,7 +1037,7 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx, /* * EV_RECEIPT should guarantee one EV_ERROR result for every change, * whether successful or not. Failed entries contain a non-zero errno - * in the `data` field. + * in the data field. */ Assert(ev_out[i].flags & EV_ERROR); @@ -1043,9 +1062,8 @@ register_socket(CURL *curl, curl_socket_t socket, int what, void *ctx, /* * Adds or removes timeouts from the multiplexer set, as directed by the - * cURL multi handle. 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. + * libcurl multi handle. 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. */ static int register_timer(CURLM *curlm, long timeout, void *ctx) @@ -1061,9 +1079,9 @@ register_timer(CURLM *curlm, long timeout, void *ctx) else if (timeout == 0) { /* - * A zero timeout means cURL wants us to call back immediately. That's - * not technically an option for timerfd, but we can make the timeout - * ridiculously short. + * A zero timeout means libcurl wants us to call back immediately. + * That's not technically an option for timerfd, but we can make the + * timeout ridiculously short. * * TODO: maybe just signal drive_request() to immediately call back in * this case? @@ -1098,8 +1116,21 @@ register_timer(CURLM *curlm, long timeout, void *ctx) return 0; } +static int +debug_callback(CURL *handle, curl_infotype *type, char *data, size_t size, + void *clientp) +{ + struct async_ctx *actx = (struct async_ctx *) clientp; + + /* For now we only store TEXT debug information, extending is a TODO */ + if (type == CURLINFO_TEXT) + appendBinaryPQExpBuffer(&actx->curl_err, data, size); + + return 0; +} + /* - * Initializes the two cURL handles in the async_ctx. The multi handle, + * Initializes the two libcurl handles in the async_ctx. The multi handle, * actx->curlm, is what drives the asynchronous engine and tells us what to do * next. The easy handle, actx->curl, encapsulates the state for a single * request/response. It's added to the multi handle as needed, during @@ -1108,17 +1139,17 @@ register_timer(CURLM *curlm, long timeout, void *ctx) static bool setup_curl_handles(struct async_ctx *actx) { - curl_version_info_data *curl_info; + curl_version_info_data *curl_info; /* * Create our multi handle. This encapsulates the entire conversation with - * cURL for this connection. + * libcurl for this connection. */ actx->curlm = curl_multi_init(); if (!actx->curlm) { /* We don't get a lot of feedback on the failure reason. */ - actx_error(actx, "failed to create cURL multi handle"); + actx_error(actx, "failed to create libcurl multi handle"); return false; } @@ -1143,7 +1174,7 @@ setup_curl_handles(struct async_ctx *actx) actx->curl = curl_easy_init(); if (!actx->curl) { - actx_error(actx, "failed to create cURL handle"); + actx_error(actx, "failed to create libcurl handle"); return false; } @@ -1160,9 +1191,14 @@ setup_curl_handles(struct async_ctx *actx) /* No alternative resolver, TODO: warn about timeouts */ } - /* TODO investigate using conn->Pfdebug and CURLOPT_DEBUGFUNCTION here */ + /* + * 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. + */ + CHECK_SETOPT(actx, CURLOPT_DEBUGDATA, actx, return false); + CHECK_SETOPT(actx, CURLOPT_DEBUGFUNCTION, debug_callback, return false); CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false); - CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err, return false); /* * Only HTTP[S] is allowed. TODO: disallow HTTP without user opt-in @@ -1175,7 +1211,12 @@ setup_curl_handles(struct async_ctx *actx) * pretty strict when it comes to provider behavior, so we have to check * what comes back anyway.) */ - actx->headers = curl_slist_append(actx->headers, "Accept:"); /* TODO: check result */ + actx->headers = curl_slist_append(actx->headers, "Accept:"); + if (actx->headers == NULL) + { + actx_error(actx, "out of memory"); + return false; + } CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers, return false); return true; @@ -1186,8 +1227,10 @@ setup_curl_handles(struct async_ctx *actx) */ /* - * Response callback from cURL; appends the response body into actx->work_data. - * See start_request(). + * Response callback from libcurl which appends the response body into + * actx->work_data (see start_request()). The maximum size of the data is + * defined by CURL_MAX_WRITE_SIZE which by default is 16kb (and can only be + * changed by recompiling libcurl). */ static size_t append_data(char *buf, size_t size, size_t nmemb, void *userdata) @@ -1195,9 +1238,19 @@ append_data(char *buf, size_t size, size_t nmemb, void *userdata) PQExpBuffer resp = userdata; size_t len = size * nmemb; - /* TODO: cap the maximum size */ + /* In case we receive data over the threshold, abort the transfer */ + if ((resp->len + len) > MAX_OAUTH_RESPONSE_SIZE) + return 0; + + /* The data passed from libcurl is not null-terminated */ appendBinaryPQExpBuffer(resp, buf, len); - /* TODO: check for broken buffer */ + + /* + * Signal an error in order to abort the transfer in case we ran out of + * memory in accepting the data. + */ + if (PQExpBufferBroken(resp)) + return 0; return len; } @@ -1214,7 +1267,6 @@ static bool start_request(struct async_ctx *actx) { CURLMcode err; - int running; resetPQExpBuffer(&actx->work_data); CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false); @@ -1228,7 +1280,7 @@ start_request(struct async_ctx *actx) return false; } - err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &running); + err = curl_multi_socket_action(actx->curlm, CURL_SOCKET_TIMEOUT, 0, &actx->running); if (err) { actx_error(actx, "asynchronous HTTP request failed: %s", @@ -1237,19 +1289,11 @@ start_request(struct async_ctx *actx) } /* - * Sanity check. - * - * TODO: even though this is nominally an asynchronous process, there are - * apparently operations that can synchronously fail by this point, such - * as connections to closed local ports. Maybe we need to let this case - * fall through to drive_request instead, or else perform a - * curl_multi_info_read immediately. + * Even though this is nominally an asynchronous process, there are some + * operations that can synchronously fail by this point like connections + * to closed local ports. Fall through and leave the sanity check for the + * next state consuming actx. */ - if (running != 1) - { - actx_error(actx, "failed to queue HTTP request"); - return false; - } return true; } @@ -1262,12 +1306,18 @@ static PostgresPollingStatusType drive_request(struct async_ctx *actx) { CURLMcode err; - int running; CURLMsg *msg; int msgs_left; bool done; - err = curl_multi_socket_all(actx->curlm, &running); + /* Sanity check the previous operation */ + if (actx->running != 1) + { + actx_error(actx, "failed to queue HTTP request"); + return false; + } + + err = curl_multi_socket_all(actx->curlm, &actx->running); if (err) { actx_error(actx, "asynchronous HTTP request failed: %s", @@ -1275,7 +1325,7 @@ drive_request(struct async_ctx *actx) return PGRES_POLLING_FAILED; } - if (running) + if (actx->running) { /* We'll come back again. */ return PGRES_POLLING_READING; @@ -1287,7 +1337,7 @@ drive_request(struct async_ctx *actx) if (msg->msg != CURLMSG_DONE) { /* - * Future cURL versions may define new message types; we don't + * Future libcurl versions may define new message types; we don't * know how to handle them, so we'll ignore them. */ continue; @@ -1304,7 +1354,7 @@ drive_request(struct async_ctx *actx) err = curl_multi_remove_handle(actx->curlm, msg->easy_handle); if (err) { - actx_error(actx, "cURL easy handle removal failed: %s", + actx_error(actx, "libcurl easy handle removal failed: %s", curl_multi_strerror(err)); return PGRES_POLLING_FAILED; } @@ -1489,7 +1539,12 @@ start_device_authz(struct async_ctx *actx, PGconn *conn) appendPQExpBuffer(work_buffer, "client_id=%s", conn->oauth_client_id); if (conn->oauth_scope) appendPQExpBuffer(work_buffer, "&scope=%s", conn->oauth_scope); - /* TODO check for broken buffer */ + + if (PQExpBufferBroken(work_buffer)) + { + actx_error(actx, "out of memory"); + return false; + } /* Make our request. */ CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri, return false); @@ -1631,37 +1686,40 @@ finish_token_request(struct async_ctx *actx, struct token *tok) CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false); /* - * Per RFC 6749, Section 5, a successful response uses 200 OK. An error - * response uses either 400 Bad Request or 401 Unauthorized. - * - * TODO: there are references online to 403 appearing in the wild... + * Per RFC 6749, Section 5, a successful response uses 200 OK. */ - if (response_code != 200 - && response_code != 400 - /* && response_code != 401 TODO */ ) + if (response_code == 200) { - actx_error(actx, "unexpected response code %ld", response_code); - return false; + actx->errctx = "failed to parse access token response"; + if (!parse_access_token(actx, tok)) + return false; /* error message already set */ + + return true; } /* - * Pull the fields we care about from the document. + * An error response uses either 400 Bad Request or 401 Unauthorized. + * There are references online to implementations using 403 for error + * return which would violate the specification. For now we stick to the + * specification but we might have to revisit this. */ - if (response_code == 200) + if (response_code == 400 || response_code == 401) { - actx->errctx = "failed to parse access token response"; - if (!parse_access_token(actx, tok)) - return false; /* error message already set */ + if (!parse_token_error(actx, &tok->err)) + return false; + + return true; } - else if (!parse_token_error(actx, &tok->err)) - return false; - return true; + /* Any other response codes are considered invalid */ + actx_error(actx, "unexpected response code %ld", response_code); + return false; } + /* - * The top-level, nonblocking entry point for the cURL implementation. This will - * be called several times to pump the async engine. + * The top-level, nonblocking entry point for the libcurl implementation. This + * will be called several times to pump the async engine. * * The architecture is based on PQconnectPoll(). The first half drives the * connection state forward as necessary, returning if we're not ready to @@ -1682,7 +1740,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) struct token tok = {0}; /* - * XXX This is not safe. cURL has stringent requirements for the thread + * 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 @@ -1691,16 +1749,16 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) * 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 cURL, - * which you can't do before you've made sure it's thread-safe... + * 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 cURL to initialize - * everything else, because other pieces of our client executable may - * already be using cURL 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. + * 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 */ @@ -1729,6 +1787,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) initPQExpBuffer(&actx->work_data); initPQExpBuffer(&actx->errbuf); + initPQExpBuffer(&actx->curl_err); if (!setup_multiplexer(actx)) goto error_return; @@ -1873,16 +1932,20 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) * errors; anything else and we bail. */ err = &tok.err; - if (!err->error || (strcmp(err->error, "authorization_pending") - && strcmp(err->error, "slow_down"))) + if (!err->error) + { + actx_error(actx, "unknown error"); + goto error_return; + } + + if (strcmp(err->error, "authorization_pending") != 0 && + strcmp(err->error, "slow_down") != 0) { - /* TODO handle !err->error */ if (err->error_description) appendPQExpBuffer(&actx->errbuf, "%s ", err->error_description); appendPQExpBuffer(&actx->errbuf, "(%s)", err->error); - goto error_return; } @@ -1892,7 +1955,14 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) */ if (strcmp(err->error, "slow_down") == 0) { - actx->authz.interval += 5; /* TODO check for overflow? */ + int prev_interval = actx->authz.interval; + + actx->authz.interval += 5; + if (actx->authz.interval < prev_interval) + { + actx_error(actx, "slow_down interval overflow"); + goto error_return; + } } /* @@ -1959,21 +2029,8 @@ error_return: else appendPQExpBufferStr(&conn->errorMessage, actx->errbuf.data); - if (actx->curl_err[0]) - { - size_t len; - - appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err); - - /* Sometimes libcurl adds a newline to the error buffer. :( */ - len = conn->errorMessage.len; - if (len >= 2 && conn->errorMessage.data[len - 2] == '\n') - { - conn->errorMessage.data[len - 2] = ')'; - conn->errorMessage.data[len - 1] = '\0'; - conn->errorMessage.len--; - } - } + if (actx->curl_err.len > 0) + appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err.data); appendPQExpBufferStr(&conn->errorMessage, "\n"); diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index 66ee8ff076..61de9ac451 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -3,7 +3,7 @@ * fe-auth-oauth.c * The front-end (client) implementation of OAuth/OIDC authentication. * - * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION @@ -247,7 +247,12 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen) return false; } - makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true); + if (!makeJsonLexContextCstringLen(&lex, msg, msglen, PG_UTF8, true)) + { + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("out of memory")); + return false; + } initPQExpBuffer(&ctx.errbuf); sem.semstate = &ctx; diff --git a/src/interfaces/libpq/fe-auth-oauth.h b/src/interfaces/libpq/fe-auth-oauth.h index 8d4ea45aa8..6e5e946364 100644 --- a/src/interfaces/libpq/fe-auth-oauth.h +++ b/src/interfaces/libpq/fe-auth-oauth.h @@ -4,7 +4,7 @@ * * Definitions for OAuth authentication implementations * - * Portions Copyright (c) 2023, PostgreSQL Global Development Group + * Portions Copyright (c) 2024, PostgreSQL Global Development Group * * src/interfaces/libpq/fe-auth-oauth.h * diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c index 09a4bf61d2..7b4dc9c494 100644 --- a/src/test/modules/oauth_validator/validator.c +++ b/src/test/modules/oauth_validator/validator.c @@ -66,7 +66,7 @@ validate_token(ValidatorModuleState *state, const char *token, const char *role) /* 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", - state->private_data); + state->private_data); res = palloc(sizeof(ValidatorModuleResult)); diff --git a/src/test/perl/PostgreSQL/Test/OAuthServer.pm b/src/test/perl/PostgreSQL/Test/OAuthServer.pm index d96733f531..9e18186f23 100644 --- a/src/test/perl/PostgreSQL/Test/OAuthServer.pm +++ b/src/test/perl/PostgreSQL/Test/OAuthServer.pm @@ -4,10 +4,10 @@ package PostgreSQL::Test::OAuthServer; use warnings; use strict; -use threads; use Scalar::Util; use Socket; use IO::Select; +use Test::More; local *server_socket; @@ -34,9 +34,9 @@ sub run my $port; my $pid = open(my $read_fh, "-|", $ENV{PYTHON}, "t/oauth_server.py") - // 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); @@ -45,14 +45,14 @@ sub run $self->{'port'} = $port; $self->{'child'} = $read_fh; - print("# OAuth provider (PID $pid) is listening on port $port\n"); + diag("OAuth provider (PID $pid) is listening on port $port\n"); } sub stop { my $self = shift; - print("# Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n"); + diag("Sending SIGTERM to OAuth provider PID: $self->{'pid'}\n"); kill(15, $self->{'pid'}); $self->{'pid'} = undef; diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index b02ee48898..7e9b1f564a 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3057,6 +3057,7 @@ VacuumStmt ValidIOData ValidateIndexState ValidatorModuleState +ValidatorModuleResult ValuesScan ValuesScanState Var -- 2.34.1