[PoC] Let libpq reject unexpected authentication requests

Jacob Champion <pchampion@vmware.com> writes:

$subject keeps coming up in threads. I think my first introduction to
it was after the TLS injection CVE, and then it came up again in the
pluggable auth thread. It's hard for me to generalize based on "sound
bites", but among the proposals I've seen are

1. reject plaintext passwords
2. reject a configurable list of unacceptable methods
3. allow client and server to negotiate a method

All of them seem to have merit.

Agreed.

Here is my take on option 2, then: you get to choose exactly one method
that the client will accept. If you want to use client certificates,
use require_auth=cert. If you want to force SCRAM, use
require_auth=scram-sha-256. If the server asks for something different,
libpq will fail. If the server tries to get away without asking you for
authentication, libpq will fail. There is no negotiation.

Seems reasonable, but I bet that for very little more code you could
accept a comma-separated list of allowed methods; libpq already allows
comma-separated lists for some other connection options. That seems
like it'd be a useful increment of flexibility.

regards, tom lane

On Fri, Mar 04, 2022 at 08:19:26PM -0500, Tom Lane wrote:

Jacob Champion <pchampion@vmware.com> writes:

Here is my take on option 2, then: you get to choose exactly one method
that the client will accept. If you want to use client certificates,
use require_auth=cert. If you want to force SCRAM, use
require_auth=scram-sha-256. If the server asks for something different,
libpq will fail. If the server tries to get away without asking you for
authentication, libpq will fail. There is no negotiation.

Fine by me to put all the control on the client-side, that makes the
whole much simpler to reason about.

Seems reasonable, but I bet that for very little more code you could
accept a comma-separated list of allowed methods; libpq already allows
comma-separated lists for some other connection options. That seems
like it'd be a useful increment of flexibility.

Same impression here, so +1 for supporting a comma-separated list of
values here. This is already handled in parse_comma_separated_list(),
now used for multiple hosts and hostaddrs.
--
Michael

On 3/4/22 20:19, Tom Lane wrote:

Jacob Champion <pchampion@vmware.com> writes:

$subject keeps coming up in threads. I think my first introduction to
it was after the TLS injection CVE, and then it came up again in the
pluggable auth thread. It's hard for me to generalize based on "sound
bites", but among the proposals I've seen are
1. reject plaintext passwords
2. reject a configurable list of unacceptable methods
3. allow client and server to negotiate a method
All of them seem to have merit.

Agreed.

Here is my take on option 2, then: you get to choose exactly one method
that the client will accept. If you want to use client certificates,
use require_auth=cert. If you want to force SCRAM, use
require_auth=scram-sha-256. If the server asks for something different,
libpq will fail. If the server tries to get away without asking you for
authentication, libpq will fail. There is no negotiation.

Seems reasonable, but I bet that for very little more code you could
accept a comma-separated list of allowed methods; libpq already allows
comma-separated lists for some other connection options. That seems
like it'd be a useful increment of flexibility.

Just about necessary I guess, since you can specify that a client cert
is required in addition to some other auth method, so for such cases you
might want something like "required_auth=cert,scram-sha-256"? Or do we
need a way of specifying the combination?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 3/4/22 20:19, Tom Lane wrote:

Seems reasonable, but I bet that for very little more code you could
accept a comma-separated list of allowed methods; libpq already allows
comma-separated lists for some other connection options. That seems
like it'd be a useful increment of flexibility.

Just about necessary I guess, since you can specify that a client cert
is required in addition to some other auth method, so for such cases you
might want something like "required_auth=cert,scram-sha-256"? Or do we
need a way of specifying the combination?

I'd view the comma as strictly meaning OR, so that you might need some
notation like "required_auth=cert+scram-sha-256" if you want to demand
ANDed conditions. It might be better to handle TLS-specific
conditions orthogonally to the authentication exchange, though.

regards, tom lane

On Sat, 2022-03-05 at 01:04 +0000, Jacob Champion wrote:

TL;DR: this patch lets you specify exactly one authentication method in
the connection string, and libpq will fail the connection if the server
doesn't use that method.

(This is not intended for PG15. I'm generally anxious about posting
experimental work during a commitfest, but there's been enough
conversation about this topic recently that I felt like it'd be useful
to have code to point to.)

== Proposal and Alternatives ==

$subject keeps coming up in threads. I think my first introduction to
it was after the TLS injection CVE, and then it came up again in the
pluggable auth thread. It's hard for me to generalize based on "sound
bites", but among the proposals I've seen are

1. reject plaintext passwords
2. reject a configurable list of unacceptable methods
3. allow client and server to negotiate a method

