Re: [PoC] Let libpq reject unexpected authentication requests - Mailing list pgsql-hackers

On Tue, Jun 07, 2022 at 02:22:28PM -0700, Jacob Champion wrote:
> v2 rebases over latest, removes the alternate spellings of "password",
> and implements OR operations with a comma-separated list. For example:
>
> - require_auth=cert means that the server must ask for, and the client
> must provide, a client certificate.

Hmm..  Nya.

> - require_auth=password,md5 means that the server must ask for a
> plaintext password or an MD5 hash.
> - require_auth=scram-sha-256,gss means that one of SCRAM, Kerberos
> authentication, or GSS transport encryption must be successfully
> negotiated.

Makes sense.

> - require_auth=scram-sha-256,cert means that either a SCRAM handshake
> must be completed, or the server must request a client certificate. It
> has a potential pitfall in that it allows a partial SCRAM handshake,
> as long as a certificate is requested and sent.

Er, this one could be a problem protocol-wise for SASL, because that
would mean that the authentication gets completed but that the
exchange has begun and is not finished?

> AND and NOT, discussed upthread, are not yet implemented. I tied
> myself up in knots trying to make AND generic, so I think I"m going to
> tackle NOT first, instead. The problem with AND is that it only makes
> sense when one (and only one) of the options is a form of transport
> authentication. (E.g. password+md5 never makes sense.) And although
> cert+<something> and gss+<something> could be useful, the latter case
> is already handled by gssencmode=require, and the gssencmode option is
> more powerful since you can disable it (or set it to don't-care).

I am on the edge regarding NOT as well, and I am unsure of the actual
benefits we could get from it as long as one can provide a white list
of auth methods.  If we don't see an immediate benefit in that, I'd
rather choose a minimal, still useful, design.  As a whole, I would
vote with adding only support for OR and a comma-separated list like
your proposal.

> I'm not generally happy with how the "cert" option is working. With
> the other methods, if you don't include a method in the list, then the
> connection fails if the server tries to negotiate it. But if you don't
> include the cert method in the list, we don't forbid the server from
> asking for a cert, because the server always asks for a client
> certificate via TLS whether it needs one or not. Behaving in the
> intuitive way here would effectively break all use of TLS.

Agreed.  Looking at what you are doing with allowed_auth_method_cert,
this makes the code harder to follow, which is risky for any
security-related feature, and that's different than the other methods
where we have the AUTH_REQ_* codes.  This leads to extra complications
in the shape of ssl_cert_requested and ssl_cert_sent, as well.  This
strongly looks like what we do for channel binding as it has
requirements separated from the actual auth methods but has dependency
with them, so a different parameter, as suggested, would make sense.
If we are not sure about this part, we could discard it in the first
instance of the patch.

> So I think Tom's recommendation that the cert method be handled by an
> orthogonal option was a good one, and if that works then maybe we
> don't need an AND syntax at all. Presumably I can just add an option
> that parallels gssencmode, and then the current don't-care semantics
> can be explicitly controlled. Skipping AND also means that I don't
> have to create a syntax that can handle AND and NOT at the same time,
> which I was dreading.

I am not convinced that we have any need for the AND grammar within
one parameter, as that's basically the same thing as defining multiple
connection parameters, isn't it?  This is somewhat a bit similar to
the interactions of channel binding with this new parameter and what
you have implemented.  For example, channel_binding=require
require_auth=md5 would imply both and should fail, even if it makes
little sense because MD5 has no idea of channel binding.  One
interesting case comes down to stuff like channel_binding=require
require_auth="md5,scram-sha-256", where I think that we should still
fail even if the server asks for MD5 and enforce an equivalent of an
AND grammar through the parameters.  This reasoning limits the
dependencies between each parameter and treats the areas where these
are checked independently, which is what check_expected_areq() does
for channel binding.  So that's more robust at the end.

Speaking of which, I would add tests to check some combinations of
channel_binding and require_auth.

+           appendPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("auth method \"%s\" required, but %s\n"),
+                             conn->require_auth, reason);
This one is going to make translation impossible.  One way to tackle
this issue is to use "auth method \"%s\" required: %s".

+   {"require_auth", NULL, NULL, NULL,
+       "Require-Auth", "", 14, /* sizeof("scram-sha-256") == 14 */
+   offsetof(struct pg_conn, require_auth)},
We could have an environment variable for that.

+/*
+ * Convenience macro for checking the allowed_auth_methods bitmask. Caller must
+ * ensure that type is not greater than 31 (high bit of the bitmask).
+ */
+#define auth_allowed(conn, type) \
+   (((conn)->allowed_auth_methods & (1 << (type))) != 0)
Better to add a compile-time check with StaticAssertDecl() then?  Or
add a note about that in pqcomm.h?

+      else if (auth_allowed(conn, AUTH_REQ_GSS) && conn->gssenc)
+      {
This field is only available under ENABLE_GSS, so this would fail to
compile when building without it?

+           method = parse_comma_separated_list(&s, &more);
+           if (method == NULL)
+               goto oom_error;
This should free the malloc'd copy of the element parsed, no?  That
means a free at the end of the while loop processing the options.

+           /*
+            * Sanity check; a duplicated method probably indicates a typo in a
+            * setting where typos are extremely risky.
+            */
Not sure why this is a problem.  Fine by me to check that, but this is
an OR list, so specifying one element twice means the same as once.

I get that it is not the priority yet as long as the design is not
completely clear, but having some docs would be nice :)
--
Michael