Re: [PoC] Let libpq reject unexpected authentication requests

From: Jacob Champion <jchampion(at)timescale(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PoC] Let libpq reject unexpected authentication requests
Date: 2022-06-22 23:36:00
Message-ID: CAAWbhmgQ1CqxswuDd72-geu61s6vh=9KmbrAq6d6hBAcn1QEnw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Jun 13, 2022 at 10:00 PM Michael Paquier <michael(at)paquier(dot)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

Attachment Content-Type Size
since-v2.diff.txt text/plain 17.6 KB
v3-0001-libpq-let-client-reject-unexpected-auth-methods.patch text/x-patch 24.4 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-06-22 23:49:08 Re: Make COPY extendable in order to support Parquet and other formats
Previous Message Andres Freund 2022-06-22 23:29:12 Re: Amcheck verification of GiST and GIN