On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <me@jeltef.nl> wrote:
>
> Patch looks good to me. Definitely an improvement over the status quo.
Thanks for the review!
> Looking at the TLS error handling though I see these two lines:
>
> && conn->allow_ssl_try /* redundant? */
> && !conn->wait_ssl_try) /* redundant? */
>
> Are they actually redundant like the comment suggests? If so, we
> should probably remove them (in another patch). If not (or if we don't
> know), should we have these same checks for GSS?
It's a fair point. GSS doesn't have an "allow" encryption mode, so
they can't be the exact same checks. And we're already not checking
the probably-redundant information, so I'd vote against speculatively
adding it back. (try_gss is already based on gssencmode, which we're
using here. So I think rechecking try_gss would only help if we wanted
to clear it manually while in the middle of processing a GSS exchange.
From a quick inspection, I don't think that's happening today -- and
I'm not really sure that it could be useful in the future, because I'd
think prefer-mode is supposed to guarantee a retry on failure.)
I suspect this is a much deeper rabbit hole; I think it's work that
needs to be done, but I can't sign myself up for it at the moment. The
complexity of this function is off the charts (for instance, why do we
recheck conn->try_gss above, if the only apparent way to get to
CONNECTION_GSS_STARTUP is by having try_gss = true to begin with? is
there a goto/retry path I'm missing?). I think it either needs heavy
assistance from a committer who already has intimate knowledge of this
state machine and all of its possible branches, or from a static
analysis tool that can help with a step-by-step simplification.
Thanks,
--Jacob