Feedback-ID: i131946ab:Fastmail
Message-ID: <8e2e5fb6-8225-de8a-b0f9-178c27402367@enterprisedb.com>
Date: Wed, 5 Oct 2022 15:33:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.0
Subject: Re: [PoC] Let libpq reject unexpected authentication requests
Content-Language: en-US
To: Jacob Champion <jchampion@timescale.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 Michael Paquier <michael@paquier.xyz>,
 "David G. Johnston" <david.g.johnston@gmail.com>
References: <9e5a8ccddb8355ea9fa4b75a1e3a9edc88a70cd3.camel@vmware.com>
 <CAAWbhmitLHUBkUUJmsG2CuVfH=OXTzj6pU1C8XYerYBM2TBfbQ@mail.gmail.com>
 <CAAWbhmi4V9zEAvfUSCDFx1pOr3ZWrV9fuxkv_2maRqvyc-m9PQ@mail.gmail.com>
 <CAAWbhmhjuHUfCs4NbB6p_MeuAgqq+et17SN=QR52HRH8zcMfFQ@mail.gmail.com>
 <77358b4c-6497-e5d3-c13c-57522654af25@enterprisedb.com>
 <CAAWbhmiKUFtkzF5_UFrJNssyTh2QEB7qF70K8zC9KWG0jMxmGw@mail.gmail.com>
 <adfab57e-bcc6-836b-0406-83bfe7f82061@enterprisedb.com>
 <CAAWbhmjMRCFjvjWu-=rmYCW4eRo8b1OjwyREm5QS-YFGGvex-w@mail.gmail.com>
 <ebf31cf7-b0af-7407-ada2-dda6d2767ba0@enterprisedb.com>
 <CAAWbhmg5ddYa=5niqvb4vY7-tpWc2Dj7e=-cNT+WZ64s8V_hVQ@mail.gmail.com>
 <CAAWbhmgOuheFzozhej0HHXJ6wi3Vu73kLrtvD6p9=mJOGOMEfw@mail.gmail.com>
 <914a0125-3dfb-920f-51b6-8d44a8edbdd2@enterprisedb.com>
 <CAAWbhmiRxVNsfaq8DLgxoAh2TBu1SthP9jycutXBz32Z2k80pw@mail.gmail.com>
 <949f1c20-771c-aef7-058f-2c10885db955@enterprisedb.com>
 <CAAWbhmimxNiGMDHju7FsEtem4Rmk1enQ_-ypenjkTQnNPLouMA@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CAAWbhmimxNiGMDHju7FsEtem4Rmk1enQ_-ypenjkTQnNPLouMA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/8e2e5fb6-8225-de8a-b0f9-178c27402367%40enterprisedb.com>
Precedence: bulk

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.