On Thu, 2021-03-25 at 14:41 +0900, Michael Paquier wrote:
> I got to look at the DN patch yesterday, so now's the turn of this
> one. Nice work.
Thanks!
> + port->authn_id = MemoryContextStrdup(TopMemoryContext, id);
> It may not be obvious that all the field is copied to TopMemoryContext
> because the Port requires that.
I've expanded the comment. (v11 attached, with incremental changes over
v10 in since-v10.diff.txt.)
> +$node->stop('fast');
> +my $log_contents = slurp_file($log);
> Like 11e1577, let's just truncate the log files in all those tests.
Hmm... having the full log file contents for the SSL tests has been
incredibly helpful for me with the NSS work. I'd hate to lose them; it
can be very hard to recreate the test conditions exactly.
> + if (auth_method < 0 || USER_AUTH_LAST < auth_method)
> + {
> + Assert((0 <= auth_method) && (auth_method <= USER_AUTH_LAST));
> What's the point of having the check and the assertion? NULL does not
> really seem like a good default here as this should never really
> happen. Wouldn't a FATAL be actually safer?
I think FATAL makes more sense. Changed, thanks.
> It looks wrong to me to include in the SSL tests some checks related
> to SCRAM authentication. This should remain in 001_password.pl, as of
> src/test/authentication/.
Agreed. Moved the bad-password SCRAM tests over, and removed the
duplicates. The last SCRAM test in that file, which tests the
interaction between client certificates and SCRAM auth, remains.
> port->gss->princ = MemoryContextStrdup(TopMemoryContext, port->gbuf.value);
> + set_authn_id(port, gbuf.value);
> I don't think that this position is right for GSSAPI. Shouldn't this
> be placed at the end of pg_GSS_checkauth() and only if the status is
> OK?
No, and the tests will catch you if you try. Authentication happens
before authorization (user mapping), and can succeed independently even
if authz doesn't. See below.
> For SSPI, I think that this should be moved down once we are sure that
> there is no error and that pg_SSPI_recvauth() reports STATUS_OK to the
> caller. There is a similar issue with CheckCertAuth(), and
> set_authn_id() is documented so as it should be called only when we
> are sure that authentication succeeded.
Authentication *has* succeeded already; that's what the SSPI machinery
has done above. Likewise for CheckCertAuth, which relies on the TLS
subsystem to validate the client signature before setting the peer_cn.
The user mapping is an authorization concern: it answers the question,
"is an authenticated user allowed to use a particular Postgres user
name?"
Postgres currently conflates authn and authz in many places, and in my
experience, that'll make it difficult to maintain new authorization
features like the ones in the wishlist upthread. This patch is only one
step towards a clearer distinction.
> I am actually in
> favor of keeping this information in the Port with those pluggability
> reasons.
That was my intent, yeah. Getting this into the stats framework was
more than I could bite off for this first patchset, but having it
stored in a central location will hopefully help people do more with
it.
--Jacob
