Re: Direct SSL connection and ALPN loose ends

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)