Обсуждение: Direct SSL connection with ALPN and HBA rules

Hi all,
(Heikki in CC.)

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.

Thoughts or comments?
--
Michael

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)

On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 19/04/2024 08:06, Michael Paquier wrote:
> > 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?

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.

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

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

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
> direct SSL in the server? What extra security do you get?

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

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

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

On 19/04/2024 19:48, Jacob Champion wrote:
> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@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.

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

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.


>> What would be the use case of requiring
>> direct SSL in the server? What extra security do you get?
> 
> 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).

Can you elaborate on the more advanced uses of SCRAM?

>> 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,
> 
> 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?

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)

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

>> 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).
>
> Can you elaborate on the more advanced uses of SCRAM?

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

>>> 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,
>>
>> 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?

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.

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

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

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

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

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.

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

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)

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 <hlinnaka@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)

On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote:
> On 22/04/2024 10:19, Michael Paquier wrote:
>> 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.
>
> 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?

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

On Tue, Apr 23, 2024 at 01:48:04AM +0300, Heikki Linnakangas wrote:
> Here's the patch for that. The error message is:
>
> "direct SSL connection was established without ALPN protocol negotiation
> extension"

WFM.

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

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

On Fri, Apr 19, 2024 at 2:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 19/04/2024 19:48, Jacob Champion wrote:
> > On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@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.

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.

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

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?

> >> What would be the use case of requiring
> >> direct SSL in the server? What extra security do you get?
> >
> > 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).
>
> 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...)

> >> 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,
> >
> > 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?
>
> Forcing it to be enabled piecemeal based on role or database has similar
> 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

On Mon, Apr 22, 2024 at 10:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote:
> > On 22/04/2024 10:19, Michael Paquier wrote:
> >> 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.
> >
> > 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?

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

On Tue, Apr 23, 2024 at 1:22 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On Mon, Apr 22, 2024 at 10:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> > On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote:
> > > On 22/04/2024 10:19, Michael Paquier wrote:
> > >> 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.
> > >
> > > 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?
>
> 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.

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

On Tue, Apr 23, 2024 at 10:43 AM Robert Haas <robertmhaas@gmail.com> wrote:
> 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.

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

On 23/04/2024 22:33, Jacob Champion wrote:
> On Tue, Apr 23, 2024 at 10:43 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
> 
> 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?

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)

On Tue, Apr 23, 2024 at 2:20 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> 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!

> (Note that the client will only attempt GSSAPI encryption if it can find
> 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

On Thu, Apr 25, 2024 at 12:16 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote
> 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.

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

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

On Thu, Apr 25, 2024 at 9:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> It is difficult to imagine a world in which we have both requiredirect
> 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

On Thu, Apr 25, 2024 at 12:28 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On Thu, Apr 25, 2024 at 9:17 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > It is difficult to imagine a world in which we have both requiredirect
> > 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 don't understand the difference between the two sets of semantics
myself, so I'm not in a good position to comment.

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

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?

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

On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> 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 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?)

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

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

On 25/04/2024 21:13, Jacob Champion wrote:
> On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.

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.

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

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
> slow things down?)

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.

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

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

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

On Thu, Apr 25, 2024 at 2:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > 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.
>
> 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.

I think reduction in attack surface is a concrete security benefit,
not a vague one. True, I don't know of any exploits today, but that
seems almost tautological -- if there were known exploits in our
upgrade handshake, I assume we'd be working to fix them ASAP?

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

If it's agreed that we don't want to require a stronger sslmode for
that sslnegotiation setting, then that would probably be an
improvement. But who is the target user for
`sslnegotiation=directonly`, in your opinion? Would they ever have a
reason to use a weak sslmode?

> >> 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.
>
> Yeah, that combination is weird. I think we should forbid it. But that's
> separate from sslnegotiation.

Separate but related, IMO. If we were all hypothetically okay with
gssencmode ignoring `sslmode=require`, then it's hard for me to claim
that `sslnegotiation=requiredirect` should behave differently. On the
other hand, if we're not okay with that and we'd like to change it,
it's easier for me to argue that `requiredirect` should also be
stricter from the get-go.

Thanks,
--Jacob

On Thu, Apr 25, 2024 at 5:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 25/04/2024 21:13, Jacob Champion wrote:
> > On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >> 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.
>
> 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.

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.

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

On 23/04/2024 20:02, Jacob Champion wrote:
> On Fri, Apr 19, 2024 at 2:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 19/04/2024 19:48, Jacob Champion wrote:
>>> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@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.
> 
> 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.

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/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca, 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)

On 26/04/2024 02:23, Jacob Champion wrote:
> On Thu, Apr 25, 2024 at 2:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> 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.
>>
>> 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.
> 
> I think reduction in attack surface is a concrete security benefit,
> not a vague one. True, I don't know of any exploits today, but that
> seems almost tautological -- if there were known exploits in our
> upgrade handshake, I assume we'd be working to fix them ASAP?

Sure, we'd try to fix them ASAP. But we've had the SSLRequest 
negotiation since time immemorial. If a new vulnerability is found, it's 
unlikely that we'd need to disable the SSLRequest negotiation altogether 
to fix it. We'd be in serious trouble with back-branches in that case. 
There's no sudden need to have a kill-switch for it.

Taking that to the extreme, you could argue for a kill-switch for every 
feature, just in case there's a vulnerability in them. I agree that 
authentication is more sensitive so reducing the surface of that is more 
reasonable. And but nevertheless.

(This discussion is moot though, because we do have the 
sslnegotiation=requiredirect mode, so you can disable the SSLRequest 
negotiation.)

>> Perhaps 'requiredirect' should be renamed to 'directonly'?
> 
> If it's agreed that we don't want to require a stronger sslmode for
> that sslnegotiation setting, then that would probably be an
> improvement. But who is the target user for
> `sslnegotiation=directonly`, in your opinion? Would they ever have a
> reason to use a weak sslmode?

It's unlikely, I agree. A user who's sophisticated enough to use 
sslnegotiation=directonly would probably also want sslmode=require and 
require_auth=scram-sha256 and channel_binding=require. Or 
sslmode=verify-full. But in principle they're orthogonal. If you only 
have v17 servers in your environment, so you know all servers support 
direct negotiation if they support SSL at all, but a mix of servers with 
and without SSL, sslnegotiation=directonly would reduce roundtrips with 
sslmode=prefer.

