Re: [PoC] Let libpq reject unexpected authentication requests

On Thu, Jun 09, 2022 at 04:29:38PM -0700, Jacob Champion wrote:
> On Wed, Jun 8, 2022 at 9:58 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> 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.

Ah right. I forgot about the part where we need to have the backend
publish the set of supported machanisms. If we don't get back
SCRAM-SHA-256-PLUS we are currently complaining after the exchange has
been initialized, true. Maybe I should look at the RFC more closely.
The backend is very strict regarding that and needs to return an error
back to the client only when the exchange is done, but I don't recall
all the bits about the client handling.

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

Hm, perhaps. You could use that as a default at application level,
but the default set in libpq should be backward-compatible (aka allow
everything, even trust where the backend just sends AUTH_REQ_OK).
This is unfortunate but there is a point in not breaking any user's
application, as well, particularly with ldap, and note that we have to
keep a certain amount of things backward-compatible as a very common
practice of packaging with Postgres is to have libpq link to binaries
across *multiple* major versions (Debian is one if I recall), with the
newest version of libpq used for all. One argument in favor of
!password would be to control whether one does not want to use ldap,
but I'd still see most users just specify one or more methods in line
with their HBA policy as an approved list.

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

If the default of require_auth is backward-compatible and allows
everything, using a different parameter for the certs won't matter
anyway?

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

My suggestion is to reword the error message so as the reason and the
main error message can be treated as two independent things. You are
right to apply two times libpq_gettext(), once to "reason" and once to
the main string.

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

My point would be to either register a max flag in the set of
AUTH_REQ_* in pqcomm.h so as we never go above 32 with an assertion to
make sure that this would never overflow, but I would add a comment in
pqcomm.h telling that we also do bitwise operations, relying on the
number of AUTH_REQ_* flags, and that we'd better be careful once the
number of flags gets higher than 32. There is some margin, still that
could be easily forgotten.

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

This is just a couple of lines. Fine by me to keep it if you feel
that's better in the long run.
--
Michael