Subject: Re: Direct SSL connection with ALPN and HBA rules

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 19 Apr 2024 16:55:55 +0300

On 19/04/2024 08:06, Michael Paquier wrote:
> Hi all,
> (Heikki in CC.)

(Adding Jacob)

> Since 91044ae4baea (require ALPN for direct SSL connections) and
> d39a49c1e459 (direct hanshake), direct SSL connections are supported
> (yeah!), still the thread where this has been discussed does not cover
> the potential impact on HBA rules:
> https://www.postgresql.org/message-id/CAM-w4HOEAzxyY01ZKOj-iq%3DM4-VDk%3DvzQgUsuqiTFjFDZaebdg%40mail.gmail.com
> 
> My point is, would there be a point in being able to enforce that ALPN
> is used from the server rather than just relying on the client-side
> sslnegotiation to decide if direct SSL connections should be forced or
> not?
> 
> Hence, I'd imagine that we could have an HBA option for hostssl rules,
> like a negotiation={direct,postgres,all} that cross-checks
> Port->alpn_used with the option value in a hostssl entry, rejecting
> the use of connections using direct connections or the default
> protocol if these are not used, giving users a way to avoid one.  As
> this is a new thing, there may be an argument in this option for
> security reasons, as well, so as it would be possible for operators to
> turn that off in the server.

I don't think ALPN gives any meaningful security benefit, when used with 
the traditional 'postgres' SSL negotiation. There's little possibility 
of mixing that up with some other protocol, so I don't see any need to 
enforce it from server side. This was briefly discussed on that original 
thread [1]. With direct SSL negotiation, we always require ALPN.

I don't see direct SSL negotiation as a security feature. Rather, the 
point is to reduce connection latency by saving one round-trip. For 
example, if gssencmode=prefer, but the server refuses GSS encryption, it 
seems fine to continue with negotiated SSL, instead of establishing a 
new connection with direct SSL. What would be the use case of requiring 
direct SSL in the server? What extra security do you get?

Controlling these in HBA is a bit inconvenient, because you only find 
out after authentication if it's allowed or not. So if e.g. direct SSL 
connections are disabled for a user, the client would still establish a 
direct SSL connection, send the startup packet, and only then get 
rejected. The client would not know if it was rejected because of the 
direct SSL or for some reason, so it needs to retry with negotiated SSL. 
Currently, as it is master, if the TLS layer is established with direct 
SSL, you never need to retry with traditional negotiation, or vice versa.

[1] 
https://www.postgresql.org/message-id/CAAWbhmjetCVgu9pHJFkQ4ejuXuaz2mD1oniXokRHft0usCa7Yg%40mail.gmail.com

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Jacob Champion <[email-masked]>
Date: Fri, 19 Apr 2024 09:48:13 -0700

> > sslnegotiation to decide if direct SSL connections should be forced or

I'm a little confused about whether we're talking about requiring ALPN
or requiring direct connections. I think you mean the latter, Michael?

Personally, I was hoping that we'd have a postgresql.conf option to
reject every network connection that wasn't direct SSL, but I ran out
of time to review the patchset for 17. I would like to see server-side
enforcement of direct SSL in some way, eventually. I hadn't given much
thought to HBA, though.

> thread [1].

Agreed. By the time you've issued a traditional SSL startup packet,
and the server responds with a go-ahead, it's pretty much understood
what protocol is in use.

> 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].)

> I don't see direct SSL negotiation as a security feature.

`direct` mode is not, since it's opportunistic, but I think people are
going to use `requiredirect` as a security feature. At least, I was
hoping to do that myself...

> Rather, the
> new connection with direct SSL.

Well, assuming the user is okay with plaintext negotiation at all.
(Was that fixed before the patch went in? Is GSS negotiation still
allowed even with requiredirect?)

> What would be the use case of requiring

You get protection against attacks that could have otherwise happened
during the plaintext portion of the handshake. That has architectural
implications for more advanced uses of SCRAM, and it should prevent
any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the
TLS handshake, they can't send you anything that you might forget is
untrusted (like, say, an error message).