Making requiredirect to imply sslmode=require, or error out unless you 
also set sslmode=require, feels like a cavalier way of forcing SSL. We 
should have a serious discussion on making sslmode=require the default 
instead. That would be a more direct way of nudging people to use SSL. 
It would cause a lot of breakage, but it would also be a big improvement 
to security.

Consider how sslnegotiation=requiredirect/directonly would feel, if we 
made sslmode=require the default. If you explicitly set "sslmode=prefer" 
or "sslmode=disable", it would be annoying if you would also need to 
remove "sslnegotiation=requiredirect" from your connection string.

I'm leaning towards renaming sslnegotiation=requiredirect to 
sslnegotiation=directonly at this point.

>>>> 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.
>>
>> Yeah, that combination is weird. I think we should forbid it. But that's
>> separate from sslnegotiation.
> 
> Separate but related, IMO. If we were all hypothetically okay with
> gssencmode ignoring `sslmode=require`, then it's hard for me to claim
> that `sslnegotiation=requiredirect` should behave differently. On the
> other hand, if we're not okay with that and we'd like to change it,
> it's easier for me to argue that `requiredirect` should also be
> stricter from the get-go.

I think the best way forward for those is something like a new 
"require_proto" parameter that you suggested upthread. Perhaps call it 
"encryption", with options "none", "ssl", "gss" so that you can provide 
multiple options and libpq will try them in the order specified. For 
example:

encryption=none
encryption=ssl, none  # like sslmode=prefer
encryption=gss
encryption=gss, ssl   # try GSS first, then SSL
encryption=ssl, gss   # try SSL first, then GSS

This would make gssencmode and sslmode=disable/allow/prefer/require 
settings obsolete. sslmode would stil be needed to distinguish between 
verify-ca/verify-full though. But sslnegotiation would be still 
orthogonal to that.

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

On 23/04/2024 10:07, Michael Paquier wrote:
> 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;

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)

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

On Mon, Apr 29, 2024 at 4:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Making requiredirect to imply sslmode=require, or error out unless you
> also set sslmode=require, feels like a cavalier way of forcing SSL. We
> should have a serious discussion on making sslmode=require the default
> instead. That would be a more direct way of nudging people to use SSL.
> It would cause a lot of breakage, but it would also be a big improvement
> to security.
>
> Consider how sslnegotiation=requiredirect/directonly would feel, if we
> made sslmode=require the default. If you explicitly set "sslmode=prefer"
> or "sslmode=disable", it would be annoying if you would also need to
> remove "sslnegotiation=requiredirect" from your connection string.

I think making sslmode=require the default is pretty unworkable,
unless we also had a way of automatically setting up SSL as part of
initdb or something. Otherwise, we'd have to add sslmode=disable to a
million places just to get the regression tests to work, and every
test cluster anyone spins up locally would break in annoying ways,
too. I had been thinking we might want to change the default to
sslmode=disable and remove allow and prefer, but maybe automating a
basic SSL setup is better. Either way, we should move toward a world
where you either ask for SSL and get it, or don't ask for it and don't
get it. Being halfway in between is bad.

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

On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.

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

> The attached patch makes that change. I used the alpn_cb() function in
> 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.

> 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

<long sigh>

I filed a bug upstream [1].

Thanks,
--Jacob

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

On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Sure, we'd try to fix them ASAP. But we've had the SSLRequest
> negotiation since time immemorial. If a new vulnerability is found, it's
> unlikely that we'd need to disable the SSLRequest negotiation altogether
> to fix it. We'd be in serious trouble with back-branches in that case.
> There's no sudden need to have a kill-switch for it.

I'm not really arguing that you'd need the kill switch to fix a
problem in the code. (At least, I'm not arguing that in this thread; I
reserve the right to argue that in the future. :D) But between the
point of time that a vulnerability is announced and a user has
upgraded, it's really nice to have a switch as a mitigation. Even
better if it's server-side, because then the DBA can protect all their
clients without requiring action on their part.

> Taking that to the extreme, you could argue for a kill-switch for every
> feature, just in case there's a vulnerability in them. I agree that
> authentication is more sensitive so reducing the surface of that is more
> reasonable. And but nevertheless.

I mean... that would be extreme, yeah. I don't think anyone's proposed that.

> If you only
> have v17 servers in your environment, so you know all servers support
> direct negotiation if they support SSL at all, but a mix of servers with
> and without SSL, sslnegotiation=directonly would reduce roundtrips with
> sslmode=prefer.

But if you're in that situation, what does the use of directonly give
you over `sslnegotiation=direct`? You already know that servers
support direct, so there's no additional performance penalty from the
less strict mode.

> Making requiredirect to imply sslmode=require, or error out unless you
> also set sslmode=require, feels like a cavalier way of forcing SSL. We
> should have a serious discussion on making sslmode=require the default
> instead. That would be a more direct way of nudging people to use SSL.
> It would cause a lot of breakage, but it would also be a big improvement
> to security.
>
> Consider how sslnegotiation=requiredirect/directonly would feel, if we
> made sslmode=require the default. If you explicitly set "sslmode=prefer"
> or "sslmode=disable", it would be annoying if you would also need to
> remove "sslnegotiation=requiredirect" from your connection string.

That's similar to how sslrootcert=system already works. To me, it
feels great, because I don't have to worry about nonsensical
combinations (with the exception of GSS, which we've touched on
above). libpq complains loudly if I try to shoot myself in the foot,
and if I'm using sslrootcert=system then it's a pretty clear signal
that I care more about security than the temporary inconvenience of
editing my connection string for one weird server that doesn't use SSL
for some reason.

> I think the best way forward for those is something like a new
> "require_proto" parameter that you suggested upthread. Perhaps call it
> "encryption", with options "none", "ssl", "gss" so that you can provide
> multiple options and libpq will try them in the order specified. For
> example:
>
> encryption=none
> encryption=ssl, none  # like sslmode=prefer
> encryption=gss
> encryption=gss, ssl   # try GSS first, then SSL
> encryption=ssl, gss   # try SSL first, then GSS
>
> This would make gssencmode and sslmode=disable/allow/prefer/require
> settings obsolete. sslmode would stil be needed to distinguish between
> verify-ca/verify-full though. But sslnegotiation would be still
> orthogonal to that.

