Обсуждение: Add "password_protocol" connection parameter to libpq
Libpq doesn't have a way to control which password protocols are used. For example, the client might expect the server to be using SCRAM, but it actually ends up using plain password authentication instead. This patch adds: password_protocol = {plaintext|md5|scram-sha-256|scram-sha-256-plus} as a connection parameter. Libpq will then reject any authentication request from the server that is less secure than this setting. Setting it to "plaintext" (default) will answer to any kind of authentication request. I'm not 100% happy with the name "password_protocol", but other names I could think of seemed likely to cause confusion. Regards, Jeff Davis
Вложения
On Thu, Aug 08, 2019 at 03:38:20PM -0700, Jeff Davis wrote: > Libpq doesn't have a way to control which password protocols are used. > For example, the client might expect the server to be using SCRAM, but > it actually ends up using plain password authentication instead. Thanks for working on this! > I'm not 100% happy with the name "password_protocol", but other names I > could think of seemed likely to cause confusion. What about auth_protocol then? It seems to me that it could be useful to have the restriction on AUTH_REQ_MD5 as well. > Sets the least-secure password protocol allowable when using password > authentication. Options are: "plaintext", "md5", "scram-sha-256", or > "scram-sha-256-plus". This makes it sound like there is a linear hierarchy among all those protocols, which is true in this case, but if the list of supported protocols is extended in the future it may be not. I think that this should have TAP tests in src/test/authentication/ so as we make sure of the semantics. For the channel-binding part, the logic path for the test would be src/test/ssl. +#define DefaultPasswordProtocol "plaintext" I think that we are going to need another default value for that, like "all" to reduce the confusion that SCRAM, MD5 and co are still included in the authorized set in this case. Another thing that was discussed on the topic would be to allow a list of authorized protocols instead. I personally don't think that we need to go necessarily this way, but it could make the integration of things line scram-sha-256,scram-sha-256-plus easier to integrate in application flows. -- Michael
Вложения
On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote: > What about auth_protocol then? It seems to me that it could be > useful > to have the restriction on AUTH_REQ_MD5 as well. auth_protocol does sound like a good name. I'm not sure what you mean regarding MD5 though. > This makes it sound like there is a linear hierarchy among all those > protocols, which is true in this case, but if the list of supported > protocols is extended in the future it may be not. We already have that concept to a lesser extent, with the md5 authentication method also permitting scram-sha-256. Also note that the server chooses what kind of authentication request to send, which imposes a hierarchy of password < md5 < sasl. Within the sasl authentication request the server can advertise multiple supported mechanisms, though, so there doesn't need to be a hierarchy among sasl mechanisms. > I think that this should have TAP tests in src/test/authentication/ > so > as we make sure of the semantics. For the channel-binding part, the > logic path for the test would be src/test/ssl. Will do. > Another thing that was discussed on the topic would be to allow a > list > of authorized protocols instead. I personally don't think that we > need to go necessarily this way, but it could make the integration of > things line scram-sha-256,scram-sha-256-plus easier to integrate in > application flows. That sounds good, but there are a lot of possibilities and I can't quite decide which way to go. We could expose it as an SASL option like: saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha- 256-plus} But that doesn't allow for multiple acceptable mechanisms, which could make migration a pain. We try to use a comma-list like: saslmode = {disable|prefer|require} saslmech = all | {scram-hash-256|scram-hash-256-plus}[,...] Or we could over-engineer it to do something like: saslmode = {disable|prefer|require} saslmech = all | {scram|future_mech}[,...] scramhash = all | {sha-256|future_hash}[,...] scram_channel_binding = {disable|prefer|require} (Aside: is the channel binding only a SCRAM concept, or also a SASL concept?) Also, working with libpq I found myself wondering why everything is based on strings instead of enums or some other structure. Do you know why it's done that way? Regards, Jeff Davis
On Thu, Aug 08, 2019 at 11:16:24PM -0700, Jeff Davis wrote: > On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote: > > What about auth_protocol then? It seems to me that it could be > > useful > > to have the restriction on AUTH_REQ_MD5 as well. > > auth_protocol does sound like a good name. I'm not sure what you mean > regarding MD5 though. Sorry, I meant krb5 here. > We already have that concept to a lesser extent, with the md5 > authentication method also permitting scram-sha-256. That's present to ease upgrades, and once the AUTH_REQ part is received the client knows what it needs to go through. > That sounds good, but there are a lot of possibilities and I can't > quite decide which way to go. > > We could expose it as an SASL option like: > > saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha- > 256-plus} Or we could shape password_protocol so as it takes a list of protocols, as a white list of authorized things in short. -- Michael
Вложения
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Thu, Aug 08, 2019 at 11:16:24PM -0700, Jeff Davis wrote: > > On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote: > > > What about auth_protocol then? It seems to me that it could be > > > useful > > > to have the restriction on AUTH_REQ_MD5 as well. > > > > auth_protocol does sound like a good name. I'm not sure what you mean > > regarding MD5 though. I don't really care for auth_protocol as that's pretty close to "auth_method" and that isn't what we're talking about here- this isn't the user picking the auth method, per se, but rather saying which of the password-based mechanisms for communicating that the user knows the password is acceptable. Letting users choose which auth methods are allowed might also be interesting (as in- we are in a Kerberized environment and therefore no client should ever be using any auth method except GSS, could be a reasonable ask) but it's not the same thing. > Sorry, I meant krb5 here. What restriction are you suggesting here wrt krb5..? > > We already have that concept to a lesser extent, with the md5 > > authentication method also permitting scram-sha-256. > > That's present to ease upgrades, and once the AUTH_REQ part is > received the client knows what it needs to go through. I don't think there's any question that, of the methods available to prove that you know what the password is, simply sending the password to the server as cleartext is the least secure. If I, as a user, decide that I don't really care what method is used, it's certainly simpler to just say 'plaintext' than to have to list out every possible option. Having an 'any' option, as mentioned before, could be an alternative though. I agree with the point that there isn't any guarantee that it'll always be clear-cut as to which of two methods is "better". From a user perspective, it seems like the main things are "don't send my password in the clear to the server", and "require channel binding to prove there isn't a MITM". I have to admit that I like the idea of requiring scram to be used and not allowing md5 though. > > That sounds good, but there are a lot of possibilities and I can't > > quite decide which way to go. > > > > We could expose it as an SASL option like: > > > > saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha- > > 256-plus} > > Or we could shape password_protocol so as it takes a list of > protocols, as a white list of authorized things in short. I'm not really a fan of "saslmode" or anything else involving SASL for this because we don't *really* do SASL- if we did, we'd have that as an auth method and we'd have our Kerberos support be through SASL along with potentially everything else... I'm not against going there but I don't think that's what you were suggesting here. Thanks, Stephen
Вложения
On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote: > Having an 'any' option, as mentioned before, could be an alternative > though. ... > I agree with the point that there isn't any guarantee that it'll > always > be clear-cut as to which of two methods is "better". > > From a user perspective, it seems like the main things are "don't > send > my password in the clear to the server", and "require channel binding > to > prove there isn't a MITM". I have to admit that I like the idea of > requiring scram to be used and not allowing md5 though. So it seems like we are leaning toward: password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha- 256-plus}[,...] Or maybe: channel_binding = {disable|prefer|require} password_plaintext = {disable|enable} password_md5 = {disable|enable} That seems reasonable. It's three options, but no normal use case would need to set more than two, because channel binding forces scram-sha- 256-plus. Regards, Jeff Davis
On 8/9/19 11:51 AM, Jeff Davis wrote: > On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote: >> Having an 'any' option, as mentioned before, could be an alternative >> though. > > ... > >> I agree with the point that there isn't any guarantee that it'll >> always >> be clear-cut as to which of two methods is "better". >> >> From a user perspective, it seems like the main things are "don't >> send >> my password in the clear to the server", and "require channel binding >> to >> prove there isn't a MITM". I have to admit that I like the idea of >> requiring scram to be used and not allowing md5 though. > > So it seems like we are leaning toward: > > password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha- > 256-plus}[,...] First, thanks for proposing / working on this, I like the idea! :) Happy to test/review. As long as this one can handle the current upgrade path that's in place for going from md5 to SCRAM (which AIUI it should) this makes sense to me. As stated above, there is a clear hierarchy. I would almost argue that "plaintext" shouldn't even be an option...if you have "any" set (arguably default?) then plaintext is available. With our currently supported versions + driver ecosystem, I hope no one needs to support a forced plaintext setup. > > Or maybe: > > channel_binding = {disable|prefer|require} > password_plaintext = {disable|enable} > password_md5 = {disable|enable} > > That seems reasonable. It's three options, but no normal use case would > need to set more than two, because channel binding forces scram-sha- > 256-plus. Seems to be a lot to configure. I'm more of a fan of the previous method; it'd work nicely with how we've presently defined things and should be easy to put into a DSN/URI/env variable. Thanks, Jonathan
Вложения
On 09/08/2019 23:27, Jonathan S. Katz wrote: > On 8/9/19 11:51 AM, Jeff Davis wrote: >> On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote: >>> Having an 'any' option, as mentioned before, could be an alternative >>> though. >> >> ... >> >>> I agree with the point that there isn't any guarantee that it'll >>> always >>> be clear-cut as to which of two methods is "better". >>> >>> From a user perspective, it seems like the main things are "don't >>> send >>> my password in the clear to the server", and "require channel binding >>> to >>> prove there isn't a MITM". I have to admit that I like the idea of >>> requiring scram to be used and not allowing md5 though. >> >> So it seems like we are leaning toward: >> >> password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha- >> 256-plus}[,...] > > First, thanks for proposing / working on this, I like the idea! :) Happy > to test/review. > > As long as this one can handle the current upgrade path that's in place > for going from md5 to SCRAM (which AIUI it should) this makes sense to > me. As stated above, there is a clear hierarchy. > > I would almost argue that "plaintext" shouldn't even be an option...if > you have "any" set (arguably default?) then plaintext is available. With > our currently supported versions + driver ecosystem, I hope no one needs > to support a forced plaintext setup. Keep in mind that RADIUS, LDAP and PAM authentication methods are 'plaintext' over the wire. It's not that bad, when used with sslmode=verify-ca/full. >> Or maybe: >> >> channel_binding = {disable|prefer|require} >> password_plaintext = {disable|enable} >> password_md5 = {disable|enable} >> >> That seems reasonable. It's three options, but no normal use case would >> need to set more than two, because channel binding forces scram-sha- >> 256-plus. > > Seems to be a lot to configure. I'm more of a fan of the previous > method; it'd work nicely with how we've presently defined things and > should be easy to put into a DSN/URI/env variable. This is a multi-dimensional problem. "channel_binding=require" is one way to prevent MITM attacks, but sslmode=verify-ca is another. (Does Kerberos also prevent MITM?) Or you might want to enable plaintext passwords over SSL, but not without SSL. I think we'll need something like the 'ssl_ciphers' GUC, where you can choose from a few reasonable default rules, but also enable/disable specific methods: # anything goes (the default) auth_methods = 'ANY' # Disable plaintext password authentication. Anything else is accepted. auth_methods = '-password' # Only authentication methods that protect from # Man-in-the-Middle attacks. This allows anything if the server's SSL # certificate can be verified, and otherwise only SCRAM with # channel binding auth_methods = 'MITM' # The same, but disable plaintext passwords and md5 altogether auth_methods = 'MITM, -password, -md5' I'm tempted to also allow 'SSL' and 'SSL-verify-full' as part of the same string, so that you could configure everything related to connection security in the same option. Not sure if that would make things simpler for users, or create more confusion. - Heikki
On Fri, 2019-08-09 at 16:27 -0400, Jonathan S. Katz wrote: > Seems to be a lot to configure. I'm more of a fan of the previous > method; it'd work nicely with how we've presently defined things and > should be easy to put into a DSN/URI/env variable. Proposals on the table: 1. Hierarchical semantics, where you specify the least-secure acceptable method: password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} 2. Comma-list approach, where you specify exactly which protocols are acceptable, or "any" to mean that we don't care. 3. three-setting approach: channel_binding = {disable|prefer|require} password_plaintext = {disable|enable} password_md5 = {disable|enable} It looks like Jonathan prefers #1. #1 seems direct and clearly applies today, and corresponds to auth methods on the server side. I'm not a fan of #2, it seems likely to result in a bunch of clients with overly-specific lists of things with long names that can never really go away. #3 is a little more abstract, but also seems more future-proof, and may tie in to what Stephen is talking about with respect to controlling auth methods from the client, or moving more protocols within SASL. Regards, Jeff Davis
On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote: > This is a multi-dimensional problem. "channel_binding=require" is > one > way to prevent MITM attacks, but sslmode=verify-ca is another. (Does > Kerberos also prevent MITM?) Or you might want to enable plaintext > passwords over SSL, but not without SSL. > > I think we'll need something like the 'ssl_ciphers' GUC, where you > can > choose from a few reasonable default rules, but also enable/disable > specific methods: .. > auth_methods = 'MITM, -password, -md5' Keep in mind this is client configuration, so something reasonable in postgresql.conf might not be so reasonable in the form: postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20- password%2C%20-md5 Another thing to consider is that there's less control configuring on the client than on the server. The server will send at most one authentication request based on its own rules, and all the client can do is either answer it, or disconnect. And the SSL stuff all happens before that, and won't use an authentication request message at all. Some protocols allow negotiation within them, like SASL, which gives the client a bit more freedom. But FE/BE doesn't allow for arbitrary subsets of authentication methods to be negoitated between client and server, so I'm worried trying to express it that way will just lead to clients that break when you upgrade your server. Regards, Jeff Davis
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote: > > auth_methods = 'MITM, -password, -md5' > > Keep in mind this is client configuration, so something reasonable in > postgresql.conf might not be so reasonable in the form: Yeah, that's a really good point. > postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20- > password%2C%20-md5 > > Another thing to consider is that there's less control configuring on > the client than on the server. The server will send at most one > authentication request based on its own rules, and all the client can > do is either answer it, or disconnect. And the SSL stuff all happens > before that, and won't use an authentication request message at all. Note that GSSAPI Encryption works the same as SSL in this regard. Thanks, Stephen
Вложения
On Fri, 9 Aug 2019 at 11:00, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Aug 08, 2019 at 03:38:20PM -0700, Jeff Davis wrote:
> Libpq doesn't have a way to control which password protocols are used.
> For example, the client might expect the server to be using SCRAM, but
> it actually ends up using plain password authentication instead.
Thanks for working on this!
> I'm not 100% happy with the name "password_protocol", but other names I
> could think of seemed likely to cause confusion.
What about auth_protocol then? It seems to me that it could be useful
to have the restriction on AUTH_REQ_MD5 as well.
> Sets the least-secure password protocol allowable when using password
> authentication. Options are: "plaintext", "md5", "scram-sha-256", or
> "scram-sha-256-plus".
This makes it sound like there is a linear hierarchy among all those
protocols, which is true in this case, but if the list of supported
protocols is extended in the future it may be not.
Before we go too far along with this, lets look at how other established protocols do things and the flaws that've been discovered in their approaches. If this isn't done with extreme care then there's a large risk of negating the benefits offered by adopting recent things like SCRAM. Frankly I kind of wish we could just use SASL, but there are many (many) reasons no to.
Off the top of my head I can think of these risks:
* Protocols that allow naïve pre-auth client/server auth negotiation (e.g. by finding the overlap in exchanged supported auth-mode lists) are subject to MiTM downgrade attacks where the attacker filters out protocols it cannot intercept and break from the proposed alternatives.
* Protocols that specify a hierarchy tend to be inflexible and result in hard to predict auth mode selections as the options grow. If my app wants GSSAPI or SuperStrongAuth but doesn't accept SSPI, and the hierarchy is GSSAPI > SSPI > SuperStrongAuth, it has to fall back to a disconnect and retry model like now.
* Protocols that announce supported auth methods before any kind of trust is established make life easier for vulnerability scanners and worms
and I'm sure there are more when it comes to auth handshakes.
On Sat, 2019-08-10 at 10:24 +0800, Craig Ringer wrote: > Before we go too far along with this, lets look at how other > established protocols do things and the flaws that've been discovered > in their approaches. If this isn't done with extreme care then > there's a large risk of negating the benefits offered by adopting > recent things like SCRAM. Agreed. I'm happy to hear any proposals better informed by history. > Frankly I kind of wish we could just use SASL, but there are many > (many) reasons no to. I'm curious what the reasons are not to use SASL; do you have a reference? > Off the top of my head I can think of these risks: > > * Protocols that allow naïve pre-auth client/server auth negotiation > (e.g. by finding the overlap in exchanged supported auth-mode lists) > are subject to MiTM downgrade attacks where the attacker filters out > protocols it cannot intercept and break from the proposed > alternatives. We already have the downgrade problem. That's what I'm trying to solve. > * Protocols that specify a hierarchy tend to be inflexible and result > in hard to predict auth mode selections as the options grow. If my > app wants GSSAPI or SuperStrongAuth but doesn't accept SSPI, and the > hierarchy is GSSAPI > SSPI > SuperStrongAuth, it has to fall back to > a disconnect and retry model like now. What do you mean "disconnect and retry model"? I agree that hierarchies are unweildly as the options grow. Then again, as options grow, we need new versions of the client to support them, and those new versions might offer more flexible ways to choose between them. Of course, we should try to think ahead to avoid needing to constantly change client connection syntax, but I'm just pointing out that it's not a one-way door. > * Protocols that announce supported auth methods before any kind of > trust is established make life easier for vulnerability scanners and > worms This discussion is about the client so I don't see how vulnerability scanners are relevant. Regards, Jeff Davis
On 8/9/19 7:54 PM, Jeff Davis wrote: > On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote: >> This is a multi-dimensional problem. "channel_binding=require" is >> one >> way to prevent MITM attacks, but sslmode=verify-ca is another. (Does >> Kerberos also prevent MITM?) Or you might want to enable plaintext >> passwords over SSL, but not without SSL. >> >> I think we'll need something like the 'ssl_ciphers' GUC, where you >> can >> choose from a few reasonable default rules, but also enable/disable >> specific methods: > > .. > >> auth_methods = 'MITM, -password, -md5' > > Keep in mind this is client configuration, so something reasonable in > postgresql.conf might not be so reasonable in the form: > > postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20- > password%2C%20-md5 Yeah, and I do agree it is a multi-dimensional problem, but the context in which I gave my opinion was for the password authentication methods that PostgreSQL supports natively, i.e. not requiring a 3rd party to arbitrate via GSSAPI, LDAP etc. That said, I dove into the code a bit more to look at the behavior specifically with LDAP, which as described does send back a request for "AuthenticationCleartextPassword" If we go with the client sending up a "password_protocol" that is not plaintext, and the server only provides LDAP authentication, does the client close the connection? I would say yes. (And as such, I would also consider adding "plaintext" back to the list, just to have the explicit option). The other question I have is that do we have it occur in the hierarchical manner, i.e. "md5 or better?" I would also say yes to that, we would just need to clearly document that. Perhaps we adopt a similar name to "sslmode" e.g. "password_protocol_mode" but that can be debated :) > Another thing to consider is that there's less control configuring on > the client than on the server. The server will send at most one > authentication request based on its own rules, and all the client can > do is either answer it, or disconnect. And the SSL stuff all happens > before that, and won't use an authentication request message at all. Yes. Using the LDAP example above, the client also needs some general awareness of how it can connect to the server, e.g. "You may want scram-sha-256 but authentication occurs over LDAP, so don't stop requesting scram-sha-256!" That said, part of that is a human problem: it's up to the server administrator to inform clients how they can connect to PostgreSQL. > Some protocols allow negotiation within them, like SASL, which gives > the client a bit more freedom. But FE/BE doesn't allow for arbitrary > subsets of authentication methods to be negoitated between client and > server, so I'm worried trying to express it that way will just lead to > clients that break when you upgrade your server. Agreed. I see this as a way of a client saying "Hey, I really want to authenticate with scram-sha-256 or better, so if you don't let me do that, I'm out." In addition to ensuring it uses the client's desired password protocol, this could be helpful for testing that the appropriate authentication rules are set in a server, e.g. one that is rolling out SCRAM authentication. And as Heikki mentions, there are other protections a client can use, e.g. verify-ca/full, to guard against eavesdropping, MITM etc. Jonathan
Вложения
On 2019-08-09 23:56, Jeff Davis wrote: > 1. Hierarchical semantics, where you specify the least-secure > acceptable method: > > password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are added? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8/11/19 1:00 PM, Peter Eisentraut wrote: > On 2019-08-09 23:56, Jeff Davis wrote: >> 1. Hierarchical semantics, where you specify the least-secure >> acceptable method: >> >> password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} > > What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are > added? password_protocol = {any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}? I'd put one length of digest over another, but I'd still rank a method that uses channel binding has more protections than one that does not. Jonathan
Вложения
On 2019-08-11 21:46, Jonathan S. Katz wrote: > On 8/11/19 1:00 PM, Peter Eisentraut wrote: >> On 2019-08-09 23:56, Jeff Davis wrote: >>> 1. Hierarchical semantics, where you specify the least-secure >>> acceptable method: >>> >>> password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} >> >> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are >> added? > > password_protocol = > {any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}? > > I'd put one length of digest over another, but I'd still rank a method > that uses channel binding has more protections than one that does not. Sure, but the opposite opinion is also possible. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8/11/19 3:56 PM, Peter Eisentraut wrote: > On 2019-08-11 21:46, Jonathan S. Katz wrote: >> On 8/11/19 1:00 PM, Peter Eisentraut wrote: >>> On 2019-08-09 23:56, Jeff Davis wrote: >>>> 1. Hierarchical semantics, where you specify the least-secure >>>> acceptable method: >>>> >>>> password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} >>> >>> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are >>> added? >> >> password_protocol = >> {any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}? >> >> I'd put one length of digest over another, but I'd still rank a method >> that uses channel binding has more protections than one that does not. > > Sure, but the opposite opinion is also possible. That's true, and when originally started composing my note I had it as (256,512,256-plus,512-plus). But upon further reflection, the reason I ranked the digest-plus methods above the digest methods is that there is any additional requirement imposed by them. The digest methods could be invoked either with/without TLS, whereas the digest-plus methods *must* use TLS. As such, 256-plus is explicitly asking for an additional security parameter over 512, i.e. transmission over TLS, so even if it's a smaller digest, it has the additional channel binding requirement. Jonathan
Вложения
On Sun, 2019-08-11 at 19:00 +0200, Peter Eisentraut wrote: > On 2019-08-09 23:56, Jeff Davis wrote: > > 1. Hierarchical semantics, where you specify the least-secure > > acceptable method: > > > > password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus} > > What would the hierarchy be if scram-sha-512 and scram-sha-512-plus > are > added? https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com The weakness of proposal #1 is that it's not very "future-proof" and we would likely need to change something about it later when we support new methods. That wouldn't break clients, but it would be annoying to need to support some old syntax and some new syntax for the connection parameters. Proposal #3 does not have this weakness. When we add sha-512, we could also add a parameter to specify that the client requires a certain hash algorithm for SCRAM. Do you favor that existing proposal #3, or are you proposing a fourth option? Regards, Jeff Davis
On 2019-08-12 18:02, Jeff Davis wrote: > https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com > > The weakness of proposal #1 is that it's not very "future-proof" and we > would likely need to change something about it later when we support > new methods. That wouldn't break clients, but it would be annoying to > need to support some old syntax and some new syntax for the connection > parameters. > > Proposal #3 does not have this weakness. When we add sha-512, we could > also add a parameter to specify that the client requires a certain hash > algorithm for SCRAM. > > Do you favor that existing proposal #3, or are you proposing a fourth > option? In this context, I would prefer #2, but I would expand that to cover all authentication methods, not only password methods. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Greetings, * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 2019-08-12 18:02, Jeff Davis wrote: > > https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com > > > > The weakness of proposal #1 is that it's not very "future-proof" and we > > would likely need to change something about it later when we support > > new methods. That wouldn't break clients, but it would be annoying to > > need to support some old syntax and some new syntax for the connection > > parameters. > > > > Proposal #3 does not have this weakness. When we add sha-512, we could > > also add a parameter to specify that the client requires a certain hash > > algorithm for SCRAM. > > > > Do you favor that existing proposal #3, or are you proposing a fourth > > option? > > In this context, I would prefer #2, but I would expand that to cover all > authentication methods, not only password methods. I'm not really thrilled with approach #2 because it means the user will have to know which of the PG authentication methods involve, eg, sending the password in the clear to the server, and which don't, if what they're really looking for is "don't send my password in the clear to the server" which seems like a really useful and sensible thing to ask for. It also ends up not being very future-proof either, since a user who is fine with scram-sha-256-plus will probably also be ok with scram-sha-512-plus, should we ever implement it. Not to mention that, at least at the moment, we don't let users pick authentication methods with that kind of specificity on the server side (how do you require channel binding..?), so the set of "authentication methods" on the client side and those on the server side end up being different sets, which strikes me as awfully confusing... Or did I misunderstand what you were suggesting here wrt "all authentication methods"? Thanks, Stephen
Вложения
Stephen Frost <sfrost@snowman.net> writes: > I'm not really thrilled with approach #2 because it means the user > will have to know which of the PG authentication methods involve, eg, > sending the password in the clear to the server, and which don't, if > what they're really looking for is "don't send my password in the clear > to the server" which seems like a really useful and sensible thing to > ask for. What problem do we actually need to solve here? If the known use-case is just "don't send my password in the clear", maybe we should just change libpq to refuse to do that, ie reject plain-password auth methods unless SSL is on (except maybe over unix sockets?). Or invent a bool connection option that enables exactly that. I'm not really convinced that there is a use-case for client side specification of allowed auth methods beyond that. In the end, if you don't trust the server you're connecting to to handle your password with reasonable safety, you have got bigger problems than this one. And we already have coverage for MITM problems (or if we don't, this sideshow isn't fixing it). regards, tom lane
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > I'm not really thrilled with approach #2 because it means the user > > will have to know which of the PG authentication methods involve, eg, > > sending the password in the clear to the server, and which don't, if > > what they're really looking for is "don't send my password in the clear > > to the server" which seems like a really useful and sensible thing to > > ask for. > > What problem do we actually need to solve here? > > If the known use-case is just "don't send my password in the clear", > maybe we should just change libpq to refuse to do that, ie reject > plain-password auth methods unless SSL is on (except maybe over > unix sockets?). Or invent a bool connection option that enables > exactly that. Right, inventing a bool connection for it was discussed up-thread and seems like a reasonable idea to me (note: we should absolutely allow the user to refuse to send the password to the server even over SSL, if they would prefer to not do so). > I'm not really convinced that there is a use-case for client side > specification of allowed auth methods beyond that. In the end, > if you don't trust the server you're connecting to to handle your > password with reasonable safety, you have got bigger problems than > this one. And we already have coverage for MITM problems (or if > we don't, this sideshow isn't fixing it). Uh, no, we really don't have MITM protection in certain cases, which is exactly what channel-binding is intended to address, but we can't have the "server" be able to say "oh, well, I don't support channel binding" and have the client go "oh, ok, that's just fine, we won't use it then"- that's a downgrade attack. What was suggest up-thread to deal with that downgrade risk was a clear connection option along the lines of "require channel binding" to prevent that kind of a MITM downgrade attack from working. I could possibly see some value in a client-side option along the lines of "only authenticate using GSSAPI", which could prevent some users from being fooled into sending their PW to a malicious server. GSSAPI does prevent MITM attacks (as much as it's able to anyway- each key is specific to a particular server, so you'd have to have the specific server's key in order to become a MITM), but if the server says "we don't do GSSAPI, we do password, please give us your password" then psql will happily go along with that even in an otherwise properly set up GSSAPI environment. Requiring GSSAPI Encryption on the client side should prevent that though, as of v12, since psql will just refuse if the server claims to not support GSSAPI Encryption. Thanks, Stephen
Вложения
On 2019-08-12 19:26, Tom Lane wrote: > What problem do we actually need to solve here? > > If the known use-case is just "don't send my password in the clear", > maybe we should just change libpq to refuse to do that, ie reject > plain-password auth methods unless SSL is on (except maybe over > unix sockets?). Or invent a bool connection option that enables > exactly that. There are several overlapping problems: 1) A downgrade attack by a malicious server. The server can collect passwords from unsuspecting clients by just requesting some weak authentication like plain-text or md5. This can currently be worked around by using SSL with server verification, except when considering the kind of attack that channel binding is supposed to address. 2) A downgrade attack to evade channel binding. This cannot currently be worked around. 3) A user not wanting to expose a weakly hashed password to the (otherwise trusted) server. This cannot currently be done. 4) A user not wanting to send a password in plain text over the wire. This can currently be done by requiring SSL. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 09, 2019 at 09:28:50AM -0400, Stephen Frost wrote: > I don't really care for auth_protocol as that's pretty close to > "auth_method" and that isn't what we're talking about here- this isn't > the user picking the auth method, per se, but rather saying which of the > password-based mechanisms for communicating that the user knows the > password is acceptable. Letting users choose which auth methods are > allowed might also be interesting (as in- we are in a Kerberized > environment and therefore no client should ever be using any auth method > except GSS, could be a reasonable ask) but it's not the same thing. > > What restriction are you suggesting here wrt krb5..? What I suggested in this previous set of emails is if it would make sense to extend what libpq can restrict at authentication time to not only be password-based authentication methods, but also if we could have a connection parameter allowing us to say "please I want krb5/gss and nothing else". My point is that password-based authentication is only one portion of the problem as what we are looking at is applying a filtering on AUTH_REQ messages that libpq receives from the server (SCRAM with and without channel binding is an exception as that's handled as part of the SASL set of messages), but at a high level we are going to need a filtering of the first authentication message received anyway. But that's also basically what you outline in this previous paragraph of yours. -- Michael
Вложения
On Mon, Aug 12, 2019 at 07:05:08PM +0200, Peter Eisentraut wrote: > In this context, I would prefer #2, but I would expand that to cover all > authentication methods, not only password methods. I tend to prefer #2 as well and that's the kind of approach we were tending to agree on when we discussed this issue during the v11 beta for the downgrade issues with libpq. And as you say extend it so as we can apply filtering of more AUTH_REQ requests, inclusing GSS and krb5. -- Michael
Вложения
On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote: > I tend to prefer #2 as well and that's the kind of approach we were > tending to agree on when we discussed this issue during the v11 beta > for the downgrade issues with libpq. And as you say extend it so as > we can apply filtering of more AUTH_REQ requests, inclusing GSS and > krb5. Can you please offer a concrete proposal? I know the proposals I've put out aren't perfect (otherwise there wouldn't be three of them), so if you have something better, please share. Regards, Jeff Davis
On 8/13/19 12:25 PM, Jeff Davis wrote: > On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote: >> I tend to prefer #2 as well and that's the kind of approach we were >> tending to agree on when we discussed this issue during the v11 beta >> for the downgrade issues with libpq. And as you say extend it so as >> we can apply filtering of more AUTH_REQ requests, inclusing GSS and >> krb5. > > Can you please offer a concrete proposal? I know the proposals I've put > out aren't perfect (otherwise there wouldn't be three of them), so if > you have something better, please share. I think all of them get at the same thing, i.e. specifying which password protocol you want to use, and a lot of it is a matter of how much onus we want to put on the user. Back to the thee proposals[1], I've warmed up to #3 a bit. I do think it puts more onus on the client to set the correct knobs to get the desired outcome, but what I like is the specific `channel_binding=require` attribute. However, I don't think it's completely future proof to adding a new hash digest. If we wanted to prevent someone from using scram-sha-256 in a scram-sha-512 world, we'd likely need an option for that. Alternatively, we could combine 2 & 3, e.g.: channel_binding = {disable|prefer|require} # comma-separated list of protocols that are ok to the user, remove # ones you don't want. empty means all is ok password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256-plus" If the client selects "channel_binding=require" but does not include a protocol that supports it, we should error. Likewise, if the client does something like "channel_binding=require" and "password_protocol=scram-sha-256,scram-sha-256-plus" but the server refuses to do channel binding, we should error. I think this gives us both future-proofing against newer password digest methods + the fix for the downgrade issue. I would not be opposed to extending "password_protocol" to read "auth_protocol" or the like and work for everything covered in AUTH_REQ, but I would need to think about it some more. Thanks, Jonathan [1] https://www.postgresql.org/message-id/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com
Вложения
On Tue, 2019-08-13 at 16:51 -0400, Jonathan S. Katz wrote: > Alternatively, we could combine 2 & 3, e.g.: > > channel_binding = {disable|prefer|require} > > # comma-separated list of protocols that are ok to the user, remove > # ones you don't want. empty means all is ok > password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256- > plus" I still feel like lists are over-specifying things. Let me step back and offer an MVP of a single new parameter: channel_binding={prefer|require} And has a lot of benefits: * solves the immediate need to make channel binding useful, which is a really nice feature * compatible with most of the other proposals we're considering, so we can always extend it when we have a better understanding and consensus * clear purpose for the user * doesn't introduce new concepts that might be confusing to the user, like SASL or the use of "-plus" to mean "with channel binding" * guides users toward the good practice of using SSL and SCRAM * simple to implement The other use cases are less clear to me, and seem less urgent. Regards, Jeff Davis
On 8/13/19 6:25 PM, Jeff Davis wrote: > On Tue, 2019-08-13 at 16:51 -0400, Jonathan S. Katz wrote: >> Alternatively, we could combine 2 & 3, e.g.: >> >> channel_binding = {disable|prefer|require} >> >> # comma-separated list of protocols that are ok to the user, remove >> # ones you don't want. empty means all is ok >> password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256- >> plus" > > I still feel like lists are over-specifying things. Let me step back > and offer an MVP of a single new parameter: > > channel_binding={prefer|require} > > And has a lot of benefits: > * solves the immediate need to make channel binding useful, which > is a really nice feature > * compatible with most of the other proposals we're considering, so > we can always extend it when we have a better understanding and > consensus > * clear purpose for the user > * doesn't introduce new concepts that might be confusing to the > user, like SASL or the use of "-plus" to mean "with channel binding" > * guides users toward the good practice of using SSL and SCRAM > * simple to implement +1; I agree with your overall argument. The only thing I debate is if we want to have an explicit "disable" option. Looking at the negotiation standard[1] specified for channel binding with SCRAM, I don't think we need an explicit disable option. I can't think of any good use cases for "disable" off the top of my head either. The only thing is it would be consistent with some of our other parameters in terms of having an explicit "opt-out." Jonathan [1] https://tools.ietf.org/html/rfc5802#section-6
Вложения
On Tue, Aug 13, 2019 at 04:51:57PM -0400, Jonathan S. Katz wrote: > On 8/13/19 12:25 PM, Jeff Davis wrote: >> On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote: >>> I tend to prefer #2 as well and that's the kind of approach we were >>> tending to agree on when we discussed this issue during the v11 beta >>> for the downgrade issues with libpq. And as you say extend it so as >>> we can apply filtering of more AUTH_REQ requests, inclusing GSS and >>> krb5. >> >> Can you please offer a concrete proposal? I know the proposals I've put >> out aren't perfect (otherwise there wouldn't be three of them), so if >> you have something better, please share. > > I think all of them get at the same thing, i.e. specifying which > password protocol you want to use, and a lot of it is a matter of how > much onus we want to put on the user. What I got in mind was a comma-separated list of authorized protocols which can be specified as a connection parameter, which extends to all the types of AUTH_REQ requests that libpq can understand, plus an extra for channel binding. I also liked the idea mentioned upthread of "any" to be an alias to authorize everything, which should be the default. So you basically get at that: auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256-plus,krb5,gss,sspi} So from an implementation point of view, just using bit flags would make things rather clean. > Back to the thee proposals[1], I've warmed up to #3 a bit. I do think it > puts more onus on the client to set the correct knobs to get the desired > outcome, but what I like is the specific `channel_binding=require` > attribute. I could see a point in separating the channel binding part into a second parameter though. We don't have (at least yet) an hba option to allow only channel binding with scram, so a one-one mapping with the elements of the connection parameter brings some consistency. > If the client selects "channel_binding=require" but does not include a > protocol that supports it, we should error. Yep. > Likewise, if the client does > something like "channel_binding=require" and > "password_protocol=scram-sha-256,scram-sha-256-plus" but the server > refuses to do channel binding, we should error. If using a second parameter to control channel binding requirement, I don't think that there is any point in keeping scram-sha-256-plus in password_protocol. > I would not be opposed to extending "password_protocol" to read > "auth_protocol" or the like and work for everything covered in AUTH_REQ, > but I would need to think about it some more. And for this one I would like to push for not only having password-based methods considered. -- Michael
Вложения
On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote: > What I got in mind was a comma-separated list of authorized protocols > which can be specified as a connection parameter, which extends to > all > the types of AUTH_REQ requests that libpq can understand, plus an > extra for channel binding. I also liked the idea mentioned upthread > of "any" to be an alias to authorize everything, which should be the > default. So you basically get at that: > auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256- > plus,krb5,gss,sspi} What about something corresponding to the auth methods "trust" and "cert", where no authentication request is sent? That's a funny case, because the server trusts the client; but that doesn't imply that the client trusts the server. This is another reason I don't really like the list. It's impossible to make it cleanly map to the auth methods, and there are a few ways it could be confusing to the users. Given that we all pretty much agree on the need for the separate channel_binding param, the question is whether we want to (a) address additional use cases with specific parameters that also justify themselves; or (b) have a generic list that is supposed to solve many future use cases. I vote (a). With (b), the generic list is likely to cause more confusion, ugliness, and clients that break needlessly in the future. Regards, Jeff Davis
Greetings, * Jeff Davis (pgsql@j-davis.com) wrote: > On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote: > > What I got in mind was a comma-separated list of authorized protocols > > which can be specified as a connection parameter, which extends to > > all > > the types of AUTH_REQ requests that libpq can understand, plus an > > extra for channel binding. I also liked the idea mentioned upthread > > of "any" to be an alias to authorize everything, which should be the > > default. So you basically get at that: > > auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256- > > plus,krb5,gss,sspi} > > What about something corresponding to the auth methods "trust" and > "cert", where no authentication request is sent? That's a funny case, > because the server trusts the client; but that doesn't imply that the > client trusts the server. I agree that "trust" is odd. If you want my 2c, we should have to explicitly *enable* that for psql, otherwise there's the potential that a MITM could perform a downgrade attack to "trust" and while that might not expose a user's password, it could expose other information that the client ends up sending (INSERTs, UPDATEs, etc). When it comes to "cert"- there is certainly an authentication that happens and we already have options in psql/libpq to require validation of the server. If users want that, they should enable it (I wish we could make it the default too but that's a different discussion...). > This is another reason I don't really like the list. It's impossible to > make it cleanly map to the auth methods, and there are a few ways it > could be confusing to the users. I agree with these concerns, just to be clear. > Given that we all pretty much agree on the need for the separate > channel_binding param, the question is whether we want to (a) address > additional use cases with specific parameters that also justify > themselves; or (b) have a generic list that is supposed to solve many > future use cases. > > I vote (a). With (b), the generic list is likely to cause more > confusion, ugliness, and clients that break needlessly in the future. Admittedly, one doesn't preclude the other, and so we could move forward with the channel binding param, and that's fine- but I seriously hope that someone finds time to work on further improving the ability for clients to control what happens during authentication as this, imv anyway, is an area that we are weak in and it'd be great to improve on it. Thanks, Stephen
Вложения
On 8/15/19 9:28 PM, Stephen Frost wrote: > Greetings, > > * Jeff Davis (pgsql@j-davis.com) wrote: >> On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote: >>> What I got in mind was a comma-separated list of authorized protocols >>> which can be specified as a connection parameter, which extends to >>> all >>> the types of AUTH_REQ requests that libpq can understand, plus an >>> extra for channel binding. I also liked the idea mentioned upthread >>> of "any" to be an alias to authorize everything, which should be the >>> default. So you basically get at that: >>> auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256- >>> plus,krb5,gss,sspi} >> >> What about something corresponding to the auth methods "trust" and >> "cert", where no authentication request is sent? That's a funny case, >> because the server trusts the client; but that doesn't imply that the >> client trusts the server. > > I agree that "trust" is odd. If you want my 2c, we should have to > explicitly *enable* that for psql, otherwise there's the potential that > a MITM could perform a downgrade attack to "trust" and while that might > not expose a user's password, it could expose other information that the > client ends up sending (INSERTs, UPDATEs, etc). > > When it comes to "cert"- there is certainly an authentication that > happens and we already have options in psql/libpq to require validation > of the server. If users want that, they should enable it (I wish we > could make it the default too but that's a different discussion...). > >> This is another reason I don't really like the list. It's impossible to >> make it cleanly map to the auth methods, and there are a few ways it >> could be confusing to the users. > > I agree with these concerns, just to be clear. +1. > >> Given that we all pretty much agree on the need for the separate >> channel_binding param, the question is whether we want to (a) address >> additional use cases with specific parameters that also justify >> themselves; or (b) have a generic list that is supposed to solve many >> future use cases. >> >> I vote (a). With (b), the generic list is likely to cause more >> confusion, ugliness, and clients that break needlessly in the future. > > Admittedly, one doesn't preclude the other, and so we could move forward > with the channel binding param, and that's fine- but I seriously hope > that someone finds time to work on further improving the ability for > clients to control what happens during authentication as this, imv > anyway, is an area that we are weak in and it'd be great to improve on > it. To be pedantic, +1 on the channel_binding param. I do agree with option (a), but we should narrow down what that means for this iteration. I do see "password_protocol" making sense as a comma-separated list of options e.g. {plaintext, md5, scram-sha-256}. I would ask if scram-sha-256-plus makes the list if we have the channel_binding param? If channel_binding = require, it would essentially ignore any non-plus options in password_protocol and require scram-sha-256-plus. In a future scram-sha-512 world, then having scram-sha-256-plus or scram-sha-512-plus options in "password_protocol" then could be necessary based on what the user would prefer or require in their application. So if we do add a "password_protocol" parameter, then we likely need to include the -plus's. I think this is also fairly easy for a user to configure. Some scenarios scenarios I can see for this are: 1. The user requiring channel binding, so only "channel_binding=require" is set. 2. A PostgreSQL cluster transitioning between SCRAM + MD5, and the user setting password_protocol="scram-sha-256" to guarantee md5 auth does not take place. 3. A user wanting to ensure a stronger method is used, with some combination of the scram methods or md5, i.e. ensuring plaintext is not used. We would need to provide documentation around the types of password validation methods are used for the external validators (e.g. LDAP) so the user's known what to expect if their server is using those methods. Jonathan
Вложения
On Fri, Aug 16, 2019 at 02:11:57PM -0400, Jonathan S. Katz wrote: > To be pedantic, +1 on the channel_binding param. Seems like we are moving in this direction then. I don't object to the introduction of this parameter. We would likely want to do something for downgrade attacks in other cases where channel binding is not used, still there is verify-ca/full even in this case which offer similar protections for MITM and eavesdropping. > I would ask if scram-sha-256-plus makes the list if we have the > channel_binding param? No in my opinion. > If channel_binding = require, it would essentially ignore any non-plus > options in password_protocol and require scram-sha-256-plus. In a future > scram-sha-512 world, then having scram-sha-256-plus or > scram-sha-512-plus options in "password_protocol" then could be > necessary based on what the user would prefer or require in their > application. Not including scram-sha-512-plus or scram-sha-256-plus in the comma-separated list would be a problem for users willing to give for example scram-sha-256,scram-sha-512-plus as an authorized list of protocols but I don't think that it makes much sense as they basically require an SSL connection for tls-server-end-point per the second element. -- Michael
Вложения
On Mon, 2019-08-19 at 14:51 +0900, Michael Paquier wrote: > On Fri, Aug 16, 2019 at 02:11:57PM -0400, Jonathan S. Katz wrote: > > To be pedantic, +1 on the channel_binding param. > > Seems like we are moving in this direction then. I don't object to > the introduction of this parameter. OK, new patch attached. Seems like everyone is in agreement that we need a channel_binding param. Regards, Jeff Davis
Вложения
On Tue, Aug 20, 2019 at 07:09:25PM -0700, Jeff Davis wrote: > OK, new patch attached. Seems like everyone is in agreement that we > need a channel_binding param. + <para> + A setting of <literal>require</literal> means that the connection must + employ channel binding; and that the client will not respond to + requests from the server for cleartext password or MD5 + authentication. The default setting is <literal>prefer</literal>, + which employs channel binding if available. + </para> This counts for other auth requests as well like krb5, no? I think that we should also add the "disable" flavor for symmetry, and also... +#define DefaultChannelBinding "prefer" If the build of Postgres does not support SSL (USE_SSL is not defined), I think that DefaultChannelBinding should be "disable". That would make things consistent with sslmode. + with <productname>PostgreSQL</productname> 11.0 or later servers using Here I would use PostgreSQL 11, only mentioning the major version as it was also available at beta time. case AUTH_REQ_OK: + if (conn->channel_binding[0] == 'r' && !conn->channel_bound) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("Channel binding required but not offered by server\n")); + return STATUS_ERROR; + } Doing the filtering at the time of AUTH_REQ_OK is necessary for "trust", but shouldn't this be done as well earlier for other request types? This could save round-trips to the server if we know that an exchange begins with a protocol which will never satisfy this request, saving efforts for a client doomed to fail after the first AUTH_REQ received. That would be the case for all AUTH_REQ, except the SASL ones and of course AUTH_REQ_OK. Could you please add negative tests in src/test/authentication/? What could be covered there is that the case where "prefer" (and "disable"?) is defined then the authentication is able to go through, and that with "require" we get a proper failure as SSL is not used. Tests in src/test/ssl/ could include: - Make sure that "require" works properly. - Test after "disable". + if (conn->channel_binding[0] == 'r') Maybe this should comment that this means "require", in a fashion similar to what is done when checking conn->sslmode. -- Michael
Вложения
On Wed, 2019-08-21 at 16:12 +0900, Michael Paquier wrote: > This counts for other auth requests as well like krb5, no? I think > that we should also add the "disable" flavor for symmetry, and > also... Added "disable" option, and I refactored so that it would look explicitly for an expected auth req based on the connection parameters. I had to also make the client tell the server that it does not support channel binding if channel_binding=disable, otherwise the server complains. > +#define DefaultChannelBinding "prefer" > If the build of Postgres does not support SSL (USE_SSL is not > defined), I think that DefaultChannelBinding should be "disable". > That would make things consistent with sslmode. Done. > Doing the filtering at the time of AUTH_REQ_OK is necessary for > "trust", but shouldn't this be done as well earlier for other request > types? This could save round-trips to the server if we know that an > exchange begins with a protocol which will never satisfy this > request, saving efforts for a client doomed to fail after the first > AUTH_REQ received. That would be the case for all AUTH_REQ, except > the SASL ones and of course AUTH_REQ_OK. Done. > > Could you please add negative tests in > src/test/authentication/? What > could be covered there is that the case where "prefer" (and > "disable"?) is defined then the authentication is able to go through, > and that with "require" we get a proper failure as SSL is not used. > Tests in src/test/ssl/ could include: > - Make sure that "require" works properly. > - Test after "disable". Done. > + if (conn->channel_binding[0] == 'r') > Maybe this should comment that this means "require", in a fashion > similar to what is done when checking conn->sslmode. Done. New patch attached. Regards, Jeff Davis
Вложения
On Wed, Sep 04, 2019 at 09:22:33PM -0700, Jeff Davis wrote: > New patch attached. Thanks for the new version, Jeff. + with <productname>PostgreSQL</productname> 11 or later servers using + the <literal>scram-sha-256</literal> authentication method. Nit here: "scram-sha-256" refers to the HBA entry. I would just use "SCRAM" instead. In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL mechanism over a non-SSL connection, should we complain even if the "disable" mode is used? It seems to me that there is a point to complain in this case as a sanity check as the server should really publicize "SCRAM-SHA-256-PLUS" only over SSL. When the server only sends SCRAM-SHA-256 in the mechanism list and "require" mode is used, we complain about "none of the server's SASL authentication mechanisms are supported" which can be confusing. Why not generating a custom error if libpq selects SCRAM-SHA-256 when "require" is used, say: "SASL authentication mechanism SCRAM-SHA-256 selected but channel binding is required" That could be done by adding an error message when selecting SCRAM-SHA-256 and then goto the error step. Actually, it looks that the handling of channel_bound is incorrect. If the server sends AUTH_REQ_SASL and libpq processes it, then the flag gets already set. Now let's imagine that the server is a rogue one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check would pass. It seems to me that we should switch the flag once we are sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when the final message is done within pg_SASL_continue(). +# SSL not in use; channel binding still can't work +reset_pg_hba($node, 'scram-sha-256'); +$ENV{"PGCHANNELBINDING"} = 'require'; +test_login($node, 'saslpreptest4a_role', "a", 2); Worth testing md5 here? PGCHANNELBINDING needs documentation. -- Michael
Вложения
On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote: > Nit here: "scram-sha-256" refers to the HBA entry. I would > just use "SCRAM" instead. Done. > In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL > mechanism over a non-SSL connection, should we complain even if > the "disable" mode is used? It seems to me that there is a point to > complain in this case as a sanity check as the server should really > publicize "SCRAM-SHA-256-PLUS" only over SSL. Done. > When the server only sends SCRAM-SHA-256 in the mechanism list and > "require" mode is used, we complain about "none of the server's SASL > authentication mechanisms are supported" which can be confusing. Why > not generating a custom error if libpq selects SCRAM-SHA-256 when > "require" is used, say: > "SASL authentication mechanism SCRAM-SHA-256 selected but channel > binding is required" > That could be done by adding an error message when selecting > SCRAM-SHA-256 and then goto the error step. Done. > Actually, it looks that the handling of channel_bound is incorrect. > If the server sends AUTH_REQ_SASL and libpq processes it, then the > flag gets already set. Now let's imagine that the server is a rogue > one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check > would pass. It seems to me that we should switch the flag once we > are > sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when > the final message is done within pg_SASL_continue(). Thank you! Fixed. I now track whether channel binding is selected, and also whether SASL actually finished successfully. > +# SSL not in use; channel binding still can't work > +reset_pg_hba($node, 'scram-sha-256'); > +$ENV{"PGCHANNELBINDING"} = 'require'; > +test_login($node, 'saslpreptest4a_role', "a", 2); > Worth testing md5 here? I added a new md5 test in the ssl test suite. Testing it in the non-SSL path doesn't seem like it adds much. > PGCHANNELBINDING needs documentation. Done. Regards, Jeff Davis
Вложения
On Sat, Sep 14, 2019 at 08:42:53AM -0700, Jeff Davis wrote: > On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote: >> Actually, it looks that the handling of channel_bound is incorrect. >> If the server sends AUTH_REQ_SASL and libpq processes it, then the >> flag gets already set. Now let's imagine that the server is a rogue >> one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check >> would pass. It seems to me that we should switch the flag once we >> are >> sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when >> the final message is done within pg_SASL_continue(). > > Thank you! Fixed. I now track whether channel binding is selected, and > also whether SASL actually finished successfully. Ah, I see. So you have added an extra flag "sasl_finished" to make sure of that, and still kept around "channel_bound". Hence the two flags have to be set to make sure that the SASL exchanged has been finished and that channel binding has been enabled. "channel_bound" is linked to the selected mechanism when the exchange begins, meaning that it could be possible to do the same check with the selected mechanism directly from fe_scram_state instead. "sasl_finished" is linked to the state where the SASL exchange is finished, so this basically maps into checking after FE_SCRAM_FINISHED. Instead of those two flags, wouldn't it be cleaner to add an extra routine to fe-auth-scram.c which does the same sanity checks, say pg_fe_scram_check_state()? This needs to be careful about three things, taking in input an opaque pointer to fe_scram_state if channel binding is required: - The data is not NULL. - The sasl mechanism selected is SCRAM-SHA-256-PLUS. - The state is FE_SCRAM_FINISHED. What do you think? There is no need to save down the connection parameter value into fe_scram_state. >> +# SSL not in use; channel binding still can't work >> +reset_pg_hba($node, 'scram-sha-256'); >> +$ENV{"PGCHANNELBINDING"} = 'require'; >> +test_login($node, 'saslpreptest4a_role', "a", 2); >> Worth testing md5 here? > > I added a new md5 test in the ssl test suite. Testing it in the non-SSL > path doesn't seem like it adds much. Good idea. + if (conn->channel_binding[0] == 'r' && /* require */ + strcmp(selected_mechanism, SCRAM_SHA_256_NAME) == 0) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("SASL authentication mechanism SCRAM-SHA-256 selected but channel binding is required\n")); + goto error; + } Nit here as there are only two mechanisms handled: I would rather cause the error if the selected mechanism does not match SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism matches SCRAM-SHA-256. Either way is actually fine :) + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("Channel binding required but not offered by server\n")); + result = false; Should that be "completed by server" instead? + if (areq == AUTH_REQ_SASL_FIN) + conn->sasl_finished = true; This should have a comment about the why it is done if you go this way with the two flags added to PGconn. + if (conn->channel_binding[0] == 'r' && /* require */ + !conn->ssl_in_use) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("Channel binding required, but SSL not in use\n")); + goto error; + } This is not necessary? If SSL is not in use but the server publishes SCRAM-SHA-256-PLUS, libpq complains. If the server sends only SCRAM-SHA-256 but channel binding is required, we complain down on "SASL authentication mechanism SCRAM-SHA selected but channel binding is required". Or you have in mind that this error message is better? I think that pgindent would complain with the comment block in check_expected_areq(). -- Michael
Вложения
On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote: > basically maps into checking after FE_SCRAM_FINISHED. Instead of > those two flags, wouldn't it be cleaner to add an extra routine to > fe-auth-scram.c which does the same sanity checks, say > pg_fe_scram_check_state()? This needs to be careful about three > things, taking in input an opaque pointer to fe_scram_state if > channel > binding is required: > - The data is not NULL. > - The sasl mechanism selected is SCRAM-SHA-256-PLUS. > - The state is FE_SCRAM_FINISHED. Yes, I think this does come out a bit cleaner, thank you. > What do you think? There is no need to save down the connection > parameter value into fe_scram_state. I'm not sure I understand this comment, but I removed the extra boolean flags. > Nit here as there are only two mechanisms handled: I would rather > cause the error if the selected mechanism does not match > SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism > matches SCRAM-SHA-256. Either way is actually fine :) Done. > + printfPQExpBuffer(&conn->errorMessage, > + libpq_gettext("Channel binding required but > not > offered by server\n")); > + result = false; > Should that be "completed by server" instead? Done. > is required". Or you have in mind that this error message is better? I felt it would be a more useful error message. > I think that pgindent would complain with the comment block in > check_expected_areq(). Changed. Regards, Jeff Davis
Вложения
On Thu, Sep 19, 2019 at 05:40:15PM -0700, Jeff Davis wrote: > On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote: >> What do you think? There is no need to save down the connection >> parameter value into fe_scram_state. > > I'm not sure I understand this comment, but I removed the extra boolean > flags. Thanks for considering it. I was just asking about removing those flags and your thoughts about my thoughts from upthread. >> is required". Or you have in mind that this error message is better? > > I felt it would be a more useful error message. Okay, fine by me. >> I think that pgindent would complain with the comment block in >> check_expected_areq(). > > Changed. A trick to make pgindent to ignore some comment blocks is to use /*--------- at its top and bottom, FWIW. +$ENV{PGUSER} = "ssltestuser"; $ENV{PGPASSWORD} = "pass"; test_connect_ok() can use a complementary string, so I would use that in the SSL test part instead of relying too much on the environment for readability, particularly for the last test added with md5testuser. Using the environment variable in src/test/authentication/ makes sense. Maybe that's just a matter of taste :) + return (state != NULL && state->state == FE_SCRAM_FINISHED && + strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0); I think that we should document in the code why those reasons are chosen. I would also add a test for an invalid value of channel_binding. A comment update is forgotten in libpq-int.h. +# using the password authentication method; channel binding can't work +reset_pg_hba($node, 'password'); +$ENV{"PGCHANNELBINDING"} = 'require'; +test_login($node, 'saslpreptest4a_role', "a", 2); + +# SSL not in use; channel binding still can't work +reset_pg_hba($node, 'scram-sha-256'); +$ENV{"PGCHANNELBINDING"} = 'require'; +test_login($node, 'saslpreptest4a_role', "a", 2); Those two tests are in the test suite dedicated to SASLprep. I think that it would be more consistent to just move them to 001_password.pl. And this does not impact the error coverage. Missing some indentation in the perl scripts (per pgperltidy). Those are mainly nits, and attached are the changes I would do to your patch. Please feel free to consider those changes as you see fit. Anyway, the base logic looks good to me, so I am switching the patch as ready for committer. -- Michael
Вложения
On Fri, 2019-09-20 at 13:07 +0900, Michael Paquier wrote: > Those are mainly nits, and attached are the changes I would do to > your > patch. Please feel free to consider those changes as you see fit. > Anyway, the base logic looks good to me, so I am switching the patch > as ready for committer. Thank you, applied. * I also changed the comment above pg_fe_scram_channel_bound() to clarify that the caller must also check that the exchange was successful. * I changed the error message when AUTH_REQ_OK is received without channel binding. It seemed confusing before. I also added a test. Regards, Jeff Davis
Вложения
On Fri, Sep 20, 2019 at 10:57:04AM -0700, Jeff Davis wrote: > Thank you, applied. Okay, I can see which parts you have changed. > * I also changed the comment above pg_fe_scram_channel_bound() to > clarify that the caller must also check that the exchange was > successful. > > * I changed the error message when AUTH_REQ_OK is received without > channel binding. It seemed confusing before. I also added a test. And both make sense. + * Return true if channel binding was employed and the scram exchange upper('scram')? Except for this nit, it looks good to me. -- Michael
Вложения
On Sat, Sep 21, 2019 at 11:24:30AM +0900, Michael Paquier wrote: > And both make sense. > > + * Return true if channel binding was employed and the scram exchange > upper('scram')? > > Except for this nit, it looks good to me. For the archive's sake: this has been committed as of d6e612f. - * We pretend that the connection is OK for the duration of these - * queries. + * We pretend that the connection is OK for the duration of + * these queries. The result had some noise diffs. Perhaps some leftover from the indentation run? That's no big deal anyway. -- Michael