commit 403d27e07922babcdf6fd5e8ad8524d92811294c Author: Jacob Champion Date: Mon Jun 6 12:32:03 2022 -0700 squash! libpq: let client reject unexpected auth methods - rebase over new sslkey() test function - remove alternate spellings of "password" - allow comma-separated methods (OR). At least one of the authentication methods in the list must complete for the connection to succeed. diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index a883959756..04f9bf4831 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -870,6 +870,13 @@ auth_description(AuthRequest areq) return libpq_gettext("an unknown authentication type"); } +/* + * Convenience macro for checking the allowed_auth_methods bitmask. Caller must + * ensure that type is not greater than 31 (high bit of the bitmask). + */ +#define auth_allowed(conn, type) \ + (((conn)->allowed_auth_methods & (1 << (type))) != 0) + /* * Verify that the authentication request is expected, given the connection * parameters. This is especially important when the client wishes to @@ -881,7 +888,11 @@ check_expected_areq(AuthRequest areq, PGconn *conn) bool result = true; char *reason = NULL; - /* If the user required a specific auth method, reject all others. */ + /* + * If the user required a specific auth method, or specified an allowed set, + * then reject all others here, and make sure the server actually completes + * an authentication exchange. + */ if (conn->require_auth) { switch (areq) @@ -890,21 +901,43 @@ check_expected_areq(AuthRequest areq, PGconn *conn) /* * Check to make sure we've actually finished our exchange. */ - if (strcmp(conn->require_auth, "cert") == 0) + if (conn->client_finished_auth) + break; + + /* + * No explicit authentication request was made by the server -- + * or perhaps it was made and not completed, in the case of + * SCRAM -- but there are two special cases to check: + * + * 1) If the user allowed "cert", then as long as we sent a + * client certificate to the server in response to its + * TLS CertificateRequest, this check is satisfied. + * + * 2) If the user allowed "gss", then a GSS-encrypted channel + * also satisfies the check. + */ + if (conn->allowed_auth_method_cert) { + /* + * Trade off a little bit of complexity to try to get these + * error messages as precise as possible. + */ if (!conn->ssl_cert_requested) { - reason = libpq_gettext("server did not request a certificate"); + reason = conn->allowed_auth_methods + ? libpq_gettext("server did not complete authentication or request a certificate") + : libpq_gettext("server did not request a certificate"); result = false; } else if (!conn->ssl_cert_sent) { - reason = libpq_gettext("server accepted connection without a valid certificate"); + reason = conn->allowed_auth_methods + ? libpq_gettext("server did not complete authentication and accepted connection without a valid certificate") + : libpq_gettext("server accepted connection without a valid certificate"); result = false; } } - else if (strcmp(conn->require_auth, "gss") == 0 - && conn->gssenc) + else if (auth_allowed(conn, AUTH_REQ_GSS) && conn->gssenc) { /* * Special case: if implicit GSS auth has already been @@ -915,49 +948,28 @@ check_expected_areq(AuthRequest areq, PGconn *conn) * are made in this case? */ } - else if (!conn->client_finished_auth) + else { reason = libpq_gettext("server did not complete authentication"), result = false; } - break; - case AUTH_REQ_PASSWORD: - if (strcmp(conn->require_auth, "bsd") - && strcmp(conn->require_auth, "ldap") - && strcmp(conn->require_auth, "pam") - && strcmp(conn->require_auth, "password") - && strcmp(conn->require_auth, "radius")) - result = false; break; + case AUTH_REQ_PASSWORD: case AUTH_REQ_MD5: - if (strcmp(conn->require_auth, "md5")) - result = false; - break; - case AUTH_REQ_GSS: - if (strcmp(conn->require_auth, "gss")) - result = false; - break; - case AUTH_REQ_SSPI: - if (strcmp(conn->require_auth, "sspi")) - result = false; - break; - case AUTH_REQ_GSS_CONT: - if (strcmp(conn->require_auth, "gss") && - strcmp(conn->require_auth, "sspi")) - result = false; - break; - case AUTH_REQ_SASL: case AUTH_REQ_SASL_CONT: case AUTH_REQ_SASL_FIN: - /* This currently assumes that SCRAM is the only SASL method. */ - if (strcmp(conn->require_auth, "scram-sha-256")) - result = false; + /* + * We don't handle these with the default case, to avoid + * bit-shifting past the end of the allowed_auth_methods mask if + * the server sends an unexpected AuthRequest. + */ + result = auth_allowed(conn, areq); break; default: diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 682f363c2b..c650687c9b 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1259,6 +1259,93 @@ connectOptions2(PGconn *conn) } } + /* + * parse and validate require_auth option + */ + if (conn->require_auth) + { + char *s = conn->require_auth; + bool more = true; + + while (more) + { + char *method; + uint32 bits; + + method = parse_comma_separated_list(&s, &more); + if (method == NULL) + goto oom_error; + + if (strcmp(method, "password") == 0) + { + bits = (1 << AUTH_REQ_PASSWORD); + } + else if (strcmp(method, "md5") == 0) + { + bits = (1 << AUTH_REQ_MD5); + } + else if (strcmp(method, "gss") == 0) + { + bits = (1 << AUTH_REQ_GSS); + bits |= (1 << AUTH_REQ_GSS_CONT); + } + else if (strcmp(method, "sspi") == 0) + { + bits = (1 << AUTH_REQ_SSPI); + bits |= (1 << AUTH_REQ_GSS_CONT); + } + 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); + } + else if (strcmp(method, "cert") == 0) + { + /* + * Special case: there is no AUTH_REQ code for certificate + * authentication since it's done as part of the TLS handshake. + * Make a note in conn, for check_expected_areq() to see later. + */ + if (conn->allowed_auth_method_cert) + { + conn->status = CONNECTION_BAD; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("require_auth method \"%s\" is specified more than once"), + method); + return false; + } + + conn->allowed_auth_method_cert = true; + continue; /* avoid the bitmask manipulation below */ + } + else + { + conn->status = CONNECTION_BAD; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid require_auth method: \"%s\""), + method); + return false; + } + + /* + * Sanity check; a duplicated method probably indicates a typo in a + * setting where typos are extremely risky. + */ + if ((conn->allowed_auth_methods & bits) == bits) + { + conn->status = CONNECTION_BAD; + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("require_auth method \"%s\" is specified more than once"), + method); + return false; + } + + conn->allowed_auth_methods |= bits; + } + } + /* * validate channel_binding option */ diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index b7701c3683..62554fedde 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -455,6 +455,9 @@ struct pg_conn bool write_failed; /* have we had a write failure on sock? */ char *write_err_msg; /* write error message, or NULL if OOM */ + uint32 allowed_auth_methods; /* bitmask of acceptable AuthRequest codes */ + bool allowed_auth_method_cert; /* tracks "cert" which has no AuthRequest code */ + /* Transient state needed while establishing connection */ PGTargetServerType target_server_type; /* desired session properties */ bool try_next_addr; /* time to advance to next address/host? */ diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index c7062ca357..dab50774c4 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -95,15 +95,15 @@ $node->connect_fails("user=scram_role require_auth=sspi", $node->connect_fails("user=scram_role require_auth=password", "password authentication can be required: fails with trust auth", expected_stderr => qr/server did not complete authentication/); -$node->connect_fails("user=scram_role require_auth=ldap", - "LDAP authentication can be required: fails with trust auth", - expected_stderr => qr/server did not complete authentication/); $node->connect_fails("user=scram_role require_auth=md5", "md5 authentication can be required: fails with trust auth", expected_stderr => qr/server did not complete authentication/); $node->connect_fails("user=scram_role require_auth=scram-sha-256", "SCRAM authentication can be required: fails with trust auth", expected_stderr => qr/server did not complete authentication/); +$node->connect_fails("user=scram_role require_auth=cert,scram-sha-256", + "multiple authentication types can be required: fails with trust auth", + expected_stderr => qr/server did not complete authentication or request a certificate/); # For plain "password" method, all users should also be able to connect. reset_pg_hba($node, 'password'); @@ -117,6 +117,8 @@ test_role($node, 'md5_role', 'password', 0, # require_auth should succeed with a plaintext password... $node->connect_ok("user=scram_role require_auth=password", "password authentication can be required: works with password auth"); +$node->connect_ok("user=scram_role require_auth=cert,password,md5", + "multiple authentication types can be required: works with password auth"); # ...and fail for other auth types. $node->connect_fails("user=scram_role require_auth=md5", @@ -142,6 +144,8 @@ test_role($node, 'md5_role', 'scram-sha-256', 2, # require_auth should succeed with SCRAM... $node->connect_ok("user=scram_role require_auth=scram-sha-256", "SCRAM authentication can be required: works with SCRAM auth"); +$node->connect_ok("user=scram_role require_auth=password,scram-sha-256,cert", + "multiple authentication types can be required: works with SCRAM auth"); # ...and fail for other auth types. $node->connect_fails("user=scram_role require_auth=password", @@ -170,6 +174,8 @@ test_role($node, 'md5_role', 'md5', 0, # require_auth should succeed with MD5... $node->connect_ok("user=md5_role require_auth=md5", "MD5 authentication can be required: works with MD5 auth"); +$node->connect_ok("user=md5_role require_auth=md5,scram-sha-256,cert", + "multiple authentication types can be required: works with MD5 auth"); # ...and fail for other auth types. $node->connect_fails("user=md5_role require_auth=password", diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl index 84e94e8e81..de4ddcbf69 100644 --- a/src/test/kerberos/t/001_auth.pl +++ b/src/test/kerberos/t/001_auth.pl @@ -353,6 +353,14 @@ test_access( test_access($node, 'test1', 'SELECT true', 2, 'gssencmode=disable', 'fails with GSS encryption disabled and hostgssenc hba'); +# require_auth=gss should succeed. +$node->connect_ok( + $node->connstr('postgres') . " user=test1 host=$host hostaddr=$hostaddr gssencmode=require require_auth=gss", + "GSS authentication can be requested: works with GSS encryption"); +$node->connect_ok( + $node->connstr('postgres') . " user=test1 host=$host hostaddr=$hostaddr gssencmode=require require_auth=gss,scram-sha-256", + "multiple authentication types can be requested: works with GSS encryption"); + unlink($node->data_dir . '/pg_hba.conf'); $node->append_conf('pg_hba.conf', qq{hostnogssenc all all $hostaddr/32 gss map=mymap}); diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl index a6b07051fc..e7c67920a1 100644 --- a/src/test/ldap/t/001_auth.pl +++ b/src/test/ldap/t/001_auth.pl @@ -216,10 +216,7 @@ test_access( qr/connection authenticated: identity="uid=test1,dc=example,dc=net" method=ldap/ ],); -# require_auth=ldap (and other plaintext password methods) should complete -# successfully; other methods should fail. -$node->connect_ok("user=test1 require_auth=ldap", - "LDAP authentication can be required: works with ldap auth"); +# require_auth=password should complete successfully; other methods should fail. $node->connect_ok("user=test1 require_auth=password", "password authentication can be required: works with ldap auth"); $node->connect_fails("user=test1 require_auth=scram-sha-256", diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 10d67eee17..68a888e11a 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -507,7 +507,8 @@ $common_connstr = # require_auth=cert should succeed against the certdb... $node->connect_ok( - "$common_connstr require_auth=cert user=ssltestuser sslcert=ssl/client.crt sslkey=$key{'client.key'}", + "$common_connstr require_auth=cert user=ssltestuser sslcert=ssl/client.crt " + . sslkey('client.key'), "certificate authentication can be required: works with cert auth"); # ...and fail against the trustdb, if no certificate is provided. @@ -515,6 +516,10 @@ $node->connect_fails( "$common_connstr require_auth=cert dbname=trustdb user=ssltestuser", "certificate authentication can be required: fails with trust auth and no cert", expected_stderr => qr/server accepted connection without a valid certificate/); +$node->connect_fails( + "$common_connstr require_auth=scram-sha-256,cert dbname=trustdb user=ssltestuser", + "multiple authentication types can be required: fails with trust auth and no cert", + expected_stderr => qr/server did not complete authentication and accepted connection without a valid certificate/); # no client cert $node->connect_fails(