I will give this some more thought.

Thanks,
--Jacob

On 29/04/2024 21:04, Jacob Champion wrote:
> On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.
> 
> Right. (And additionally, we reject clients that don't advertise ALPN
> over direct SSL, also during the TLS handshake.)
> 
>> The attached patch makes that change. I used the alpn_cb() function in
>> 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.

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.

>> 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
> 
> <long sigh>
> 
> I filed a bug upstream [1].

Thanks!

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

On Mon, Apr 29, 2024 at 11:43 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.

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

On 29/04/2024 21:43, Jacob Champion wrote:
> On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> If you only
>> have v17 servers in your environment, so you know all servers support
>> direct negotiation if they support SSL at all, but a mix of servers with
>> and without SSL, sslnegotiation=directonly would reduce roundtrips with
>> sslmode=prefer.
> 
> But if you're in that situation, what does the use of directonly give
> you over `sslnegotiation=direct`? You already know that servers
> support direct, so there's no additional performance penalty from the
> less strict mode.

Well, by that argument we don't need requiredirect/directonly at all. 
This goes back to whether it's a security feature or a performance feature.

There is a small benefit with sslmode=prefer if you connect to a server 
that doesn't support SSL, though. With sslnegotiation=direct, if the 
server rejects the direct SSL connection, the client will reconnect and 
try SSL with SSLRequest. The server will respond with 'N', and the 
client will proceed without encryption. sslnegotiation=directonly 
removes that SSLRequest attempt, eliminating one roundtrip.

>> Making requiredirect to imply sslmode=require, or error out unless you
>> also set sslmode=require, feels like a cavalier way of forcing SSL. We
>> should have a serious discussion on making sslmode=require the default
>> instead. That would be a more direct way of nudging people to use SSL.
>> It would cause a lot of breakage, but it would also be a big improvement
>> to security.
>>
>> Consider how sslnegotiation=requiredirect/directonly would feel, if we
>> made sslmode=require the default. If you explicitly set "sslmode=prefer"
>> or "sslmode=disable", it would be annoying if you would also need to
>> remove "sslnegotiation=requiredirect" from your connection string.
> 
> That's similar to how sslrootcert=system already works. To me, it
> feels great, because I don't have to worry about nonsensical
> combinations (with the exception of GSS, which we've touched on
> above). libpq complains loudly if I try to shoot myself in the foot,
> and if I'm using sslrootcert=system then it's a pretty clear signal
> that I care more about security than the temporary inconvenience of
> editing my connection string for one weird server that doesn't use SSL
> for some reason.

Oh I was not aware sslrootcert=system works like that. That's a bit 
surprising, none of the other ssl-related settings imply or require that 
SSL is actually used. Did we intend to set a precedence for new settings 
with that?

(adding Daniel in case he has an opinion)

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

On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 29/04/2024 21:43, Jacob Champion wrote:
> > But if you're in that situation, what does the use of directonly give
> > you over `sslnegotiation=direct`? You already know that servers
> > support direct, so there's no additional performance penalty from the
> > less strict mode.
>
> Well, by that argument we don't need requiredirect/directonly at all.
> This goes back to whether it's a security feature or a performance feature.

That's what I've been trying to argue, yeah. If it's not a security
feature... why's it there?

> There is a small benefit with sslmode=prefer if you connect to a server
> that doesn't support SSL, though. With sslnegotiation=direct, if the
> server rejects the direct SSL connection, the client will reconnect and
> try SSL with SSLRequest. The server will respond with 'N', and the
> client will proceed without encryption. sslnegotiation=directonly
> removes that SSLRequest attempt, eliminating one roundtrip.

Okay, agreed that in this case, there is a performance benefit. It's
not enough to convince me, honestly, but are there any other cases I
missed as well?

> Oh I was not aware sslrootcert=system works like that. That's a bit
> surprising, none of the other ssl-related settings imply or require that
> SSL is actually used.

For sslrootcert=system in particular, the danger of accidentally weak
sslmodes is pretty high, especially for verify-ca mode. (It goes back
to that other argument -- there should be, effectively, zero users who
both opt in to the public CA system, and are also okay with silently
falling back and not using it.)

> Did we intend to set a precedence for new settings
> with that?

(I'll let committers answer whether they intended that or not -- I was
just bringing up that we already have a setting that works like that,
and I really like how it works in practice. But it's probably
unsurprising that I like it.)

--Jacob

On Mon, Apr 29, 2024 at 12:32 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > On 29/04/2024 21:43, Jacob Champion wrote:
> > > But if you're in that situation, what does the use of directonly give
> > > you over `sslnegotiation=direct`? You already know that servers
> > > support direct, so there's no additional performance penalty from the
> > > less strict mode.
> >
> > Well, by that argument we don't need requiredirect/directonly at all.
> > This goes back to whether it's a security feature or a performance feature.
>
> That's what I've been trying to argue, yeah. If it's not a security
> feature... why's it there?

Er, I should clarify this. I _want_ requiredirect. I just want it to
be a security feature.

--Jacob

> On 29 Apr 2024, at 21:06, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> Oh I was not aware sslrootcert=system works like that. That's a bit surprising, none of the other ssl-related
settingsimply or require that SSL is actually used. Did we intend to set a precedence for new settings with that? 

It was very much intentional, and documented, an sslmode other than verify-full
makes little sense when combined with sslrootcert=system.  It wasn't intended
to set a precedence (though there is probably a fair bit of things we can do,
getting this right is hard enough as it is), rather it was footgun prevention.

--
Daniel Gustafsson

On 29/04/2024 22:32, Jacob Champion wrote:
> On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> There is a small benefit with sslmode=prefer if you connect to a server
>> that doesn't support SSL, though. With sslnegotiation=direct, if the
>> server rejects the direct SSL connection, the client will reconnect and
>> try SSL with SSLRequest. The server will respond with 'N', and the
>> client will proceed without encryption. sslnegotiation=directonly
>> removes that SSLRequest attempt, eliminating one roundtrip.
> 
> Okay, agreed that in this case, there is a performance benefit. It's
> not enough to convince me, honestly, but are there any other cases I
> missed as well?

