commit c299b1abf0ffc477371cdd0d0d789e824da56328 Author: Jacob Champion Date: Fri Jun 10 11:20:08 2022 -0700 squash! libpq: let client reject unexpected auth methods Per review suggestions: - Fix a memory leak when parsing require_auth. - Add PGREQUIREAUTH to the envvars. - Remove the "cert" method from require_auth; this will be handled with a separate conninfo option later. - Fix builds without GSSAPI. - Test combinations of require_auth and channel_binding. - Statically assert when AUTH_REQ_* grows too large to hold safely in our allowed_auth_methods bitmask. - Improve ease of translation. - Add a first draft of documentation. diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 37ec3cb4e5..00990ce47d 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1221,6 +1221,76 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname + + require_auth + + + Specifies the authentication method that the client requires from the + server. If the server does not use the required method to authenticate + the client, or if the authentication handshake is not fully completed by + the server, the connection will fail. A comma-separated list of methods + may also be provided, of which the server must use exactly one in order + for the connection to succeed. By default, any authentication method is + accepted, and the server is free to skip authentication altogether. + + + The following methods may be specified: + + + + password + + + The server must request plaintext password authentication. + + + + + + md5 + + + The server must request MD5 hashed password authentication. + + + + + + gss + + + The server must either request a Kerberos handshake via + GSSAPI or establish a + GSS-encrypted channel (see also + ). + + + + + + sspi + + + The server must request Windows SSPI + authentication. + + + + + + scram-sha-256 + + + The server must successfully complete a SCRAM-SHA-256 authentication + exchange with the client. + + + + + + + + channel_binding @@ -7761,6 +7831,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) + + + + PGREQUIREAUTH + + PGREQUIREAUTH behaves the same as the connection parameter. + + + diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index b418283d5f..a47f7b2d91 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -161,6 +161,7 @@ extern PGDLLIMPORT bool Db_user_namespace; #define AUTH_REQ_SASL 10 /* Begin SASL authentication */ #define AUTH_REQ_SASL_CONT 11 /* Continue SASL authentication */ #define AUTH_REQ_SASL_FIN 12 /* Final SASL message */ +#define AUTH_REQ_MAX AUTH_REQ_SASL_FIN /* maximum AUTH_REQ_* value */ typedef uint32 AuthRequest; diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 04f9bf4831..59c3575cc3 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -877,6 +877,9 @@ auth_description(AuthRequest areq) #define auth_allowed(conn, type) \ (((conn)->allowed_auth_methods & (1 << (type))) != 0) +StaticAssertDecl(AUTH_REQ_MAX < CHAR_BIT * sizeof(((PGconn){0}).allowed_auth_methods), + "AUTH_REQ_MAX overflows the allowed_auth_methods bitmask"); + /* * Verify that the authentication request is expected, given the connection * parameters. This is especially important when the client wishes to @@ -907,48 +910,24 @@ check_expected_areq(AuthRequest areq, PGconn *conn) /* * 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. + * SCRAM -- but there is one special case to check. 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 = 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 = 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 (auth_allowed(conn, AUTH_REQ_GSS) && conn->gssenc) +#ifdef ENABLE_GSS + if (auth_allowed(conn, AUTH_REQ_GSS) && conn->gssenc) { /* - * Special case: if implicit GSS auth has already been - * performed via GSS encryption, we don't need to have - * performed an AUTH_REQ_GSS exchange. + * If implicit GSS auth has already been performed via GSS + * encryption, we don't need to have performed an + * AUTH_REQ_GSS exchange. * * TODO: check this assumption. What mutual auth guarantees * are made in this case? */ } else +#endif { reason = libpq_gettext("server did not complete authentication"), result = false; @@ -982,12 +961,8 @@ check_expected_areq(AuthRequest areq, PGconn *conn) { if (reason) { - /* - * XXX double call to libpq_gettext() is probably not easy to - * translate from English - */ appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("auth method \"%s\" required, but %s\n"), + libpq_gettext("auth method \"%s\" requirement failed: %s\n"), conn->require_auth, reason); } else diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index c650687c9b..f2579ff7ee 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -310,7 +310,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "Require-Peer", "", 10, offsetof(struct pg_conn, requirepeer)}, - {"require_auth", NULL, NULL, NULL, + {"require_auth", "PGREQUIREAUTH", NULL, NULL, "Require-Auth", "", 14, /* sizeof("scram-sha-256") == 14 */ offsetof(struct pg_conn, require_auth)}, @@ -1301,31 +1301,14 @@ connectOptions2(PGconn *conn) 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); + + free(method); return false; } @@ -1339,10 +1322,13 @@ connectOptions2(PGconn *conn) appendPQExpBuffer(&conn->errorMessage, libpq_gettext("require_auth method \"%s\" is specified more than once"), method); + + free(method); return false; } conn->allowed_auth_methods |= bits; + free(method); } } diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 6fb9d48896..fc91cae7a2 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -477,32 +477,6 @@ verify_cb(int ok, X509_STORE_CTX *ctx) return ok; } -/* - * Certificate selection callback - * - * This callback lets us choose the client certificate we send to the server - * after seeing its CertificateRequest. We only support sending a single - * hard-coded certificate via sslcert, so we don't actually set any certificates - * here; we just it to record whether or not the server has actually asked for - * one and whether we have one to send. - */ -static int -cert_cb(SSL *ssl, void *arg) -{ - PGconn *conn = arg; - conn->ssl_cert_requested = true; - - /* Do we have a certificate loaded to send back? */ - if (SSL_get_certificate(ssl)) - conn->ssl_cert_sent = true; - - /* - * Tell OpenSSL that the callback succeeded; we're not required to actually - * make any changes to the SSL handle. - */ - return 1; -} - /* * OpenSSL-specific wrapper around * pq_verify_peer_name_matches_certificate_name(), converting the ASN1_STRING @@ -997,9 +971,6 @@ initialize_SSL(PGconn *conn) SSL_CTX_set_default_passwd_cb_userdata(SSL_context, conn); } - /* Set up a certificate selection callback. */ - SSL_CTX_set_cert_cb(SSL_context, cert_cb, conn); - /* Disable old protocol versions */ SSL_CTX_set_options(SSL_context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3); diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 62554fedde..6216af7f87 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -456,7 +456,6 @@ struct pg_conn 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 */ @@ -522,8 +521,6 @@ struct pg_conn /* SSL structures */ bool ssl_in_use; - bool ssl_cert_requested; /* Did the server ask us for a cert? */ - bool ssl_cert_sent; /* Did we send one in reply? */ #ifdef USE_SSL bool allow_ssl_try; /* Allowed to try SSL negotiation */ diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index dab50774c4..dd27092134 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -83,9 +83,6 @@ test_role($node, 'md5_role', 'trust', 0, log_unlike => [qr/connection authenticated:/]); # All require_auth options should fail. -$node->connect_fails("user=scram_role require_auth=cert", - "certificate authentication can be required: fails with trust auth", - expected_stderr => qr/server did not request a certificate/); $node->connect_fails("user=scram_role require_auth=gss", "GSS authentication can be required: fails with trust auth", expected_stderr => qr/server did not complete authentication/); @@ -101,9 +98,9 @@ $node->connect_fails("user=scram_role require_auth=md5", $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", +$node->connect_fails("user=scram_role require_auth=password,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/); + expected_stderr => qr/server did not complete authentication/); # For plain "password" method, all users should also be able to connect. reset_pg_hba($node, 'password'); @@ -117,7 +114,7 @@ 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", +$node->connect_ok("user=scram_role require_auth=scram-sha-256,password,md5", "multiple authentication types can be required: works with password auth"); # ...and fail for other auth types. @@ -144,7 +141,7 @@ 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", +$node->connect_ok("user=scram_role require_auth=password,scram-sha-256,md5", "multiple authentication types can be required: works with SCRAM auth"); # ...and fail for other auth types. @@ -174,7 +171,7 @@ 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", +$node->connect_ok("user=md5_role require_auth=md5,scram-sha-256,password", "multiple authentication types can be required: works with MD5 auth"); # ...and fail for other auth types. diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 68a888e11a..c0b4a5739c 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -505,22 +505,6 @@ note "running server tests"; $common_connstr = "$default_ssl_connstr sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR host=localhost"; -# require_auth=cert should succeed against the certdb... -$node->connect_ok( - "$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. -$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( "$common_connstr user=ssltestuser sslcert=invalid", diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index 588f47a39b..9957a96e69 100644 --- a/src/test/ssl/t/002_scram.pl +++ b/src/test/ssl/t/002_scram.pl @@ -132,4 +132,29 @@ $node->connect_ok( qr/connection authenticated: identity="ssltestuser" method=scram-sha-256/ ]); +# channel_binding should continue to function independently of require_auth. +$node->connect_ok("$common_connstr user=ssltestuser channel_binding=disable require_auth=scram-sha-256", + "SCRAM with SSL, channel_binding=disable, and require_auth=scram-sha-256"); +$node->connect_fails( + "$common_connstr user=md5testuser require_auth=md5 channel_binding=require", + "channel_binding can fail even when require_auth succeeds", + expected_stderr => + qr/channel binding required but not supported by server's authentication request/ +); +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"); +} +else +{ + $node->connect_fails( + "$common_connstr user=ssltestuser channel_binding=require require_auth=scram-sha-256", + "SCRAM with SSL, channel_binding=require, and require_auth=scram-sha-256", + expected_stderr => + qr/channel binding is required, but server did not offer an authentication method that supports channel binding/ + ); +} + done_testing();