Thread: [PoC] Let libpq reject unexpected authentication requests
Hello,
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. 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.
== Why Force Authn? ==
I think my decision to fail if the server doesn't authenticate might be
controversial. It doesn't provide additional protection against active
attack unless you're using a mutual authentication method (SCRAM),
because you can't prove that the server actually did anything with its
side of the handshake. But this approach grew on me for a few reasons:
- When using SCRAM, it allows the client to force a server to
authenticate itself, even when channel bindings aren't being used. (I
really think it's weird that we let the server get away with that
today.)
- In cases where you want to ensure that your actions are logged for
later audit, you can be reasonably sure that you're connecting to a
database that hasn't been configured with a `trust` setting.
- For cert authentication, it ensures that the server asked for a cert
and that you actually sent one. This is more forward-looking; today, we
always ask for a certificate from the client even if we don't use it.
But if implicit TLS takes off, I'd expect to see more middleware, with
more potential for misconfiguration.
== General Thoughts ==
I like that this approach fits nicely into the existing code. The
majority of the patch just beefs up check_expected_areq(). The new flag
that tracks whether or not we've authenticated is scattered around more
than I would like, but I'm hopeful that some of the pluggable auth
conversations will lead to nice refactoring opportunities for those
internal helpers.
There's currently no way to prohibit client certificates from being
sent. If my use case is "servers shouldn't be able to extract password
info if the client expects certificates", then someone else may very
well say "servers shouldn't be able to extract my client certificate if
I wanted to use SCRAM". Likewise, this feature won't prevent a GSS
authenticated channel -- but we do have gssencmode=disable, so I'm less
concerned there.
I made the assumption that a GSS encrypted channel authenticates both
parties to each other, but I don't actually know what guarantees are
made there. I have not tested SSPI.
I'm not a fan of the multiple spellings of "password" ("ldap", "pam",
et al). My initial thought was that it'd be nice to match the client
setting to the HBA setting in the server. But I don't think it's really
all that helpful; worst-case, it pretends to do something it can't,
since the client can't determine what the plaintext password is
actually used for on the backend. And if someone devises (say) a SASL
scheme for proxied LDAP authentication, that spelling becomes obsolete.
Speaking of obsolete, the current implementation assumes that any SASL
exchange must be for SCRAM. That won't be anywhere close to future-
proof.
Thanks,
--Jacob
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
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
Attachment
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
Attachment
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
specifiedrequire_auth and having the chosen method happen anyway. So whatever happens today with an md5 password
promptwhile channel_binding is set to require (not in the mood right now to figure out how to test that on a compiled
againstHEAD 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
Attachment
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
Attachment
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
Attachment
On 27.06.22 23:40, Jacob Champion wrote: > -HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr,port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout,sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version,ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate,fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, fetch_size, batch_size, async_capable,parallel_commit, keep_connections > +HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr,port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout,sslmode, sslcompression, sslcert, sslkey, sslcertmode, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer,require_auth, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs,use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, fetch_size,batch_size, async_capable, parallel_commit, keep_connections It's not strictly related to your patch, but maybe this hint has outlived its usefulness? I mean, we don't list all available tables when you try to reference a table that doesn't exist. And unordered on top of that.
On Wed, Jun 29, 2022 at 6:36 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> It's not strictly related to your patch, but maybe this hint has
> outlived its usefulness? I mean, we don't list all available tables
> when you try to reference a table that doesn't exist. And unordered on
> top of that.
Yeah, maybe it'd be better to tell the user the correct context for an
otherwise-valid option ("the 'sslcert' option may only be applied to
USER MAPPING"), and avoid the option dump entirely?
--
v7, attached, fixes configuration on Windows.
--Jacob
Attachment
On Thu, Jun 30, 2022 at 04:26:54PM -0700, Jacob Champion wrote:
> Yeah, maybe it'd be better to tell the user the correct context for an
> otherwise-valid option ("the 'sslcert' option may only be applied to
> USER MAPPING"), and avoid the option dump entirely?
Yes, that would be nice. Now, this HINT has been an annoyance in the
context of the regression tests when adding features entirely
unrelated to postgres_fdw, at least for me. I would be more tempted
to get rid of it entirely, FWIW.
--
Michael
Attachment
I'm wondering about making the list of things you can specify in require_auth less confusing and future proof. For example, before long someone is going to try putting "ldap" into require_auth. The fact that the methods in pg_hba.conf are not what libpq sees is not something that was really exposed to users until now. "none" vs. "trust" takes advantage of that. But then I think we could also make "password" clearer, which surely sounds like any kind of password, encrypted or not, and that's also how pg_hba.conf behaves. The protocol specification calls that "AuthenticationCleartextPassword"; maybe we could pick a name based on that. And then, what if we add a new method in the future, and someone puts that into their connection string. Old clients will just refuse to parse that. Ok, that effectively gives you the same behavior as rejecting the server's authentication offer. But what about the negated version? Also, what if we add new SASL methods. How would we modify this code to be able to pick and choose and also have backward and forward compatible behavior? In general, I like this. We just need to think about the above things a bit more.
On Thu, Sep 8, 2022 at 6:25 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > For example, before long someone is going to try putting "ldap" into > require_auth. The fact that the methods in pg_hba.conf are not what > libpq sees is not something that was really exposed to users until now. > "none" vs. "trust" takes advantage of that. But then I think we could > also make "password" clearer, which surely sounds like any kind of > password, encrypted or not, and that's also how pg_hba.conf behaves. > The protocol specification calls that "AuthenticationCleartextPassword"; > maybe we could pick a name based on that. Sounds fair. "cleartext"? "plaintext"? "plain" (like SASL's PLAIN)? > And then, what if we add a new method in the future, and someone puts > that into their connection string. Old clients will just refuse to > parse that. Ok, that effectively gives you the same behavior as > rejecting the server's authentication offer. But what about the negated > version? I assume the alternative behavior you're thinking of is to ignore negated "future methods"? I think the danger with that (for a feature that's supposed to be locking communication down) is that it's not possible to differentiate between a maybe-future method and a typo. If I want "!password" because my intention is to disallow a plaintext exchange, I really don't want "!pasword" to silently allow anything. > Also, what if we add new SASL methods. How would we modify > this code to be able to pick and choose and also have backward and > forward compatible behavior? On the SASL front: In the back of my head I'd been considering adding a "sasl:" prefix to "scram-sha-256", so that we have a namespace for new SASL methods. That would also give us a jumping-off point in the future if we decide to add SASL method negotiation to the protocol. What do you think about that? Backwards compatibility will, I think, be handled trivially by a newer client. The only way to break backwards compatibility would be to remove support for a method, which I assume would be independent of this feature. Forwards compatibility doesn't seem like something this feature can add by itself (old clients can't speak new methods). Though we could backport new method names to allow them to be used in negations, if maintaining that aspect of compatibility is worth the effort. > In general, I like this. We just need to think about the above things a > bit more. Thanks! --Jacob
On 08.09.22 20:18, Jacob Champion wrote: > Sounds fair. "cleartext"? "plaintext"? "plain" (like SASL's PLAIN)? > On the SASL front: In the back of my head I'd been considering adding > a "sasl:" prefix to "scram-sha-256", so that we have a namespace for > new SASL methods. That would also give us a jumping-off point in the > future if we decide to add SASL method negotiation to the protocol. > What do you think about that? After thinking about this a bit more, I think it would be best if the words used here match exactly with what is used in pg_hba.conf. That's the only thing the user cares about: reject "password", reject "trust", require "scram-sha-256", etc. How this maps to the protocol and that some things are SASL or not is not something they have needed to care about and don't really need to know for this. So I would suggest to organize it that way. Another idea: Maybe instead of the "!" syntax, use two settings, require_auth and reject_auth? Might be simpler?
On Fri, Sep 16, 2022 at 7:56 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > On 08.09.22 20:18, Jacob Champion wrote: > After thinking about this a bit more, I think it would be best if the > words used here match exactly with what is used in pg_hba.conf. That's > the only thing the user cares about: reject "password", reject "trust", > require "scram-sha-256", etc. How this maps to the protocol and that > some things are SASL or not is not something they have needed to care > about and don't really need to know for this. So I would suggest to > organize it that way. I tried that in v1, if you'd like to see what that ends up looking like. As a counterexample, I believe `cert` auth looks identical to `trust` on the client side. (The server always asks for a client certificate even if it doesn't use it. Otherwise, this proposal would probably have looked different.) And `ldap` auth is indistinguishable from `password`, etc. In my opinion, how it maps to the protocol is more honest to the user than how it maps to HBA, because the auth request sent by the protocol determines your level of risk. I also like `none` over `trust` because you don't have to administer a server to understand what it means. That's why I was on board with your proposal to change the name of `password`. And you don't have to ignore the natural meaning of client-side "trust", which IMO means "trust the server." There's opportunity for confusion either way, unfortunately, but naming them differently may help make it clear that they _are_ different. This problem overlaps a bit with the last remaining TODO in the code. I treat gssenc tunnels as satisfying require_auth=gss. Maybe that's useful, because it kind of maps to how HBA treats it? But it's not consistent with the TLS side of things, and it overlaps with gssencmode=require, complicating the relationship with the new sslcertmode. > Another idea: Maybe instead of the "!" syntax, use two settings, > require_auth and reject_auth? Might be simpler? Might be. If that means we have to handle the case where both are set to something, though, it might make things harder. We can error out if they conflict, which adds a decent but not huge amount of complication. Or we can require that only one is set, which is both easy and overly restrictive. But either choice makes it harder to adopt a `reject password` default, as many people seem to be interested in doing. Because if you want to override that default, then you have to first unset reject_auth and then set require_auth, as opposed to just saying require_auth=something and being done with it. I'm not sure that's worth it. Thoughts? I'm happy to implement proofs of concept for that, or any other ideas, given the importance of getting this "right enough" the first time. Just let me know. Thanks, --Jacob
On Fri, Sep 16, 2022 at 1:29 PM Jacob Champion <jchampion@timescale.com> wrote: > I'm happy to implement proofs of concept for that, or any other ideas, > given the importance of getting this "right enough" the first time. > Just let me know. v8 rebases over the postgres_fdw HINT changes; there are no functional differences. --Jacob
Attachment
On 21.09.22 17:33, Jacob Champion wrote:
> On Fri, Sep 16, 2022 at 1:29 PM Jacob Champion <jchampion@timescale.com> wrote:
>> I'm happy to implement proofs of concept for that, or any other ideas,
>> given the importance of getting this "right enough" the first time.
>> Just let me know.
>
> v8 rebases over the postgres_fdw HINT changes; there are no functional
> differences.
So let's look at the two TODO comments you have:
* TODO: how should !auth_required interact with an incomplete
* SCRAM exchange?
What specific combination of events are you thinking of here?
/*
* If implicit GSS auth has already been performed via GSS
* encryption, we don't need to have performed an
* AUTH_REQ_GSS exchange.
*
* TODO: check this assumption. What mutual auth guarantees
* are made in this case?
*/
I don't understand the details involved here, but I would be surprised
if this assumption is true. For example, does GSS encryption deal with
user names and a user name map? I don't see how these can be
equivalent. In any case, it seems to me that it would be safer to *not*
make this assumption at first and then have someone more knowledgeable
make the argument that it would be safe.
On Wed, Sep 21, 2022 at 3:36 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> So let's look at the two TODO comments you have:
>
> * TODO: how should !auth_required interact with an incomplete
> * SCRAM exchange?
>
> What specific combination of events are you thinking of here?
Let's say the client is using `require_auth=!password`. If the server
starts a SCRAM exchange. but doesn't finish it, the connection will
still succeed with the current implementation (because it satisfies
the "none" case). This is also true for a client setting of
`require_auth=scram-sha-256,none`. I think this is potentially
dangerous, but it mirrors the current behavior of libpq and I'm not
sure that we should change it as part of this patch.
> /*
> * If implicit GSS auth has already been performed via GSS
> * encryption, we don't need to have performed an
> * AUTH_REQ_GSS exchange.
> *
> * TODO: check this assumption. What mutual auth guarantees
> * are made in this case?
> */
>
> I don't understand the details involved here, but I would be surprised
> if this assumption is true. For example, does GSS encryption deal with
> user names and a user name map?
To my understanding, yes. There are explicit tests for it.
> In any case, it seems to me that it would be safer to *not*
> make this assumption at first and then have someone more knowledgeable
> make the argument that it would be safe.
I think I'm okay with that, regardless. Here's one of the wrinkles:
right now, both of the following connstrings work:
require_auth=gss gssencmode=require
require_auth=gss gssencmode=prefer
If we don't treat gssencmode as providing GSS auth, then the first
case will always fail, because there will be no GSS authentication
packet over an encrypted connection. Likewise, the second case will
almost always fail, unless the server doesn't support gssencmode at
all (so why are you using prefer?).
If you're okay with those limitations, I will rip out the code. The
reason I'm not too worried about it is, I don't think it makes much
sense to be strict about your authentication requirements while at the
same time leaving the choice of transport encryption up to the server.
Thanks,
--Jacob
On 22.09.22 01:37, Jacob Champion wrote: > On Wed, Sep 21, 2022 at 3:36 PM Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: >> So let's look at the two TODO comments you have: >> >> * TODO: how should !auth_required interact with an incomplete >> * SCRAM exchange? >> >> What specific combination of events are you thinking of here? > > Let's say the client is using `require_auth=!password`. If the server > starts a SCRAM exchange. but doesn't finish it, the connection will > still succeed with the current implementation (because it satisfies > the "none" case). This is also true for a client setting of > `require_auth=scram-sha-256,none`. I think this is potentially > dangerous, but it mirrors the current behavior of libpq and I'm not > sure that we should change it as part of this patch. It might be worth reviewing that behavior for other reasons, but I think semantics of your patch are correct. >> In any case, it seems to me that it would be safer to *not* >> make this assumption at first and then have someone more knowledgeable >> make the argument that it would be safe. > > I think I'm okay with that, regardless. Here's one of the wrinkles: > right now, both of the following connstrings work: > > require_auth=gss gssencmode=require > require_auth=gss gssencmode=prefer > > If we don't treat gssencmode as providing GSS auth, then the first > case will always fail, because there will be no GSS authentication > packet over an encrypted connection. Likewise, the second case will > almost always fail, unless the server doesn't support gssencmode at > all (so why are you using prefer?). > > If you're okay with those limitations, I will rip out the code. The > reason I'm not too worried about it is, I don't think it makes much > sense to be strict about your authentication requirements while at the > same time leaving the choice of transport encryption up to the server. The way I understand what you explained here is that it would be more sensible to leave that code in. I would be okay with that.
On Thu, Sep 22, 2022 at 4:52 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > On 22.09.22 01:37, Jacob Champion wrote: > > I think this is potentially > > dangerous, but it mirrors the current behavior of libpq and I'm not > > sure that we should change it as part of this patch. > > It might be worth reviewing that behavior for other reasons, but I think > semantics of your patch are correct. Sounds good. v9 removes the TODO and adds a better explanation. > > If you're okay with those [GSS] limitations, I will rip out the code. The > > reason I'm not too worried about it is, I don't think it makes much > > sense to be strict about your authentication requirements while at the > > same time leaving the choice of transport encryption up to the server. > > The way I understand what you explained here is that it would be more > sensible to leave that code in. I would be okay with that. I've added a comment there explaining the gssencmode interaction. That leaves no TODOs inside the code itself. I removed the commit message note about not being able to prevent unexpected client cert requests or GSS encryption, since we've decided to handle those cases outside of require_auth. I'm not able to test SSPI easily at the moment; if anyone is able to try it out, that'd be really helpful. There's also the question of SASL forwards compatibility -- if someone adds a new SASL mechanism, the code will treat it like scram-sha-256 until it's changed, and there will be no test to catch it. Should we leave that to the future mechanism implementer to fix, or add a mechanism check now so the client is safe even if they forget? Thanks! --Jacob
Attachment
On 23.09.22 02:02, Jacob Champion wrote:
> On Thu, Sep 22, 2022 at 4:52 AM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> On 22.09.22 01:37, Jacob Champion wrote:
>>> I think this is potentially
>>> dangerous, but it mirrors the current behavior of libpq and I'm not
>>> sure that we should change it as part of this patch.
>>
>> It might be worth reviewing that behavior for other reasons, but I think
>> semantics of your patch are correct.
>
> Sounds good. v9 removes the TODO and adds a better explanation.
I'm generally okay with these patches now.
> I'm not able to test SSPI easily at the moment; if anyone is able to
> try it out, that'd be really helpful. There's also the question of
> SASL forwards compatibility -- if someone adds a new SASL mechanism,
> the code will treat it like scram-sha-256 until it's changed, and
> there will be no test to catch it. Should we leave that to the future
> mechanism implementer to fix, or add a mechanism check now so the
> client is safe even if they forget?
I think it would be good to put some provisions in place here, even if
they are elementary. Otherwise, there will be a significant burden on
the person who implements the next SASL method (i.e., you ;-) ) to
figure that out then.
I think you could just stick a string list of allowed SASL methods into
PGconn.
By the way, I'm not sure all the bit fiddling is really worth it. An
array of integers (or unsigned char or whatever) would work just as
well. Especially if you are going to have a string list for SASL
anyway. You're not really saving any bits or bytes either way in the
normal case.
Minor comments:
Pasting together error messages like with auth_description() isn't going
to work. You either need to expand the whole message in
check_expected_areq(), or perhaps rephrase the message like
libpq_gettext("auth method \"%s\" required, but server requested \"%s\"\n"),
conn->require_auth,
auth_description(areq)
and make auth_description() just return a single word not subject to
translation.
spurious whitespace change in fe-secure-openssl.c
whitespace error in patch:
.git/rebase-apply/patch:109: tab in indent.
via TLS, nor GSS authentication via its
encrypted transport.)
In the 0002 patch, the configure test needs to be added to meson.build.
On 10/5/22 06:33, Peter Eisentraut wrote:
> I think it would be good to put some provisions in place here, even if
> they are elementary. Otherwise, there will be a significant burden on
> the person who implements the next SASL method (i.e., you ;-) ) to
> figure that out then.
Sounds good, I'll work on that. v10 does not yet make changes in this area.
> I think you could just stick a string list of allowed SASL methods into
> PGconn.
>
> By the way, I'm not sure all the bit fiddling is really worth it. An
> array of integers (or unsigned char or whatever) would work just as
> well. Especially if you are going to have a string list for SASL
> anyway. You're not really saving any bits or bytes either way in the
> normal case.
Yeah, with the SASL case added in, the bitmasks might not be long for
this world. It is nice to be able to invert the whole thing, but a
separate boolean saying "invert the list" could accomplish the same goal
and I think we'll need to have that for the SASL mechanism list anyway.
> Minor comments:
>
> Pasting together error messages like with auth_description() isn't going
> to work. You either need to expand the whole message in
> check_expected_areq(), or perhaps rephrase the message like
>
> libpq_gettext("auth method \"%s\" required, but server requested \"%s\"\n"),
> conn->require_auth,
> auth_description(areq)
>
> and make auth_description() just return a single word not subject to
> translation.
Right. Michael tried to warn me about that upthread, but I only ended up
fixing one of the two error cases for some reason. I've merged the two
into one code path for v10.
Quick error messaging bikeshed: do you prefer
auth method "!password,!md5" requirement failed: ...
or
auth method requirement "!password,!md5" failed: ...
?
> spurious whitespace change in fe-secure-openssl.c
Fixed.
> whitespace error in patch:
>
> .git/rebase-apply/patch:109: tab in indent.
> via TLS, nor GSS authentication via its
> encrypted transport.)
Fixed.
> In the 0002 patch, the configure test needs to be added to meson.build.
Added.
Thanks,
--Jacob
Attachment
On Wed, Oct 12, 2022 at 9:40 AM Jacob Champion <jchampion@timescale.com> wrote: > On 10/5/22 06:33, Peter Eisentraut wrote: > > I think it would be good to put some provisions in place here, even if > > they are elementary. Otherwise, there will be a significant burden on > > the person who implements the next SASL method (i.e., you ;-) ) to > > figure that out then. > > Sounds good, I'll work on that. v10 does not yet make changes in this area. v11 makes an attempt at this (see 0003), using the proposed string list. Personally I'm not happy with the amount of complexity it adds in exchange for flexibility we can't use yet. Maybe there's a way to simplify it, but I think the two-tiered approach of the patch has to remain, unless we find a way to move SASL mechanism selection to a different part of the code. I'm not sure that'd be helpful. Maybe I should just add a basic Assert here, to trip if someone adds a new SASL mechanism, and point that lucky person to this thread with a comment? --Jacob
Attachment
Hi Jacob, > v11 makes an attempt at this (see 0003), using the proposed string list. I noticed that this patchset stuck a bit so I decided to take a look. In 0001: ``` + conn->auth_required = false; + conn->allowed_auth_methods = -1; ... + uint32 allowed_auth_methods; /* bitmask of acceptable AuthRequest codes */ ``` Assigning a negative number to uint32 doesn't necessarily work on all platforms. I suggest using PG_UINT32_MAX. In 0002: ``` + <term><literal>require</literal></term> + <listitem> + <para> + the server <emphasis>must</emphasis> request a certificate. The + connection will fail if the server authenticates the client despite + not requesting or receiving one. ``` The commit message IMO has a better description of "require". I suggest adding the part about "This doesn't add any additional security ..." to the documentation. ``` + * hard-coded certificate via sslcert, so we don't actually set any certificates + * here; we just it to record whether or not the server has actually asked for ``` Something is off with the wording here in the "we just it to ..." part. The patchset seems to be in very good shape except for these few nitpicks. I'm inclined to change its status to "Ready for Committer" as soon as the new version will pass cfbot unless there are going to be any objections from the community. -- Best regards, Aleksander Alekseev
On 11/11/22 05:52, Aleksander Alekseev wrote: > I noticed that this patchset stuck a bit so I decided to take a look. Thanks! > Assigning a negative number to uint32 doesn't necessarily work on all > platforms. I suggest using PG_UINT32_MAX. Hmm -- on which platforms is "-1 converted to unsigned" not equivalent to the maximum value? Are they C-compliant? > The commit message IMO has a better description of "require". I > suggest adding the part about "This doesn't add any additional > security ..." to the documentation. Sounds good; see what you think of v12. > ``` > + * hard-coded certificate via sslcert, so we don't actually set any > certificates > + * here; we just it to record whether or not the server has actually asked for > ``` > > Something is off with the wording here in the "we just it to ..." part. Fixed. > The patchset seems to be in very good shape except for these few > nitpicks. I'm inclined to change its status to "Ready for Committer" > as soon as the new version will pass cfbot unless there are going to > be any objections from the community. Thank you! I expect a maintainer will need to weigh in on the cost/benefit of 0003 either way. --Jacob
Attachment
Hi Jacob, > > Assigning a negative number to uint32 doesn't necessarily work on all > > platforms. I suggest using PG_UINT32_MAX. > > Hmm -- on which platforms is "-1 converted to unsigned" not equivalent > to the maximum value? Are they C-compliant? I did a little more research and I think you are right. What happens according to the C standard: """ the value is converted to unsigned by adding to it one greater than the largest number that can be represented in the unsigned integer type """ so this is effectively -1 + (PG_UINT32_MAX + 1). -- Best regards, Aleksander Alekseev
On 11/11/22 22:57, Aleksander Alekseev wrote: > I did a little more research and I think you are right. What happens > according to the C standard: Thanks for confirming! (I personally prefer -1 to a *MAX macro, because it works regardless of the length of the type.) --Jacob
On Thu, Oct 20, 2022 at 11:36:34AM -0700, Jacob Champion wrote: > Maybe I should just add a basic Assert here, to trip if someone adds a > new SASL mechanism, and point that lucky person to this thread with a > comment? I am beginning to look at the last version proposed, which has been marked as RfC. Does this patch need a refresh in light of a9e9a9f and 0873b2d? The changes for libpq_append_conn_error() should be straight-forward. The CF bot is still happy. -- Michael
Attachment
On Tue, Nov 15, 2022 at 11:07 PM Michael Paquier <michael@paquier.xyz> wrote: > I am beginning to look at the last version proposed, which has been > marked as RfC. Does this patch need a refresh in light of a9e9a9f and > 0873b2d? The changes for libpq_append_conn_error() should be > straight-forward. Updated in v13, thanks! --Jacob
Attachment
On 16.11.22 18:26, Jacob Champion wrote: > On Tue, Nov 15, 2022 at 11:07 PM Michael Paquier <michael@paquier.xyz> wrote: >> I am beginning to look at the last version proposed, which has been >> marked as RfC. Does this patch need a refresh in light of a9e9a9f and >> 0873b2d? The changes for libpq_append_conn_error() should be >> straight-forward. > > Updated in v13, thanks! What is the status of this patch set? Michael had registered himself as committer and then removed himself again. So I hadn't been paying much attention myself. Was there anything left to discuss?
Hi Peter, > > Updated in v13, thanks! > > What is the status of this patch set? Michael had registered himself as > committer and then removed himself again. So I hadn't been paying much > attention myself. Was there anything left to discuss? Previously I marked the patch as RfC. Although it's been a few months ago and I don't recall all the details, it should have been in good shape (in my personal opinion at least). The commits a9e9a9f and 0873b2d Michael referred to are message refactorings so I doubt Jacob had serious problems with them. Of course, I'll take another fresh look and let you know my findings in a bit. -- Best regards, Aleksander Alekseev
Hi Peter, > > What is the status of this patch set? Michael had registered himself as > > committer and then removed himself again. So I hadn't been paying much > > attention myself. Was there anything left to discuss? > > Previously I marked the patch as RfC. Although it's been a few months > ago and I don't recall all the details, it should have been in good > shape (in my personal opinion at least). The commits a9e9a9f and > 0873b2d Michael referred to are message refactorings so I doubt Jacob > had serious problems with them. > > Of course, I'll take another fresh look and let you know my findings in a bit. The code is well written, documented and test-covered. All the tests pass. To my knowledge there are no open questions left. I think the patch is as good as it will ever get. -- Best regards, Aleksander Alekseev
On Tue, Jan 31, 2023 at 5:20 AM Aleksander Alekseev <aleksander@timescale.com> wrote: > To my knowledge there are no open questions left. I think the > patch is as good as it will ever get. A committer will need to decide whether they're willing to maintain 0003 or not, as mentioned with the v11 post. Which I suppose is the last open question, but not one I can answer from here. Thanks! --Jacob
On Tue, Jan 31, 2023 at 02:03:54PM +0300, Aleksander Alekseev wrote: >> What is the status of this patch set? Michael had registered himself as >> committer and then removed himself again. So I hadn't been paying much >> attention myself. Was there anything left to discuss? Yes, sorry about not following up on that. I was registered as such for a few weeks, but I have not been able to follow up. It did not seem fair for this patch to wait on only me, which is why I have removed my name, at least temporarily, so as somebody may be able to come back to it before me. I am not completely sure whether I will be able to come back and dive deeply into this thread soon, TBH :/ > Previously I marked the patch as RfC. Although it's been a few months > ago and I don't recall all the details, it should have been in good > shape (in my personal opinion at least). The commits a9e9a9f and > 0873b2d Michael referred to are message refactorings so I doubt Jacob > had serious problems with them. > > Of course, I'll take another fresh look and let you know my findings in a bit. (There were a few things around certificate handling that need careful consideration, at least that was my impression.) -- Michael
Attachment
v14 rebases over the test and solution conflicts from 9244c11afe2. Thanks, --Jacob
Attachment
On 2/16/23 10:57, Jacob Champion wrote: > v14 rebases over the test and solution conflicts from 9244c11afe2. Since we're to the final CF for PG16, here's a rough summary. This patchset provides two related features: 1) the ability for a client to explicitly allow or deny particular methods of in-band authentication (that is, things like password exchange), and 2) the ability to withhold a client certificate from a server that asks for it. Feature 1 was originally proposed to mitigate abuse where a successful MITM attack can then be used to fish for client credentials [1]. It also lets users disable undesirable authentication types (like plaintext) by default, which seems to be a common interest. Both features came up again in the context of proxies such as postgres_fdw, where it's sometimes important that users authenticate using only their credentials and not piggyback on the authority of the proxy host [2]. And another use case for feature 2 just came up independently [3], to fix connections where the default client certificate isn't valid for a particular server. Since this is all client-side, it's compatible with existing servers. Also since it's client-side, it can't prevent connections from being established by an eager server; it can only drop the connection once it sees that its requirement was not met, similar to how we handle target_session_attrs. That means it can't prevent a login trigger from being processed on behalf of a confused proxy. (I think that would require server-side support.) 0001 and 0002 are the core features. 0003 is a more future-looking refactoring of the internals, to make it easier to handle more SASL mechanisms, but it's not required and contains some unexercised code. Thanks, --Jacob [1] https://www.postgresql.org/message-id/fcc3ebeb7f05775b63f3207ed52a54ea5d17fb42.camel%40vmware.com [2] https://www.postgresql.org/message-id/20230123015255.h3jro3yyitlsqykp%40awork3.anarazel.de [3] https://www.postgresql.org/message-id/CAAWbhmh_QqCnRVV8ct3gJULReQjWxLTaTBqs%2BfV7c7FpH0zbew%40mail.gmail.com
On Tue, Feb 28, 2023 at 03:38:21PM -0800, Jacob Champion wrote: > 0001 and 0002 are the core features. 0003 is a more future-looking > refactoring of the internals, to make it easier to handle more SASL > mechanisms, but it's not required and contains some unexercised code. I was refreshing my mind with 0001 yesterday, and except for the two parts where we need to worry about AUTH_REQ_OK being sent too early and the business with gssenc, this is a rather straight-forward. It also looks like the the participants of the thread are OK with the design you are proposing (list of keywords, potentially negative patterns). I think that I can get this part merged for this CF, at least, not sure about the rest :p -- Michael
Attachment
On Fri, Mar 3, 2023 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote: > I was refreshing my mind with 0001 yesterday, and except for the two > parts where we need to worry about AUTH_REQ_OK being sent too early > and the business with gssenc, this is a rather straight-forward. It > also looks like the the participants of the thread are OK with the > design you are proposing (list of keywords, potentially negative > patterns). I think that I can get this part merged for this CF, at > least, not sure about the rest :p Thanks! Is there anything that would make the sslcertmode patch more palatable? Or any particular areas of concern? --Jacob
On Mon, Mar 06, 2023 at 04:02:25PM -0800, Jacob Champion wrote:
> On Fri, Mar 3, 2023 at 6:35 PM Michael Paquier <michael@paquier.xyz> wrote:
>> I was refreshing my mind with 0001 yesterday, and except for the two
>> parts where we need to worry about AUTH_REQ_OK being sent too early
>> and the business with gssenc, this is a rather straight-forward. It
>> also looks like the the participants of the thread are OK with the
>> design you are proposing (list of keywords, potentially negative
>> patterns). I think that I can get this part merged for this CF, at
>> least, not sure about the rest :p
>
> Thanks! Is there anything that would make the sslcertmode patch more
> palatable? Or any particular areas of concern?
I have been reviewing 0001, finishing with the attached, and that's
nice work. My notes are below.
pqDropServerData() is in charge of cleaning up the transient data of a
connection between different attempts. Shouldn't client_finished_auth
be reset to false there? No parameters related to the connection
parameters should be reset in this code path, but this state is
different. It does not seem possible that we could reach
pqDropServerData() after client_finished_auth has been set to true,
but that feels safer. I was tempted first to do that as well in
makeEmptyPGconn(), but we do a memset(0) there, so there is no point
in doing that anyway ;)
require_auth needs a cleanup in freePGconn().
+ case AUTH_REQ_SCM_CREDS:
+ return libpq_gettext("server requested UNIX socket credentials");
I am not really cool with the fact that this would fail and that we
offer no options to control that. Now, this involves servers up to
9.1, which is also a very good to rip of this code entirely. For now,
I think that we'd better allow this option, and discuss the removal of
that in a separate thread.
pgindent has been complaining on the StaticAssertDecl() in fe-auth.c:
src/interfaces/libpq/fe-auth.c: Error@847: Unbalanced parens
Warning@847: Extra )
Warning@847: Extra )
Warning@848: Extra )
From what I can see, this comes from the use of {0} within the
expression itself. I don't really want to dig into why pg_bsd_indent
thinks this is a bad idea, so let's just move the StaticAssertDecl() a
bit, like in the attached. The result is the same.
As of the "sensitive" cases of the patch:
- I don't really think that we have to care much of the cases like
"none,scram" meaning that a SASL exchange hastily downgraded to
AUTH_REQ_OK by the server would be a success, as "none" means that the
client is basically OK with trust-level. This said, "none" could be a
dangerous option in some cases, while useful in others.
- SSPI is the default connection setup for the TAP tests on Windows.
We could stick a small test somewhere, perhaps, certainly not in
src/test/authentication/.
- SASL/SCRAM is indeed a problem on its own. My guess is that we
should let channel_binding do the job for SASL, or introduce a new
option to decide which sasl mechanisms are authorized. At the end,
using "scram-sha-256" as the keyword is fine by me as we use that even
for HBA files, so that's quite known now, I hope.
--
Michael
Attachment
On 3/8/23 22:35, Michael Paquier wrote:
> I have been reviewing 0001, finishing with the attached, and that's
> nice work. My notes are below.
Thanks!
> pqDropServerData() is in charge of cleaning up the transient data of a
> connection between different attempts. Shouldn't client_finished_auth
> be reset to false there? No parameters related to the connection
> parameters should be reset in this code path, but this state is
> different. It does not seem possible that we could reach
> pqDropServerData() after client_finished_auth has been set to true,
> but that feels safer.
Yeah, that seems reasonable.
> + case AUTH_REQ_SCM_CREDS:
> + return libpq_gettext("server requested UNIX socket credentials");
> I am not really cool with the fact that this would fail and that we
> offer no options to control that. Now, this involves servers up to
> 9.1, which is also a very good to rip of this code entirely. For now,
> I think that we'd better allow this option, and discuss the removal of
> that in a separate thread.
Fair enough.
> pgindent has been complaining on the StaticAssertDecl() in fe-auth.c:
> src/interfaces/libpq/fe-auth.c: Error@847: Unbalanced parens
> Warning@847: Extra )
> Warning@847: Extra )
> Warning@848: Extra )
>
> From what I can see, this comes from the use of {0} within the
> expression itself. I don't really want to dig into why pg_bsd_indent
> thinks this is a bad idea, so let's just move the StaticAssertDecl() a
> bit, like in the attached. The result is the same.
Works for me. I wonder if
sizeof(((PGconn*) 0)->allowed_auth_methods)
would make pgindent any happier? That'd let you keep the assertion local
to auth_method_allowed, but it looks scarier. :)
> As of the "sensitive" cases of the patch:
> - I don't really think that we have to care much of the cases like
> "none,scram" meaning that a SASL exchange hastily downgraded to
> AUTH_REQ_OK by the server would be a success, as "none" means that the
> client is basically OK with trust-level. This said, "none" could be a
> dangerous option in some cases, while useful in others.
Yeah. I think a server shouldn't be allowed to abandon a SCRAM exchange
partway through, but that's completely independent of this patchset.
> - SSPI is the default connection setup for the TAP tests on Windows.
Oh, I don't think I ever noticed that.
> We could stick a small test somewhere, perhaps, certainly not in
> src/test/authentication/.
Where were you thinking? (Would it be so bad to have a tiny
t/005_sspi.pl that's just skipped on *nix?)
> - SASL/SCRAM is indeed a problem on its own. My guess is that we
> should let channel_binding do the job for SASL, or introduce a new
> option to decide which sasl mechanisms are authorized. At the end,
> using "scram-sha-256" as the keyword is fine by me as we use that even
> for HBA files, so that's quite known now, I hope.
Did you have any thoughts about the 0003 generalization attempt?
> -+ if (conn->require_auth)
> ++ if (conn->require_auth && conn->require_auth[0])
Thank you for that catch. I guess we should test somewhere that
`require_auth=` behaves normally?
> + reason = libpq_gettext("server did not complete authentication"),
> -+ result = false;
> ++ result = false;
> + }
This reindentation looks odd.
nit: some of the new TAP test names have been rewritten with commas,
others with colons.
Thanks,
--Jacob
On Fri, Mar 10, 2023 at 02:32:17PM -0800, Jacob Champion wrote:
> On 3/8/23 22:35, Michael Paquier wrote:
> Works for me. I wonder if
>
> sizeof(((PGconn*) 0)->allowed_auth_methods)
>
> would make pgindent any happier? That'd let you keep the assertion local
> to auth_method_allowed, but it looks scarier. :)
I can check that, now it's not bad to keep the assertion as it is,
either.
>> As of the "sensitive" cases of the patch:
>> - I don't really think that we have to care much of the cases like
>> "none,scram" meaning that a SASL exchange hastily downgraded to
>> AUTH_REQ_OK by the server would be a success, as "none" means that the
>> client is basically OK with trust-level. This said, "none" could be a
>> dangerous option in some cases, while useful in others.
>
> Yeah. I think a server shouldn't be allowed to abandon a SCRAM exchange
> partway through, but that's completely independent of this patchset.
Agreed.
>> We could stick a small test somewhere, perhaps, certainly not in
>> src/test/authentication/.
>
> Where were you thinking? (Would it be so bad to have a tiny
> t/005_sspi.pl that's just skipped on *nix?)
Hmm, OK. It may be worth having a 005_sspi.pl in
src/test/authentication/ specifically for Windows. This patch gives
at least one reason to do so. Looking at pg_regress.c, we have that:
if (config_auth_datadir)
{
#ifdef ENABLE_SSPI
if (!use_unix_sockets)
config_sspi_auth(config_auth_datadir, user);
#endif
exit(0);
}
So applying a check on $use_unix_sockets should be OK, I hope.
>> - SASL/SCRAM is indeed a problem on its own. My guess is that we
>> should let channel_binding do the job for SASL, or introduce a new
>> option to decide which sasl mechanisms are authorized. At the end,
>> using "scram-sha-256" as the keyword is fine by me as we use that even
>> for HBA files, so that's quite known now, I hope.
>
> Did you have any thoughts about the 0003 generalization attempt?
Not yet, unfortunately.
> > -+ if (conn->require_auth)
> > ++ if (conn->require_auth && conn->require_auth[0])
>
> Thank you for that catch. I guess we should test somewhere that
> `require_auth=` behaves normally?
Yeah, that seems like an idea. That would be cheap enough.
>> + reason = libpq_gettext("server did not complete authentication"),
>> -+ result = false;
>> ++ result = false;
>> + }
>
> This reindentation looks odd.
That's because the previous line has a comma. So the reindent is
right, not the code.
> nit: some of the new TAP test names have been rewritten with commas,
> others with colons.
Indeed, I thought to have caught all of them, but you wrote a lot of
tests :)
Could you send a new patch with all these adjustments? That would
help a lot.
--
Michael
Attachment
On Fri, Mar 10, 2023 at 3:09 PM Michael Paquier <michael@paquier.xyz> wrote:
> >> + reason = libpq_gettext("server did not complete authentication"),
> >> -+ result = false;
> >> ++ result = false;
> >> + }
> >
> > This reindentation looks odd.
>
> That's because the previous line has a comma. So the reindent is
> right, not the code.
Whoops. :(
> Could you send a new patch with all these adjustments? That would
> help a lot.
Will do!
Thanks,
--Jacob
On Fri, Mar 10, 2023 at 3:16 PM Jacob Champion <jchampion@timescale.com> wrote: > > Could you send a new patch with all these adjustments? That would > > help a lot. > > Will do! Here's a v16: - updated 0001 patch message - all test names should have commas rather than colons now - new test for an empty require_auth - new SSPI suite (note that it doesn't run by default on Cirrus, due to the use of PG_TEST_USE_UNIX_SOCKETS) - fixed errant comma at EOL Thanks, --Jacob
Attachment
On Mon, Mar 13, 2023 at 12:38:10PM -0700, Jacob Champion wrote: > Here's a v16: > - updated 0001 patch message > - all test names should have commas rather than colons now > - new test for an empty require_auth > - new SSPI suite (note that it doesn't run by default on Cirrus, due > to the use of PG_TEST_USE_UNIX_SOCKETS) > - fixed errant comma at EOL 0001 was looking fine enough seen from here, so applied it after tweaking a few comments. That's enough to cover most of the needs of this thread. 0002 looks pretty simple as well, I think that's worth a look for this CF. I am not sure about 0003, to be honest, as I am wondering if there could be a better solution than tying more the mechanism names with the expected AUTH_REQ_* values.. -- Michael
Attachment
On Mon, Mar 13, 2023 at 10:39 PM Michael Paquier <michael@paquier.xyz> wrote: > 0001 was looking fine enough seen from here, so applied it after > tweaking a few comments. That's enough to cover most of the needs of > this thread. Thank you very much! > 0002 looks pretty simple as well, I think that's worth a look for this > CF. Cool. v17 just rebases the set over HEAD, then, for cfbot. > I am not sure about 0003, to be honest, as I am wondering if > there could be a better solution than tying more the mechanism names > with the expected AUTH_REQ_* values.. Yeah, I'm not particularly excited about the approach I took. It'd be easier if we had a second SASL method to verify the implementation... I'd also proposed just adding an Assert, as a third option, to guide the eventual SASL implementer back to this conversation? --Jacob
Attachment
On Tue, Mar 14, 2023 at 12:14:40PM -0700, Jacob Champion wrote:
> On Mon, Mar 13, 2023 at 10:39 PM Michael Paquier <michael@paquier.xyz> wrote:
>> 0002 looks pretty simple as well, I think that's worth a look for this
>> CF.
>
> Cool. v17 just rebases the set over HEAD, then, for cfbot.
I have looked at 0002, and I am on board with using a separate
connection parameter for this case, orthogonal to require_auth, with
the three value "allow", "disable" and "require". So that's one thing
:)
- # Function introduced in OpenSSL 1.0.2.
+ # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all of these.
['X509_get_signature_nid'],
+ ['SSL_CTX_set_cert_cb'],
From what I can see, X509_get_signature_nid() is in LibreSSL, but not
SSL_CTX_set_cert_cb(). Perhaps that's worth having two different
comments?
+ <para>
+ a certificate may be sent, if the server requests one and it has
+ been provided via <literal>sslcert</literal>
+ </para>
It seems to me that this description is not completely exact. The
default is to look at ~/.postgresql/postgresql.crt, so sslcert is not
mandatory. There could be a certificate even without sslcert set.
+ libpq_append_conn_error(conn, "sslcertmode value \"%s\" invalid when SSL support is not compiled in",
+ conn->sslcertmode);
This string could be combined with the same one used for sslmode,
saving a bit in translation effortm by making the connection parameter
name a value of the string ("%s value \"%s\" invalid .."). The second
string where HAVE_SSL_CTX_SET_CERT_CB is not set could be refactored
the same way, I guess.
+ * figure out if a certficate was actually requested, so "require" is
s/certficate/certificate/.
contrib/sslinfo/ has ssl_client_cert_present(), that we could use in
the tests to make sure that the client has actually sent a
certificate? How about adding some of these tests to 003_sslinfo.pl
for the "allow" and "require" cases? Even for "disable", we could
check check that ssl_client_cert_present() returns false? That would
make four tests if everything is covered:
- "allow" without a certificate sent.
- "allow" with a certificate sent.
- "disable".
- "require"
+ if (!conn->ssl_cert_requested)
+ {
+ libpq_append_conn_error(conn, "server did not request a certificate");
+ return false;
+ }
+ else if (!conn->ssl_cert_sent)
+ {
+ libpq_append_conn_error(conn, "server accepted connection without a valid certificate");
+ return false;
+ }
Perhaps useless question: should this say "SSL certificate"?
freePGconn() is missing a free(sslcertmode).
--
Michael
Attachment
On Tue, Mar 21, 2023 at 11:01 PM Michael Paquier <michael@paquier.xyz> wrote:
> - # Function introduced in OpenSSL 1.0.2.
> + # Functions introduced in OpenSSL 1.0.2. LibreSSL doesn't have all of these.
> ['X509_get_signature_nid'],
> + ['SSL_CTX_set_cert_cb'],
>
> From what I can see, X509_get_signature_nid() is in LibreSSL, but not
> SSL_CTX_set_cert_cb(). Perhaps that's worth having two different
> comments?
I took a stab at that in v18. I diverged a bit between Meson and
Autoconf, which you may not care for.
> + <para>
> + a certificate may be sent, if the server requests one and it has
> + been provided via <literal>sslcert</literal>
> + </para>
>
> It seems to me that this description is not completely exact. The
> default is to look at ~/.postgresql/postgresql.crt, so sslcert is not
> mandatory. There could be a certificate even without sslcert set.
Reworded.
> + libpq_append_conn_error(conn, "sslcertmode value \"%s\" invalid when SSL support is not compiled in",
> + conn->sslcertmode);
>
> This string could be combined with the same one used for sslmode,
> saving a bit in translation effortm by making the connection parameter
> name a value of the string ("%s value \"%s\" invalid ..").
Done.
> + * figure out if a certficate was actually requested, so "require" is
> s/certficate/certificate/.
Heh, fixed. I need new glasses, clearly.
> contrib/sslinfo/ has ssl_client_cert_present(), that we could use in
> the tests to make sure that the client has actually sent a
> certificate? How about adding some of these tests to 003_sslinfo.pl
> for the "allow" and "require" cases?
Added; see what you think.
> + if (!conn->ssl_cert_requested)
> + {
> + libpq_append_conn_error(conn, "server did not request a certificate");
> + return false;
> + }
> + else if (!conn->ssl_cert_sent)
> + {
> + libpq_append_conn_error(conn, "server accepted connection without a valid certificate");
> + return false;
> + }
> Perhaps useless question: should this say "SSL certificate"?
I have no objection, so done that way.
> freePGconn() is missing a free(sslcertmode).
Argh, I keep forgetting that. Fixed, thanks!
--Jacob
Attachment
On Thu, Mar 23, 2023 at 03:40:55PM -0700, Jacob Champion wrote: > On Tue, Mar 21, 2023 at 11:01 PM Michael Paquier <michael@paquier.xyz> wrote: >> contrib/sslinfo/ has ssl_client_cert_present(), that we could use in >> the tests to make sure that the client has actually sent a >> certificate? How about adding some of these tests to 003_sslinfo.pl >> for the "allow" and "require" cases? > > Added; see what you think. That's a pretty good test design, covering all 4 cases. Nice. >> freePGconn() is missing a free(sslcertmode). > > Argh, I keep forgetting that. Fixed, thanks! I have spent a couple of hours looking at the whole again today, testing that with OpenSSL to make sure that everything was OK. Apart from a few tweaks, that seemed pretty good. So, applied. -- Michael
Attachment
On Thu, Mar 23, 2023 at 10:18 PM Michael Paquier <michael@paquier.xyz> wrote: > I have spent a couple of hours looking at the whole again today, > testing that with OpenSSL to make sure that everything was OK. Apart > from a few tweaks, that seemed pretty good. So, applied. Thank you! --Jacob
On Fri, Mar 24, 2023 at 09:30:06AM -0700, Jacob Champion wrote: > On Thu, Mar 23, 2023 at 10:18 PM Michael Paquier <michael@paquier.xyz> wrote: >> I have spent a couple of hours looking at the whole again today, >> testing that with OpenSSL to make sure that everything was OK. Apart >> from a few tweaks, that seemed pretty good. So, applied. > > Thank you! Please note that the CF entry has been marked as committed. We should really do something about having a cleaner separation between SASL, the mechanisms and the AUTH_REQ_* codes, in the long term, though honestly I don't know yet what would be the most elegant and the least error-prone approach. And for anything that touches authentication, simpler means better. -- Michael