> connections are disabled for a user,

Hopefully disabling direct SSL piecemeal is not a desired use case?
I'm not sure it makes sense to focus on that. Forcing it to be enabled
shouldn't have the same problem, should it?

--Jacob

[1] https://www.postgresql.org/message-id/CAOYmi%2B%3DcnV-8V8TndSkEF6Htqa7qHQUL_KnQU8-DrT0Jjnm3_Q%40mail.gmail.com

---

From: Heikki Linnakangas <[email-masked]>
Date: Sat, 20 Apr 2024 00:43:24 +0300

> 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.

> allowed even with requiredirect?)

To disable sending the startup packet in plaintext, you need to use 
sslmode=require. Same as before the patch. GSS is still allowed, as it 
takes precedence over SSL if both are enabled in libpq. Per the docs:

> Note that if gssencmode is set to prefer, a GSS connection is
> attempted first. If the server rejects GSS encryption, SSL is
> negotiated over the same TCP connection using the traditional
> postgres protocol, regardless of sslnegotiation. In other words, the
> direct SSL handshake is not used, if a TCP connection has already
> been established and can be used for the SSL handshake.

> untrusted (like, say, an error message).

Can you elaborate on the more advanced uses of SCRAM?

> shouldn't have the same problem, should it?

Forcing it to be enabled piecemeal based on role or database has similar 
problems. Forcing it enabled for all connections seems sensible, though. 
Forcing it enabled based on the client's source IP address, but not 
user/database would be somewhat sensible too, but we don't currently 
have the HBA code to check the source IP and accept/reject SSLRequest 
based on that. The HBA rejection always happens after the client has 
sent the startup packet.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Michael Paquier <[email-masked]>
Date: Mon, 22 Apr 2024 16:19:53 +0900

On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote:
> 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.

> Can you elaborate on the more advanced uses of SCRAM?

I'm not sure what you mean here, either, Jacob.

>> shouldn't have the same problem, should it?

I'd get behind the case where a server rejects everything except
direct SSL, yeah.  Sticking that into a format similar to HBA rules
would easily give the flexibility to be able to accept or reject
direct or default SSL, though, while making it easy to parse.  The
implementation is not really complicated, and not far from the
existing hostssl and nohostssl.

As a whole, I can get behind a unique GUC that forces the use of
direct.  Or, we could extend the existing "ssl" GUC with a new
"direct" value to accept only direct connections and restrict the
original protocol (and a new "postgres" for the pre-16 protocol,
rejecting direct?), while "on" is able to accept both.

> user/database would be somewhat sensible too, but we don't currently have
> the HBA code to check the source IP and accept/reject SSLRequest based on
> that. The HBA rejection always happens after the client has sent the startup
> packet.

Hmm.  Splitting the logic checking HBA entries (around check_hba) so
as we'd check for a portion of its contents depending on what the
server has received or not from the client would not be that
complicated.  I'd question whether it makes sense to mix this
information within the same configuration files as the ones holding 
the current HBA rules.  If the same rules are used for the
pre-startup-packet phase and the post-startup-packet phase, we'd want
new keywords for these HBA rules, something different than the
existing sslmode and no sslmode?
--
Michael

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 22 Apr 2024 10:47:51 +0300

> at this code more closely, as well.

I plan to write the patch later today.

> rejecting direct?), while "on" is able to accept both.

I'd be OK with that, although I still don't really see the point of 
forcing this from the server side. We could also add this later.

> existing sslmode and no sslmode?

Sounds complicated, and I don't really see the use case for controlling 
the direct SSL support in such a fine-grained fashion.

It would be nice if we could reject non-SSL connections before the 
client sends the startup packet, but that's not possible because in a 
plaintext connection, that's the first packet that the client sends. The 
reverse would be possible: reject SSLRequest or direct SSL connection 
immediately, if HBA doesn't allow non-SSL connections from that IP 
address. But that's not very interesting.

HBA-based control would certainly be v18 material.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 23 Apr 2024 01:48:04 +0300

