public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/3] bootstrap: convert Typ to a List* 4+ messages / 3 participants [nested] [flat]
* [PATCH 1/3] bootstrap: convert Typ to a List* @ 2020-11-20 02:48 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 69 ++++++++++++++----------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..18eb62ca47 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -159,7 +159,7 @@ struct typmap FormData_pg_type am_typ; }; -static struct typmap **Typ = NULL; +static List *Typ = NIL; /* List of struct typmap* */ static struct typmap *Ap = NULL; static Datum values[MAXATTR]; /* current row's attribute values */ @@ -597,7 +597,7 @@ boot_openrel(char *relname) * pg_type must be filled before any OPEN command is executed, hence we * can now populate the Typ array if we haven't yet. */ - if (Typ == NULL) + if (Typ == NIL) populate_typ_array(); if (boot_reldesc != NULL) @@ -688,7 +688,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness) typeoid = gettype(type); - if (Typ != NULL) + if (Typ != NIL) { attrtypes[attnum]->atttypid = Ap->am_oid; attrtypes[attnum]->attlen = Ap->am_typ.typlen; @@ -877,36 +877,25 @@ populate_typ_array(void) Relation rel; TableScanDesc scan; HeapTuple tup; - int nalloc; - int i; - - Assert(Typ == NULL); - nalloc = 512; - Typ = (struct typmap **) - MemoryContextAlloc(TopMemoryContext, nalloc * sizeof(struct typmap *)); + Assert(Typ == NIL); rel = table_open(TypeRelationId, NoLock); scan = table_beginscan_catalog(rel, 0, NULL); - i = 0; while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup); + struct typmap *newtyp; + MemoryContext old; - /* make sure there will be room for a trailing NULL pointer */ - if (i >= nalloc - 1) - { - nalloc *= 2; - Typ = (struct typmap **) - repalloc(Typ, nalloc * sizeof(struct typmap *)); - } - Typ[i] = (struct typmap *) - MemoryContextAlloc(TopMemoryContext, sizeof(struct typmap)); - Typ[i]->am_oid = typForm->oid; - memcpy(&(Typ[i]->am_typ), typForm, sizeof(Typ[i]->am_typ)); - i++; + old = MemoryContextSwitchTo(TopMemoryContext); + newtyp = (struct typmap *) palloc(sizeof(struct typmap)); + Typ = lappend(Typ, newtyp); + MemoryContextSwitchTo(old); + + newtyp->am_oid = typForm->oid; + memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ)); } - Typ[i] = NULL; /* Fill trailing NULL pointer */ table_endscan(scan); table_close(rel, NoLock); } @@ -925,16 +914,17 @@ populate_typ_array(void) static Oid gettype(char *type) { - if (Typ != NULL) + if (Typ != NIL) { - struct typmap **app; + ListCell *lc; - for (app = Typ; *app != NULL; app++) + foreach (lc, Typ) { - if (strncmp(NameStr((*app)->am_typ.typname), type, NAMEDATALEN) == 0) + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) { - Ap = *app; - return (*app)->am_oid; + Ap = app; + return app->am_oid; } } } @@ -980,14 +970,17 @@ boot_get_type_io_data(Oid typid, if (Typ != NULL) { /* We have the boot-time contents of pg_type, so use it */ - struct typmap **app; - struct typmap *ap; - - app = Typ; - while (*app && (*app)->am_oid != typid) - ++app; - ap = *app; - if (ap == NULL) + struct typmap *ap = NULL; + ListCell *lc; + + foreach (lc, Typ) + { + ap = lfirst(lc); + if (ap->am_oid == typid) + break; + } + + if (!ap || ap->am_oid != typid) elog(ERROR, "type OID %u not found in Typ list", typid); *typlen = ap->am_typ.typlen; -- 2.26.2 --------------8FD28E9B65A94BB176003065 Content-Type: text/x-patch; charset=UTF-8; name="0002-Allow-composite-types-in-bootstrap-20210116.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Allow-composite-types-in-bootstrap-20210116.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Refactor SASL exchange in preparation for OAuth Bearer @ 2024-02-28 22:54 Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Daniel Gustafsson @ 2024-02-28 22:54 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> > On 26 Feb 2024, at 19:56, Jacob Champion <[email protected]> wrote: >> + * SASL_FAILED: The exchance has failed and the connection should be > > s/exchance/exchange/ I rank that as one of my better typos actually. Fixed though. >> - if (final && !done) >> + if (final && !(status == SASL_FAILED || status == SASL_COMPLETE)) > > Since there's not yet a SASL_ASYNC, I wonder if this would be more > readable if it were changed to > if (final && status == SASL_CONTINUE) > to match the if condition shortly after it. Fair point, that's more readable in this commit. > In 0002, at the beginning of pg_SASL_init, the `password` variable now > has an uninitialized code path. The OAuth patchset initializes it to > NULL: Nice catch, fixed. -- Daniel Gustafsson Attachments: [application/octet-stream] v2-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch (9.9K, ../../[email protected]/2-v2-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch) download | inline diff: From ae32698c528fce4829d4deccb922699d0b516f88 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Fri, 23 Feb 2024 11:09:54 +0100 Subject: [PATCH v2 1/2] Refactor SASL exchange to return tri-state status The SASL exchange callback returned state in to output variables: done and success. This refactors that logic by introducing a new return variable of type SASLStatus which makes the code easier to read and understand, and prepares for future SASL exchanges which operate asynchronously. This was extracted from a larger patchset to introduce OAuthBearer authentication and authorization. Author: Jacob Champion <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/interfaces/libpq/fe-auth-sasl.h | 31 +++++++---- src/interfaces/libpq/fe-auth-scram.c | 78 +++++++++++++--------------- src/interfaces/libpq/fe-auth.c | 28 +++++----- src/tools/pgindent/typedefs.list | 1 + 4 files changed, 71 insertions(+), 67 deletions(-) diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h index ee5d1525b5..4eecf53a15 100644 --- a/src/interfaces/libpq/fe-auth-sasl.h +++ b/src/interfaces/libpq/fe-auth-sasl.h @@ -21,6 +21,17 @@ #include "libpq-fe.h" +/* + * Possible states for the SASL exchange, see the comment on exchange for an + * explanation of these. + */ +typedef enum +{ + SASL_COMPLETE = 0, + SASL_FAILED, + SASL_CONTINUE, +} SASLStatus; + /* * Frontend SASL mechanism callbacks. * @@ -59,7 +70,8 @@ typedef struct pg_fe_sasl_mech * Produces a client response to a server challenge. As a special case * for client-first SASL mechanisms, exchange() is called with a NULL * server response once at the start of the authentication exchange to - * generate an initial response. + * generate an initial response. Returns a SASLStatus indicating the + * state and status of the exchange. * * Input parameters: * @@ -79,22 +91,23 @@ typedef struct pg_fe_sasl_mech * * output: A malloc'd buffer containing the client's response to * the server (can be empty), or NULL if the exchange should - * be aborted. (*success should be set to false in the + * be aborted. (The callback should return SASL_FAILED in the * latter case.) * * outputlen: The length (0 or higher) of the client response buffer, * ignored if output is NULL. * - * done: Set to true if the SASL exchange should not continue, - * because the exchange is either complete or failed + * Return value: * - * success: Set to true if the SASL exchange completed successfully. - * Ignored if *done is false. + * SASL_CONTINUE: The output buffer is filled with a client response. + * Additional server challenge is expected + * SASL_COMPLETE: The SASL exchange has completed successfully. + * SASL_FAILED: The exchange has failed and the connection should be + * dropped. *-------- */ - void (*exchange) (void *state, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success); + SASLStatus (*exchange) (void *state, char *input, int inputlen, + char **output, int *outputlen); /*-------- * channel_bound() diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 04f0e5713d..0bb820e0d9 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -24,9 +24,8 @@ /* The exported SCRAM callback mechanism. */ static void *scram_init(PGconn *conn, const char *password, const char *sasl_mechanism); -static void scram_exchange(void *opaq, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success); +static SASLStatus scram_exchange(void *opaq, char *input, int inputlen, + char **output, int *outputlen); static bool scram_channel_bound(void *opaq); static void scram_free(void *opaq); @@ -202,17 +201,14 @@ scram_free(void *opaq) /* * Exchange a SCRAM message with backend. */ -static void +static SASLStatus scram_exchange(void *opaq, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success) + char **output, int *outputlen) { fe_scram_state *state = (fe_scram_state *) opaq; PGconn *conn = state->conn; const char *errstr = NULL; - *done = false; - *success = false; *output = NULL; *outputlen = 0; @@ -225,12 +221,12 @@ scram_exchange(void *opaq, char *input, int inputlen, if (inputlen == 0) { libpq_append_conn_error(conn, "malformed SCRAM message (empty message)"); - goto error; + return SASL_FAILED; } if (inputlen != strlen(input)) { libpq_append_conn_error(conn, "malformed SCRAM message (length mismatch)"); - goto error; + return SASL_FAILED; } } @@ -240,61 +236,59 @@ scram_exchange(void *opaq, char *input, int inputlen, /* Begin the SCRAM handshake, by sending client nonce */ *output = build_client_first_message(state); if (*output == NULL) - goto error; + return SASL_FAILED; *outputlen = strlen(*output); - *done = false; state->state = FE_SCRAM_NONCE_SENT; - break; + return SASL_CONTINUE; case FE_SCRAM_NONCE_SENT: /* Receive salt and server nonce, send response. */ if (!read_server_first_message(state, input)) - goto error; + return SASL_FAILED; *output = build_client_final_message(state); if (*output == NULL) - goto error; + return SASL_FAILED; *outputlen = strlen(*output); - *done = false; state->state = FE_SCRAM_PROOF_SENT; - break; + return SASL_CONTINUE; case FE_SCRAM_PROOF_SENT: - /* Receive server signature */ - if (!read_server_final_message(state, input)) - goto error; - - /* - * Verify server signature, to make sure we're talking to the - * genuine server. - */ - if (!verify_server_signature(state, success, &errstr)) - { - libpq_append_conn_error(conn, "could not verify server signature: %s", errstr); - goto error; - } - - if (!*success) { - libpq_append_conn_error(conn, "incorrect server signature"); + bool match; + + /* Receive server signature */ + if (!read_server_final_message(state, input)) + return SASL_FAILED; + + /* + * Verify server signature, to make sure we're talking to the + * genuine server. + */ + if (!verify_server_signature(state, &match, &errstr)) + { + libpq_append_conn_error(conn, "could not verify server signature: %s", errstr); + return SASL_FAILED; + } + + if (!match) + { + libpq_append_conn_error(conn, "incorrect server signature"); + } + state->state = FE_SCRAM_FINISHED; + state->conn->client_finished_auth = true; + return match ? SASL_COMPLETE : SASL_FAILED; } - *done = true; - state->state = FE_SCRAM_FINISHED; - state->conn->client_finished_auth = true; - break; default: /* shouldn't happen */ libpq_append_conn_error(conn, "invalid SCRAM exchange state"); - goto error; + break; } - return; -error: - *done = true; - *success = false; + return SASL_FAILED; } /* diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 1a8e4f6fbf..cf8af4c62e 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -423,11 +423,10 @@ pg_SASL_init(PGconn *conn, int payloadlen) { char *initialresponse = NULL; int initialresponselen; - bool done; - bool success; const char *selected_mechanism; PQExpBufferData mechanism_buf; char *password; + SASLStatus status; initPQExpBuffer(&mechanism_buf); @@ -575,12 +574,11 @@ pg_SASL_init(PGconn *conn, int payloadlen) goto oom_error; /* Get the mechanism-specific Initial Client Response, if any */ - conn->sasl->exchange(conn->sasl_state, - NULL, -1, - &initialresponse, &initialresponselen, - &done, &success); + status = conn->sasl->exchange(conn->sasl_state, + NULL, -1, + &initialresponse, &initialresponselen); - if (done && !success) + if (status == SASL_FAILED) goto error; /* @@ -629,10 +627,9 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) { char *output; int outputlen; - bool done; - bool success; int res; char *challenge; + SASLStatus status; /* Read the SASL challenge from the AuthenticationSASLContinue message. */ challenge = malloc(payloadlen + 1); @@ -651,13 +648,12 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) /* For safety and convenience, ensure the buffer is NULL-terminated. */ challenge[payloadlen] = '\0'; - conn->sasl->exchange(conn->sasl_state, - challenge, payloadlen, - &output, &outputlen, - &done, &success); + status = conn->sasl->exchange(conn->sasl_state, + challenge, payloadlen, + &output, &outputlen); free(challenge); /* don't need the input anymore */ - if (final && !done) + if (final && status == SASL_CONTINUE) { if (outputlen != 0) free(output); @@ -670,7 +666,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) * If the exchange is not completed yet, we need to make sure that the * SASL mechanism has generated a message to send back. */ - if (output == NULL && !done) + if (output == NULL && status == SASL_CONTINUE) { libpq_append_conn_error(conn, "no client response found after SASL exchange success"); return STATUS_ERROR; @@ -692,7 +688,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) return STATUS_ERROR; } - if (done && !success) + if (status == SASL_FAILED) return STATUS_ERROR; return STATUS_OK; diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index fc8b15d0cf..2461567026 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2423,6 +2423,7 @@ RuleLock RuleStmt RunningTransactions RunningTransactionsData +SASLStatus SC_HANDLE SECURITY_ATTRIBUTES SECURITY_STATUS -- 2.32.1 (Apple Git-133) [application/octet-stream] v2-0002-Explicitly-require-password-for-SCRAM-exchange.patch (3.2K, ../../[email protected]/3-v2-0002-Explicitly-require-password-for-SCRAM-exchange.patch) download | inline diff: From ed4a2e0c169e7b3a762aa93ee349b90d898962b2 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Fri, 23 Feb 2024 11:19:55 +0100 Subject: [PATCH v2 2/2] Explicitly require password for SCRAM exchange This refactors the SASL init flow to set password_needed on the two SCRAM exchanges currently supported. The code already required this but was set up in such a way that all SASL exchanges required using a password, a restriction which may not hold for all exchanges (the example at hand being the proposed OAuthbearer exchange). This was extracted from a larger patchset to introduce OAuthBearer authentication and authorization. Author: Jacob Champion <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/interfaces/libpq/fe-auth.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index cf8af4c62e..81ec08485d 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -425,7 +425,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) int initialresponselen; const char *selected_mechanism; PQExpBufferData mechanism_buf; - char *password; + char *password = NULL; SASLStatus status; initPQExpBuffer(&mechanism_buf); @@ -446,8 +446,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) /* * Parse the list of SASL authentication mechanisms in the * AuthenticationSASL message, and select the best mechanism that we - * support. SCRAM-SHA-256-PLUS and SCRAM-SHA-256 are the only ones - * supported at the moment, listed by order of decreasing importance. + * support. Mechanisms are listed by order of decreasing importance. */ selected_mechanism = NULL; for (;;) @@ -487,6 +486,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) { selected_mechanism = SCRAM_SHA_256_PLUS_NAME; conn->sasl = &pg_scram_mech; + conn->password_needed = true; } #else /* @@ -522,6 +522,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) { selected_mechanism = SCRAM_SHA_256_NAME; conn->sasl = &pg_scram_mech; + conn->password_needed = true; } } @@ -545,18 +546,19 @@ pg_SASL_init(PGconn *conn, int payloadlen) /* * First, select the password to use for the exchange, complaining if - * there isn't one. Currently, all supported SASL mechanisms require a - * password, so we can just go ahead here without further distinction. + * there isn't one and the selected SASL mechanism needs it. */ - conn->password_needed = true; - password = conn->connhost[conn->whichhost].password; - if (password == NULL) - password = conn->pgpass; - if (password == NULL || password[0] == '\0') + if (conn->password_needed) { - appendPQExpBufferStr(&conn->errorMessage, - PQnoPasswordSupplied); - goto error; + password = conn->connhost[conn->whichhost].password; + if (password == NULL) + password = conn->pgpass; + if (password == NULL || password[0] == '\0') + { + appendPQExpBufferStr(&conn->errorMessage, + PQnoPasswordSupplied); + goto error; + } } Assert(conn->sasl); -- 2.32.1 (Apple Git-133) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Refactor SASL exchange in preparation for OAuth Bearer @ 2024-02-29 19:58 Jacob Champion <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Jacob Champion @ 2024-02-29 19:58 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson <[email protected]> wrote: > I rank that as one of my better typos actually. Fixed though. LGTM! Thanks, --Jacob ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Refactor SASL exchange in preparation for OAuth Bearer @ 2024-03-20 14:28 Daniel Gustafsson <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Daniel Gustafsson @ 2024-03-20 14:28 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]> > On 29 Feb 2024, at 20:58, Jacob Champion <[email protected]> wrote: > > On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson <[email protected]> wrote: >> I rank that as one of my better typos actually. Fixed though. > > LGTM! Thanks for review, and since Heikki marked it ready for committer I assume that counting as a +1 as well. Attached is a rebase on top of HEAD to get a fresh run from the CFBot before applying this. -- Daniel Gustafsson Attachments: [application/octet-stream] v3-0002-Explicitly-require-password-for-SCRAM-exchange.patch (3.2K, ../../[email protected]/2-v3-0002-Explicitly-require-password-for-SCRAM-exchange.patch) download | inline diff: From 2ec88f69b9ad6e98403237a36a72ec5b7282f304 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Fri, 23 Feb 2024 11:19:55 +0100 Subject: [PATCH v3 2/2] Explicitly require password for SCRAM exchange This refactors the SASL init flow to set password_needed on the two SCRAM exchanges currently supported. The code already required this but was set up in such a way that all SASL exchanges required using a password, a restriction which may not hold for all exchanges (the example at hand being the proposed OAuthbearer exchange). This was extracted from a larger patchset to introduce OAuthBearer authentication and authorization. Author: Jacob Champion <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/interfaces/libpq/fe-auth.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index cf8af4c62e..81ec08485d 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -425,7 +425,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) int initialresponselen; const char *selected_mechanism; PQExpBufferData mechanism_buf; - char *password; + char *password = NULL; SASLStatus status; initPQExpBuffer(&mechanism_buf); @@ -446,8 +446,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) /* * Parse the list of SASL authentication mechanisms in the * AuthenticationSASL message, and select the best mechanism that we - * support. SCRAM-SHA-256-PLUS and SCRAM-SHA-256 are the only ones - * supported at the moment, listed by order of decreasing importance. + * support. Mechanisms are listed by order of decreasing importance. */ selected_mechanism = NULL; for (;;) @@ -487,6 +486,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) { selected_mechanism = SCRAM_SHA_256_PLUS_NAME; conn->sasl = &pg_scram_mech; + conn->password_needed = true; } #else /* @@ -522,6 +522,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) { selected_mechanism = SCRAM_SHA_256_NAME; conn->sasl = &pg_scram_mech; + conn->password_needed = true; } } @@ -545,18 +546,19 @@ pg_SASL_init(PGconn *conn, int payloadlen) /* * First, select the password to use for the exchange, complaining if - * there isn't one. Currently, all supported SASL mechanisms require a - * password, so we can just go ahead here without further distinction. + * there isn't one and the selected SASL mechanism needs it. */ - conn->password_needed = true; - password = conn->connhost[conn->whichhost].password; - if (password == NULL) - password = conn->pgpass; - if (password == NULL || password[0] == '\0') + if (conn->password_needed) { - appendPQExpBufferStr(&conn->errorMessage, - PQnoPasswordSupplied); - goto error; + password = conn->connhost[conn->whichhost].password; + if (password == NULL) + password = conn->pgpass; + if (password == NULL || password[0] == '\0') + { + appendPQExpBufferStr(&conn->errorMessage, + PQnoPasswordSupplied); + goto error; + } } Assert(conn->sasl); -- 2.32.1 (Apple Git-133) [application/octet-stream] v3-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch (9.9K, ../../[email protected]/3-v3-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch) download | inline diff: From 874f5fa5398af89d237f8146349cb1cbc859e0af Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Fri, 23 Feb 2024 11:09:54 +0100 Subject: [PATCH v3 1/2] Refactor SASL exchange to return tri-state status The SASL exchange callback returned state in to output variables: done and success. This refactors that logic by introducing a new return variable of type SASLStatus which makes the code easier to read and understand, and prepares for future SASL exchanges which operate asynchronously. This was extracted from a larger patchset to introduce OAuthBearer authentication and authorization. Author: Jacob Champion <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/interfaces/libpq/fe-auth-sasl.h | 31 +++++++---- src/interfaces/libpq/fe-auth-scram.c | 78 +++++++++++++--------------- src/interfaces/libpq/fe-auth.c | 28 +++++----- src/tools/pgindent/typedefs.list | 1 + 4 files changed, 71 insertions(+), 67 deletions(-) diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h index ee5d1525b5..4eecf53a15 100644 --- a/src/interfaces/libpq/fe-auth-sasl.h +++ b/src/interfaces/libpq/fe-auth-sasl.h @@ -21,6 +21,17 @@ #include "libpq-fe.h" +/* + * Possible states for the SASL exchange, see the comment on exchange for an + * explanation of these. + */ +typedef enum +{ + SASL_COMPLETE = 0, + SASL_FAILED, + SASL_CONTINUE, +} SASLStatus; + /* * Frontend SASL mechanism callbacks. * @@ -59,7 +70,8 @@ typedef struct pg_fe_sasl_mech * Produces a client response to a server challenge. As a special case * for client-first SASL mechanisms, exchange() is called with a NULL * server response once at the start of the authentication exchange to - * generate an initial response. + * generate an initial response. Returns a SASLStatus indicating the + * state and status of the exchange. * * Input parameters: * @@ -79,22 +91,23 @@ typedef struct pg_fe_sasl_mech * * output: A malloc'd buffer containing the client's response to * the server (can be empty), or NULL if the exchange should - * be aborted. (*success should be set to false in the + * be aborted. (The callback should return SASL_FAILED in the * latter case.) * * outputlen: The length (0 or higher) of the client response buffer, * ignored if output is NULL. * - * done: Set to true if the SASL exchange should not continue, - * because the exchange is either complete or failed + * Return value: * - * success: Set to true if the SASL exchange completed successfully. - * Ignored if *done is false. + * SASL_CONTINUE: The output buffer is filled with a client response. + * Additional server challenge is expected + * SASL_COMPLETE: The SASL exchange has completed successfully. + * SASL_FAILED: The exchange has failed and the connection should be + * dropped. *-------- */ - void (*exchange) (void *state, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success); + SASLStatus (*exchange) (void *state, char *input, int inputlen, + char **output, int *outputlen); /*-------- * channel_bound() diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 04f0e5713d..0bb820e0d9 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -24,9 +24,8 @@ /* The exported SCRAM callback mechanism. */ static void *scram_init(PGconn *conn, const char *password, const char *sasl_mechanism); -static void scram_exchange(void *opaq, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success); +static SASLStatus scram_exchange(void *opaq, char *input, int inputlen, + char **output, int *outputlen); static bool scram_channel_bound(void *opaq); static void scram_free(void *opaq); @@ -202,17 +201,14 @@ scram_free(void *opaq) /* * Exchange a SCRAM message with backend. */ -static void +static SASLStatus scram_exchange(void *opaq, char *input, int inputlen, - char **output, int *outputlen, - bool *done, bool *success) + char **output, int *outputlen) { fe_scram_state *state = (fe_scram_state *) opaq; PGconn *conn = state->conn; const char *errstr = NULL; - *done = false; - *success = false; *output = NULL; *outputlen = 0; @@ -225,12 +221,12 @@ scram_exchange(void *opaq, char *input, int inputlen, if (inputlen == 0) { libpq_append_conn_error(conn, "malformed SCRAM message (empty message)"); - goto error; + return SASL_FAILED; } if (inputlen != strlen(input)) { libpq_append_conn_error(conn, "malformed SCRAM message (length mismatch)"); - goto error; + return SASL_FAILED; } } @@ -240,61 +236,59 @@ scram_exchange(void *opaq, char *input, int inputlen, /* Begin the SCRAM handshake, by sending client nonce */ *output = build_client_first_message(state); if (*output == NULL) - goto error; + return SASL_FAILED; *outputlen = strlen(*output); - *done = false; state->state = FE_SCRAM_NONCE_SENT; - break; + return SASL_CONTINUE; case FE_SCRAM_NONCE_SENT: /* Receive salt and server nonce, send response. */ if (!read_server_first_message(state, input)) - goto error; + return SASL_FAILED; *output = build_client_final_message(state); if (*output == NULL) - goto error; + return SASL_FAILED; *outputlen = strlen(*output); - *done = false; state->state = FE_SCRAM_PROOF_SENT; - break; + return SASL_CONTINUE; case FE_SCRAM_PROOF_SENT: - /* Receive server signature */ - if (!read_server_final_message(state, input)) - goto error; - - /* - * Verify server signature, to make sure we're talking to the - * genuine server. - */ - if (!verify_server_signature(state, success, &errstr)) - { - libpq_append_conn_error(conn, "could not verify server signature: %s", errstr); - goto error; - } - - if (!*success) { - libpq_append_conn_error(conn, "incorrect server signature"); + bool match; + + /* Receive server signature */ + if (!read_server_final_message(state, input)) + return SASL_FAILED; + + /* + * Verify server signature, to make sure we're talking to the + * genuine server. + */ + if (!verify_server_signature(state, &match, &errstr)) + { + libpq_append_conn_error(conn, "could not verify server signature: %s", errstr); + return SASL_FAILED; + } + + if (!match) + { + libpq_append_conn_error(conn, "incorrect server signature"); + } + state->state = FE_SCRAM_FINISHED; + state->conn->client_finished_auth = true; + return match ? SASL_COMPLETE : SASL_FAILED; } - *done = true; - state->state = FE_SCRAM_FINISHED; - state->conn->client_finished_auth = true; - break; default: /* shouldn't happen */ libpq_append_conn_error(conn, "invalid SCRAM exchange state"); - goto error; + break; } - return; -error: - *done = true; - *success = false; + return SASL_FAILED; } /* diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 1a8e4f6fbf..cf8af4c62e 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -423,11 +423,10 @@ pg_SASL_init(PGconn *conn, int payloadlen) { char *initialresponse = NULL; int initialresponselen; - bool done; - bool success; const char *selected_mechanism; PQExpBufferData mechanism_buf; char *password; + SASLStatus status; initPQExpBuffer(&mechanism_buf); @@ -575,12 +574,11 @@ pg_SASL_init(PGconn *conn, int payloadlen) goto oom_error; /* Get the mechanism-specific Initial Client Response, if any */ - conn->sasl->exchange(conn->sasl_state, - NULL, -1, - &initialresponse, &initialresponselen, - &done, &success); + status = conn->sasl->exchange(conn->sasl_state, + NULL, -1, + &initialresponse, &initialresponselen); - if (done && !success) + if (status == SASL_FAILED) goto error; /* @@ -629,10 +627,9 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) { char *output; int outputlen; - bool done; - bool success; int res; char *challenge; + SASLStatus status; /* Read the SASL challenge from the AuthenticationSASLContinue message. */ challenge = malloc(payloadlen + 1); @@ -651,13 +648,12 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) /* For safety and convenience, ensure the buffer is NULL-terminated. */ challenge[payloadlen] = '\0'; - conn->sasl->exchange(conn->sasl_state, - challenge, payloadlen, - &output, &outputlen, - &done, &success); + status = conn->sasl->exchange(conn->sasl_state, + challenge, payloadlen, + &output, &outputlen); free(challenge); /* don't need the input anymore */ - if (final && !done) + if (final && status == SASL_CONTINUE) { if (outputlen != 0) free(output); @@ -670,7 +666,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) * If the exchange is not completed yet, we need to make sure that the * SASL mechanism has generated a message to send back. */ - if (output == NULL && !done) + if (output == NULL && status == SASL_CONTINUE) { libpq_append_conn_error(conn, "no client response found after SASL exchange success"); return STATUS_ERROR; @@ -692,7 +688,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final) return STATUS_ERROR; } - if (done && !success) + if (status == SASL_FAILED) return STATUS_ERROR; return STATUS_OK; diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e294f8bc4e..c87ef6107a 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2430,6 +2430,7 @@ RuleLock RuleStmt RunningTransactions RunningTransactionsData +SASLStatus SC_HANDLE SECURITY_ATTRIBUTES SECURITY_STATUS -- 2.32.1 (Apple Git-133) ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-03-20 14:28 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 02:48 [PATCH 1/3] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]> 2024-02-28 22:54 Re: Refactor SASL exchange in preparation for OAuth Bearer Daniel Gustafsson <[email protected]> 2024-02-29 19:58 ` Re: Refactor SASL exchange in preparation for OAuth Bearer Jacob Champion <[email protected]> 2024-03-20 14:28 ` Re: Refactor SASL exchange in preparation for OAuth Bearer Daniel Gustafsson <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox