From 8cc020598c7c939e3a45088f18ca04ea801fc87e Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 18 Oct 2022 16:55:36 -0700 Subject: [PATCH v16 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 | 34 +++++++++++++++++++ src/interfaces/libpq/fe-connect.c | 41 +++++++++++++++++++---- src/interfaces/libpq/libpq-int.h | 3 +- src/test/authentication/t/001_password.pl | 14 +++++--- src/test/ssl/t/002_scram.pl | 6 ++++ 5 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 53c7d30eff..7927aebed8 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -522,6 +522,40 @@ 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)) + { + libpq_append_conn_error(conn, "auth method \"%s\" requirement failed: server requested unacceptable SASL mechanism \"%s\"", + 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 cbadb3f6af..a048793b46 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1258,12 +1258,25 @@ connectOptions2(PGconn *conn) 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 + * 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) { @@ -1290,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) { @@ -1334,10 +1350,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, "creds") == 0) { diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index f1f1d973cc..ab26292586 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -465,7 +465,8 @@ struct pg_conn * codes */ bool client_finished_auth; /* have we finished our half of the * authentication exchange? */ - + 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 cba5d7d648..015532893c 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -301,30 +301,34 @@ $node->connect_fails( "user=scram_role require_auth=password", "password authentication required, fails with SCRAM auth", expected_stderr => - qr/auth method "password" requirement failed: server requested SASL authentication/ + qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/ ); $node->connect_fails( "user=scram_role require_auth=md5", "md5 authentication required, fails with SCRAM auth", expected_stderr => - qr/auth method "md5" requirement failed: server requested SASL authentication/ + qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/ ); $node->connect_fails( "user=scram_role require_auth=none", "all authentication forbidden, fails with SCRAM auth", expected_stderr => - qr/auth method "none" requirement failed: server requested SASL authentication/ + qr/server requested unacceptable SASL mechanism "SCRAM-SHA-256"/ ); # Authentication fails if SCRAM authentication is forbidden. $node->connect_fails( "user=scram_role require_auth=!scram-sha-256", "SCRAM authentication 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 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 8038135697..173ac8d86b 100644 --- a/src/test/ssl/t/002_scram.pl +++ b/src/test/ssl/t/002_scram.pl @@ -157,6 +157,12 @@ if ($supports_tls_server_end_point) "$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