Re: Experiments with Postgres and SSL

On 01/03/2024 23:49, Jacob Champion wrote:
> On Wed, Feb 28, 2024 at 4:10 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> I think we'd want to *avoid* changing the major protocol version in a
>> way that would introduce a new roundtrip, though.
> 
> I'm starting to get up to speed with this patchset. So far I'm mostly
> testing how it works; I have yet to take an in-depth look at the
> implementation.

Thank you!

> I'll squint more closely at the MITM-protection changes in 0008 later.
> First impressions, though: it looks like that code has gotten much
> less straightforward, which I think is dangerous given the attack it's
> preventing. (Off-topic: I'm skeptical of future 0-RTT support. Our
> protocol doesn't seem particularly replay-safe to me.)

Let's drop that patch. AFAICS it's not needed by the rest of the patches.

> If we're interested in ALPN negotiation in the future, we may also
> want to look at GREASE [1] to keep those options open in the presence
> of third-party implementations. Unfortunately OpenSSL doesn't do this
> automatically yet.

Can you elaborate? Do we need to do something extra in the server to be 
compatible with GREASE?

> If we don't have a reason not to, it'd be good to follow the strictest
> recommendations from [2] to avoid cross-protocol attacks. (For anyone
> currently running web servers and Postgres on the same host, they
> really don't want browsers "talking" to their Postgres servers.) That
> would mean checking the negotiated ALPN on both the server and client
> side, and failing if it's not what we expect.

Hmm, I thought that's what the patches does. But looking closer, libpq 
is not checking that ALPN was used. We should add that. Am I right?

> I'm not excited about the proliferation of connection options. I don't
> have a lot of ideas on how to fix it, though, other than to note that
> the current sslnegotiation option names are very unintuitive to me:
> - "postgres": only legacy handshakes
> - "direct": might be direct... or maybe legacy
> - "requiredirect": only direct handshakes... unless other options are
> enabled and then we fall back again to legacy? How many people willing
> to break TLS compatibility with old servers via "requiredirect" are
> going to be okay with lazy fallback to GSS or otherwise?

Yeah, this is my biggest complaint about all this. Not so much the names 
of the options, but the number of combinations of different options, and 
how we're going to test them all. I don't have any great solutions, 
except adding a lot of tests to cover them, like Matthias did.

> Heikki mentioned possibly hard-coding a TLS alert if direct SSL is
> attempted without server TLS support. I think that's a cool idea, but
> without an official "TLS not supported" alert code (which, honestly,
> would be strange to standardize) I'm kinda -0.5 on it. If the client
> tells me about a handshake_failure or similar, I'm going to start
> investigating protocol versions and ciphersuites; I'm not going to
> think to myself that maybe the server lacks TLS support altogether.

Agreed.

> (Plus, we need to have a good error message when connecting to older
> servers anyway.I think we should be able to key off of the EOF coming
> back from OpenSSL; it'd be a good excuse to give that part of the code
> some love.)

Hmm, if OpenSSL sends ClientHello and the server responds with a 
Postgres error packet, OpenSSL will presumably consume the error packet 
or at least part of it. But with our custom BIO, we can peek at the 
server response before handing it to OpenSSL.

If it helps, we could backport a nicer error message to old server 
versions, similar to what we did with SCRAM in commit 96d0f988b1.

> For the record, I'm adding some one-off tests for this feature to a
> local copy of my OAuth pytest suite, which is designed to do the kinds
> of testing you're running into trouble with. It's not in any way
> viable for a PG17 commit, but if you're interested I can make the
> patches available.

Yes please, it would be nice to see what tests you've performed, and 
have it archived.

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