> 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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 23 Apr 2024 14:42:08 +0900

On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote:
> I'd be OK with that, although I still don't really see the point of forcing
> this from the server side. We could also add this later.

I'd be OK with doing something only in v18, if need be.  Jacob, what
do you think?

> Sounds complicated, and I don't really see the use case for controlling the
> direct SSL support in such a fine-grained fashion.
> 
> It would be nice if we could reject non-SSL connections before the client
> sends the startup packet, but that's not possible because in a plaintext
> connection, that's the first packet that the client sends. The reverse would
> be possible: reject SSLRequest or direct SSL connection immediately, if HBA
> doesn't allow non-SSL connections from that IP address. But that's not very
> interesting.

I'm not completely sure, actually.  We have the APIs to do that in
simple ways with existing keywords and new options.  And there is some
merit being able to have more complex connection policies.  If both of
you object to that, I won't insist.

> HBA-based control would certainly be v18 material.

Surely.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 23 Apr 2024 16:07:32 +0900

> "direct SSL connection was established without ALPN protocol negotiation

WFM.

> malicious) generic SSL man-in-the-middle that doesn't support it. Or you're
> trying to connect to an HTTPS server. Suggestions welcome.

Hmm.  Is there any point in calling SSL_get0_alpn_selected() in
open_client_SSL() to get the ALPN if current_enc_method is not
ENC_DIRECT_SSL?

In the documentation of PQsslAttribute(), it is mentioned that empty
string is returned for "alpn" if ALPN was not used, however the code
returns NULL in this case:
        SSL_get0_alpn_selected(conn->ssl, &data, &len);
        if (data == NULL || len == 0 || len > sizeof(alpn_str) - 1)
            return NULL;
--
Michael

---

From: Jacob Champion <[email-masked]>
Date: Tue, 23 Apr 2024 10:02:10 -0700

> that to the Open Items so we don't forget again.

Yes, the client should also reject, but that's not what I'm referring
to above. The server needs to fail the TLS handshake itself with the
proper error code (I think it's `no_application_protocol`?); otherwise
a client implementing a different protocol could consume the
application-level bytes coming back from the server and act on them.
That's the protocol confusion attack from ALPACA we're trying to
avoid.

> > been established and can be used for the SSL handshake.

Oh. That's actually disappointing, since gssencmode=prefer is the
default. A question I had in the original thread was, what's the
rationale behind a "require direct ssl" option that doesn't actually
require it?

> Can you elaborate on the more advanced uses of SCRAM?

If you're using SCRAM to authenticate the server, as opposed to just a
really strong password auth, then it really helps an analysis of the
security to know that there are no plaintext bytes that have been
interpreted by the client. This came up briefly in the conversations
that led to commit d0f4824a.

To be fair, it's a more academic concern at the moment; my imagination
can only come up with problems for SCRAM-based TLS that would also be
vulnerabilities for standard certificate-based TLS. But whether or not
it's an advantage for the code today is also kind of orthogonal to my
point. The security argument of direct SSL mode is that it reduces
risk for the system as a whole, even in the face of future code
changes or regressions. If you can't force its use, you're not
reducing that risk very much. (If anything, a "require" option that
doesn't actually require it makes the analysis more complicated, not
less...)

> problems.

Hm. For some reason I thought it was easier the other direction, but I
can't remember why I thought that. I'll withdraw the comment for now
:)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 23 Apr 2024 10:22:11 -0700

> I'd be OK with doing something only in v18, if need be.  Jacob, what

I think it would be nice to have an option like that. Whether it's
done now or in 18, I don't have a strong opinion about. But I do think
it'd be helpful to have a consensus on whether or not this is a
security improvement, or a performance enhancement only, before adding
said option. As it's implemented, if the requiredirect option doesn't
actually requiredirect, I think it looks like security but isn't
really.

(My ideal server-side option removes all plaintext negotiation and
forces the use of direct SSL for every connection, paired with a new
postgresqls:// scheme for the client. But I don't have any experience
making a switchover like that at scale, and I'd like to avoid a
StartTLS-vs-LDAPS sort of situation. That's obviously not a
conversation for 17.)