All of them seem to have merit. I'm personally motivated by the case
brought up by the CVE: if I'm expecting client certificate
authentication, it's not acceptable for the server to extract _any_
information about passwords from my system, whether they're plaintext,
hashed, or SCRAM-protected. So I chose not to implement option 1. And
option 3 looked like a lot of work to take on in an experiment without
a clear consensus.

Here is my take on option 2, then: you get to choose exactly one method
that the client will accept.

I am all for the idea, but you implemented the reverse of proposal 2.

Wouldn't it be better to list the *rejected* authentication methods?
Then we could have "password" on there by default.

Yours,
Laurenz Albe

On Mon, 2022-03-07 at 11:44 +0100, Laurenz Albe wrote:

I am all for the idea, but you implemented the reverse of proposal 2.

(This email was caught in my spam filter; sorry for the delay.)

Wouldn't it be better to list the *rejected* authentication methods?
Then we could have "password" on there by default.

Specifying the allowed list rather than the denied list tends to have
better security properties.

In the case I'm pursuing (the attack vector from the CVE), the end user
expects certificates to be used. Any other authentication method --
plaintext, hashed, SCRAM, Kerberos -- is unacceptable; it shouldn't be
possible for the server to extract any information about the client
environment other than the cert. And I don't want to have to specify
the whole list of things that _aren't_ allowed, and keep that list
updated as we add new fancy auth methods, if I just want certs to be
used. So that's my argument for making the methods opt-in rather than
opt-out.

But that doesn't help your case; you want to choose a good default, and
I agree that's important. Since there are arguments already for
accepting a OR in the list, and -- if we couldn't find a good
orthogonal method for certs, like Tom suggested -- an AND, maybe it
wouldn't be so bad to accept a NOT as well?

require_auth=cert # certs only
require_auth=cert+scram-sha-256 # SCRAM wrapped by certs
require_auth=cert,scram-sha-256 # SCRAM or certs (or both)
require_auth=!password # anything but plaintext
require_auth=!password,!md5 # no plaintext or MD5

But it doesn't ever make sense to mix them:

require_auth=cert,!password # error: !password is useless
require_auth=!password,password # error: nonsense

--Jacob

On Wed, 2022-03-23 at 21:31 +0000, Jacob Champion wrote:

On Mon, 2022-03-07 at 11:44 +0100, Laurenz Albe wrote:

I am all for the idea, but you implemented the reverse of proposal 2.

Wouldn't it be better to list the *rejected* authentication methods?
Then we could have "password" on there by default.

Specifying the allowed list rather than the denied list tends to have
better security properties.

In the case I'm pursuing (the attack vector from the CVE), the end user
expects certificates to be used. Any other authentication method --
plaintext, hashed, SCRAM, Kerberos -- is unacceptable;

That makes sense.

But that doesn't help your case; you want to choose a good default, and
I agree that's important. Since there are arguments already for
accepting a OR in the list, and -- if we couldn't find a good
orthogonal method for certs, like Tom suggested -- an AND, maybe it
wouldn't be so bad to accept a NOT as well?

    require_auth=cert                # certs only
    require_auth=cert+scram-sha-256  # SCRAM wrapped by certs
    require_auth=cert,scram-sha-256  # SCRAM or certs (or both)
    require_auth=!password           # anything but plaintext
    require_auth=!password,!md5      # no plaintext or MD5

Great, if there is a !something syntax, then I have nothing left to wish.
It may not be the most secure way do do it, but it sure is convenient.

Yours,
Laurenz Albe

On Sat, Mar 05, 2022 at 01:04:05AM +0000, Jacob Champion wrote:

the connection string, and libpq will fail the connection if the server
doesn't use that method.

(This is not intended for PG15. I'm generally anxious about posting
experimental work during a commitfest, but there's been enough
conversation about this topic recently that I felt like it'd be useful
to have code to point to.)

Jacob, do you still have plans to work on this patch?
--
Michael

On Wed, Jun 1, 2022 at 12:55 AM Michael Paquier <michael@paquier.xyz> wrote:

Jacob, do you still have plans to work on this patch?

Yes, definitely. That said, the more the merrier if there are others
interested in taking a shot at it. There are a large number of
alternative implementation proposals.

Thanks,
--Jacob

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

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

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.

--Jacob

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

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]/messages/by-id/a14b1f89dcde75fb20afa7a1ffd2c2587b8d1a08.camel@vmware.com).

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]: /messages/by-id/a14b1f89dcde75fb20afa7a1ffd2c2587b8d1a08.camel@vmware.com

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

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

On Thu, Jun 9, 2022 at 4:30 PM Jacob Champion <jchampion@timescale.com>
wrote:

On Wed, Jun 8, 2022 at 9:58 PM Michael Paquier <michael@paquier.xyz>
wrote:

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.

