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

On Wed, Jun 8, 2022 at 9:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> > - 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?

I think it's already a problem, if you're not using channel_binding.
The cert behavior here makes it even less intuitive.

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

Personally I think the ability to provide a default of `!password` is
very compelling. Any allowlist we hardcode won't be future-proof (see
also my response to Laurenz upthread [1]).

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

I'm pretty motivated to provide the ability to say "I want cert auth
only, nothing else." Using a separate parameter would mean we'd need
something like `require_auth=none`, but I think that makes a certain
amount of sense.

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

Agreed.

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

Sounds good.

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

Yeah, I think I have a TODO somewhere about that somewhere. I'm
confused about your suggested fix though; can you elaborate?

> +   {"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.

I think that'd be a good idea. It'd be nice to have the option of
forcing a particular auth type across a process tree.

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

If we only passed constants, that would work, but we also determine
the type at runtime, from the server message. Or am I missing
something?

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

Yes, thank you for the catch. Will fix.

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

Good catch again, thanks!

> +           /*
> +            * 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.

Since this is likely to be a set-and-forget sort of option, and it
needs to behave correctly across server upgrades, I'd personally
prefer that the client tell me immediately if I've made a silly
mistake. Even for something relatively benign like this (but arguably,
it's more important for the NOT case).

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

Agreed; I will tackle that soon.

Thanks!
--Jacob

[1] https://www.postgresql.org/message-id/a14b1f89dcde75fb20afa7a1ffd2c2587b8d1a08.camel%40vmware.com