From 483129c1ca931f60b76ef93bf2f646cea2f00568 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Thu, 6 Feb 2025 13:16:04 -0800 Subject: [PATCH v47 02/11] fixup! Add OAUTHBEARER SASL mechanism --- src/interfaces/libpq/fe-auth-oauth-curl.c | 35 +++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c index 2407200ea97..eeddace7060 100644 --- a/src/interfaces/libpq/fe-auth-oauth-curl.c +++ b/src/interfaces/libpq/fe-auth-oauth-curl.c @@ -973,11 +973,20 @@ parse_access_token(struct async_ctx *actx, struct token *tok) {"access_token", JSON_TOKEN_STRING, {&tok->access_token}, REQUIRED}, {"token_type", JSON_TOKEN_STRING, {&tok->token_type}, REQUIRED}, - /* - * The following fields are technically REQUIRED, but we don't use - * them anywhere yet: + /*--- + * We currently have no use for the following OPTIONAL fields: + * + * - expires_in: This will be important for maintaining a token cache, + * but we do not yet implement one. * - * - scope (only required if different than requested -- TODO check) + * - refresh_token: Ditto. + * + * - scope: This is only sent when the authorization server sees fit to + * change our scope request. It's not clear what we should do + * about this; either it's been done as a matter of policy, or + * the user has explicitly denied part of the authorization, + * and either way the server-side validator is in a better + * place to complain if the change isn't acceptable. */ {0}, @@ -1252,8 +1261,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? + * There might be an optimization opportunity here: if timeout == 0, we + * could signal drive_request to immediately call + * curl_multi_socket_action, rather than returning all the way up the + * stack only to come right back. But it's not clear that the additional + * code complexity is worth it. */ if (!set_timer(actx, timeout)) return -1; /* actx_error already called */ @@ -1415,7 +1427,14 @@ setup_curl_handles(struct async_ctx *actx) CHECK_SETOPT(actx, popt, protos, return false); } - /* TODO: would anyone use this in "real" situations, or just testing? */ + /* + * If we're in debug mode, allow the developer to change the trusted CA + * list. For now, this is not something we expose outside of the UNSAFE + * mode, because it's not clear that it's useful in production: both libpq + * and the user's browser must trust the same authorization servers for + * the flow to work at all, so any changes to the roots are likely to be + * done system-wide. + */ if (actx->debugging) { const char *env; @@ -1824,8 +1843,6 @@ check_issuer(struct async_ctx *actx, PGconn *conn) * of the authorization server where the authorization request was * sent to. This comparison MUST use simple string comparison as defined * in Section 6.2.1 of [RFC3986]. - * - * TODO: Encoding support? */ if (strcmp(conn->oauth_issuer_id, provider->issuer) != 0) { -- 2.34.1