Re: [PoC] Let libpq reject unexpected authentication requests

On Mon, Jun 13, 2022 at 10:00 PM Michael Paquier <michael@paquier.xyz> wrote:
> > 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.

I am 100% with you on this, but there's been a lot of chatter around
removing plaintext password support from libpq. I would much rather
see them rejected by default than removed entirely. !password would
provide an easy path for that.

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

If you want cert authentication only, allowing "everything" will let
the server extract a password and then you're back at square one.
There needs to be a way to prohibit all explicit authentication
requests.

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

Ah, thanks for the clarification. Done that way in v3.

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

Makes sense; done.

v3 also removes "cert" from require_auth while I work on a replacement
connection option, fixes the bugs/suggestions pointed out upthread,
and adds a documentation first draft. I tried combining this with the
NOT work but it was too much to juggle, so that'll wait for a v4+,
along with require_auth=none and "cert mode".

Thanks for the detailed review!
--Jacob