On Mon, Jun 24, 2024 at 11:30:53PM +0300, Heikki Linnakangas wrote:
> Given that, I think it is a good thing to fail the connection completely on
> receiving a V2 error.
>
> Attached is a patch to fix the other issue, with falling back from SSL to
> plaintext. And some tests and comment fixes I spotted while at it.
>
> 0001: A small comment fix
Already committed as of cc68ca6d420e.
> 0002: This is the main patch that fixes the SSL fallback issue
+ conn->failed_enc_methods |= conn->allowed_enc_methods &
(~conn->current_enc_method);
Sounds reasonable to me.
It's a bit annoying to have to guess that current_enc_method is
tracking only one method at a time (aka these three fields are not
documented in libpq-int.h), while allowed_enc_methods and
failed_enc_methods is a bitwise combination of the methods that are
still allowed or that have already failed.
> 0003: This adds fault injection tests to exercise these early error
> codepaths. It is not ready to be merged, as it contains a hack to skip
> locking. See thread at
> https://www.postgresql.org/message-id/e1ffb822-054e-4006-ac06-50532767f75b%40iki.fi.
Locking when running an injection point has been replaced by some
atomics in 86db52a5062a.
+ if (IsInjectionPointAttached("backend-initialize-v2-error"))
+ {
+ FrontendProtocol = PG_PROTOCOL(2,0);
+ elog(FATAL, "protocol version 2 error triggered");
+ }
This is an attempt to do stack manipulation with an injection point
set. FrontendProtocol is a global variable, so you could have a new
callback setting up this global variable directly, then FATAL (I
really don't mind is modules/injection_points finishes with a library
of callbacks).
Not sure to like much this new IsInjectionPointAttached() that does a
search in the existing injection point pool, though. This leads to
more code footprint in the core backend, and I'm trying to minimize
that. Not everybody agrees with this view, I'd guess, which is also
fine.
> 0004: More tests, for what happens if the server sends an error after
> responding "yes" to the SSLRequest or GSSRequest, but before performing the
> SSL/GSS handshake.
No objections to these two additions.
> Attached is also a little stand-alone perl program that listens on a socket,
> and when you connect to it, it immediately sends a V2 or V3 error, depending
> on the argument. That's useful for testing. It could be used as an
> alternative strategy to the injection points I used in the 0003-0004
> patches, but for now I just used it for manual testing.
Nice toy.
--
Michael