On Mon, 20 Mar 2023 at 16:31, Greg Stark <st...@mit.edu> wrote:
>
> Here's a first cut at ALPN support.
>
> Currently it's using a hard coded "Postgres/3.0" protocol
Apparently that is explicitly disrecommended by the IETF folk. They
want something like "TBD" so people don't start using a string until
it's been added to the registry. So I've changed this for now (to
"TBD-pgsql")
Ok, I think this has pretty much everything I was hoping to do.
The one thing I'm not sure of is it seems some codepaths in postmaster
have ereport(COMMERROR) followed by returning an error whereas other
codepaths just have ereport(FATAL). And I don't actually see much
logic in which do which. (I get the principle behind COMMERR it just
seems like it doesn't really match the code).
I realized I had exactly the infrastructure needed to allow pipelining
the SSL ClientHello like Neon wanted to do so I added that too. It's
kind of redundant with direct SSL connections but seems like there may
be reasons to use that instead.
--
greg
From 083df15eff52f025064e2879a404270e405f7dde Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Thu, 16 Mar 2023 11:55:16 -0400
Subject: [PATCH v5 2/6] Direct SSL connections client support
---
src/interfaces/libpq/fe-connect.c | 91 +++++++++++++++++++++++++++++--
src/interfaces/libpq/libpq-fe.h | 1 +
src/interfaces/libpq/libpq-int.h | 3 +
3 files changed, 89 insertions(+), 6 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index bb7347cb0c..7cd0eb261f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -274,6 +274,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"SSL-Mode", "", 12, /* sizeof("verify-full") == 12 */
offsetof(struct pg_conn, sslmode)},
+ {"sslnegotiation", "PGSSLNEGOTIATION", "postgres", NULL,
+ "SSL-Negotiation", "", 14, /* strlen("requiredirect") == 14 */
+ offsetof(struct pg_conn, sslnegotiation)},
+
{"sslcompression", "PGSSLCOMPRESSION", "0", NULL,
"SSL-Compression", "", 1,
offsetof(struct pg_conn, sslcompression)},
@@ -1504,11 +1508,36 @@ connectOptions2(PGconn *conn)
}
#endif
}
+
+ /*
+ * validate sslnegotiation option, default is "postgres" for the postgres
+ * style negotiated connection with an extra round trip but more options.
+ */
+ if (conn->sslnegotiation)
+ {
+ if (strcmp(conn->sslnegotiation, "postgres") != 0
+ && strcmp(conn->sslnegotiation, "direct") != 0
+ && strcmp(conn->sslnegotiation, "requiredirect") != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+ "sslnegotiation", conn->sslnegotiation);
+ return false;
+ }
+
+#ifndef USE_SSL
+ if (conn->sslnegotiation[0] != 'p') {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "sslnegotiation value \"%s\" invalid when SSL support is not compiled in",
+ conn->sslnegotiation);
+ return false;
+ }
+#endif
+ }
else
{
- conn->sslmode = strdup(DefaultSSLMode);
- if (!conn->sslmode)
- goto oom_error;
+ libpq_append_conn_error(conn, "sslnegotiation missing?");
+ return false;
}
/*
@@ -1614,6 +1643,18 @@ connectOptions2(PGconn *conn)
conn->gssencmode);
return false;
}
+#endif
+#ifdef USE_SSL
+ /* GSS is incompatible with direct SSL connections so it requires the
+ * default postgres style connection ssl negotiation */
+ if (strcmp(conn->gssencmode, "require") == 0 &&
+ strcmp(conn->sslnegotiation, "postgres") != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "gssencmode value \"%s\" invalid when Direct SSL negotiation is enabled",
+ conn->gssencmode);
+ return false;
+ }
#endif
}
else
@@ -2747,11 +2788,12 @@ keep_going: /* We will come back to here until there is
/* initialize these values based on SSL mode */
conn->allow_ssl_try = (conn->sslmode[0] != 'd'); /* "disable" */
conn->wait_ssl_try = (conn->sslmode[0] == 'a'); /* "allow" */
+ /* direct ssl is incompatible with "allow" or "disabled" ssl */
+ conn->allow_direct_ssl_try = conn->allow_ssl_try && !conn->wait_ssl_try && (conn->sslnegotiation[0] != 'p');
#endif
#ifdef ENABLE_GSS
conn->try_gss = (conn->gssencmode[0] != 'd'); /* "disable" */
#endif
-
reset_connection_state_machine = false;
need_new_connection = true;
}
@@ -3209,6 +3251,28 @@ keep_going: /* We will come back to here until there is
if (pqsecure_initialize(conn, false, true) < 0)
goto error_return;
+ /* If SSL is enabled and direct SSL connections are enabled
+ * and we haven't already established an SSL connection (or
+ * already tried a direct connection and failed or succeeded)
+ * then try just enabling SSL directly.
+ *
+ * If we fail then we'll either fail the connection (if
+ * sslnegotiation is set to requiredirect or turn
+ * allow_direct_ssl_try to false
+ */
+ if (conn->allow_ssl_try
+ && !conn->wait_ssl_try
+ && conn->allow_direct_ssl_try
+ && !conn->ssl_in_use
+#ifdef ENABLE_GSS
+ && !conn->gssenc
+#endif
+ )
+ {
+ conn->status = CONNECTION_SSL_STARTUP;
+ return PGRES_POLLING_WRITING;
+ }
+
/*
* If SSL is enabled and we haven't already got encryption of
* some sort running, request SSL instead of sending the
@@ -3285,9 +3349,11 @@ keep_going: /* We will come back to here until there is
/*
* On first time through, get the postmaster's response to our
- * SSL negotiation packet.
+ * SSL negotiation packet. If we are trying a direct ssl
+ * connection skip reading the negotiation packet and go
+ * straight to initiating an ssl connection.
*/
- if (!conn->ssl_in_use)
+ if (!conn->ssl_in_use && !conn->allow_direct_ssl_try)
{
/*
* We use pqReadData here since it has the logic to
@@ -3393,6 +3459,18 @@ keep_going: /* We will come back to here until there is
}
if (pollres == PGRES_POLLING_FAILED)
{
+ /* Failed direct ssl connection, possibly try a new connection with postgres negotiation */
+ if (conn->allow_direct_ssl_try)
+ {
+ /* if it's requiredirect then it's a hard failure */
+ if (conn->sslnegotiation[0] == 'r')
+ goto error_return;
+ /* otherwise only retry using postgres connection */
+ conn->allow_direct_ssl_try = false;
+ need_new_connection = true;
+ goto keep_going;
+ }
+
/*
* Failed ... if sslmode is "prefer" then do a non-SSL
* retry
@@ -4395,6 +4473,7 @@ freePGconn(PGconn *conn)
free(conn->keepalives_interval);
free(conn->keepalives_count);
free(conn->sslmode);
+ free(conn->sslnegotiation);
free(conn->sslcert);
free(conn->sslkey);
if (conn->sslpassword)
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index f3d9220496..05821b8473 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -72,6 +72,7 @@ typedef enum
CONNECTION_AUTH_OK, /* Received authentication; waiting for
* backend startup. */
CONNECTION_SETENV, /* This state is no longer used. */
+ CONNECTION_DIRECT_SSL_STARTUP, /* Starting SSL without PG Negotiation. */
CONNECTION_SSL_STARTUP, /* Negotiating SSL. */
CONNECTION_NEEDED, /* Internal state: connect() needed */
CONNECTION_CHECK_WRITABLE, /* Checking if session is read-write. */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d93e976ca5..db63afd786 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -390,6 +390,7 @@ struct pg_conn
char *keepalives_count; /* maximum number of TCP keepalive
* retransmits */
char *sslmode; /* SSL mode (require,prefer,allow,disable) */
+ char *sslnegotiation; /* SSL initiation style (postgres,direct,requiredirect) */
char *sslcompression; /* SSL compression (0 or 1) */
char *sslkey; /* client key filename */
char *sslcert; /* client certificate filename */
@@ -549,6 +550,8 @@ struct pg_conn
bool ssl_cert_sent; /* Did we send one in reply? */
#ifdef USE_SSL
+ bool allow_direct_ssl_try; /* Try to make a direct SSL connection
+ * without an "SSL negotiation packet" */
bool allow_ssl_try; /* Allowed to try SSL negotiation */
bool wait_ssl_try; /* Delay SSL negotiation until after
* attempting normal connection */
--
2.40.0
From 9357d177f07f20f21e896958cec9992a60515195 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Fri, 31 Mar 2023 03:01:35 -0400
Subject: [PATCH v5 6/6] Some added docs
---
doc/src/sgml/protocol.sgml | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 8b5e7b1ad7..dd363656fd 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1532,17 +1532,54 @@ SELCT 1/0;<!-- this typo is intentional -->
bytes.
</para>
+ <para>
+ Likewise the server expects the client to not begin
+ the <acronym>SSL</acronym> negotiation until it receives the server's
+ single byte response to the <acronym>SSL</acronym> request. If the
+ client begins the <acronym>SSL</acronym> negotiation immediately without
+ waiting for the server response to be received it can reduce connection
+ latency by one round-trip. However this comes at the cost of not being
+ able to handle the case where the server sends a negative response to the
+ <acronym>SSL</acronym> request. In that case instead of continuing with either GSSAPI or an
+ unencrypted connection or a protocol error the server will simply
+ disconnect.
+ </para>
+
<para>
An initial SSLRequest can also be used in a connection that is being
opened to send a CancelRequest message.
</para>
+ <para>
+ A second alternate way to initiate <acronym>SSL</acronym> encryption is
+ available. The server will recognize connections which immediately
+ begin <acronym>SSL</acronym> negotiation without any previous SSLRequest
+ packets. Once the <acronym>SSL</acronym> connection is established the
+ server will expect a normal startup-request packet and continue
+ negotiation over the encrypted channel. In this case any other requests
+ for encryption will be refused. This method is not preferred for general
+ purpose tools as it cannot negotiate the best connection encryption
+ available or handle unencrypted connections. However it is useful for
+ environments where both the server and client are controlled together.
+ In that case it avoids one round trip of latency and allows the use of
+ network tools that depend on standard <acronym>SSL</acronym> connections.
+ When using <acronym>SSL</acronym> connections in this style the client is
+ required to use the ALPN extension defined
+ by <ulink url="https://tools.ietf.org/html/rfc7301";>RFC 7301</ulink> to
+ protect against protocol confusion attacks.
+ The <productname>PostgreSQL</productname> protocol is "TBD-pgsql" as
+ registered
+ at <ulink url="https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids";>IANA
+ TLS ALPN Protocol IDs</ulink> registry.
+ </para>
+
<para>
While the protocol itself does not provide a way for the server to
force <acronym>SSL</acronym> encryption, the administrator can
configure the server to reject unencrypted sessions as a byproduct
of authentication checking.
</para>
+
</sect2>
<sect2 id="protocol-flow-gssapi">
--
2.40.0
From 9c3e0c5d3457322b9fa4aa618ffdeb88978e9109 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Mon, 20 Mar 2023 14:09:30 -0400
Subject: [PATCH v5 4/6] alpn support
---
src/backend/libpq/be-secure-openssl.c | 66 ++++++++++++++++++++++++
src/backend/libpq/be-secure.c | 3 ++
src/backend/postmaster/postmaster.c | 25 +++++++++
src/backend/utils/misc/guc_tables.c | 9 ++++
src/bin/psql/command.c | 7 ++-
src/include/libpq/libpq-be.h | 1 +
src/include/libpq/libpq.h | 1 +
src/include/libpq/pqcomm.h | 19 +++++++
src/interfaces/libpq/fe-connect.c | 5 ++
src/interfaces/libpq/fe-secure-openssl.c | 31 +++++++++++
src/interfaces/libpq/libpq-int.h | 1 +
11 files changed, 166 insertions(+), 2 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 685aa2ed69..620ffafb0b 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -67,6 +67,12 @@ static int ssl_external_passwd_cb(char *buf, int size, int rwflag, void *userdat
static int dummy_ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
static int verify_cb(int ok, X509_STORE_CTX *ctx);
static void info_cb(const SSL *ssl, int type, int args);
+static int alpn_cb(SSL *ssl,
+ const unsigned char **out,
+ unsigned char *outlen,
+ const unsigned char *in,
+ unsigned int inlen,
+ void *userdata);
static bool initialize_dh(SSL_CTX *context, bool isServerStart);
static bool initialize_ecdh(SSL_CTX *context, bool isServerStart);
static const char *SSLerrmessage(unsigned long ecode);
@@ -432,6 +438,13 @@ be_tls_open_server(Port *port)
/* set up debugging/info callback */
SSL_CTX_set_info_callback(SSL_context, info_cb);
+ if (ssl_enable_alpn) {
+ elog(DEBUG2, "Enabling OpenSSL ALPN callback");
+ SSL_CTX_set_alpn_select_cb(SSL_context, alpn_cb, port);
+ } else {
+ elog(DEBUG2, "OpenSSL ALPN is disabled, not setting callback");
+ }
+
if (!(port->ssl = SSL_new(SSL_context)))
{
ereport(COMMERROR,
@@ -692,6 +705,12 @@ be_tls_close(Port *port)
pfree(port->peer_dn);
port->peer_dn = NULL;
}
+
+ if (port->ssl_alpn_protocol)
+ {
+ pfree(port->ssl_alpn_protocol);
+ port->ssl_alpn_protocol = NULL;
+ }
}
ssize_t
@@ -1250,6 +1269,53 @@ info_cb(const SSL *ssl, int type, int args)
}
}
+/* See pqcomm.h comments on OpenSSL implementation of ALPN (RFC 7301) */
+static unsigned char alpn_protos[] = PG_ALPN_PROTOCOL_VECTOR;
+
+/*
+ * Server callback for ALPN negotiation. We use use the standard "helper"
+ * function even though currently we only accept one value. We store the
+ * negotiated protocol in Port->ssl_alpn_protocol and rely on higher level
+ * logic (in postmaster.c) to decide what to do with that info.
+ */
+static int alpn_cb(SSL *ssl,
+ const unsigned char **out,
+ unsigned char *outlen,
+ const unsigned char *in,
+ unsigned int inlen,
+ void *userdata)
+{
+ /* Why does OpenSSL provide a helper function that requires a nonconst
+ * vector when the callback is declared to take a const vector? What are
+ * we to do with that?
+ */
+ int retval;
+ Assert(userdata != NULL);
+ Assert(out != NULL);
+ Assert(outlen != NULL);
+ Assert(in != NULL);
+
+ retval = SSL_select_next_proto((unsigned char **)out, outlen,
+ alpn_protos, sizeof(alpn_protos),
+ in, inlen);
+ if (*out == NULL || *outlen > sizeof(alpn_protos) || outlen <= 0)
+ return SSL_TLSEXT_ERR_NOACK; /* can't happen */
+
+ if (retval == OPENSSL_NPN_NEGOTIATED)
+ {
+ struct Port *port = (struct Port *)userdata;
+ char *alpn_protocol = MemoryContextAllocZero(TopMemoryContext, *outlen+1);
+ memcpy(alpn_protocol, *out, *outlen);
+ port->ssl_alpn_protocol = alpn_protocol;
+ return SSL_TLSEXT_ERR_OK;
+ } else if (retval == OPENSSL_NPN_NO_OVERLAP) {
+ return SSL_TLSEXT_ERR_NOACK;
+ } else {
+ return SSL_TLSEXT_ERR_NOACK;
+ }
+}
+
+
/*
* Set DH parameters for generating ephemeral DH keys. The
* DH parameters can take a long time to compute, so they must be
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 1020b3adb0..79a61900ba 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -61,6 +61,9 @@ bool SSLPreferServerCiphers;
int ssl_min_protocol_version = PG_TLS1_2_VERSION;
int ssl_max_protocol_version = PG_TLS_ANY;
+/* GUC variable: if false ignore ALPN negotiation */
+bool ssl_enable_alpn = true;
+
/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
/* ------------------------------------------------------------ */
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index ec1d895a23..9b4b37b997 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1934,6 +1934,10 @@ ServerLoop(void)
* any bytes from the stream if it's not a direct SSL connection.
*/
+#ifdef USE_SSL
+static const char *expected_alpn_protocol = PG_ALPN_PROTOCOL;
+#endif
+
static int
ProcessSSLStartup(Port *port)
{
@@ -1970,6 +1974,10 @@ ProcessSSLStartup(Port *port)
char *buf = NULL;
elog(LOG, "Detected direct SSL handshake");
+ if (!ssl_enable_alpn) {
+ elog(WARNING, "Received direct SSL connection without ssl_enable_alpn enabled");
+ }
+
/* push unencrypted buffered data back through SSL setup */
len = pq_buffer_has_data();
if (len > 0)
@@ -2000,6 +2008,23 @@ ProcessSSLStartup(Port *port)
return STATUS_ERROR;
}
pfree(port->raw_buf);
+
+ if (port->ssl_alpn_protocol == NULL)
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("Received direct SSL connection request without required ALPN protocol negotiation extension")));
+ return STATUS_ERROR;
+ }
+ if (strcmp(port->ssl_alpn_protocol, expected_alpn_protocol) != 0)
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("Received direct SSL connection request with unexpected ALPN protocol \"%s\" expected \"%s\"",
+ port->ssl_alpn_protocol,
+ expected_alpn_protocol)));
+ return STATUS_ERROR;
+ }
#else
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..79a1d0a100 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1088,6 +1088,15 @@ struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+ {
+ {"ssl_enable_alpn", PGC_SIGHUP, CONN_AUTH_SSL,
+ gettext_noop("Respond to TLS ALPN Extension Requests."),
+ NULL,
+ },
+ &ssl_enable_alpn,
+ true,
+ NULL, NULL, NULL
+ },
{
{"fsync", PGC_SIGHUP, WAL_SETTINGS,
gettext_noop("Forces synchronization of updates to disk."),
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index d7731234b6..816b336973 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3715,6 +3715,7 @@ printSSLInfo(void)
const char *protocol;
const char *cipher;
const char *compression;
+ const char *alpn;
if (!PQsslInUse(pset.db))
return; /* no SSL */
@@ -3722,11 +3723,13 @@ printSSLInfo(void)
protocol = PQsslAttribute(pset.db, "protocol");
cipher = PQsslAttribute(pset.db, "cipher");
compression = PQsslAttribute(pset.db, "compression");
+ alpn = PQsslAttribute(pset.db, "alpn");
- printf(_("SSL connection (protocol: %s, cipher: %s, compression: %s)\n"),
+ printf(_("SSL connection (protocol: %s, cipher: %s, compression: %s, ALPN: %s)\n"),
protocol ? protocol : _("unknown"),
cipher ? cipher : _("unknown"),
- (compression && strcmp(compression, "off") != 0) ? _("on") : _("off"));
+ (compression && strcmp(compression, "off") != 0) ? _("on") : _("off"),
+ alpn ? alpn : _("none"));
}
/*
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 824b28e824..2258241770 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -217,6 +217,7 @@ typedef struct Port
char *peer_cn;
char *peer_dn;
bool peer_cert_valid;
+ char *ssl_alpn_protocol;
/*
* OpenSSL structures. (Keep these last so that the locations of other
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 2b02f67257..e7adf4a989 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -123,6 +123,7 @@ extern PGDLLIMPORT char *SSLECDHCurve;
extern PGDLLIMPORT bool SSLPreferServerCiphers;
extern PGDLLIMPORT int ssl_min_protocol_version;
extern PGDLLIMPORT int ssl_max_protocol_version;
+extern PGDLLIMPORT bool ssl_enable_alpn;
enum ssl_protocol_versions
{
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index c85090259d..d9fc02adfc 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -152,6 +152,25 @@ typedef struct CancelRequestPacket
uint32 cancelAuthCode; /* secret key to authorize cancel */
} CancelRequestPacket;
+/* Application-Layer Protocol Negotiation is required for direct connections
+ * to avoid protocol confusion attacks (e.g https://alpaca-attack.com/).
+ *
+ * ALPN is specified in RFC 7301
+ *
+ * This string should be registered at:
+ * https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
+ *
+ * OpenSSL uses this wire-format for the list of alpn protocols even in the
+ * API. Both server and client take the same format parameter but the client
+ * actually sends it to the server as-is and the server it specifies the
+ * preference order to use to choose the one selected to send back.
+ *
+ * c.f. https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_alpn_select_cb.html
+ *
+ * The #define can be used to initialize a char[] vector to use directly in the API
+ */
+#define PG_ALPN_PROTOCOL "TBD-pgsql"
+#define PG_ALPN_PROTOCOL_VECTOR { 9, 'T','B','D','-','p','g','s','q','l' }
/*
* A client can also start by sending a SSL or GSSAPI negotiation request to
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7cd0eb261f..3118c19edd 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -314,6 +314,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"SSL-SNI", "", 1,
offsetof(struct pg_conn, sslsni)},
+ {"sslalpn", "PGSSLALPN", "1", NULL,
+ "SSL-ALPN", "", 1,
+ offsetof(struct pg_conn, sslalpn)},
+
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
"Require-Peer", "", 10,
offsetof(struct pg_conn, requirepeer)},
@@ -4487,6 +4491,7 @@ freePGconn(PGconn *conn)
free(conn->sslcrldir);
free(conn->sslcompression);
free(conn->sslsni);
+ free(conn->sslalpn);
free(conn->requirepeer);
free(conn->require_auth);
free(conn->ssl_min_protocol_version);
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 4d1e4009ef..02af7bf571 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -912,6 +912,9 @@ destroy_ssl_system(void)
#endif
}
+/* See pqcomm.h comments on OpenSSL implementation of ALPN (RFC 7301) */
+static unsigned char alpn_protos[] = PG_ALPN_PROTOCOL_VECTOR;
+
/*
* Create per-connection SSL object, and load the client certificate,
* private key, and trusted CA certs.
@@ -1240,6 +1243,20 @@ initialize_SSL(PGconn *conn)
}
}
+ if (conn->sslalpn && conn->sslalpn[0] == '1')
+ {
+ int retval;
+ retval = SSL_set_alpn_protos(conn->ssl, alpn_protos, sizeof(alpn_protos));
+
+ if (retval != 0)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+ libpq_append_conn_error(conn, "could not set ssl alpn extension: %s", err);
+ SSLerrfree(err);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
@@ -1723,6 +1740,7 @@ PQsslAttributeNames(PGconn *conn)
"cipher",
"compression",
"protocol",
+ "alpn",
NULL
};
static const char *const empty_attrs[] = {NULL};
@@ -1777,6 +1795,19 @@ PQsslAttribute(PGconn *conn, const char *attribute_name)
if (strcmp(attribute_name, "protocol") == 0)
return SSL_get_version(conn->ssl);
+ if (strcmp(attribute_name, "alpn") == 0)
+ {
+ const unsigned char *data;
+ unsigned int len;
+ static char alpn_str[256]; /* alpn doesn't support longer than 255 bytes */
+ SSL_get0_alpn_selected(conn->ssl, &data, &len);
+ if (data == NULL || len==0 || len > sizeof(alpn_str)-1)
+ return NULL;
+ memcpy(alpn_str, data, len);
+ alpn_str[len] = 0;
+ return alpn_str;
+ }
+
return NULL; /* unknown attribute */
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index db63afd786..17e5b2528f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -400,6 +400,7 @@ struct pg_conn
char *sslcrl; /* certificate revocation list filename */
char *sslcrldir; /* certificate revocation list directory name */
char *sslsni; /* use SSL SNI extension (0 or 1) */
+ char *sslalpn; /* use SSL ALPN extension (0 or 1) */
char *requirepeer; /* required peer credentials for local sockets */
char *gssencmode; /* GSS mode (require,prefer,disable) */
char *krbsrvname; /* Kerberos service name */
--
2.40.0
From 07197e9aba2715e6d864a95d09b06c3486e4cbca Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Fri, 31 Mar 2023 02:31:31 -0400
Subject: [PATCH v5 5/6] Allow pipelining data after ssl request
---
src/backend/postmaster/postmaster.c | 54 ++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9b4b37b997..33b317f98b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2001,13 +2001,14 @@ ProcessSSLStartup(Port *port)
if (port->raw_buf_remaining > 0)
{
- /* This shouldn't be possible -- it would mean the client sent
- * encrypted data before we established a session key...
- */
- elog(LOG, "Buffered unencrypted data remains after negotiating native SSL connection");
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("received unencrypted data after SSL request"),
+ errdetail("This could be either a client-software bug or evidence of an attempted man-in-the-middle attack.")));
return STATUS_ERROR;
}
- pfree(port->raw_buf);
+ if (port->raw_buf)
+ pfree(port->raw_buf);
if (port->ssl_alpn_protocol == NULL)
{
@@ -2158,15 +2159,44 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
}
#ifdef USE_SSL
- if (SSLok == 'S' && secure_open_server(port) == -1)
- return STATUS_ERROR;
+ if (SSLok == 'S')
+ {
+ /* push unencrypted buffered data back through SSL setup */
+ len = pq_buffer_has_data();
+ if (len > 0)
+ {
+ buf = palloc(len);
+ if (pq_getbytes(buf, len) == EOF)
+ return STATUS_ERROR; /* shouldn't be possible */
+ port->raw_buf = buf;
+ port->raw_buf_remaining = len;
+ port->raw_buf_consumed = 0;
+ }
+ Assert(pq_buffer_has_data() == 0);
+
+ if (secure_open_server(port) == -1)
+ return STATUS_ERROR;
+
+ /*
+ * At this point we should have no data already buffered. If we do,
+ * it was received before we performed the SSL handshake, so it wasn't
+ * encrypted and indeed may have been injected by a man-in-the-middle.
+ * We report this case to the client.
+ */
+ if (port->raw_buf_remaining > 0)
+ ereport(FATAL,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("received unencrypted data after SSL request"),
+ errdetail("This could be either a client-software bug or evidence of an attempted man-in-the-middle attack.")));
+ if (port->raw_buf)
+ pfree(port->raw_buf);
+ }
#endif
- /*
- * At this point we should have no data already buffered. If we do,
- * it was received before we performed the SSL handshake, so it wasn't
- * encrypted and indeed may have been injected by a man-in-the-middle.
- * We report this case to the client.
+ /* This can only really occur now if there was data pipelined after
+ * the SSL Request but we have refused to do SSL. In that case we need
+ * to give up because the client has presumably assumed the SSL
+ * request would be accepted.
*/
if (pq_buffer_has_data())
ereport(FATAL,
--
2.40.0
From 8d0abefa640fab9d9be16ae52fe65b86b272a537 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Thu, 16 Mar 2023 15:10:15 -0400
Subject: [PATCH v5 3/6] Direct SSL connections documentation
---
doc/src/sgml/libpq.sgml | 102 ++++++++++++++++++++++++++++++++++++----
1 file changed, 93 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9f72dd29d8..6efc70f801 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1691,10 +1691,13 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
Note that if <acronym>GSSAPI</acronym> encryption is possible,
that will be used in preference to <acronym>SSL</acronym>
encryption, regardless of the value of <literal>sslmode</literal>.
- To force use of <acronym>SSL</acronym> encryption in an
- environment that has working <acronym>GSSAPI</acronym>
- infrastructure (such as a Kerberos server), also
- set <literal>gssencmode</literal> to <literal>disable</literal>.
+ To negotiate <acronym>SSL</acronym> encryption in an environment that
+ has working <acronym>GSSAPI</acronym> infrastructure (such as a
+ Kerberos server), also set <literal>gssencmode</literal>
+ to <literal>disable</literal>.
+ Use of non-default values of <literal>sslnegotiation</literal> can
+ also cause <acronym>SSL</acronym> to be used instead of
+ negotiating <acronym>GSSAPI</acronym> encryption.
</para>
</listitem>
</varlistentry>
@@ -1721,6 +1724,75 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-sslnegotiation" xreflabel="sslnegotiation">
+ <term><literal>sslnegotiation</literal></term>
+ <listitem>
+ <para>
+ This option controls whether <productname>PostgreSQL</productname>
+ will perform its protocol negotiation to request encryption from the
+ server or will just directly make a standard <acronym>SSL</acronym>
+ connection. Traditional <productname>PostgreSQL</productname>
+ protocol negotiation is the default and the most flexible with
+ different server configurations. If the server is known to support
+ direct <acronym>SSL</acronym> connections then the latter requires one
+ fewer round trip reducing connection latency and also allows the use
+ of protocol agnostic SSL network tools.
+ </para>
+
+ <variablelist>
+ <varlistentry>
+ <term><literal>postgres</literal></term>
+ <listitem>
+ <para>
+ perform <productname>PostgreSQL</productname> protocol
+ negotiation. This is the default if the option is not provided.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>direct</literal></term>
+ <listitem>
+ <para>
+ first attempt to establish a standard SSL connection and if that
+ fails reconnect and perform the negotiation. This fallback
+ process adds significant latency if the initial SSL connection
+ fails.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>requiredirect</literal></term>
+ <listitem>
+ <para>
+ attempt to establish a standard SSL connection and if that fails
+ return a connection failure immediately.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ <para>
+ If <literal>sslmode</literal> set to <literal>disable</literal>
+ or <literal>allow</literal> then <literal>sslnegotiation</literal> is
+ ignored. If <literal>gssencmode</literal> is set
+ to <literal>require</literal> then <literal>sslnegotiation</literal>
+ must be the default <literal>postgres</literal> value.
+ </para>
+
+ <para>
+ Moreover, note if <literal>gssencmode</literal> is set
+ to <literal>prefer</literal> and <literal>sslnegotiation</literal>
+ to <literal>direct</literal> then the effective preference will be
+ direct <acronym>SSL</acronym> connections, followed by
+ negotiated <acronym>GSS</acronym> connections, followed by
+ negotiated <acronym>SSL</acronym> connections, possibly followed
+ lastly by unencrypted connections.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-sslcompression" xreflabel="sslcompression">
<term><literal>sslcompression</literal></term>
<listitem>
@@ -1930,11 +2002,13 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
<para>
The Server Name Indication can be used by SSL-aware proxies to route
- connections without having to decrypt the SSL stream. (Note that this
- requires a proxy that is aware of the PostgreSQL protocol handshake,
- not just any SSL proxy.) However, <acronym>SNI</acronym> makes the
- destination host name appear in cleartext in the network traffic, so
- it might be undesirable in some cases.
+ connections without having to decrypt the SSL stream. (Note that
+ unless the proxy is aware of the PostgreSQL protocol handshake this
+ would require setting <literal>sslnegotiation</literal>
+ to <literal>direct</literal> or <literal>requiredirect</literal>.)
+ However, <acronym>SNI</acronym> makes the destination host name appear
+ in cleartext in the network traffic, so it might be undesirable in
+ some cases.
</para>
</listitem>
</varlistentry>
@@ -8073,6 +8147,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLNEGOTIATION</envar></primary>
+ </indexterm>
+ <envar>PGSSLNEGOTIATION</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslnegotiation"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
--
2.40.0
From 6829925a144ac2acc605bbdebed7653b333e7e04 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Wed, 15 Mar 2023 15:51:02 -0400
Subject: [PATCH v5 1/6] Direct SSL connections postmaster support
---
src/backend/libpq/be-secure.c | 13 +++
src/backend/libpq/pqcomm.c | 9 +-
src/backend/postmaster/postmaster.c | 133 ++++++++++++++++++++++------
src/include/libpq/libpq-be.h | 10 +++
src/include/libpq/libpq.h | 2 +-
5 files changed, 134 insertions(+), 33 deletions(-)
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index a0f7084018..1020b3adb0 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -235,6 +235,19 @@ secure_raw_read(Port *port, void *ptr, size_t len)
{
ssize_t n;
+ /* Read from the "unread" buffered data first. c.f. libpq-be.h */
+ if (port->raw_buf_remaining > 0)
+ {
+ /* consume up to len bytes from the raw_buf */
+ if (len > port->raw_buf_remaining)
+ len = port->raw_buf_remaining;
+ Assert(port->raw_buf);
+ memcpy(ptr, port->raw_buf + port->raw_buf_consumed, len);
+ port->raw_buf_consumed += len;
+ port->raw_buf_remaining -= len;
+ return len;
+ }
+
/*
* Try to read from the socket without blocking. If it succeeds we're
* done, otherwise we'll wait for the socket using the latch mechanism.
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index da5bb5fc5d..17d025bb8e 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1126,13 +1126,16 @@ pq_discardbytes(size_t len)
/* --------------------------------
* pq_buffer_has_data - is any buffered data available to read?
*
- * This will *not* attempt to read more data.
+ * Actually returns the number of bytes in the buffer...
+ *
+ * This will *not* attempt to read more data. And reading up to that number of
+ * bytes should not cause reading any more data either.
* --------------------------------
*/
-bool
+size_t
pq_buffer_has_data(void)
{
- return (PqRecvPointer < PqRecvLength);
+ return (PqRecvLength - PqRecvPointer);
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 4c49393fc5..ec1d895a23 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -422,6 +422,7 @@ static void BackendRun(Port *port) pg_attribute_noreturn();
static void ExitPostmaster(int status) pg_attribute_noreturn();
static int ServerLoop(void);
static int BackendStartup(Port *port);
+static int ProcessSSLStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done);
static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
static void processCancelRequest(Port *port, void *pkt);
@@ -1926,6 +1927,97 @@ ServerLoop(void)
}
}
+/*
+ * Check for a native direct SSL connection.
+ *
+ * This happens before startup packets so we are careful not to actual read
+ * any bytes from the stream if it's not a direct SSL connection.
+ */
+
+static int
+ProcessSSLStartup(Port *port)
+{
+ int firstbyte;
+
+ pq_startmsgread();
+
+ firstbyte = pq_peekbyte();
+ if (firstbyte == EOF)
+ {
+ /*
+ * If we get no data at all, don't clutter the log with a complaint;
+ * such cases often occur for legitimate reasons. An example is that
+ * we might be here after responding to NEGOTIATE_SSL_CODE, and if the
+ * client didn't like our response, it'll probably just drop the
+ * connection. Service-monitoring software also often just opens and
+ * closes a connection without sending anything. (So do port
+ * scanners, which may be less benign, but it's not really our job to
+ * notice those.)
+ */
+ return STATUS_ERROR;
+ }
+
+ /*
+ * First byte indicates standard SSL handshake message
+ *
+ * (It can't be a Postgres startup length because in network byte order
+ * that would be a startup packet hundreds of megabytes long)
+ */
+ if (firstbyte == 0x16)
+ {
+#ifdef USE_SSL
+ ssize_t len;
+ char *buf = NULL;
+ elog(LOG, "Detected direct SSL handshake");
+
+ /* push unencrypted buffered data back through SSL setup */
+ len = pq_buffer_has_data();
+ if (len > 0)
+ {
+ buf = palloc(len);
+ if (pq_getbytes(buf, len) == EOF)
+ return STATUS_ERROR; /* shouldn't be possible */
+ port->raw_buf = buf;
+ port->raw_buf_remaining = len;
+ port->raw_buf_consumed = 0;
+ }
+
+ Assert(pq_buffer_has_data() == 0);
+ if (secure_open_server(port) == -1)
+ {
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("SSL Protocol Error during direct SSL connection initiation")));
+ return STATUS_ERROR;
+ }
+
+ if (port->raw_buf_remaining > 0)
+ {
+ /* This shouldn't be possible -- it would mean the client sent
+ * encrypted data before we established a session key...
+ */
+ elog(LOG, "Buffered unencrypted data remains after negotiating native SSL connection");
+ return STATUS_ERROR;
+ }
+ pfree(port->raw_buf);
+#else
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("Received direct SSL connection request with no SSL support")));
+ return STATUS_ERROR;
+#endif
+ }
+
+ pq_endmsgread();
+
+ if (port->ssl_in_use)
+ ereport(DEBUG2,
+ (errmsg_internal("Direct SSL connection established")));
+
+ return STATUS_OK;
+}
+
+
/*
* Read a client's startup packet and do something according to it.
*
@@ -1954,28 +2046,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
pq_startmsgread();
- /*
- * Grab the first byte of the length word separately, so that we can tell
- * whether we have no data at all or an incomplete packet. (This might
- * sound inefficient, but it's not really, because of buffering in
- * pqcomm.c.)
- */
- if (pq_getbytes((char *) &len, 1) == EOF)
- {
- /*
- * If we get no data at all, don't clutter the log with a complaint;
- * such cases often occur for legitimate reasons. An example is that
- * we might be here after responding to NEGOTIATE_SSL_CODE, and if the
- * client didn't like our response, it'll probably just drop the
- * connection. Service-monitoring software also often just opens and
- * closes a connection without sending anything. (So do port
- * scanners, which may be less benign, but it's not really our job to
- * notice those.)
- */
- return STATUS_ERROR;
- }
-
- if (pq_getbytes(((char *) &len) + 1, 3) == EOF)
+ if (pq_getbytes(((char *) &len), 4) == EOF)
{
/* Got a partial length word, so bleat about that */
if (!ssl_done && !gss_done)
@@ -2039,8 +2110,11 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
char SSLok;
#ifdef USE_SSL
- /* No SSL when disabled or on Unix sockets */
- if (!LoadedSSL || port->laddr.addr.ss_family == AF_UNIX)
+ /* No SSL when disabled or on Unix sockets.
+ *
+ * Also no SSL negotiation if we already have a direct SSL connection
+ */
+ if (!LoadedSSL || port->laddr.addr.ss_family == AF_UNIX || port->ssl_in_use)
SSLok = 'N';
else
SSLok = 'S'; /* Support for SSL */
@@ -2048,11 +2122,10 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
SSLok = 'N'; /* No support for SSL */
#endif
-retry1:
- if (send(port->sock, &SSLok, 1, 0) != 1)
+ while (secure_write(port, &SSLok, 1) != 1)
{
if (errno == EINTR)
- goto retry1; /* if interrupted, just retry */
+ continue; /* if interrupted, just retry */
ereport(COMMERROR,
(errcode_for_socket_access(),
errmsg("failed to send SSL negotiation response: %m")));
@@ -2093,7 +2166,7 @@ retry1:
GSSok = 'G';
#endif
- while (send(port->sock, &GSSok, 1, 0) != 1)
+ while (secure_write(port, &GSSok, 1) != 1)
{
if (errno == EINTR)
continue;
@@ -4396,7 +4469,9 @@ BackendInitialize(Port *port)
* Receive the startup packet (which might turn out to be a cancel request
* packet).
*/
- status = ProcessStartupPacket(port, false, false);
+ status = ProcessSSLStartup(port);
+ if (status == STATUS_OK)
+ status = ProcessStartupPacket(port, false, false);
/*
* Disable the timeout, and prevent SIGTERM again.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index ac6407e9f6..824b28e824 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -226,6 +226,16 @@ typedef struct Port
SSL *ssl;
X509 *peer;
#endif
+ /* This is a bit of a hack. The raw_buf is data that was previously read
+ * and buffered in a higher layer but then "unread" and needs to be read
+ * again while establishing an SSL connection via the SSL library layer.
+ *
+ * There's no API to "unread", the upper layer just places the data in the
+ * Port structure in raw_buf and sets raw_buf_remaining to the amount of
+ * bytes unread and raw_buf_consumed to 0.
+ */
+ char *raw_buf;
+ ssize_t raw_buf_consumed, raw_buf_remaining;
} Port;
#ifdef USE_SSL
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 50fc781f47..2b02f67257 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -80,7 +80,7 @@ extern int pq_getmessage(StringInfo s, int maxlen);
extern int pq_getbyte(void);
extern int pq_peekbyte(void);
extern int pq_getbyte_if_available(unsigned char *c);
-extern bool pq_buffer_has_data(void);
+extern size_t pq_buffer_has_data(void);
extern int pq_putmessage_v2(char msgtype, const char *s, size_t len);
extern bool pq_check_connection(void);
--
2.40.0
