On 22/04/2024 10:47, Heikki Linnakangas wrote:
On 22/04/2024 10:19, Michael Paquier wrote:
On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote:
On 19/04/2024 19:48, Jacob Champion wrote:
On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
With direct SSL negotiation, we always require ALPN.
(As an aside: I haven't gotten to test the version of the patch that
made it into 17 yet, but from a quick glance it looks like we're not
rejecting mismatched ALPN during the handshake as noted in [1].)
Ah, good catch, that fell through the cracks. Agreed, the client should
reject a direct SSL connection if the server didn't send ALPN. I'll add that
to the Open Items so we don't forget again.
Would somebody like to write a patch for that? I'm planning to look
at this code more closely, as well.
I plan to write the patch later today.
Here's the patch for that. The error message is:
"direct SSL connection was established without ALPN protocol negotiation
extension"
That's accurate, but I wonder if we could make it more useful to a user
who's wondering what went wrong. I'd imagine that if the server doesn't
support ALPN, it's because you have some kind of a (not necessarily
malicious) generic SSL man-in-the-middle that doesn't support it. Or
you're trying to connect to an HTTPS server. Suggestions welcome.
--
Heikki Linnakangas
Neon (https://neon.tech)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ec20e3f3a9..f9156e29ca 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3524,6 +3524,13 @@ keep_going: /* We will come back to here until there is
pollres = pqsecure_open_client(conn);
if (pollres == PGRES_POLLING_OK)
{
+ /* ALPN is mandatory with direct SSL negotiation */
+ if (conn->current_enc_method == ENC_DIRECT_SSL && !conn->ssl_alpn_used)
+ {
+ libpq_append_conn_error(conn, "direct SSL connection was established without ALPN protocol negotiation extension");
+ CONNECTION_FAILED();
+ }
+
/*
* At this point we should have no data already buffered.
* If we do, it was received before we performed the SSL
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index e7a4d006e1..8df3c751db 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1585,6 +1585,31 @@ open_client_SSL(PGconn *conn)
}
}
+ /* Get the protocol selected by ALPN */
+ {
+ const unsigned char *selected;
+ unsigned int len;
+
+ SSL_get0_alpn_selected(conn->ssl, &selected, &len);
+
+ /* If ALPN is used, check that we negotiated the expected protocol */
+ if (selected != NULL)
+ {
+ if (len == strlen(PG_ALPN_PROTOCOL) &&
+ memcmp(selected, PG_ALPN_PROTOCOL, strlen(PG_ALPN_PROTOCOL)) == 0)
+ {
+ conn->ssl_alpn_used = true;
+ }
+ else
+ {
+ /* shouldn't happen */
+ libpq_append_conn_error(conn, "SSL connection was established with unexpected ALPN protocol");
+ pgtls_close(conn);
+ return PGRES_POLLING_FAILED;
+ }
+ }
+ }
+
/*
* We already checked the server certificate in initialize_SSL() using
* SSL_CTX_set_verify(), if root.crt exists.
@@ -1632,6 +1657,7 @@ pgtls_close(PGconn *conn)
conn->ssl = NULL;
conn->ssl_in_use = false;
conn->ssl_handshake_started = false;
+ conn->ssl_alpn_used = false;
destroy_needed = true;
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3691e5ee96..a6792917cf 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -569,6 +569,7 @@ struct pg_conn
bool ssl_handshake_started;
bool ssl_cert_requested; /* Did the server ask us for a cert? */
bool ssl_cert_sent; /* Did we send one in reply? */
+ bool ssl_alpn_used; /* Did we negotiate a protocol with TLS ALPN? */
#ifdef USE_SSL
#ifdef USE_OPENSSL
