Re: Experiments with Postgres and SSL - Mailing list pgsql-hackers

On Wed, 10 Jan 2024 at 09:31, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> Some more comments on this:
>
> 1. It feels weird that the combination of "gssencmode=require
> sslnegotiation=direct" combination is forbidden. Sure, the ssl
> negotiation will never happen with gssencmode=require, so the
> sslnegotiation option has no effect. But by that token, should we also
> forbid the combination "sslmode=disable sslnegotiation=direct"? I think
> not. The sslnegotiation option should mean "if we are going to try SSL,
> should we try it in direct or negotiated mode?"

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.

> 2. Should we allow direct SSL only at the very beginning of a TCP
> connection, or should we also allow it after we have requested GSS and
> the server said no? Like this:
>
> Client: GSSENCRequest
> Server: 'N' (gss not supported)
> Client: TLS client Hello
>
> On one hand, why not? It saves you a round-trip in this case too. If we
> don't allow it, the client will have to send SSLRequest and wait for
> response, or reconnect to try direct SSL. On the other hand, flexibility
> is not necessarily a good thing in security-critical code like this.

I think this should be "no".
Once we start accepting PostgreSQL protocol packets (such as the
GSSENCRequest packet) I don't think we should start treating data
stream corruption as attempted SSL connections.

> The patch set is confused on whether that's allowed or not. The server
> rejects it. But if you use "gssencmode=prefer
> sslnegotiation=requiredrect", libpq will attempt to do it, and fail.

That should then be detected as an incorrect combination of flags in
psql: you can't have direct-to-ssl and put something in front of it.

> 3. With "sslmode=verify-full sslnegotiation=direct", if the direct SSL
> connection fails because of a problem with the certificate, libpq will
> try again in negotiated SSL mode. That seems pointless. If the server
> responded to the direct TLS Client Hello message with a valid
> ServerHello, that indicates that the server supports direct SSL. If
> anything goes wrong after that, retrying in negotiated mode is not going
> to help.

This makes sense.

> 4. The number of combinations of sslmode, gssencmode and sslnegotiation
> settings is scary. And we have very few tests for them.

Yeah, it's not great. We could easily automate this better though. I
mean, can't we run the tests using a "cube" configuration, i.e. test
every combination of parameters? We would use a mapping function of
(psql connection parameter values -> expectations), which would be
along the lines of the attached pl testfile. I feel it's a bit more
approachable than the lists of manual option configurations, and makes
it a bit easier to program the logic of which connection security
option we should have used to connect.
The attached file would be a drop-in replacement; it's tested to work
with SSL only - without GSS - because I've been having issues getting
GSS working on my machine.

> I'm going to put this down for now. The attached patch set is even more
> raw than v6, but I'm including it here to "save the work".

v6 doesn't apply cleanly anymore after 774bcffe, but here are some notes:

Several patches are still very much WIP. Reviewing them on a
patch-by-patch basis is therefore nigh impossible; the specific
reviews below are thus on changes that could be traced back to a
specific patch. A round of cleanup would be appreciated.

> 0003: Direct SSL connections postmaster support
> [...]
> -extern bool pq_buffer_has_data(void);
> +extern size_t pq_buffer_has_data(void);

This should probably be renamed to pg_buffer_remaining_data or such,
if we change the signature like this.

> +    /* Read from the "unread" buffered data first. c.f. libpq-be.h */
> +    if (port->raw_buf_remaining > 0)
> +    {
> +        /* consume up to len bytes from the raw_buf */
> +        if (len > port->raw_buf_remaining)
> +            len = port->raw_buf_remaining;

Shouldn't we also try to read from the socket, instead of only
consuming bytes from the raw buffer if it contains bytes?

> 0008: Allow pipelining data after ssl request
> +            /*
> +             * At this point we should have no data already buffered.  If we do,
> +             * it was received before we performed the SSL handshake, so it wasn't
> +             * encrypted and indeed may have been injected by a man-in-the-middle.
> +             * We report this case to the client.
> +             */
> +            if (port->raw_buf_remaining > 0)
> +                ereport(FATAL,
> +                        (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                         errmsg("received unencrypted data after SSL request"),
> +                         errdetail("This could be either a client-software bug or evidence of an attempted
man-in-the-middleattack.")));
 

We currently don't support 0-RTT SSL connections because (among other
reasons) we haven't yet imported many features from TLS1.3, but it
seems reasonable that clients may want to use 0RTT (or, session
resumption in 0 round trips), which would allow encrypted data after
the SSL startup packet.
It seems wise to add something to this note to these comments in
ProcessStartupPacket.

> ALPN

Does the TLS ALPN spec allow protocol versions in the protocol tag? It
would be very useful to detect clients with new capabilities at the
first connection, rather than having to wait for one round trip, and
would allow one avenue for changing the protocol version.

Apart from this, I didn't really find any serious problems in the sum
of these patches. The intermediate states were not great though, with
various broken states in between.

Kind regards,

Matthias van de Meent