Re: [PoC] Let libpq reject unexpected authentication requests

On Fri, Mar 10, 2023 at 02:32:17PM -0800, Jacob Champion wrote:
> On 3/8/23 22:35, Michael Paquier wrote:
> 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. :)

I can check that, now it's not bad to keep the assertion as it is,
either.

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

Agreed.

>> 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?)

Hmm, OK. It may be worth having a 005_sspi.pl in
src/test/authentication/ specifically for Windows. This patch gives
at least one reason to do so. Looking at pg_regress.c, we have that:
if (config_auth_datadir)
{
#ifdef ENABLE_SSPI
if (!use_unix_sockets)
config_sspi_auth(config_auth_datadir, user);
#endif
exit(0);
}

So applying a check on $use_unix_sockets should be OK, I hope.

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

Not yet, unfortunately.

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

Yeah, that seems like an idea. That would be cheap enough.

>> + reason = libpq_gettext("server did not complete authentication"),
>> -+ result = false;
>> ++ result = false;
>> + }
>
> This reindentation looks odd.

That's because the previous line has a comma. So the reindent is
right, not the code.

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

Indeed, I thought to have caught all of them, but you wrote a lot of
tests :)

Could you send a new patch with all these adjustments? That would
help a lot.
--
Michael