From: Jacob Champion <jchampion@timescale.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Aleksander Alekseev <aleksander@timescale.com>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Subject: Re: [PoC] Let libpq reject unexpected authentication requests
Date: Fri, 10 Mar 2023 14:32:17 -0800
Message-ID: <c136387c-731e-729b-a49f-c1f17768aaf0@timescale.com> (raw)
In-Reply-To: <ZAl+Lj+4Q8+aHimx@paquier.xyz>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jchampion@timescale.com, michael@paquier.xyz, aleksander@timescale.com, peter.eisentraut@enterprisedb.com, david.g.johnston@gmail.com
Subject: Re: [PoC] Let libpq reject unexpected authentication requests
In-Reply-To: <c136387c-731e-729b-a49f-c1f17768aaf0@timescale.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox