On Thu, 19 Jan 2023 at 15:49, Vladimir Sitnikov
<sitnikov.vladi...@gmail.com> wrote:
>
> What if the server that supports 'fast TLS' added an extra notification in
> case client connects with a classic TLS?
> Then a capable client could remember host:port and try with newer TLS appoach
> the next time it connects.
>
> It would be transparent to the clients, and the users won't need to configure
> 'prefer classic or fast TLS'
> The old clients could discard the notification.
Hm. I hadn't really thought about the case of a new client connecting
to an old server. I don't think it's worth implementing a code path in
the server like this as it would then become cruft that would be hard
to ever get rid of.
I think you can do the same thing, more or less, in the client. Like
if the driver tries to connect via SSL and gets an error it remembers
that host/port and connects using negotiation in the future.
In practice though, by the time drivers support this it'll probably be
far enough in the future that they can just enable it and you can
disable it if you're connecting to an old server. The main benefit for
the near term is going to be clients that are specifically designed to
take advantage of it because it's necessary to enable the environment
they need -- like monitoring tools and proxies.
I've attached the POC. It's not near committable, mainly because of
the lack of any proper interface to the added fields in Port. I
actually had a whole API but ripped it out while debugging because it
wasn't working out.
But here's an example of psql connecting to the same server via
negotiated SSL or through stunnel where stunnel establishes the SSL
connection and psql is just doing plain text:
stark@hatter:~/src/postgresql$ ~/pgsql-sslhacked/bin/psql
'postgresql://localhost:9432/postgres'
psql (16devel)
SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384,
compression: off)
Type "help" for help.
postgres=# select * from pg_stat_ssl;
pid | ssl | version | cipher | bits | client_dn |
client_serial | issuer_dn
-------+-----+---------+------------------------+------+-----------+---------------+-----------
48771 | t | TLSv1.3 | TLS_AES_256_GCM_SHA384 | 256 | |
|
(1 row)
postgres=# \q
stark@hatter:~/src/postgresql$ ~/pgsql-sslhacked/bin/psql
'postgresql://localhost:8999/postgres'
psql (16devel)
Type "help" for help.
postgres=# select * from pg_stat_ssl;
pid | ssl | version | cipher | bits | client_dn |
client_serial | issuer_dn
-------+-----+---------+------------------------+------+-----------+---------------+-----------
48797 | t | TLSv1.3 | TLS_AES_256_GCM_SHA384 | 256 | |
|
(1 row)
--
greg
From 4508f872720a0977cf00041a865d76a4d5f77028 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Wed, 18 Jan 2023 15:34:34 -0500
Subject: [PATCH v1] Direct SSL Connections
---
src/backend/libpq/be-secure.c | 13 +++
src/backend/libpq/pqcomm.c | 9 +-
src/backend/postmaster/postmaster.c | 140 ++++++++++++++++++++++------
src/include/libpq/libpq-be.h | 3 +
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..39366d04dd 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;
+ /* XXX feed the raw_buf into SSL */
+ 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 864c9debe8..60fab6a52b 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1119,13 +1119,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 9cedc1b9f0..b1631e0830 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -412,6 +412,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);
@@ -1909,6 +1910,104 @@ 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);
+ ereport(DEBUG2,
+ (errmsg_internal("Direct native SSL connection set up")));
+
+#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 native SSL connection set up")));
+ else
+ ereport(DEBUG2,
+ (errmsg_internal("Direct native SSL connection NOT set up")));
+
+
+ return STATUS_OK;
+}
+
+
/*
* Read a client's startup packet and do something according to it.
*
@@ -1937,28 +2036,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)
@@ -2015,8 +2093,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
+ */
+ if (!LoadedSSL || port->laddr.addr.ss_family == AF_UNIX || port->ssl_in_use)
SSLok = 'N';
else
SSLok = 'S'; /* Support for SSL */
@@ -2024,11 +2105,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")));
@@ -2069,7 +2149,7 @@ retry1:
GSSok = 'G';
#endif
- while (send(port->sock, &GSSok, 1, 0) != 1)
+ while (secure_write(port, &GSSok, 1) != 1)
{
if (errno == EINTR)
continue;
@@ -4372,7 +4452,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 8c70b2fd5b..c2402ea8b8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -226,6 +226,9 @@ typedef struct Port
SSL *ssl;
X509 *peer;
#endif
+ /* XXX */
+ 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.39.0
