Re: [PoC] Let libpq reject unexpected authentication requests - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: [PoC] Let libpq reject unexpected authentication requests |
Date | |
Msg-id | ZAu4rdSKQTHvBqS7@paquier.xyz Whole thread Raw |
In response to | Re: [PoC] Let libpq reject unexpected authentication requests (Jacob Champion <jchampion@timescale.com>) |
Responses |
Re: [PoC] Let libpq reject unexpected authentication requests
(Jacob Champion <jchampion@timescale.com>)
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: