From d747093ac9fe9875ae1f9cc5273e6133627f9691 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 18 Oct 2022 16:55:36 -0700 Subject: [PATCH v11 3/3] require_auth: decouple SASL and SCRAM Rather than assume that an AUTH_REQ_SASL* code refers to SCRAM-SHA-256, future-proof by separating the single allowlist into a list of allowed authentication request codes and a list of allowed SASL mechanisms. The require_auth check is now separated into two tiers. The AUTH_REQ_SASL* codes are always allowed. If the server sends one, responsibility for the check then falls to pg_SASL_init(), which compares the selected mechanism against the list of allowed mechanisms. (Other SCRAM code is already responsible for rejecting unexpected or out-of-order AUTH_REQ_SASL_* codes, so that's not explicitly handled with this addition.) Since there's only one recognized SASL mechanism, conn->sasl_mechs currently only points at static hardcoded lists. Whenever a second mechanism is added, the list will need to be managed dynamically. --- src/interfaces/libpq/fe-auth.c | 35 +++++++++++++++++++ src/interfaces/libpq/fe-connect.c | 42 +++++++++++++++++++---- src/interfaces/libpq/libpq-int.h | 2 ++ src/test/authentication/t/001_password.pl | 10 +++--- src/test/ssl/t/002_scram.pl | 6 ++++ 5 files changed, 84 insertions(+), 11 deletions(-) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 295b978525..24c31ae624 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -536,6 +536,41 @@ pg_SASL_init(PGconn *conn, int payloadlen) goto error; } + /* + * Before going ahead with any SASL exchange, ensure that the user has + * allowed (or, alternatively, has not forbidden) this particular mechanism. + * + * In a hypothetical future where a server responds with multiple SASL + * mechanism families, we would need to instead consult this list up above, + * during mechanism negotiation. We don't live in that world yet. The server + * presents one auth method and we decide whether that's acceptable or not. + */ + if (conn->sasl_mechs) + { + const char **mech; + bool found = false; + + Assert(conn->require_auth); + + for (mech = conn->sasl_mechs; *mech; mech++) + { + if (strcmp(*mech, selected_mechanism) == 0) + { + found = true; + break; + } + } + + if ((conn->sasl_mechs_denied && found) + || (!conn->sasl_mechs_denied && !found)) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("auth method \"%s\" requirement failed: server requested unacceptable SASL mechanism \"%s\"\n"), + 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 eaca8cee1f..be002b4fda 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1259,11 +1259,25 @@ connectOptions2(PGconn *conn) bool first, more; bool negated = false; + static const uint32 default_methods = ( + 1 << AUTH_REQ_SASL + | 1 << AUTH_REQ_SASL_CONT + | 1 << AUTH_REQ_SASL_FIN + ); + static const char *no_mechs[] = { NULL }; + /* - * By default, start from an empty set of allowed options and add to it. + * By default, start from a minimum set of allowed options and add to + * it. + * + * NB: The SASL method codes are always "allowed" here. If the server + * requests SASL auth, pg_SASL_init() will enforce adherence to the + * sasl_mechs list, which by default is empty. */ conn->auth_required = true; - conn->allowed_auth_methods = 0; + conn->allowed_auth_methods = default_methods; + conn->sasl_mechs = no_mechs; + conn->sasl_mechs_denied = false; for (first = true, more = true; more; first = false) { @@ -1289,6 +1303,9 @@ connectOptions2(PGconn *conn) */ conn->auth_required = false; conn->allowed_auth_methods = -1; + + /* conn->sasl_mechs is now a list of denied mechanisms. */ + conn->sasl_mechs_denied = true; } else if (!negated) { @@ -1335,10 +1352,23 @@ connectOptions2(PGconn *conn) } 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); + static const char *scram_mechs[] = { + SCRAM_SHA_256_NAME, + SCRAM_SHA_256_PLUS_NAME, + NULL /* list terminator */ + }; + + /* + * This currently assumes that SCRAM is the only SASL method. + * Once a second mechanism is added, this code will need to add + * to the list instead of replacing it wholesale. + */ + if (conn->sasl_mechs[0]) + goto duplicate; + conn->sasl_mechs = scram_mechs; + + free(part); + continue; /* avoid the bitmask manipulation below */ } else if (strcmp(method, "none") == 0) { diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index ad6f8b78c6..6fdba960da 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -461,6 +461,8 @@ struct pg_conn bool auth_required; /* require an authentication challenge from the server? */ uint32 allowed_auth_methods; /* bitmask of acceptable AuthRequest codes */ + const char **sasl_mechs; /* list of allowed/denied SASL mechanisms */ + bool sasl_mechs_denied; /* is the sasl_mechs list forbidden? */ /* Transient state needed while establishing connection */ PGTargetServerType target_server_type; /* desired session properties */ diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index fb738886a3..23d934892e 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -235,21 +235,21 @@ $node->connect_ok("user=scram_role require_auth=password,scram-sha-256,md5", # ...fail for other auth types... $node->connect_fails("user=scram_role require_auth=password", "password authentication can be required: fails with SCRAM auth", - expected_stderr => qr/server requested SASL authentication/); + expected_stderr => qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/); $node->connect_fails("user=scram_role require_auth=md5", "md5 authentication can be required: fails with SCRAM auth", - expected_stderr => qr/server requested SASL authentication/); + expected_stderr => qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/); $node->connect_fails("user=scram_role require_auth=none", "all authentication can be forbidden: fails with SCRAM auth", - expected_stderr => qr/server requested SASL authentication/); + expected_stderr => qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/); # ...and allow SCRAM authentication to be prohibited. $node->connect_fails("user=scram_role require_auth=!scram-sha-256", "SCRAM authentication can be forbidden: fails with SCRAM auth", - expected_stderr => qr/server requested SASL authentication/); + expected_stderr => qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/); $node->connect_fails("user=scram_role require_auth=!password,!md5,!scram-sha-256", "multiple authentication types can be forbidden: fails with SCRAM auth", - expected_stderr => qr/server requested SASL authentication/); + expected_stderr => qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/); # Test that bad passwords are rejected. $ENV{"PGPASSWORD"} = 'badpass'; diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index cf61bc7d0d..472b2efdf1 100644 --- a/src/test/ssl/t/002_scram.pl +++ b/src/test/ssl/t/002_scram.pl @@ -150,6 +150,12 @@ if ($supports_tls_server_end_point) $node->connect_ok( "$common_connstr user=ssltestuser channel_binding=require require_auth=scram-sha-256", "SCRAM with SSL, channel_binding=require, and require_auth=scram-sha-256"); + $node->connect_fails( + "$common_connstr user=ssltestuser channel_binding=require require_auth=password", + "SCRAM with SSL, channel_binding=require, and require_auth=password", + expected_stderr => + qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256-PLUS"/ + ); } else { -- 2.25.1