Re: [PoC] Let libpq reject unexpected authentication requests - Mailing list pgsql-hackers

From Peter Eisentraut
Subject Re: [PoC] Let libpq reject unexpected authentication requests
Date
Msg-id 8e2e5fb6-8225-de8a-b0f9-178c27402367@enterprisedb.com
Whole thread Raw
In response to Re: [PoC] Let libpq reject unexpected authentication requests  (Jacob Champion <jchampion@timescale.com>)
Responses Re: [PoC] Let libpq reject unexpected authentication requests
List pgsql-hackers
On 23.09.22 02:02, Jacob Champion wrote:
> On Thu, Sep 22, 2022 at 4:52 AM Peter Eisentraut
> <peter.eisentraut@enterprisedb.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.




pgsql-hackers by date:

Previous
From: "Drouvot, Bertrand"
Date:
Subject: Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf
Next
From: Alvaro Herrera
Date:
Subject: Re: shadow variables - pg15 edition