I realized one case that hasn't been discussed so far: If you use the 
combination of "sslmode=prefer sslnegotiation=requiredirect" to connect 
to a pre-v17 server that has SSL enabled but does not support direct SSL 
connections, you will fall back to a plaintext connection instead. 
That's almost certainly not what you wanted. I'm coming around to your 
opinion that we should not allow that combination.

Stepping back to summarize my thoughts, there are now three things I 
don't like about the status quo:

1. As noted above, the sslmode=prefer and sslnegotiation=requiredirect 
combination is somewhat dangerous, as you might unintentionally fall 
back to plaintext authentication when connecting to a pre-v17 server.

2. There is an asymmetry between "postgres" and "direct"
option names. "postgres" means "try only traditional negotiation", while
"direct" means "try direct first, and fall back to traditional
negotiation if it fails". That is apparent only if you know that the
"requiredirect" mode also exists.

3. The "require" word in "requiredirect" suggests that it's somehow
more strict or more secure, similar to sslmode=require. However, I don't 
consider direct SSL connections to be a security feature.


New proposal:

- Remove the "try both" mode completely, and rename "requiredirect" to 
just "direct". So there would be just two modes: "postgres" and 
"direct". On reflection, the automatic fallback mode doesn't seem very 
useful. It would make sense as the default, because then you would get 
the benefits automatically in most cases but still be compatible with 
old servers. But if it's not the default, you have to fiddle with libpq 
settings anyway to enable it, and then you might as well use the 
"requiredirect" mode when you know the server supports it. There isn't 
anything wrong with it as such, but given how much confusion there's 
been on how this all works, I'd prefer to cut this back to the bare 
minimum now. We can add it back in the future, and perhaps make it the 
default at the same time. This addresses points 2. and 3. above.

and:

- Only allow sslnegotiation=direct with sslmode=require or higher. This 
is what you, Jacob, wanted to do all along, and addresses point 1.

Thoughts?

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

On Fri, 10 May 2024 at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> New proposal:
>
> - Remove the "try both" mode completely, and rename "requiredirect" to
> just "direct". So there would be just two modes: "postgres" and
> "direct". On reflection, the automatic fallback mode doesn't seem very
> useful. It would make sense as the default, because then you would get
> the benefits automatically in most cases but still be compatible with
> old servers. But if it's not the default, you have to fiddle with libpq
> settings anyway to enable it, and then you might as well use the
> "requiredirect" mode when you know the server supports it. There isn't
> anything wrong with it as such, but given how much confusion there's
> been on how this all works, I'd prefer to cut this back to the bare
> minimum now. We can add it back in the future, and perhaps make it the
> default at the same time. This addresses points 2. and 3. above.
>
> and:
>
> - Only allow sslnegotiation=direct with sslmode=require or higher. This
> is what you, Jacob, wanted to do all along, and addresses point 1.
>
> Thoughts?

Sounds mostly good to me. But I think we'd want to automatically
increase sslmode to require if it is unset, but sslnegotation is set
to direct. Similar to how we bump sslmode to verify-full if
sslrootcert is set to system, but sslmode is unset. i.e. it seems
unnecessary/unwanted to throw an error if the connection string only
contains sslnegotiation=direct

On 11/05/2024 23:45, Jelte Fennema-Nio wrote:
> On Fri, 10 May 2024 at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> New proposal:
>>
>> - Remove the "try both" mode completely, and rename "requiredirect" to
>> just "direct". So there would be just two modes: "postgres" and
>> "direct". On reflection, the automatic fallback mode doesn't seem very
>> useful. It would make sense as the default, because then you would get
>> the benefits automatically in most cases but still be compatible with
>> old servers. But if it's not the default, you have to fiddle with libpq
>> settings anyway to enable it, and then you might as well use the
>> "requiredirect" mode when you know the server supports it. There isn't
>> anything wrong with it as such, but given how much confusion there's
>> been on how this all works, I'd prefer to cut this back to the bare
>> minimum now. We can add it back in the future, and perhaps make it the
>> default at the same time. This addresses points 2. and 3. above.
>>
>> and:
>>
>> - Only allow sslnegotiation=direct with sslmode=require or higher. This
>> is what you, Jacob, wanted to do all along, and addresses point 1.
>>
>> Thoughts?
> 
> Sounds mostly good to me. But I think we'd want to automatically
> increase sslmode to require if it is unset, but sslnegotation is set
> to direct. Similar to how we bump sslmode to verify-full if
> sslrootcert is set to system, but sslmode is unset. i.e. it seems
> unnecessary/unwanted to throw an error if the connection string only
> contains sslnegotiation=direct

I find that error-prone. For example:

1. Try to connect to a server with direct negotiation: psql "host=foobar 
dbname=mydb sslnegotiation=direct"

2. It fails. Maybe it was an old server? Let's change it to 
sslnegotiation=postgres.

3. Now it succeeds. Great!

You might miss that by changing sslnegotiation to 'postgres', or by 
removing it altogether, you not only made it compatible with older 
server versions, but you also allowed falling back to a plaintext 
connection. Maybe you're fine with that, but maybe not. I'd like to 
nudge people to use sslmode=require, not rely on implicit stuff like 
this just to make connection strings a little shorter.

I'm not a fan of sslrootcert=system implying sslmode=verify-full either, 
for the same reasons. But at least "sslrootcert" is a clearly 
security-related setting, so removing it might give you a pause, whereas 
sslnegotition is about performance and compatibility.

In v18, I'd like to make sslmode=require the default. Or maybe introduce 
a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we 
want to encourage encryption, that's the right way to do it. (I'd still 
recommend everyone to use an explicit sslmode=require in their 
connection strings for many years, though, because you might be using an 
older client without realizing it.)

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

On Sun, 12 May 2024 at 23:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> You might miss that by changing sslnegotiation to 'postgres', or by
> removing it altogether, you not only made it compatible with older
> server versions, but you also allowed falling back to a plaintext
> connection. Maybe you're fine with that, but maybe not. I'd like to
> nudge people to use sslmode=require, not rely on implicit stuff like
> this just to make connection strings a little shorter.

