> On 28 Feb 2024, at 15:05, Jacob Champion <jacob.champ...@enterprisedb.com> > wrote: > > [Trying again, with all patches unzipped and the CC list temporarily > removed to avoid flooding people's inboxes. Original message follows.] > > On Fri, Feb 23, 2024 at 5:01 PM Jacob Champion > <jacob.champ...@enterprisedb.com> wrote: >> The >> patchset is now carrying a lot of squash-cruft, and I plan to flatten >> it in the next version. > > This is done in v17, which is also now based on the two patches pulled > out by Daniel in [1]. Besides the squashes, which make up most of the > range-diff, I've fixed a call to strncasecmp() which is not available > on Windows. > > Daniel and I discussed trying a Python version of the test server, > since the standard library there should give us more goodies to work > with. A proof of concept is in 0009. I think the big question I have > for it is, how would we communicate what we want the server to do for > the test? (We could perhaps switch on magic values of the client ID?) > In the end I'd like to be testing close to 100% of the failure modes, > and that's likely to mean a lot of back-and-forth if the server > implementation isn't in the Perl process.
Thanks for the new version, I'm digesting the test patches but for now I have a few smaller comments: +#define ALLOC(size) malloc(size) I wonder if we should use pg_malloc_extended(size, MCXT_ALLOC_NO_OOM) instead to self document the code. We clearly don't want feature-parity with server- side palloc here. I know we use malloc in similar ALLOC macros so it's not unique in that regard, but maybe? +#ifdef FRONTEND + destroyPQExpBuffer(lex->errormsg); +#else + pfree(lex->errormsg->data); + pfree(lex->errormsg); +#endif Wouldn't it be nicer if we abstracted this into a destroyStrVal function to a) avoid the ifdefs and b) make it more like the rest of the new API? While it's only used in two places (close to each other) it's a shame to let the underlying API bleed through the abstraction. + CURLM *curlm; /* top-level multi handle for cURL operations */ Nitpick, but curl is not capitalized cURL anymore (for some value of "anymore" since it changed in 2016 [0]). I do wonder if we should consistently write "libcurl" as well since we don't use curl but libcurl. + PQExpBufferData work_data; /* scratch buffer for general use (remember + to clear out prior contents first!) */ This seems like asking for subtle bugs due to uncleared buffers bleeding into another operation (especially since we are writing this data across the wire). How about having an array the size of OAuthStep of unallocated buffers where each step use it's own? Storing the content of each step could also be useful for debugging. Looking at the statemachine here it's not an obvious change but also not impossible. + * TODO: This disables DNS resolution timeouts unless libcurl has been + * compiled against alternative resolution support. We should check that. curl_version_info() can be used to check for c-ares support. + * so you don't have to write out the error handling every time. They assume + * that they're embedded in a function returning bool, however. It feels a bit iffy to encode the returntype in the macro, we can use the same trick that DISABLE_SIGPIPE employs where a failaction is passed in. + if (!strcmp(name, field->name)) Project style is to test for (strcmp(x,y) == 0) rather than (!strcmp()) to improve readability. + libpq_append_conn_error(conn, "out of memory"); While not introduced in this patch, it's not an ideal pattern to report "out of memory" errors via a function which may allocate memory. + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("server's error message contained an embedded NULL")); We should maybe add ", discarding" or something similar after this string to indicate that there was an actual error which has been thrown away, the error wasn't that the server passed an embedded NULL. +#ifdef USE_OAUTH + else if (strcmp(mechanism_buf.data, OAUTHBEARER_NAME) == 0 && + !selected_mechanism) I wonder if we instead should move the guards inside the statement and error out with "not built with OAuth support" or something similar like how we do with TLS and other optional components? + errdetail("Comma expected, but found character %s.", + sanitize_char(*p)))); The %s formatter should be wrapped like '%s' to indicate that the message part is the character in question (and we can then reuse the translation since the error message already exist for SCRAM). + temp = curl_slist_append(temp, "authorization_code"); + if (!temp) + oom = true; + + temp = curl_slist_append(temp, "implicit"); While not a bug per se, it reads a bit odd to call another operation that can allocate memory when the oom flag has been set. I think we can move some things around a little to make it clearer. The attached diff contains some (most?) of the above as a patch on top of your v17, but as a .txt to keep the CFBot from munging on it. -- Daniel Gustafsson
commit 62b6bda852d09458ffcd5854b7810b3b316e2b1f Author: Daniel Gustafsson <dan...@yesql.se> Date: Wed Feb 28 18:26:03 2024 +0100 Suggested changes diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 765a18b9b2..f5cf271566 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -204,7 +204,7 @@ oauth_exchange(void *opaq, const char *input, int inputlen, ereport(ERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("malformed OAUTHBEARER message"), - errdetail("Comma expected, but found character %s.", + errdetail("Comma expected, but found character \"%s\".", sanitize_char(*p)))); p++; break; diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c index 479310f598..2d1f30353a 100644 --- a/src/common/jsonapi.c +++ b/src/common/jsonapi.c @@ -42,6 +42,7 @@ #define appendStrValChar appendPQExpBufferChar #define createStrVal createPQExpBuffer #define resetStrVal resetPQExpBuffer +#define destroyStrVal destroyPQExpBuffer #else /* !FRONTEND */ @@ -53,6 +54,7 @@ #define appendStrValChar appendStringInfoChar #define createStrVal makeStringInfo #define resetStrVal resetStringInfo +#define destroyStrVal destroyStringInfo #endif @@ -223,23 +225,11 @@ freeJsonLexContext(JsonLexContext *lex) static const JsonLexContext empty = {0}; if (lex->flags & JSONLEX_FREE_STRVAL) - { -#ifdef FRONTEND - destroyPQExpBuffer(lex->strval); -#else - pfree(lex->strval->data); - pfree(lex->strval); -#endif - } + destroyStrVal(lex->strval); + if (lex->errormsg) - { -#ifdef FRONTEND - destroyPQExpBuffer(lex->errormsg); -#else - pfree(lex->errormsg->data); - pfree(lex->errormsg); -#endif - } + destroyStrVal(lex->errormsg); + if (lex->flags & JSONLEX_FREE_STRUCT) pfree(lex); else diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c index c61d5c58f3..09419f6042 100644 --- a/src/common/stringinfo.c +++ b/src/common/stringinfo.c @@ -350,3 +350,10 @@ enlargeStringInfo(StringInfo str, int needed) str->maxlen = newlen; } + +void +destroyStringInfo(StringInfo str) +{ + pfree(str->data); + pfree(str); +} diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h index 2cd636b01c..64ec6419af 100644 --- a/src/include/lib/stringinfo.h +++ b/src/include/lib/stringinfo.h @@ -233,4 +233,6 @@ extern void appendBinaryStringInfoNT(StringInfo str, */ extern void enlargeStringInfo(StringInfo str, int needed); + +extern void destroyStringInfo(StringInfo str); #endif /* STRINGINFO_H */ diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c index 3e20ba5818..7a27198d66 100644 --- a/src/interfaces/libpq/fe-auth-oauth-curl.c +++ b/src/interfaces/libpq/fe-auth-oauth-curl.c @@ -293,40 +293,39 @@ free_curl_async_ctx(PGconn *conn, void *ctx) /* * 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. They assume - * that they're embedded in a function returning bool, however. + * so you don't have to write out the error handling every time. */ -#define CHECK_MSETOPT(ACTX, OPT, VAL) \ +#define CHECK_MSETOPT(ACTX, OPT, VAL, FAILACTION) \ do { \ struct async_ctx *_actx = (ACTX); \ CURLMcode _setopterr = curl_multi_setopt(_actx->curlm, OPT, VAL); \ if (_setopterr) { \ actx_error(_actx, "failed to set %s on OAuth connection: %s",\ #OPT, curl_multi_strerror(_setopterr)); \ - return false; \ + FAILACTION; \ } \ } while (0) -#define CHECK_SETOPT(ACTX, OPT, VAL) \ +#define CHECK_SETOPT(ACTX, OPT, VAL, FAILACTION) \ do { \ struct async_ctx *_actx = (ACTX); \ CURLcode _setopterr = curl_easy_setopt(_actx->curl, OPT, VAL); \ if (_setopterr) { \ actx_error(_actx, "failed to set %s on OAuth connection: %s",\ #OPT, curl_easy_strerror(_setopterr)); \ - return false; \ + FAILACTION; \ } \ } while (0) -#define CHECK_GETINFO(ACTX, INFO, OUT) \ +#define CHECK_GETINFO(ACTX, INFO, OUT, FAILACTION) \ do { \ struct async_ctx *_actx = (ACTX); \ CURLcode _getinfoerr = curl_easy_getinfo(_actx->curl, INFO, OUT); \ if (_getinfoerr) { \ actx_error(_actx, "failed to get %s from OAuth response: %s",\ #INFO, curl_easy_strerror(_getinfoerr)); \ - return false; \ + FAILACTION; \ } \ } while (0) @@ -450,7 +449,7 @@ oauth_json_object_field_start(void *state, char *name, bool isnull) while (field->name) { - if (!strcmp(name, field->name)) + if (strcmp(name, field->name) == 0) { ctx->active = field; break; @@ -616,7 +615,7 @@ parse_oauth_json(struct async_ctx *actx, const struct json_field *fields) bool success = false; /* Make sure the server thinks it's given us JSON. */ - CHECK_GETINFO(actx, CURLINFO_CONTENT_TYPE, &content_type); + CHECK_GETINFO(actx, CURLINFO_CONTENT_TYPE, &content_type, return false); if (!content_type) { @@ -1109,6 +1108,8 @@ register_timer(CURLM *curlm, long timeout, void *ctx) 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 * cURL for this connection. @@ -1121,14 +1122,19 @@ setup_curl_handles(struct async_ctx *actx) return false; } + /* + * 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. */ - CHECK_MSETOPT(actx, CURLMOPT_SOCKETFUNCTION, register_socket); - CHECK_MSETOPT(actx, CURLMOPT_SOCKETDATA, actx); - CHECK_MSETOPT(actx, CURLMOPT_TIMERFUNCTION, register_timer); - CHECK_MSETOPT(actx, CURLMOPT_TIMERDATA, actx); + CHECK_MSETOPT(actx, CURLMOPT_SOCKETFUNCTION, register_socket, return false); + CHECK_MSETOPT(actx, CURLMOPT_SOCKETDATA, actx, return false); + CHECK_MSETOPT(actx, CURLMOPT_TIMERFUNCTION, register_timer, return false); + CHECK_MSETOPT(actx, CURLMOPT_TIMERDATA, actx, return false); /* * Set up an easy handle. All of our requests are made serially, so we @@ -1145,25 +1151,26 @@ setup_curl_handles(struct async_ctx *actx) * Multi-threaded applications must set CURLOPT_NOSIGNAL. This requires us * to handle the possibility of SIGPIPE ourselves. * - * TODO: This disables DNS resolution timeouts unless libcurl has been - * compiled against alternative resolution support. We should check that. - * * TODO: handle SIGPIPE via pq_block_sigpipe(), or via a * CURLOPT_SOCKOPTFUNCTION maybe... */ - CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L); + CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L, return false); + if (!curl_info->ares_num) + { + /* No alternative resolver, TODO: warn about timeouts */ + } /* TODO investigate using conn->Pfdebug and CURLOPT_DEBUGFUNCTION here */ - CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L); - CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err); + CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false); + CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err, return false); /* TODO */ - CHECK_SETOPT(actx, CURLOPT_WRITEDATA, stderr); + CHECK_SETOPT(actx, CURLOPT_WRITEDATA, stderr, return false); /* * Only HTTP[S] is allowed. TODO: disallow HTTP without user opt-in */ - CHECK_SETOPT(actx, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + CHECK_SETOPT(actx, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS, return false); /* * Suppress the Accept header to make our request as minimal as possible. @@ -1172,7 +1179,7 @@ setup_curl_handles(struct async_ctx *actx) * what comes back anyway.) */ actx->headers = curl_slist_append(actx->headers, "Accept:"); /* TODO: check result */ - CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers); + CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers, return false); return true; } @@ -1213,8 +1220,8 @@ start_request(struct async_ctx *actx) int running; resetPQExpBuffer(&actx->work_data); - CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data); - CHECK_SETOPT(actx, CURLOPT_WRITEDATA, &actx->work_data); + CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false); + CHECK_SETOPT(actx, CURLOPT_WRITEDATA, &actx->work_data, return false); err = curl_multi_add_handle(actx->curlm, actx->curl); if (err) @@ -1339,8 +1346,8 @@ drive_request(struct async_ctx *actx) static bool start_discovery(struct async_ctx *actx, const char *discovery_uri) { - CHECK_SETOPT(actx, CURLOPT_HTTPGET, 1L); - CHECK_SETOPT(actx, CURLOPT_URL, discovery_uri); + CHECK_SETOPT(actx, CURLOPT_HTTPGET, 1L, return false); + CHECK_SETOPT(actx, CURLOPT_URL, discovery_uri, return false); return start_request(actx); } @@ -1363,7 +1370,7 @@ finish_discovery(struct async_ctx *actx) * validation into question), or non-authoritative responses, or any other * complications. */ - CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code); + CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false); if (response_code != 200) { @@ -1387,20 +1394,17 @@ finish_discovery(struct async_ctx *actx) * Per Section 3, the default is ["authorization_code", "implicit"]. */ struct curl_slist *temp = actx->provider.grant_types_supported; - bool oom = false; temp = curl_slist_append(temp, "authorization_code"); - if (!temp) - oom = true; - - temp = curl_slist_append(temp, "implicit"); - if (!temp) - oom = true; - - if (oom) + if (temp) { - actx_error(actx, "out of memory"); - return false; + temp = curl_slist_append(temp, "implicit"); + if (!temp) + { + curl_slist_free_all(temp); + actx_error(actx, "out of memory"); + return false; + } } actx->provider.grant_types_supported = temp; @@ -1491,8 +1495,8 @@ start_device_authz(struct async_ctx *actx, PGconn *conn) /* TODO check for broken buffer */ /* Make our request. */ - CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri); - CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data); + CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri, return false); + CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data, return false); if (conn->oauth_client_secret) { @@ -1506,12 +1510,12 @@ start_device_authz(struct async_ctx *actx, PGconn *conn) * * TODO: should we omit client_id from the body in this case? */ - CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC); - CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id); - CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret); + CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC, return false); + CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id, return false); + CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret, return false); } else - CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE); + CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE, return false); return start_request(actx); } @@ -1521,7 +1525,7 @@ finish_device_authz(struct async_ctx *actx) { long response_code; - CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code); + CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false); /* * The device authorization endpoint uses the same error response as the @@ -1593,8 +1597,8 @@ start_token_request(struct async_ctx *actx, PGconn *conn) /* TODO check for broken buffer */ /* Make our request. */ - CHECK_SETOPT(actx, CURLOPT_URL, token_uri); - CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data); + CHECK_SETOPT(actx, CURLOPT_URL, token_uri, return false); + CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data, return false); if (conn->oauth_client_secret) { @@ -1608,16 +1612,16 @@ start_token_request(struct async_ctx *actx, PGconn *conn) * * TODO: should we omit client_id from the body in this case? */ - CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC); - CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id); - CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret); + CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC, return false); + CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id, return false); + CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret, return false); } else - CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE); + CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE, return false); resetPQExpBuffer(work_buffer); - CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data); - CHECK_SETOPT(actx, CURLOPT_WRITEDATA, work_buffer); + CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false); + CHECK_SETOPT(actx, CURLOPT_WRITEDATA, work_buffer, return false); return start_request(actx); } @@ -1627,7 +1631,7 @@ finish_token_request(struct async_ctx *actx, struct token *tok) { long response_code; - CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code); + CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false); /* * Per RFC 6749, Section 5, a successful response uses 200 OK. An error @@ -1889,7 +1893,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock) * 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")) + if (strcmp(err->error, "slow_down") == 0) { actx->authz.interval += 5; /* TODO check for overflow? */ } diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index 2a35c7438c..66ee8ff076 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -49,7 +49,7 @@ oauth_init(PGconn *conn, const char *password, * error. */ Assert(sasl_mechanism != NULL); - Assert(!strcmp(sasl_mechanism, OAUTHBEARER_NAME)); + Assert(strcmp(sasl_mechanism, OAUTHBEARER_NAME) == 0); state = calloc(1, sizeof(*state)); if (!state) @@ -156,17 +156,17 @@ oauth_json_object_field_start(void *state, char *name, bool isnull) if (ctx->nested == 1) { - if (!strcmp(name, ERROR_STATUS_FIELD)) + if (strcmp(name, ERROR_STATUS_FIELD) == 0) { ctx->target_field_name = ERROR_STATUS_FIELD; ctx->target_field = &ctx->status; } - else if (!strcmp(name, ERROR_SCOPE_FIELD)) + else if (strcmp(name, ERROR_SCOPE_FIELD) == 0) { ctx->target_field_name = ERROR_SCOPE_FIELD; ctx->target_field = &ctx->scope; } - else if (!strcmp(name, ERROR_OPENID_CONFIGURATION_FIELD)) + else if (strcmp(name, ERROR_OPENID_CONFIGURATION_FIELD) == 0) { ctx->target_field_name = ERROR_OPENID_CONFIGURATION_FIELD; ctx->target_field = &ctx->discovery_uri; @@ -243,7 +243,7 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen) if (strlen(msg) != msglen) { appendPQExpBufferStr(&conn->errorMessage, - libpq_gettext("server's error message contained an embedded NULL")); + libpq_gettext("server's error message contained an embedded NULL, and was discarded")); return false; } @@ -316,7 +316,7 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen) return false; } - if (!strcmp(ctx.status, "invalid_token")) + if (strcmp(ctx.status, "invalid_token") == 0) { /* * invalid_token is the only error code we'll automatically retry for, diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 9e084dd1c7..6e3538c9fd 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -525,15 +525,18 @@ pg_SASL_init(PGconn *conn, int payloadlen, bool *async) conn->sasl = &pg_scram_mech; conn->password_needed = true; } -#ifdef USE_OAUTH else if (strcmp(mechanism_buf.data, OAUTHBEARER_NAME) == 0 && !selected_mechanism) { +#ifdef USE_OAUTH selected_mechanism = OAUTHBEARER_NAME; conn->sasl = &pg_oauth_mech; conn->password_needed = false; - } +#else + libpq_append_conn_error(conn, "OAuth is required, but client does not support it"); + goto error; #endif + } } if (!selected_mechanism)