As for HBA control: overall, I don't see a burning need for an
HBA-based configuration, honestly. I'd prefer to reduce the number of
knobs and make it easier to apply the strongest security with a broad
brush.

--Jacob

---

From: Robert Haas <[email-masked]>
Date: Tue, 23 Apr 2024 13:43:34 -0400

> actually requiredirect, I think it looks like security but isn't

I've not followed this thread closely enough to understand the comment
about requiredirect maybe not actually requiring direct, but if that
were true it seems like it might be concerning.

But as far as having a GUC to force direct SSL or not, I agree that's
a good idea, and that it's better than only being able to control the
behavior through pg_hba.conf, because it removes room for any possible
doubt about whether you're really enforcing the behavior you want to
be enforcing. It might also mean that the connection can be rejected
earlier in the handshaking process on the basis of the GUC value,
which could conceivably prevent a client from reaching some piece of
code that turns out to have a security vulnerability. For example, if
we found out that direct SSL connections let you take over the
Pentagon before reaching the authentication stage, but for some reason
regular connections don't have the same problem, being able to
categorically shut off direct SSL would be valuable.

However, I don't really see why this has to be done for this release.
It seems like a separate feature from direct SSL itself. If direct SSL
hadn't been committed at the very last minute, then it would have been
great if this had been done for this release, too. But it was. The
moral we ought to take from that is "perhaps get the big features in a
bit further in advance of the freeze," not "well we'll just keep
hacking after the freeze."

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Jacob Champion <[email-masked]>
Date: Tue, 23 Apr 2024 12:33:58 -0700

It may be my misunderstanding. This seems to imply bad behavior:

> If the server rejects GSS encryption, SSL is
> negotiated over the same TCP connection using the traditional postgres
> protocol, regardless of <literal>sslnegotiation</literal>.

As does this comment:

> +   /*
> +    * If enabled, try direct SSL. Unless we have a valid TCP connection that
> +    * failed negotiating GSSAPI encryption or a plaintext connection in case
> +    * of sslmode='allow'; in that case we prefer to reuse the connection with
> +    * negotiated SSL, instead of reconnecting to do direct SSL. The point of
> +    * direct SSL is to avoid the roundtrip from the negotiation, but
> +    * reconnecting would also incur a roundtrip.
> +    */

but when I actually try those cases, I see that requiredirect does
actually cause a direct SSL connection to be done, even with
sslmode=allow. So maybe it's just misleading documentation (or my
misreading of it) that needs to be expanded? Am I missing a different
corner case where requiredirect is ignored, Heikki?

I still question the utility of allowing sslmode=allow with
sslnegotiation=requiredirect, because it seems like you've made both
the performance and security characteristics actively worse if you
choose that combination. But I want to make sure I understand the
current behavior correctly before I derail the discussion too much...

Thanks,
--Jacob

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 24 Apr 2024 00:19:57 +0300

> corner case where requiredirect is ignored, Heikki?

You're right, the comment is wrong about sslmode=allow. There is no 
negotiation of a plaintext connection, the client just sends the startup 
packet directly. The HBA rules can reject it, but the client will have 
to disconnect and reconnect in that case.

The documentation and that comment are misleading about failed GSSAPI 
encryption too, and I also misremembered that. With 
sslnegotiation=requiredirect, libpq never uses negotiated SSL mode. It 
will reconnect after a rejected GSSAPI request. So that comment applies 
to sslnegotiation=direct, but not sslnegotiation=requiredirect.

Attached patch tries to fix and clarify those.

(Note that the client will only attempt GSSAPI encryption if it can find 
kerberos credentials in the environment.)