I understand your worry, but I'm not sure that this is actually much
of a security issue in practice. sslmode=prefer and sslmode=require
are the same amount of insecure imho (i.e. extremely insecure). The
only reason sslmode=prefer would connect as non-ssl to a server that
supports ssl is if an attacker has write access to the network in the
middle (i.e. eavesdropping ability alone is not enough). Once an
attacker has this level of network access, it's trivial for this
attacker to read any data sent to Postgres by intercepting the TLS
handshake and doing TLS termination with some arbitrary cert (any cert
is trusted by sslmode=require).

So the only actual case where this is a security issue I can think of
is when an attacker has only eavesdropping ability on the network. And
somehow the Postgres server that the client tries to connect to is
configured incorrectly, so that no ssl is set up at all. Then a client
would drop to plaintext, when connecting to this server instead of
erroring, and the attacker could now read the traffic. But I don't
really see this scenario end up any differently when requiring people
to enter sslmode=require. The only action a user can take to connect
to a server that does not have ssl support is to remove
sslmode=require from the connection string. Except if they are also
the server operator, in which case they could enable ssl on the
server. But if ssl is not set up, then it was probably never set up,
and thus providing sslnegotiation=direct would be to test if ssl
works.

But, if you disagree I'm fine with erroring on plain sslnegotiation=direct

> In v18, I'd like to make sslmode=require the default. Or maybe introduce
> a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we
> want to encourage encryption, that's the right way to do it. (I'd still
> recommend everyone to use an explicit sslmode=require in their
> connection strings for many years, though, because you might be using an
> older client without realizing it.)

I'm definitely a huge proponent of making the connection defaults more
secure. But as described above sslmode=require is still extremely
insecure and I don't think we gain anything substantial by making it
the default. I think the only useful secure default would be to use
sslmode=verify-full (with probably some automatic fallback to
sslmode=prefer when connecting to hardcoded IPs or localhost). Which
probably means that sslrootcert=system should also be made the
default. Which would mean that ~/.postgresql/root.crt would not be the
default anymore, which I personally think is fine but others likely
disagree.

On 13/05/2024 12:50, Jelte Fennema-Nio wrote:
> On Sun, 12 May 2024 at 23:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> In v18, I'd like to make sslmode=require the default. Or maybe introduce
>> a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we
>> want to encourage encryption, that's the right way to do it. (I'd still
>> recommend everyone to use an explicit sslmode=require in their
>> connection strings for many years, though, because you might be using an
>> older client without realizing it.)
> 
> I'm definitely a huge proponent of making the connection defaults more
> secure. But as described above sslmode=require is still extremely
> insecure and I don't think we gain anything substantial by making it
> the default. I think the only useful secure default would be to use
> sslmode=verify-full (with probably some automatic fallback to
> sslmode=prefer when connecting to hardcoded IPs or localhost). Which
> probably means that sslrootcert=system should also be made the
> default. Which would mean that ~/.postgresql/root.crt would not be the
> default anymore, which I personally think is fine but others likely
> disagree.

"channel_binding=require sslmode=require" also protects from MITM attacks.

I think these options should be designed from the user's point of view, 
so that the user can specify the risks they're willing to accept, and 
the details of how that's accomplished are handled by libpq. For 
example, I'm OK with (tick all that apply):

[ ] passive eavesdroppers seeing all the traffic
[ ] MITM being able to hijack the session
[ ] connecting without verifying the server's identity
[ ] divulging the plaintext password to the server
[ ] ...

The requirements for whether SSL or GSS encryption is required, whether 
the server's certificate needs to signed with known CA, etc. can be 
derived from those. For example, if you need protection from 
eavesdroppers, SSL or GSS encryption must be used. If you need to verify 
the server's identity, it implies sslmode=verify-CA or 
channel_binding=true. If you don't want to divulge the password, it 
implies a suitable require_auth setting. I don't have a concrete 
proposal yet, but something like that. And the defaults for those are up 
for debate.

psql could perhaps help by listing the above properties at the beginning 
of the session, something like:

psql (16.2)
WARNING: Connection is not encrypted.
WARNING: The server's identity has not been verified
Type "help" for help.

postgres=#

Although for the "divulge plaintext password to server" property, it's 
too late to print a warning after connecting, because the damage has 
already been done.

A different line of thought is that to keep the attack surface as smal 
as possible, you should specify very explicitly what exactly you expect 
to happen in the authentication, and disallow any variance. For example, 
you expect SCRAM to be used, with a certificate signed by a particular 
CA, and if the server requests anything else, that's suspicious and the 
connection is aborted. We should make that possible too, but the above 
flexible risk-based approach seems good for the defaults.

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

On 10/05/2024 16:50, Heikki Linnakangas wrote:
> New proposal:
> 
> - Remove the "try both" mode completely, and rename "requiredirect" to
> just "direct". So there would be just two modes: "postgres" and
> "direct". On reflection, the automatic fallback mode doesn't seem very
> useful. It would make sense as the default, because then you would get
> the benefits automatically in most cases but still be compatible with
> old servers. But if it's not the default, you have to fiddle with libpq
> settings anyway to enable it, and then you might as well use the
> "requiredirect" mode when you know the server supports it. There isn't
> anything wrong with it as such, but given how much confusion there's
> been on how this all works, I'd prefer to cut this back to the bare
> minimum now. We can add it back in the future, and perhaps make it the
> default at the same time. This addresses points 2. and 3. above.
> 
> and:
> 
> - Only allow sslnegotiation=direct with sslmode=require or higher. This
> is what you, Jacob, wanted to do all along, and addresses point 1.

Here's a patch to implement that.

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

On Mon, 13 May 2024 at 15:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Here's a patch to implement that.

+       if (conn->sslnegotiation[0] == 'd' &&
+           conn->sslmode[0] != 'r' && conn->sslmode[0] != 'v')

I think these checks should use strcmp instead of checking magic first
characters. I see this same clever trick is used in the recently added
init_allowed_encryption_methods, and I think that should be changed to
use strcmp too for readability.

On Mon, 13 May 2024 at 13:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> "channel_binding=require sslmode=require" also protects from MITM attacks.

Cool, I didn't realize we had this connection option and it could be
used like this. But I think there's a few security downsides of
channel_binding=require over sslmode=verify-full: If the client relies
on channel_binding to validate server authenticity, a leaked
server-side SCRAM hash is enough for an attacker to impersonate a
server. While normally a leaked scram hash isn't really much of a
security concern (assuming long enough passwords). I also don't know
of many people rotating their scram hashes, even though many rotate
TLS certs.

> I think these options should be designed from the user's point of view,
> so that the user can specify the risks they're willing to accept, and
> the details of how that's accomplished are handled by libpq. For
> example, I'm OK with (tick all that apply):
>
> [ ] passive eavesdroppers seeing all the traffic
> [ ] MITM being able to hijack the session
> [ ] connecting without verifying the server's identity
> [ ] divulging the plaintext password to the server
> [ ] ...

I think that sounds like a great idea, looking forward to the proposal.

On 13/05/2024 16:55, Jelte Fennema-Nio wrote:
> On Mon, 13 May 2024 at 15:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Here's a patch to implement that.
> 
> +       if (conn->sslnegotiation[0] == 'd' &&
> +           conn->sslmode[0] != 'r' && conn->sslmode[0] != 'v')
> 
> I think these checks should use strcmp instead of checking magic first
> characters. I see this same clever trick is used in the recently added
> init_allowed_encryption_methods, and I think that should be changed to
> use strcmp too for readability.

Oh yeah, I hate that too. These should be refactored into enums, with a 
clear separate stage of parsing the options from strings. But we use 
that pattern all over the place, so I didn't want to start reforming it 
with this patch.

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

On Mon, May 13, 2024 at 9:37 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 10/05/2024 16:50, Heikki Linnakangas wrote:
> > New proposal:
> >
> > - Remove the "try both" mode completely, and rename "requiredirect" to
> > just "direct". So there would be just two modes: "postgres" and
> > "direct". On reflection, the automatic fallback mode doesn't seem very
> > useful. It would make sense as the default, because then you would get
> > the benefits automatically in most cases but still be compatible with
> > old servers. But if it's not the default, you have to fiddle with libpq
> > settings anyway to enable it, and then you might as well use the
> > "requiredirect" mode when you know the server supports it. There isn't
> > anything wrong with it as such, but given how much confusion there's
> > been on how this all works, I'd prefer to cut this back to the bare
> > minimum now. We can add it back in the future, and perhaps make it the
> > default at the same time. This addresses points 2. and 3. above.
> >
> > and:
> >
> > - Only allow sslnegotiation=direct with sslmode=require or higher. This
> > is what you, Jacob, wanted to do all along, and addresses point 1.
>
> Here's a patch to implement that.

I find this idea to be a massive improvement over the status quo, and
I didn't spot any major problems when I read through the patch,
either. I'm not quite sure if the patch takes the right approach in
emphasizing that weaker sslmode settings are not allowed because of
unintended fallbacks. It seems to me that we could equally well say
that those combinations are nonsensical. If we're making a direct SSL
connection, SSL is eo ipso required.

I don't have a strong opinion about whether sslnegotiation=direct
should error out (as you propose here) or silently promote sslmode to
require. I think either is defensible. Had I been implementing it, I
think I would have done as Jacob proposes, just because once we've
forced a direct SSL negotiation surely the only sensible behavior is
to be using SSL, unless you think there should be a
silently-reconnect-without-SSL behavior, which I sure don't. However,
I disagree with Jacob's assertion that sslmode=require has no security
benefits over sslmode=prefer. That seems like the kind of pessimism
that makes people hate security professionals. There have got to be
some attacks that are foreclosed by encrypting the connection, even if
you don't stop MITM attacks or other things that are more
sophisticated than running wireshark and seeing what goes by on the
wire.

I'm pleased to hear that you will propose to make sslmode=require the
default in v18. I think we'll need to do some work to figure out how
much collateral damage that will cause, and maybe it will be more than
we can stomach, but Magnus has been saying for years that the current
default is terrible. I'm not sure I was entirely convinced of that the
first time I heard him say it, but I'm smarter now than I was then.
It's really hard to believe in 2024 that anyone should ever be using a
setting that may or may not encrypt the connection. There's http and
https but there's no httpmaybes.

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

(There's, uh, a lot to respond to above and I'm trying to figure out
how best to type up all of it.)

On Mon, May 13, 2024 at 9:13 AM Robert Haas <robertmhaas@gmail.com> wrote:
> However,
> I disagree with Jacob's assertion that sslmode=require has no security
> benefits over sslmode=prefer.

For the record, I didn't say that... You mean Jelte's quote up above?:

> sslmode=prefer and sslmode=require
> are the same amount of insecure imho (i.e. extremely insecure).

I agree that requiring passive security is tangibly better than
allowing fallback to plaintext. I think Jelte's point might be better
stated as, =prefer and =require give the same amount of protection
against active attack (none).

--Jacob

On Mon, 13 May 2024 at 18:14, Robert Haas <robertmhaas@gmail.com> wrote:
> I disagree with Jacob's assertion that sslmode=require has no security
> benefits over sslmode=prefer. That seems like the kind of pessimism
> that makes people hate security professionals. There have got to be
> some attacks that are foreclosed by encrypting the connection, even if
> you don't stop MITM attacks or other things that are more
> sophisticated than running wireshark and seeing what goes by on the
> wire.

Like Jacob already said, I guess you meant me here. The main point I
was trying to make is that sslmode=require is extremely insecure too,
so if we're changing the default then I'd rather bite the bullet and
actually make the default a secure one this time. No-ones browser
trusts self-signed certs by default, but currently lots of people
trust self-signed certs when connecting to their production database
without realizing.

IMHO the only benefit that sslmode=require brings over sslmode=prefer
is detecting incorrectly configured servers i.e. servers that are
supposed to support ssl but somehow don't due to a misconfigured
GUC/pg_hba. Such "incorrectly configured server" detection avoids
sending data to such a server, which an eavesdropper on the network
could see. Which is definitely a security benefit, but it's an
extremely small one. In all other cases sslmode=prefer brings exactly
the same protection as sslmode=require, because sslmode=prefer
encrypts the connection unless postgres actively tells the client to
downgrade to plaintext (which never happens when the server is
configured correctly).

