Re: Direct SSL connection and ALPN loose ends - Mailing list pgsql-hackers

From Heikki Linnakangas
Subject Re: Direct SSL connection and ALPN loose ends
Date
Msg-id b643f810-aa58-4814-818b-98172ddd2f16@iki.fi
Whole thread Raw
In response to Re: Direct SSL connection and ALPN loose ends  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Direct SSL connection and ALPN loose ends
List pgsql-hackers
On 16/07/2024 09:54, Michael Paquier wrote:
> On Mon, Jun 24, 2024 at 11:30:53PM +0300, Heikki Linnakangas wrote:
>> 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.

Yeah. In hindsight I'm still not very happy with the code structure with 
"allowed_enc_methods" and "current_enc_methods" and all that. The 
fallback logic is still complicated. It's better than in v16, IMHO, but 
still not great. This patch seems like the best fix for v17, but I 
wouldn't mind another round of refactoring for v18, if anyone's got some 
good ideas on how to structure it better. All these new tests are a 
great asset when refactoring this again.

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

Yeah, I'm also not too excited about the additional code in the backend, 
but I'm also not excited about writing another test C module just for 
this. I'm inclined to commit this as it is, but we can certainly revisit 
this later, since it's just test code.

Here's a new rebased version with some minor cleanup. Notably, I added 
docs for the new IS_INJECTION_POINT_ATTACHED() macro.

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

Attachment

pgsql-hackers by date:

Previous
From: Jeff Davis
Date:
Subject: Re: [18] Policy on IMMUTABLE functions and Unicode updates
Next
From: Fujii Masao
Date:
Subject: Re: Add privileges test for pg_stat_statements to improve coverage