Re: [PoC] Let libpq reject unexpected authentication requests

On Mon, Mar 06, 2023 at 04:02:25PM -0800, Jacob Champion wrote:
> On Fri, Mar 3, 2023 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote:
>> I was refreshing my mind with 0001 yesterday, and except for the two
>> parts where we need to worry about AUTH_REQ_OK being sent too early
>> and the business with gssenc, this is a rather straight-forward.  It
>> also looks like the the participants of the thread are OK with the
>> design you are proposing (list of keywords, potentially negative
>> patterns).  I think that I can get this part merged for this CF, at
>> least, not sure about the rest :p
>
> Thanks! Is there anything that would make the sslcertmode patch more
> palatable? Or any particular areas of concern?

I have been reviewing 0001, finishing with the attached, and that's
nice work.  My notes are below.

pqDropServerData() is in charge of cleaning up the transient data of a
connection between different attempts.  Shouldn't client_finished_auth
be reset to false there?  No parameters related to the connection
parameters should be reset in this code path, but this state is
different.  It does not seem possible that we could reach
pqDropServerData() after client_finished_auth has been set to true,
but that feels safer.  I was tempted first to do that as well in
makeEmptyPGconn(), but we do a memset(0) there, so there is no point
in doing that anyway ;)

require_auth needs a cleanup in freePGconn().

+       case AUTH_REQ_SCM_CREDS:
+           return libpq_gettext("server requested UNIX socket credentials");
I am not really cool with the fact that this would fail and that we
offer no options to control that.  Now, this involves servers up to
9.1, which is also a very good to rip of this code entirely.  For now,
I think that we'd better allow this option, and discuss the removal of
that in a separate thread.

pgindent has been complaining on the StaticAssertDecl() in fe-auth.c:
src/interfaces/libpq/fe-auth.c: Error@847: Unbalanced parens
Warning@847: Extra )
Warning@847: Extra )
Warning@848: Extra )

From what I can see, this comes from the use of {0} within the
expression itself.  I don't really want to dig into why pg_bsd_indent
thinks this is a bad idea, so let's just move the StaticAssertDecl() a
bit, like in the attached.  The result is the same.

As of the "sensitive" cases of the patch:
- I don't really think that we have to care much of the cases like
"none,scram" meaning that a SASL exchange hastily downgraded to
AUTH_REQ_OK by the server would be a success, as "none" means that the
client is basically OK with trust-level.  This said, "none" could be a
dangerous option in some cases, while useful in others.
- SSPI is the default connection setup for the TAP tests on Windows.
We could stick a small test somewhere, perhaps, certainly not in
src/test/authentication/.
- SASL/SCRAM is indeed a problem on its own.  My guess is that we
should let channel_binding do the job for SASL, or introduce a new
option to decide which sasl mechanisms are authorized.  At the end,
using "scram-sha-256" as the keyword is fine by me as we use that even
for HBA files, so that's quite known now, I hope.
--
Michael