commit 62b6bda852d09458ffcd5854b7810b3b316e2b1f Author: Daniel Gustafsson 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)