Re: [PoC] Let libpq reject unexpected authentication requests

On 23.09.22 02:02, Jacob Champion wrote:
> On Thu, Sep 22, 2022 at 4:52 AM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>> On 22.09.22 01:37, Jacob Champion wrote:
>>> I think this is potentially
>>> dangerous, but it mirrors the current behavior of libpq and I'm not
>>> sure that we should change it as part of this patch.
>>
>> It might be worth reviewing that behavior for other reasons, but I think
>> semantics of your patch are correct.
>
> Sounds good. v9 removes the TODO and adds a better explanation.

I'm generally okay with these patches now.

> I'm not able to test SSPI easily at the moment; if anyone is able to
> try it out, that'd be really helpful. There's also the question of
> SASL forwards compatibility -- if someone adds a new SASL mechanism,
> the code will treat it like scram-sha-256 until it's changed, and
> there will be no test to catch it. Should we leave that to the future
> mechanism implementer to fix, or add a mechanism check now so the
> client is safe even if they forget?

I think it would be good to put some provisions in place here, even if
they are elementary. Otherwise, there will be a significant burden on
the person who implements the next SASL method (i.e., you ;-) ) to
figure that out then.

I think you could just stick a string list of allowed SASL methods into
PGconn.

By the way, I'm not sure all the bit fiddling is really worth it. An
array of integers (or unsigned char or whatever) would work just as
well. Especially if you are going to have a string list for SASL
anyway. You're not really saving any bits or bytes either way in the
normal case.

Minor comments:

Pasting together error messages like with auth_description() isn't going
to work. You either need to expand the whole message in
check_expected_areq(), or perhaps rephrase the message like

libpq_gettext("auth method \"%s\" required, but server requested \"%s\"\n"),
conn->require_auth,
auth_description(areq)

and make auth_description() just return a single word not subject to
translation.

spurious whitespace change in fe-secure-openssl.c

whitespace error in patch:

.git/rebase-apply/patch:109: tab in indent.
via TLS, nor GSS authentication via its
encrypted transport.)

In the 0002 patch, the configure test needs to be added to meson.build.