-- 
Heikki Linnakangas
Neon (https://neon.tech)
From 664555decb695123a4bf25ea56f789202b926ea0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 24 Apr 2024 00:10:24 +0300
Subject: [PATCH 1/1] Fix documentation and comments on what happens after GSS
 rejection

The paragraph in the docs and the comment applied to
sslnegotiaton=direct, but not sslnegotiation=requiredirect. In
'requiredirect' mode, negotiated SSL is never used. Move the paragraph
in the docs under the description of 'direct' mode, and rephrase it.

Also the comment's reference to reusing a plaintext connection was
bogus. Authentication failure in plaintext mode only happens after
sending the startup packet, so the connection cannot be reused.
---
 doc/src/sgml/libpq.sgml           | 19 +++++++++----------
 src/interfaces/libpq/fe-connect.c | 11 ++++++-----
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9199d0d2e5..7f854edfa2 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1803,6 +1803,15 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
              process adds significant latency if the initial SSL connection
              fails.
            </para>
+           <para>
+             An exception is if <literal>gssencmode</literal> is set
+             to <literal>prefer</literal>, but the server rejects GSS encryption.
+             In that case, SSL is negotiated over the same TCP connection using
+             <productname>PostgreSQL</productname> protocol negotiation. In
+             other words, the direct SSL handshake is not used, if a TCP
+             connection has already been established and can be used for the
+             SSL handshake.
+           </para>
           </listitem>
          </varlistentry>
 
@@ -1816,16 +1825,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
           </listitem>
          </varlistentry>
         </variablelist>
-
-       <para>
-        Note that if <literal>gssencmode</literal> is set
-        to <literal>prefer</literal>, a <acronym>GSS</acronym> connection is
-        attempted first. If the server rejects GSS encryption, SSL is
-        negotiated over the same TCP connection using the traditional postgres
-        protocol, regardless of <literal>sslnegotiation</literal>. In other
-        words, the direct SSL handshake is not used, if a TCP connection has
-        already been established and can be used for the SSL handshake.
-       </para>
       </listitem>
      </varlistentry>
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ec20e3f3a9..b1d3bfa59d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4430,11 +4430,12 @@ select_next_encryption_method(PGconn *conn, bool have_valid_connection)
 
 	/*
 	 * If enabled, try direct SSL. Unless we have a valid TCP connection that
-	 * failed negotiating GSSAPI encryption or a plaintext connection in case
-	 * of sslmode='allow'; in that case we prefer to reuse the connection with
-	 * negotiated SSL, instead of reconnecting to do direct SSL. The point of
-	 * direct SSL is to avoid the roundtrip from the negotiation, but
-	 * reconnecting would also incur a roundtrip.
+	 * failed negotiating GSSAPI encryption; in that case we prefer to reuse
+	 * the connection with negotiated SSL, instead of reconnecting to do
+	 * direct SSL. The point of sslnegotiation=direct is to avoid the
+	 * roundtrip from the negotiation, but reconnecting would also incur a
+	 * roundtrip. (In sslnegotiation=requiredirect mode, negotiatied SSL is
+	 * not in the list of allowed methods and we will reconnect.)
 	 */
 	if (have_valid_connection)
 		SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
-- 
2.39.2

---

From: Jacob Champion <[email-masked]>
Date: Thu, 25 Apr 2024 09:16:16 -0700

> Attached patch tries to fix and clarify those.

s/negotiatied/negotiated/ in the attached patch, but other than that
this seems like a definite improvement. Thanks!

> kerberos credentials in the environment.)

Right. I don't like that it still happens with
sslnegotiation=requiredirect, but I suspect that this is not the
thread to complain about it in. Maybe I can propose a
sslnegotiation=forcedirect or something for 18, to complement a
postgresqls:// scheme.

