On 01/03/2024 23:49, Jacob Champion wrote:
On Wed, Feb 28, 2024 at 4:10 AM Heikki Linnakangas <hlinn...@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)
