commit f8bbb0f2a2e0f4840bdeb5c9a7b9a35797280aaf Author: Jacob Champion Date: Mon Dec 16 13:57:14 2024 -0800 require_auth: prepare for multiple SASL mechanisms Prior to this patch, the require_auth implementation assumed that the AuthenticationSASL protocol message was synonymous with SCRAM-SHA-256. In preparation for the OAUTHBEARER SASL mechanism, split the implementation into two tiers: the first checks the acceptable AUTH_REQ_* codes, and the second checks acceptable mechanisms if AUTH_REQ_SASL et al are permitted. conn->allowed_sasl_mechs is the list of pointers to acceptable mechanisms. (Since we'll support only a small number of mechanisms, this is an array of static length to minimize bookkeeping.) pg_SASL_init() will bail if the selected mechanism isn't contained in this array. Since there's only one mechansism supported right now, one branch of the second tier cannot be exercised yet (it's marked with Assert(false)). This assertion will need to be removed when the next mechanism is added. diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 14a9a862f51..722bb47ee14 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -543,6 +543,35 @@ pg_SASL_init(PGconn *conn, int payloadlen) goto error; } + /* Make sure require_auth is satisfied. */ + if (conn->require_auth) + { + bool allowed = false; + + for (int i = 0; i < lengthof(conn->allowed_sasl_mechs); i++) + { + if (conn->sasl == conn->allowed_sasl_mechs[i]) + { + allowed = true; + break; + } + } + + if (!allowed) + { + /* + * TODO: this is dead code until a second SASL mechanism is added; + * the connection can't have proceeded past check_expected_areq() + * if no SASL methods are allowed. + */ + Assert(false); + + libpq_append_conn_error(conn, "authentication method requirement \"%s\" failed: server requested %s authentication", + conn->require_auth, selected_mechanism); + goto error; + } + } + if (conn->channel_binding[0] == 'r' && /* require */ strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0) { diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 8f211821eb2..6f262706b0a 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1110,6 +1110,56 @@ libpq_prng_init(PGconn *conn) pg_prng_seed(&conn->prng_state, rseed); } +/* + * Fills the connection's allowed_sasl_mechs list with all supported SASL + * mechanisms. + */ +static inline void +fill_allowed_sasl_mechs(PGconn *conn) +{ + /*--- + * We only support one mechanism at the moment, so rather than deal with a + * linked list, conn->allowed_sasl_mechs is an array of static length. We + * rely on the compile-time assertion here to keep us honest. + * + * To add a new mechanism to require_auth, + * - update the length of conn->allowed_sasl_mechs, + * - add the new pg_fe_sasl_mech pointer to this function, and + * - handle the new mechanism name in the require_auth portion of + * pqConnectOptions2(), below. + */ + StaticAssertDecl(lengthof(conn->allowed_sasl_mechs) == 1, + "fill_allowed_sasl_mechs() must be updated when resizing conn->allowed_sasl_mechs[]"); + + conn->allowed_sasl_mechs[0] = &pg_scram_mech; +} + +/* + * Clears the connection's allowed_sasl_mechs list. + */ +static inline void +clear_allowed_sasl_mechs(PGconn *conn) +{ + for (int i = 0; i < lengthof(conn->allowed_sasl_mechs); i++) + conn->allowed_sasl_mechs[i] = NULL; +} + +/* + * Helper routine that searches the static allowed_sasl_mechs list for a + * specific mechanism. + */ +static inline int +index_of_allowed_sasl_mech(PGconn *conn, const pg_fe_sasl_mech *mech) +{ + for (int i = 0; i < lengthof(conn->allowed_sasl_mechs); i++) + { + if (conn->allowed_sasl_mechs[i] == mech) + return i; + } + + return -1; +} + /* * pqConnectOptions2 * @@ -1351,17 +1401,19 @@ pqConnectOptions2(PGconn *conn) bool negated = false; /* - * By default, start from an empty set of allowed options and add to - * it. + * By default, start from an empty set of allowed methods and + * mechanisms, and add to it. */ conn->auth_required = true; conn->allowed_auth_methods = 0; + clear_allowed_sasl_mechs(conn); for (first = true, more = true; more; first = false) { char *method, *part; - uint32 bits; + uint32 bits = 0; + const pg_fe_sasl_mech *mech = NULL; part = parse_comma_separated_list(&s, &more); if (part == NULL) @@ -1377,11 +1429,12 @@ pqConnectOptions2(PGconn *conn) if (first) { /* - * Switch to a permissive set of allowed options, and - * subtract from it. + * Switch to a permissive set of allowed methods and + * mechanisms, and subtract from it. */ conn->auth_required = false; conn->allowed_auth_methods = -1; + fill_allowed_sasl_mechs(conn); } else if (!negated) { @@ -1406,6 +1459,10 @@ pqConnectOptions2(PGconn *conn) return false; } + /* + * First group: methods that can be handled solely with the + * authentication request codes. + */ if (strcmp(method, "password") == 0) { bits = (1 << AUTH_REQ_PASSWORD); @@ -1424,13 +1481,22 @@ pqConnectOptions2(PGconn *conn) bits = (1 << AUTH_REQ_SSPI); bits |= (1 << AUTH_REQ_GSS_CONT); } + + /* + * Next group: SASL mechanisms. All of these use the same request + * codes, so the list of allowed mechanisms is tracked separately. + * + * fill_allowed_sasl_mechs() must be updated when adding a new + * mechanism here! + */ else if (strcmp(method, "scram-sha-256") == 0) { - /* This currently assumes that SCRAM is the only SASL method. */ - bits = (1 << AUTH_REQ_SASL); - bits |= (1 << AUTH_REQ_SASL_CONT); - bits |= (1 << AUTH_REQ_SASL_FIN); + mech = &pg_scram_mech; } + + /* + * Final group: meta-options. + */ else if (strcmp(method, "none") == 0) { /* @@ -1466,20 +1532,68 @@ pqConnectOptions2(PGconn *conn) return false; } - /* Update the bitmask. */ - if (negated) + if (mech) { - if ((conn->allowed_auth_methods & bits) == 0) - goto duplicate; + /* + * Update the mechanism set only. The method bitmask will be + * updated for SASL further down. + */ + Assert(!bits); + + if (negated) + { + /* Remove the existing mechanism from the list. */ + i = index_of_allowed_sasl_mech(conn, mech); + if (i < 0) + goto duplicate; - conn->allowed_auth_methods &= ~bits; + conn->allowed_sasl_mechs[i] = NULL; + } + else + { + /* + * Find a space to put the new mechanism (after making + * sure it's not already there). + */ + i = index_of_allowed_sasl_mech(conn, mech); + if (i >= 0) + goto duplicate; + + i = index_of_allowed_sasl_mech(conn, NULL); + if (i < 0) + { + /* Should not happen; the pointer list is corrupted. */ + Assert(false); + + conn->status = CONNECTION_BAD; + libpq_append_conn_error(conn, + "internal error: no space in allowed_sasl_mechs"); + free(part); + return false; + } + + conn->allowed_sasl_mechs[i] = mech; + } } else { - if ((conn->allowed_auth_methods & bits) == bits) - goto duplicate; + /* Update the method bitmask. */ + Assert(bits); + + if (negated) + { + if ((conn->allowed_auth_methods & bits) == 0) + goto duplicate; + + conn->allowed_auth_methods &= ~bits; + } + else + { + if ((conn->allowed_auth_methods & bits) == bits) + goto duplicate; - conn->allowed_auth_methods |= bits; + conn->allowed_auth_methods |= bits; + } } free(part); @@ -1498,6 +1612,36 @@ pqConnectOptions2(PGconn *conn) free(part); return false; } + + /* + * Finally, allow SASL authentication requests if (and only if) we've + * allowed any mechanisms. + */ + { + bool allowed = false; + const uint32 sasl_bits = + (1 << AUTH_REQ_SASL) + | (1 << AUTH_REQ_SASL_CONT) + | (1 << AUTH_REQ_SASL_FIN); + + for (i = 0; i < lengthof(conn->allowed_sasl_mechs); i++) + { + if (conn->allowed_sasl_mechs[i]) + { + allowed = true; + break; + } + } + + /* + * For the standard case, add the SASL bits to the (default-empty) + * set if needed. For the negated case, remove them. + */ + if (!negated && allowed) + conn->allowed_auth_methods |= sasl_bits; + else if (negated && !allowed) + conn->allowed_auth_methods &= ~sasl_bits; + } } /* diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 4a5a7c8b5e3..d372276c486 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -501,6 +501,8 @@ struct pg_conn * the server? */ uint32 allowed_auth_methods; /* bitmask of acceptable AuthRequest * codes */ + const pg_fe_sasl_mech *allowed_sasl_mechs[1]; /* and acceptable SASL + * mechanisms */ bool client_finished_auth; /* have we finished our half of the * authentication exchange? */ char current_auth_response; /* used by pqTraceOutputMessage to diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index 773238b76fd..1357f806b6f 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -277,6 +277,16 @@ $node->connect_fails( "require_auth methods cannot be duplicated, !none case", expected_stderr => qr/require_auth method "!none" is specified more than once/); +$node->connect_fails( + "user=scram_role require_auth=scram-sha-256,scram-sha-256", + "require_auth methods cannot be duplicated, scram-sha-256 case", + expected_stderr => + qr/require_auth method "scram-sha-256" is specified more than once/); +$node->connect_fails( + "user=scram_role require_auth=!scram-sha-256,!scram-sha-256", + "require_auth methods cannot be duplicated, !scram-sha-256 case", + expected_stderr => + qr/require_auth method "!scram-sha-256" is specified more than once/); # Unknown value defined in require_auth. $node->connect_fails(