And the cfbot wants a small tweak
From 3d0a502c25504da32b7a362831c700b4e891f79b Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Fri, 31 Mar 2023 02:31:31 -0400
Subject: [PATCH v6 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 5413f1a1ee897640bd3bb99bae226eec7e2e9f50 Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Mon, 20 Mar 2023 14:09:30 -0400
Subject: [PATCH v6 4/6] Direct SSL connections 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/backend/utils/misc/postgresql.conf.sample | 1 +
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 +
12 files changed, 167 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/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ee49ca3937..f93bcc23d0 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -118,6 +118,7 @@
#ssl_dh_params_file = ''
#ssl_passphrase_command = ''
#ssl_passphrase_command_supports_reload = off
+#ssl_enable_alpn = on
#------------------------------------------------------------------------------
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 083df15eff52f025064e2879a404270e405f7dde Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Thu, 16 Mar 2023 11:55:16 -0400
Subject: [PATCH v6 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 8d0abefa640fab9d9be16ae52fe65b86b272a537 Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Thu, 16 Mar 2023 15:10:15 -0400
Subject: [PATCH v6 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 af1646f62f0486a86be2afb771c44ed42e430dd1 Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Fri, 31 Mar 2023 03:01:35 -0400
Subject: [PATCH v6 6/6] Direct SSL connections some additional 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 6829925a144ac2acc605bbdebed7653b333e7e04 Mon Sep 17 00:00:00 2001
From: Greg Stark <[email protected]>
Date: Wed, 15 Mar 2023 15:51:02 -0400
Subject: [PATCH v6 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