That leaves the ALPACA handshake correction, I think. (Peter had some
questions on the original thread [1] that I've tried to answer.) And
the overall consensus, or lack thereof, on whether or not
`requiredirect` should be considered a security feature.

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/e782e9f4-a0cd-49f5-800b-5e32a1b29183%40eisentraut.org

---

From: Robert Haas <[email-masked]>
Date: Thu, 25 Apr 2024 12:17:41 -0400

On Thu, Apr 25, 2024 at 12:16 PM Jacob Champion
<[email-masked]> wrote
> sslnegotiation=forcedirect or something for 18, to complement a

It is difficult to imagine a world in which we have both requiredirect
and forcedirect and people are not confused.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 25 Apr 2024 09:28:16 -0700

> and forcedirect and people are not confused.

Yeah... Any thoughts on a better scheme? require_auth was meant to
lock down overly general authentication; maybe a require_proto or
something could do the same for the transport?

I hate that we have so many options that most people don't need but
take precedence, especially when they're based on the existence of
magic third-party environmental cues (e.g. Kerberos caches). And it
was nice that we got sslrootcert=system to turn on strong security and
reject nonsensical combinations. If someone sets `requiredirect` and
leaves the default sslmode, or chooses a weaker one... Is that really
useful to someone?

--Jacob

---

From: Robert Haas <[email-masked]>
Date: Thu, 25 Apr 2024 13:35:12 -0400

> something could do the same for the transport?

I don't understand the difference between the two sets of semantics
myself, so I'm not in a good position to comment.

> leaves the default sslmode, or chooses a weaker one... Is that really

Maybe I'm missing something here, but why doesn't sslnegotiation
override sslmode completely? Or alternatively, why not remove
sslnegotiation entirely and just have more sslmode values? I mean
maybe this shouldn't happen categorically, but if I say I want to
require a direct SSL connection, to me that implies that I don't want
an indirect SSL connection, and I really don't want a non-SSL
connection.

I think it's pretty questionable in 2024 whether sslmode=allow and
sslmode=prefer make any sense at all. I don't think it would be crazy
to remove them entirely. But I certainly don't think that they should
be allowed to bleed into the behavior of new, higher-security
configurations. Surely if I say I want direct SSL, it's that or
nothing, right?

---

From: Jacob Champion <[email-masked]>
Date: Thu, 25 Apr 2024 11:13:12 -0700

> an indirect SSL connection, and I really don't want a non-SSL

I think that comes down to the debate upthread, and whether you think
it's a performance tweak or a security feature. My take on it is,
`direct` mode is performance, and `requiredirect` is security.
(Especially since, with the current implementation, requiredirect can
slow things down?)

> configurations. Surely if I say I want direct SSL, it's that or

I agree, but I more or less lost the battle at [1]. Like Matthias
mentioned in [2]:

> I'm not sure about this either. The 'gssencmode' option is already
> quite weird in that it seems to override the "require"d priority of
> "sslmode=require", which it IMO really shouldn't.

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/CAOYmi%2B%3DcnV-8V8TndSkEF6Htqa7qHQUL_KnQU8-DrT0Jjnm3_Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAEze2Wi9j5Q3mRnuoD2Hr%3DeOFV-cMzWAUZ88YmSXSwsiJLQOWA%40mail.gmail.com

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 26 Apr 2024 00:50:33 +0300

>> an indirect SSL connection, and I really don't want a non-SSL

My thinking with sslnegotiation is that it controls how SSL is 
negotiated with the server, if SSL is to be used at all. It does not 
control whether SSL is used or required; that's what sslmode is for.

> `direct` mode is performance, and `requiredirect` is security.

Agreed, although the the security benefits from `requiredirect` are 
pretty vague. It reduces the attack surface, but there are no known 
issues with the 'postgres' or 'direct' negotiation either.

Perhaps 'requiredirect' should be renamed to 'directonly'?

> (Especially since, with the current implementation, requiredirect can

Yes: the case is gssencmode=prefer, kerberos credentical cache present 
in client, and server doesn't support GSS. With 
sslnegotiation='postgres' or 'direct', libpq can do the SSL negotiation 
over the same TCP connection after the server rejected the GSSRequest. 
With sslnegotiation='requiredirect', it needs to open a new TCP connection.

>> "sslmode=require", which it IMO really shouldn't.

Yeah, that combination is weird. I think we should forbid it. But that's 
separate from sslnegotiation.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Robert Haas <[email-masked]>
Date: Fri, 26 Apr 2024 11:25:21 -0400

> control whether SSL is used or required; that's what sslmode is for.

I think this might boil down to the order in which someone thinks that
different settings should be applied. It sounds like your mental model
is that GSS settings are applied first, and then SSL settings are
applied afterwards, and then within the SSL bucket you can select how
you want to do SSL (direct or negotiated) and how required it is. My
mental model is different: I imagine that since direct SSL happens
from the first byte exchanged over the socket, direct SSL "happens
first", making settings that pertain to negotiated GSS and negotiated
SSL irrelevant. Because, logically, if you've decided to use direct
SSL, you're not even going to get a chance to negotiate those things.
I understand that the code as written works around that, by being able
to open a new connection if it turns out that we need to negotiate
that stuff after all, but IMHO that's rather confusing.

---

From: Heikki Linnakangas <[email-masked]>
Date: Sat, 27 Apr 2024 01:50:54 +0300

> That's the protocol confusion attack from ALPACA we're trying to

I finally understood what you mean. So if the client supports ALPN, but 
the list of protocols that it provides does not include 'postgresql', 
the server should reject the connection with 'no_applicaton_protocol' 
alert. Makes sense. I thought OpenSSL would do that with the alpn 
callback we have, but it does not.

The attached patch makes that change. I used the alpn_cb() function in 
openssl's own s_server program as example for that.

Unfortunately the error message you got in the client with that was 
horrible (I modified the server to not accept the 'postgresql' protocol):

psql "dbname=postgres sslmode=require host=localhost"
psql: error: connection to server at "localhost" (::1), port 5432 
failed: SSL error: SSL error code 167773280

This is similar to the case with system errors discussed at 
https://postgr.es/m/[email-masked], but 
this one is equally bad on OpenSSL 1.1.1 and 3.3.0. It seems like an 
OpenSSL bug to me, because there is an error string "no application 
protocol" in the OpenSSL sources (ssl/ssl_err.c):

     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_NO_APPLICATION_PROTOCOL),
     "no application protocol"},

