Hello,
TL;DR: this patch lets you specify exactly one authentication method in
the connection string, and libpq will fail the connection if the server
doesn't use that method.
(This is not intended for PG15. I'm generally anxious about posting
experimental work during a commitfest, but there's been enough
conversation about this topic recently that I felt like it'd be useful
to have code to point to.)
== Proposal and Alternatives ==
$subject keeps coming up in threads. I think my first introduction to
it was after the TLS injection CVE, and then it came up again in the
pluggable auth thread. It's hard for me to generalize based on "sound
bites", but among the proposals I've seen are
1. reject plaintext passwords
2. reject a configurable list of unacceptable methods
3. allow client and server to negotiate a method
All of them seem to have merit. I'm personally motivated by the case
brought up by the CVE: if I'm expecting client certificate
authentication, it's not acceptable for the server to extract _any_
information about passwords from my system, whether they're plaintext,
hashed, or SCRAM-protected. So I chose not to implement option 1. And
option 3 looked like a lot of work to take on in an experiment without
a clear consensus.
Here is my take on option 2, then: you get to choose exactly one method
that the client will accept. If you want to use client certificates,
use require_auth=cert. If you want to force SCRAM, use
require_auth=scram-sha-256. If the server asks for something different,
libpq will fail. If the server tries to get away without asking you for
authentication, libpq will fail. There is no negotiation.
== Why Force Authn? ==
I think my decision to fail if the server doesn't authenticate might be
controversial. It doesn't provide additional protection against active
attack unless you're using a mutual authentication method (SCRAM),
because you can't prove that the server actually did anything with its
side of the handshake. But this approach grew on me for a few reasons:
- When using SCRAM, it allows the client to force a server to
authenticate itself, even when channel bindings aren't being used. (I
really think it's weird that we let the server get away with that
today.)
- In cases where you want to ensure that your actions are logged for
later audit, you can be reasonably sure that you're connecting to a
database that hasn't been configured with a `trust` setting.
- For cert authentication, it ensures that the server asked for a cert
and that you actually sent one. This is more forward-looking; today, we
always ask for a certificate from the client even if we don't use it.
But if implicit TLS takes off, I'd expect to see more middleware, with
more potential for misconfiguration.
== General Thoughts ==
I like that this approach fits nicely into the existing code. The
majority of the patch just beefs up check_expected_areq(). The new flag
that tracks whether or not we've authenticated is scattered around more
than I would like, but I'm hopeful that some of the pluggable auth
conversations will lead to nice refactoring opportunities for those
internal helpers.
There's currently no way to prohibit client certificates from being
sent. If my use case is "servers shouldn't be able to extract password
info if the client expects certificates", then someone else may very
well say "servers shouldn't be able to extract my client certificate if
I wanted to use SCRAM". Likewise, this feature won't prevent a GSS
authenticated channel -- but we do have gssencmode=disable, so I'm less
concerned there.
I made the assumption that a GSS encrypted channel authenticates both
parties to each other, but I don't actually know what guarantees are
made there. I have not tested SSPI.
I'm not a fan of the multiple spellings of "password" ("ldap", "pam",
et al). My initial thought was that it'd be nice to match the client
setting to the HBA setting in the server. But I don't think it's really
all that helpful; worst-case, it pretends to do something it can't,
since the client can't determine what the plaintext password is
actually used for on the backend. And if someone devises (say) a SASL
scheme for proxied LDAP authentication, that spelling becomes obsolete.
Speaking of obsolete, the current implementation assumes that any SASL
exchange must be for SCRAM. That won't be anywhere close to future-
proof.
Thanks,
--Jacob
From 545a89aafacd0f997cd3e14cd20b192335eafadc Mon Sep 17 00:00:00 2001
From: Jacob Champion <pchamp...@vmware.com>
Date: Mon, 28 Feb 2022 09:40:43 -0800
Subject: [PATCH] libpq: let client reject unexpected auth methods
The require_auth connection option allows the client to choose one (and
only one) authentication type for use with the server. There is no
negotiation: if the server does not present the expected authentication
request, the connection fails.
Internally, the patch expands the role of check_expected_areq() to
ensure that the incoming request matches conn->require_auth. It also
introduces a new flag, conn->client_finished_auth, which is set by
various authentication routines when the client side of the handshake is
finished. This signals to check_expected_areq() that an OK message from
the server is expected, and allows the client to complain if the server
forgoes authentication entirely.
(Since the client can't generally prove that the server is actually
doing the work of authentication, this last part is mostly useful for
SCRAM without channel binding. But it could be used to diagnose
configuration problems with client certificate authentication. It could
also provide a client with a decent signal that, at the very least, it's
not connecting to a database with trust auth, and so the connection can
be tied to the client in a later audit.)
Certificate authentication poses an additional complication.
conn->ssl_cert_requested and conn->ssl_cert_sent have been added so that
check_expected_areq() can ensure that we sent a certificate during the
TLS handshake.
Deficiencies:
- This feature currently doesn't prevent unexpected certificate exchange
or unexpected GSSAPI encryption.
- require_auth isn't validated against the supported list of methods.
- It's unclear whether allowing various spellings of "password" (like
"ldap", "pam", etc.) is actually helpful.
- This is unlikely to be very forwards-compatible at the moment,
especially with SASL/SCRAM.
- SSPI support is "implemented" but untested.
---
src/interfaces/libpq/fe-auth-scram.c | 1 +
src/interfaces/libpq/fe-auth.c | 146 ++++++++++++++++++++++
src/interfaces/libpq/fe-connect.c | 4 +
src/interfaces/libpq/fe-secure-openssl.c | 28 +++++
src/interfaces/libpq/libpq-int.h | 6 +
src/test/authentication/t/001_password.pl | 59 +++++++++
src/test/kerberos/t/001_auth.pl | 18 +++
src/test/ldap/t/001_auth.pl | 9 ++
src/test/ssl/t/001_ssltests.pl | 11 ++
9 files changed, 282 insertions(+)
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index e616200704..d918e8b87f 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -289,6 +289,7 @@ scram_exchange(void *opaq, char *input, int inputlen,
}
*done = true;
state->state = FE_SCRAM_FINISHED;
+ state->conn->client_finished_auth = true;
break;
default:
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 6fceff561b..27ce4b8898 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -138,7 +138,10 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
}
if (maj_stat == GSS_S_COMPLETE)
+ {
+ conn->client_finished_auth = true;
gss_release_name(&lmin_s, &conn->gtarg_nam);
+ }
return STATUS_OK;
}
@@ -328,6 +331,9 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
FreeContextBuffer(outbuf.pBuffers[0].pvBuffer);
}
+ if (r == SEC_E_OK)
+ conn->client_finished_auth = true;
+
/* Cleanup is handled by the code in freePGconn() */
return STATUS_OK;
}
@@ -836,6 +842,34 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
return ret;
}
+/*
+ * Translate an AuthRequest into a human-readable description.
+ */
+static const char *
+auth_description(AuthRequest areq)
+{
+ switch (areq)
+ {
+ case AUTH_REQ_PASSWORD:
+ return libpq_gettext("a cleartext password");
+ case AUTH_REQ_MD5:
+ return libpq_gettext("a hashed password");
+ case AUTH_REQ_GSS:
+ case AUTH_REQ_GSS_CONT:
+ return libpq_gettext("GSSAPI authentication");
+ case AUTH_REQ_SSPI:
+ return libpq_gettext("SSPI authentication");
+ case AUTH_REQ_SCM_CREDS:
+ return libpq_gettext("UNIX socket credentials");
+ case AUTH_REQ_SASL:
+ case AUTH_REQ_SASL_CONT:
+ case AUTH_REQ_SASL_FIN:
+ return libpq_gettext("SASL authentication");
+ }
+
+ return libpq_gettext("an unknown authentication type");
+}
+
/*
* Verify that the authentication request is expected, given the connection
* parameters. This is especially important when the client wishes to
@@ -845,6 +879,115 @@ static bool
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 (conn->require_auth)
+ {
+ switch (areq)
+ {
+ case AUTH_REQ_OK:
+ /*
+ * Check to make sure we've actually finished our exchange.
+ */
+ if (strcmp(conn->require_auth, "cert") == 0)
+ {
+ if (!conn->ssl_cert_requested)
+ {
+ reason = 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");
+ result = false;
+ }
+ }
+ else if (strcmp(conn->require_auth, "gss") == 0
+ && 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.
+ *
+ * TODO: check this assumption. What mutual auth guarantees
+ * are made in this case?
+ */
+ }
+ else if (!conn->client_finished_auth)
+ {
+ 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_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;
+ break;
+
+ default:
+ result = false;
+ break;
+ }
+ }
+
+ if (!result)
+ {
+ 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"),
+ conn->require_auth, reason);
+ }
+ else
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("auth method \"%s\" required, but server requested %s\n"),
+ conn->require_auth,
+ auth_description(areq));
+ }
+
+ return result;
+ }
/*
* When channel_binding=require, we must protect against two cases: (1) we
@@ -1046,6 +1189,9 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
"fe_sendauth: error sending password authentication\n");
return STATUS_ERROR;
}
+
+ /* We expect no further authentication requests. */
+ conn->client_finished_auth = true;
break;
}
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 1c5a2b43e9..9982ce6e4e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -310,6 +310,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Require-Peer", "", 10,
offsetof(struct pg_conn, requirepeer)},
+ {"require_auth", NULL, NULL, NULL,
+ "Require-Auth", "", 14, /* sizeof("scram-sha-256") == 14 */
+ offsetof(struct pg_conn, require_auth)},
+
{"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
"SSL-Minimum-Protocol-Version", "", 8, /* sizeof("TLSv1.x") == 8 */
offsetof(struct pg_conn, ssl_min_protocol_version)},
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index c6a80d30c2..0b8e500afb 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -477,6 +477,31 @@ 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
@@ -960,6 +985,9 @@ 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 e0cee4b142..f6dc21cc24 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -393,6 +393,7 @@ struct pg_conn
char *ssl_min_protocol_version; /* minimum TLS protocol version */
char *ssl_max_protocol_version; /* maximum TLS protocol version */
char *target_session_attrs; /* desired session properties */
+ char *require_auth; /* name of the expected auth method */
/* Optional file to write trace info to */
FILE *Pfdebug;
@@ -509,12 +510,17 @@ struct pg_conn
bool error_result; /* do we need to make an ERROR result? */
PGresult *next_result; /* next result (used in single-row mode) */
+ bool client_finished_auth; /* have we finished our half of the
+ * authentication exchange? */
+
/* Assorted state for SASL, SSL, GSS, etc */
const pg_fe_sasl_mech *sasl;
void *sasl_state;
/* 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 3e3079c824..c7062ca357 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -82,6 +82,29 @@ test_role($node, 'scram_role', 'trust', 0,
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/);
+$node->connect_fails("user=scram_role require_auth=sspi",
+ "GSS authentication can be required: fails with trust auth",
+ expected_stderr => qr/server did not complete authentication/);
+$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/);
+
# For plain "password" method, all users should also be able to connect.
reset_pg_hba($node, 'password');
test_role($node, 'scram_role', 'password', 0,
@@ -91,6 +114,18 @@ test_role($node, 'md5_role', 'password', 0,
log_like =>
[qr/connection authenticated: identity="md5_role" method=password/]);
+# 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");
+
+# ...and fail for other auth types.
+$node->connect_fails("user=scram_role require_auth=md5",
+ "md5 authentication can be required: fails with password auth",
+ expected_stderr => qr/server requested a cleartext password/);
+$node->connect_fails("user=scram_role require_auth=scram-sha-256",
+ "SCRAM authentication can be required: fails with password auth",
+ expected_stderr => qr/server requested a cleartext password/);
+
# For "scram-sha-256" method, user "scram_role" should be able to connect.
reset_pg_hba($node, 'scram-sha-256');
test_role(
@@ -104,6 +139,18 @@ test_role(
test_role($node, 'md5_role', 'scram-sha-256', 2,
log_unlike => [qr/connection authenticated:/]);
+# 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");
+
+# ...and 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/);
+$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/);
+
# Test that bad passwords are rejected.
$ENV{"PGPASSWORD"} = 'badpass';
test_role($node, 'scram_role', 'scram-sha-256', 2,
@@ -120,6 +167,18 @@ test_role($node, 'md5_role', 'md5', 0,
log_like =>
[qr/connection authenticated: identity="md5_role" method=md5/]);
+# require_auth should succeed with MD5...
+$node->connect_ok("user=md5_role require_auth=md5",
+ "MD5 authentication can be required: works with MD5 auth");
+
+# ...and fail for other auth types.
+$node->connect_fails("user=md5_role require_auth=password",
+ "password authentication can be required: fails with MD5 auth",
+ expected_stderr => qr/server requested a hashed password/);
+$node->connect_fails("user=md5_role require_auth=scram-sha-256",
+ "SCRAM authentication can be required: fails with MD5 auth",
+ expected_stderr => qr/server requested a hashed password/);
+
# Tests for channel binding without SSL.
# Using the password authentication method; channel binding can't work
reset_pg_hba($node, 'password');
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 62e0542639..84e94e8e81 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -307,6 +307,24 @@ test_query(
'gssencmode=require',
'sending 100K lines works');
+# require_auth=gss should succeed...
+$node->connect_ok(
+ $node->connstr('postgres') . " user=test1 host=$host hostaddr=$hostaddr gssencmode=disable require_auth=gss",
+ "GSS authentication can be requested: works with GSS auth without encryption");
+$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 auth with encryption");
+
+# ...and require_auth=sspi should fail.
+$node->connect_fails(
+ $node->connstr('postgres') . " user=test1 host=$host hostaddr=$hostaddr gssencmode=disable require_auth=sspi",
+ "SSPI authentication can be requested: fails with GSS auth without encryption",
+ expected_stderr => qr/server requested GSSAPI authentication/);
+$node->connect_fails(
+ $node->connstr('postgres') . " user=test1 host=$host hostaddr=$hostaddr gssencmode=require require_auth=sspi",
+ "SSPI authentication can be requested: fails with GSS auth with encryption",
+ expected_stderr => qr/server did not complete authentication/);
+
unlink($node->data_dir . '/pg_hba.conf');
$node->append_conf('pg_hba.conf',
qq{hostgssenc 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 094270cb5d..8197dad87b 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -206,6 +206,15 @@ 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");
+$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",
+ "SCRAM authentication can be required: fails with ldap auth");
+
note "search+bind";
unlink($node->data_dir . '/pg_hba.conf');
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 6c73c0f9ea..1c6ff81c41 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -491,6 +491,17 @@ note "running server tests";
$common_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=$key{'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/);
+
# no client cert
$node->connect_fails(
"$common_connstr user=ssltestuser sslcert=invalid",
--
2.25.1
