On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <m...@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
