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