and in the server log, you get that message. But the error code seen in 
the client is different. There are also messages for other alerts, for 
example:

     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_TLSV13_ALERT_MISSING_EXTENSION),
     "tlsv13 alert missing extension"},

The bottom line is that that seems like a bug of omission to me in 
OpenSSL, but I wouldn't hold my breath waiting for it to be fixed. We 
can easily check for that error code and print the right message 
ourselves however, as in the attached patch.

-- 
Heikki Linnakangas
Neon (https://neon.tech)
From 125e9adda6cdab644b660772e29c713863e93cc2 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Sat, 27 Apr 2024 01:47:55 +0300
Subject: [PATCH 1/1] Reject SSL connection if ALPN is used but there's no
 common protocol

If the client supports ALPN but tries to use some other protocol, like
HTTPS, reject the connection in the server. That is surely a confusion
of some sort like trying to PostgreSQL server with a
browser. Furthermore, the ALPN RFC 7301 says:

> In the event that the server supports no protocols that the client
> advertises, then the server SHALL respond with a fatal
> "no_application_protocol" alert.

This commit makes the server follow that advice.

In the client, specifically check for the OpenSSL error code for the
"no_application_protocol" alert. Otherwise you got a cryptic "SSL
error: SSL error code 167773280" error if you tried to connect to a
non-PostgreSQL server that rejects the connection with
"no_application_protocol". ERR_reason_error_string() returns NULL for
that code, which frankly seems like an OpenSSL bug to me, but we can
easily print a better message ourselves.
---
 src/backend/libpq/be-secure-openssl.c    | 10 +++++++---
 src/interfaces/libpq/fe-secure-openssl.c | 12 ++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index fc46a33539..60cf68aac4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1336,10 +1336,14 @@ alpn_cb(SSL *ssl,
 
 	if (retval == OPENSSL_NPN_NEGOTIATED)
 		return SSL_TLSEXT_ERR_OK;
-	else if (retval == OPENSSL_NPN_NO_OVERLAP)
-		return SSL_TLSEXT_ERR_NOACK;
 	else
-		return SSL_TLSEXT_ERR_NOACK;
+	{
+		/*
+		 * The client doesn't support our protocol.  Reject the connection
+		 * with TLS "no_application_protocol" alert, per RFC 7301.
+		 */
+		return SSL_TLSEXT_ERR_ALERT_FATAL;
+	}
 }

diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0de21dc7e4..ee1a47f2b1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1741,6 +1741,18 @@ SSLerrmessage(unsigned long ecode)
 		return errbuf;
 	}
 
+	if (ERR_GET_LIB(ecode) == ERR_LIB_SSL &&
+		ERR_GET_REASON(ecode) == SSL_AD_REASON_OFFSET + SSL_AD_NO_APPLICATION_PROTOCOL)
+	{
+		/*
+		 * Server aborted the connection with TLS "no_application_protocol"
+		 * alert.  The ERR_reason_error_string() function doesn't give any
+		 * error string for that for some reason, so do it ourselves.
+		 */
+		snprintf(errbuf, SSL_ERR_LEN, libpq_gettext("no application protocol"));
+		return errbuf;
+	}
+
 	/*
 	 * In OpenSSL 3.0.0 and later, ERR_reason_error_string randomly refuses to
 	 * map system errno values.  We can cover that shortcoming with this bit
-- 
2.39.2

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 29 Apr 2024 12:43:18 +0300

>          if (data == NULL || len == 0 || len > sizeof(alpn_str) - 1)

Good catch. I changed the code to return an empty string, as the 
documentation says.

I considered if NULL or empty string would be better here. The docs for 
PQsslAttribute also says:

"Returns NULL if the connection does not use SSL or the specified 
attribute name is not defined for the library in use."

If a caller wants to distinguish between "libpq or the SSL library 
doesn't support ALPN at all" from "the server didn't support ALPN", you 
can tell from whether PQsslAttribute returns NULL or an empty string. So 
I think an empty string is better.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Michael Paquier <[email-masked]>
Date: Mon, 29 Apr 2024 21:07:58 +0900

On Mon, Apr 29, 2024 at 12:43:18PM +0300, Heikki Linnakangas wrote:
> If a caller wants to distinguish between "libpq or the SSL library doesn't
> support ALPN at all" from "the server didn't support ALPN", you can tell
> from whether PQsslAttribute returns NULL or an empty string. So I think an
> empty string is better.

Thanks.  I would also have used an empty string to differenciate these
two cases.
--
Michael

---

From: Jacob Champion <[email-masked]>
Date: Mon, 29 Apr 2024 11:04:41 -0700

On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <[email-masked]> wrote:
> alert.

Right. (And additionally, we reject clients that don't advertise ALPN
over direct SSL, also during the TLS handshake.)

> openssl's own s_server program as example for that.

This patch as written will apply the new requirement to the old
negotiation style, though, won't it? My test suite sees a bunch of
failures with that.

> failed: SSL error: SSL error code 167773280

<long sigh>

I filed a bug upstream [1].

Thanks,
--Jacob

[1] https://github.com/openssl/openssl/issues/24300

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 29 Apr 2024 21:43:22 +0300

> negotiation style, though, won't it? My test suite sees a bunch of

Yes, and that is what we want, right? If the client uses old negotiation 
style, and includes ALPN in its ClientHello, but requests protocol 
"noodles" instead of "postgresql", it seems good to reject the connection.

Note that if the client does not request ALPN at all, the callback is 
not called, and the connection is accepted. Old clients still work 
because they do not request ALPN.

> I filed a bug upstream [1].

Thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Jacob Champion <[email-masked]>
Date: Mon, 29 Apr 2024 11:51:54 -0700

> because they do not request ALPN.

Ugh, sorry for the noise -- I couldn't figure out why all my old
clients were failing and then realized it was because I'd left some
test code in place for the OpenSSL bug. I'll rebuild everything and
keep reviewing.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 14 May 2024 10:14:38 -0700

> I filed a bug upstream [1].

I think this is on track to be fixed in a future set of OpenSSL 3.x
releases [2]. We'll still need to carry the workaround while we
support 1.1.1.

--Jacob

[2] https://github.com/openssl/openssl/pull/24351