[soapbox thread, so I've changed the Subject]

On Mon, May 13, 2024 at 4:08 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> "channel_binding=require sslmode=require" also protects from MITM attacks.

This isn't true in the same way that "standard" TLS protects against
MITM. I know you know that, but for the benefit of bystanders reading
the threads, I think we should stop phrasing it like this. Most people
who want MITM protection need to be using verify-full.

Details for those bystanders: Channel binding alone will only
disconnect you after the MITM is discovered, after your startup packet
is leaked but before you send any queries to the server. A hash of
your password will also be leaked in that situation, which starts the
timer on an offline attack. And IIRC, you won't get an alert that says
"someone's in the middle"; it'll just look like you mistyped your
password.

(Stronger passwords provide stronger protection in this situation,
which is not a property that most people are used to. If I choose to
sign into Google with the password "hunter2", it doesn't somehow make
the TLS protection weaker. But if you rely on SCRAM by itself for
server authentication, it does more or less work like that.)

Use channel_binding *in addition to* sslmode=verify-full if you want
enhanced authentication of the peer, as suggested in the docs [1].
Don't rely on channel binding alone for the vast majority of use
cases, and if you know better for your particular use case, then you
already know enough to be able to ignore my advice.

[/soapbox]

Thanks,
--Jacob

[1] https://www.postgresql.org/docs/current/preventing-server-spoofing.html

On Mon, May 13, 2024 at 12:45 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> For the record, I didn't say that... You mean Jelte's quote up above?

Yeah, sorry, I got my J-named hackers confused. Apologies.

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

On Mon, May 13, 2024 at 1:42 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> Like Jacob already said, I guess you meant me here. The main point I
> was trying to make is that sslmode=require is extremely insecure too,
> so if we're changing the default then I'd rather bite the bullet and
> actually make the default a secure one this time. No-ones browser
> trusts self-signed certs by default, but currently lots of people
> trust self-signed certs when connecting to their production database
> without realizing.
>
> IMHO the only benefit that sslmode=require brings over sslmode=prefer
> is detecting incorrectly configured servers i.e. servers that are
> supposed to support ssl but somehow don't due to a misconfigured
> GUC/pg_hba. Such "incorrectly configured server" detection avoids
> sending data to such a server, which an eavesdropper on the network
> could see. Which is definitely a security benefit, but it's an
> extremely small one. In all other cases sslmode=prefer brings exactly
> the same protection as sslmode=require, because sslmode=prefer
> encrypts the connection unless postgres actively tells the client to
> downgrade to plaintext (which never happens when the server is
> configured correctly).

I think I agree with *nearly* every word of this. However:

(1) I don't want to hijack this thread about a v17 open item to talk
too much about a hypothetical v18 proposal.

(2) While in general you need more than just SSL to ensure security,
I'm not sure that there's only one way to do it, and I doubt that we
should try to pick a winner.

(3) I suspect that even going as far as sslmode=require by default is
going to be extremely painful for hackers, the project, and users.
Moving the goalposts further increases the likelihood of nothing
happening at all.

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

On Mon, May 13, 2024 at 9:13 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I find this idea to be a massive improvement over the status quo,

+1

> and
> I didn't spot any major problems when I read through the patch,
> either.

Definitely not a major problem, but I think
select_next_encryption_method() has gone stale, since it originally
provided generality and lines of fallback that no longer exist. In
other words, I think the following code is now misleading:

> if (conn->sslmode[0] == 'a')
>     SELECT_NEXT_METHOD(ENC_PLAINTEXT);
>
> SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
> SELECT_NEXT_METHOD(ENC_DIRECT_SSL);
>
> if (conn->sslmode[0] != 'a')
>     SELECT_NEXT_METHOD(ENC_PLAINTEXT);

To me, that implies that negotiated mode takes precedence over direct,
but the point of the patch is that it's not possible to have both. And
if direct SSL is in use, then sslmode can't be "allow" anyway, and we
definitely don't want ENC_PLAINTEXT.

So if someone proposes a change to select_next_encryption_method(),
you'll have to remember to stare at init_allowed_encryption_methods()
as well, and think really hard about what's going on. And vice-versa.
That worries me.

> I don't have a strong opinion about whether sslnegotiation=direct
> should error out (as you propose here) or silently promote sslmode to
> require. I think either is defensible.

I'm comforted that, since sslrootcert=system already does it, plenty
of use cases will get that for free. And if you decide in the future
that you really really want it to promote, it won't be a compatibility
break to make that change. (That gives us more time for wider v16-17
adoption, to see how the sslrootcert=system magic promotion behavior
is going in practice.)

> Had I been implementing it, I
> think I would have done as Jacob proposes, just because once we've
> forced a direct SSL negotiation surely the only sensible behavior is
> to be using SSL, unless you think there should be a
> silently-reconnect-without-SSL behavior, which I sure don't.

We still allow GSS to preempt SSL, though, so "forced" is probably
overstating things.

> It's really hard to believe in 2024 that anyone should ever be using a
> setting that may or may not encrypt the connection. There's http and
> https but there's no httpmaybes.

+1. I think (someone hop in and correct me please) that Opportunistic
Encryption for HTTP mostly fizzled, and they gave it a *lot* of
thought.

--Jacob

[this should probably belong to a different thread, but I'm not sure
what to title it]

On Mon, May 13, 2024 at 4:08 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> I think these options should be designed from the user's point of view,
> so that the user can specify the risks they're willing to accept, and
> the details of how that's accomplished are handled by libpq. For
> example, I'm OK with (tick all that apply):
>
> [ ] passive eavesdroppers seeing all the traffic
> [ ] MITM being able to hijack the session
> [ ] connecting without verifying the server's identity
> [ ] divulging the plaintext password to the server
> [ ] ...

I'm pessimistic about a quality-of-protection scheme for this use case
(*). I don't think users need more knobs in their connection strings,
and many of the goals of transport encryption are really not
independent from each other in practice. As evidence I point to the
absolute mess of GSSAPI wrapping, which lets you check separate boxes
for things like "require the server to authenticate itself" and
"require integrity" and "allow MITMs to reorder messages" and so on,
as if the whole idea of "integrity" is useful if you don't know who
you're talking to in the first place. I think I recall slapd having
something similarly arcane (but at least it doesn't make the clients
do it!). Those kinds of APIs don't evolve well, in my opinion.

I think most people probably just want browser-grade security as
quickly and cheaply as possible, and we don't make that very easy
today. I'll try to review a QoP scheme if someone works on it, don't
get me wrong, but I'd much rather spend time on a "very secure by
default" mode that gets rid of most of the options (i.e. a
postgresqls:// scheme).

(*) I've proposed quality-of-protection in the past, for Postgres
proxy authentication [1]. But I'm comfortable in my hypocrisy, because
in that case, the end user doing the configuration is a DBA with a
config file who is expected to understand the whole system, and it's a
niche use case (IMO) without an obvious "common setup". And frankly I
think my proposal is unlikely to go anywhere; the cost/benefit
probably isn't good enough.

> If you need to verify
> the server's identity, it implies sslmode=verify-CA or
> channel_binding=true.

Neither of those two options provides strong authentication of the
peer, and personally I wouldn't want them to be considered worthy of
"verify the server's identity" mode.

And -- taking a wild leap here -- if we disagree, then granularity
becomes a problem: either the QoP scheme now has to have
sub-checkboxes for "if the server knows my password, that's good
enough" and "it's fine if the server's hostname doesn't match the
cert, for some reason", or it smashes all of those different ideas
into one setting and then I have to settle for the weakest common
denominator during an active attack. Assuming I haven't missed a third
option, will that be easier/better than the status quo of
require_auth+sslmode?

> A different line of thought is that to keep the attack surface as smal
> as possible, you should specify very explicitly what exactly you expect
> to happen in the authentication, and disallow any variance. For example,
> you expect SCRAM to be used, with a certificate signed by a particular
> CA, and if the server requests anything else, that's suspicious and the
> connection is aborted. We should make that possible too

That's 'require_auth=scram-sha-256 sslmode=verify-ca', no?

--Jacob

[1] https://www.postgresql.org/message-id/0768cedb-695a-8841-5f8b-da2aa64c8f3a%40timescale.com

On Mon, Apr 29, 2024 at 11:04 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > 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
>
> <long sigh>
>
> 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

On 14/05/2024 01:29, Jacob Champion wrote:
> Definitely not a major problem, but I think
> select_next_encryption_method() has gone stale, since it originally
> provided generality and lines of fallback that no longer exist. In
> other words, I think the following code is now misleading:
> 
>> if (conn->sslmode[0] == 'a')
>>      SELECT_NEXT_METHOD(ENC_PLAINTEXT);
>>
>> SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
>> SELECT_NEXT_METHOD(ENC_DIRECT_SSL);
>>
>> if (conn->sslmode[0] != 'a')
>>      SELECT_NEXT_METHOD(ENC_PLAINTEXT);
> 
> To me, that implies that negotiated mode takes precedence over direct,
> but the point of the patch is that it's not possible to have both. And
> if direct SSL is in use, then sslmode can't be "allow" anyway, and we
> definitely don't want ENC_PLAINTEXT.
> 
> So if someone proposes a change to select_next_encryption_method(),
> you'll have to remember to stare at init_allowed_encryption_methods()
> as well, and think really hard about what's going on. And vice-versa.
> That worries me.

Ok, yeah, I can see that now. Here's a new version to address that. I 
merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, 
ENC_SSL. The places that need to distinguish between them now check 
conn-sslnegotiation. That seems more clear now that there is no fallback.

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

On Wed, May 15, 2024 at 6:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Ok, yeah, I can see that now. Here's a new version to address that. I
> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method,
> ENC_SSL. The places that need to distinguish between them now check
> conn-sslnegotiation. That seems more clear now that there is no fallback.

That change and the new comment that were added seem a lot clearer to
me, too; +1. And I like that this potentially preps for
encryption=gss/ssl/none or similar.

This assertion seems a little strange to me:

>                   if (conn->sslnegotiation[0] == 'p')
>                   {
>                       ProtocolVersion pv;
>
>                       Assert(conn->sslnegotiation[0] == 'p');

But other than that nitpick, nothing else jumps out at me at the moment.

Thanks,
--Jacob

On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Ok, yeah, I can see that now. Here's a new version to address that. I
> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method,
> ENC_SSL. The places that need to distinguish between them now check
> conn-sslnegotiation. That seems more clear now that there is no fallback.

Unless there is a compelling reason to do otherwise, we should
expedite getting this committed so that it is included in beta1.
Release freeze begins Saturday.

Thanks,

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

> On 16 May 2024, at 15:54, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Ok, yeah, I can see that now. Here's a new version to address that. I
>> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method,
>> ENC_SSL. The places that need to distinguish between them now check
>> conn-sslnegotiation. That seems more clear now that there is no fallback.
>
> Unless there is a compelling reason to do otherwise, we should
> expedite getting this committed so that it is included in beta1.
> Release freeze begins Saturday.

+1. Having reread the thread and patch I think we should go for this one.

./daniel

On 16/05/2024 17:08, Daniel Gustafsson wrote:
>> On 16 May 2024, at 15:54, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> Ok, yeah, I can see that now. Here's a new version to address that. I
>>> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method,
>>> ENC_SSL. The places that need to distinguish between them now check
>>> conn-sslnegotiation. That seems more clear now that there is no fallback.
>>
>> Unless there is a compelling reason to do otherwise, we should
>> expedite getting this committed so that it is included in beta1.
>> Release freeze begins Saturday.
> 
> +1. Having reread the thread and patch I think we should go for this one.

Yep, committed. Thanks everyone!

On 15/05/2024 21:24, Jacob Champion wrote:
> This assertion seems a little strange to me:
> 
>>                   if (conn->sslnegotiation[0] == 'p')
>>                   {
>>                       ProtocolVersion pv;
>>
>>                       Assert(conn->sslnegotiation[0] == 'p');
> 
> But other than that nitpick, nothing else jumps out at me at the moment.

Fixed that. It was a leftover, I had the if-else conditions the other 
way round at one point during development.

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

On Thu, May 16, 2024 at 10:23 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Yep, committed. Thanks everyone!

Thanks!

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