That just makes me want to not implement OR'ing...

The existing set of individual parameters doesn't work as an API for
try-and-fallback.

Something like would be less problematic when it comes to setting multiple
related options:

--auth-try
"1;sslmode=require;channel_binding=require;method=scram-sha-256;sslcert=/tmp/machine.cert;sslkey=/tmp/machine.key"
--auth-try
"2;sslmode=require;method=cert;sslcert=/tmp/me.cert;sslkey=/tmp/me.key"
--auth-try "3;sslmode=prefer;method=md5"

Absent that radical idea, require_auth probably shouldn't change any
behavior that exists today without having specified require_auth and having
the chosen method happen anyway. So whatever happens today with an md5
password prompt while channel_binding is set to require (not in the mood
right now to figure out how to test that on a compiled against HEAD
instance).

David J.

On Wed, Jun 22, 2022 at 5:56 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

That just makes me want to not implement OR'ing...

The existing set of individual parameters doesn't work as an API for try-and-fallback.

Something like would be less problematic when it comes to setting multiple related options:

--auth-try "1;sslmode=require;channel_binding=require;method=scram-sha-256;sslcert=/tmp/machine.cert;sslkey=/tmp/machine.key"
--auth-try "2;sslmode=require;method=cert;sslcert=/tmp/me.cert;sslkey=/tmp/me.key"
--auth-try "3;sslmode=prefer;method=md5"

I think that's a fair point, and your --auth-try example definitely
illustrates why having require_auth try to do everything is probably
not a viable strategy. My arguments for keeping OR in spite of that
are

- the default is effectively an OR of all available methods (plus "none");
- I think NOT is a important case in practice, which is effectively a
negative OR ("anything but this/these"); and
- not providing an explicit, positive OR to complement the above seems
like it would be a frustrating user experience once you want to get
just a little bit more creative.

It's also low-hanging fruit that doesn't require multiple connections
to the server per attempt (which I think your --auth-try proposal
might, if I understand it correctly).

Absent that radical idea, require_auth probably shouldn't change any behavior that exists today without having specified require_auth and having the chosen method happen anyway. So whatever happens today with an md5 password prompt while channel_binding is set to require (not in the mood right now to figure out how to test that on a compiled against HEAD instance).

I think the newest tests in v3 should enforce that, but let me know if
I've missed something.

--Jacob

On Thu, Jun 23, 2022 at 10:33 AM Jacob Champion <jchampion@timescale.com> wrote:

- I think NOT is a important case in practice, which is effectively a
negative OR ("anything but this/these")

Both NOT (via ! negation) and "none" are implemented in v4.

Examples:

# The server must use SCRAM.
require_auth=scram-sha-256
# The server must use SCRAM or Kerberos.
require_auth=scram-sha-256,gss,sspi
# The server may optionally use SCRAM.
require_auth=none,scram-sha-256
# The server must not use any application-level authentication.
require_auth=none
# The server may optionally use authentication, except plaintext
# passwords.
require_auth=!password
# The server may optionally use authentication, except weaker password
# challenges.
require_auth=!password,!md5
# The server must use an authentication method.
require_auth=!none
# The server must use a non-plaintext authentication method.
require_auth=!none,!password

Note that `require_auth=none,scram-sha-256` allows the server to
abandon a SCRAM exchange early, same as it can today. That might be a
bit surprising.

--Jacob

On Fri, Jun 24, 2022 at 12:17 PM Jacob Champion <jchampion@timescale.com> wrote:

Both NOT (via ! negation) and "none" are implemented in v4.

v5 adds a second patch which implements a client-certificate analogue
to gssencmode; I've named it sslcertmode. This takes the place of the
require_auth=[!]cert setting implemented previously.

As I mentioned upthread, I think sslcertmode=require is the weakest
feature here, since the server always sends a certificate request if
you are using TLS. It would potentially be more useful if we start
expanding TLS setups and middlebox options, but I still only see it as
a troubleshooting feature for administrators. By contrast,
sslcertmode=disable lets you turn off the use of the certificate, no
matter what libpq is able to find in your environment or home
directory. That seems more immediately useful.

With this addition, I'm wondering if GSS encrypted transport should be
removed from the definition/scope of require_auth=gss. We already have
gssencmode to control that, and it would remove an ugly special case
from the patch.

I'll add this patchset to the commitfest.

--Jacob

On Mon, Jun 27, 2022 at 12:05 PM Jacob Champion <jchampion@timescale.com> wrote:

v5 adds a second patch which implements a client-certificate analogue
to gssencmode; I've named it sslcertmode.

...and v6 fixes check-world, because I always forget about postgres_fdw.

--Jacob