Message-ID: <c136387c-731e-729b-a49f-c1f17768aaf0@timescale.com>
Date: Fri, 10 Mar 2023 14:32:17 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PoC] Let libpq reject unexpected authentication requests
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 Aleksander Alekseev <aleksander@timescale.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 "David G. Johnston" <david.g.johnston@gmail.com>
References: 
 <CAAWbhmgjs6kSm1n25a-+ZKOb5QnEzxerMfcmML_rXz2JTKvrpw@mail.gmail.com>
 <Y3SMDoHfS8sy2wwW@paquier.xyz>
 <CAAWbhmixDLmLMNUcMgCGYbG_z85umzUAD_6v=9bZHvxsjnHLpA@mail.gmail.com>
 <9de74f8d-fbf4-63d9-7bcb-b9c27edf8b73@enterprisedb.com>
 <CAJ7c6TP0em9qcoh_iN_8xdgCwg-fsqSxngXydOBGWyKsmBMJ7A@mail.gmail.com>
 <Y9ms6HPSxEiXpw+4@paquier.xyz>
 <CAAWbhmihPDBe+sHPxY0N+9YUj8MoRErdeSYjMeiic0evLWYtmQ@mail.gmail.com>
 <add8d351-1a03-378b-d8fa-9e2aa04bd0be@timescale.com>
 <ZAKucp7m0purD2cm@paquier.xyz>
 <CAAWbhmjRHYG_HN-yuej5Q7tUYuViodXm7BusjUkUyv2fW-+KPQ@mail.gmail.com>
 <ZAl+Lj+4Q8+aHimx@paquier.xyz>
From: Jacob Champion <jchampion@timescale.com>
In-Reply-To: <ZAl+Lj+4Q8+aHimx@paquier.xyz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c136387c-731e-729b-a49f-c1f17768aaf0%40timescale.com>
Precedence: bulk

On 3/8/23 22:35, Michael Paquier wrote:
> I have been reviewing 0001, finishing with the attached, and that's
> nice work.  My notes are below.

Thanks!

> 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.

Yeah, that seems reasonable.

> +       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.

Fair enough.

> 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.

Works for me. I wonder if

   sizeof(((PGconn*) 0)->allowed_auth_methods)

would make pgindent any happier? That'd let you keep the assertion local
to auth_method_allowed, but it looks scarier. :)

> 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.

Yeah. I think a server shouldn't be allowed to abandon a SCRAM exchange
partway through, but that's completely independent of this patchset.

> - SSPI is the default connection setup for the TAP tests on Windows.

Oh, I don't think I ever noticed that.

> We could stick a small test somewhere, perhaps, certainly not in
> src/test/authentication/.

Where were you thinking? (Would it be so bad to have a tiny
t/005_sspi.pl that's just skipped on *nix?)

> - 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.

Did you have any thoughts about the 0003 generalization attempt?

>     -+  if (conn->require_auth)
>     ++  if (conn->require_auth && conn->require_auth[0])

Thank you for that catch. I guess we should test somewhere that
`require_auth=` behaves normally?

>      +                  reason = libpq_gettext("server did not complete authentication"),
>     -+                  result = false;
>     ++                      result = false;
>      +              }

This reindentation looks odd.

nit: some of the new TAP test names have been rewritten with commas,
others with colons.

Thanks,
--Jacob