Thread: [HACKERS] scram and \password
Should the \password tool in psql inspect password_encryption and act on it being 'scram'?
I didn't see this issue discussed, but the ability to search the archives for backslashes is rather limited.
Cheers,
Jeff
On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote: > Should the \password tool in psql inspect password_encryption and act on it > being 'scram'? Not sure if it is wise to change the default fot this release. > I didn't see this issue discussed, but the ability to search the archives > for backslashes is rather limited. I'll save you some time: it has not been discussed. Nor has PQencryptPassword been mentioned. Changing to SCRAM is not that complicated, just call scram_build_verifier() and you are good to go. Instead of changing the default, I think that we should take this occasion to rename PQencryptPassword to something like PQhashPassword(), and extend it with a method argument to support both md5 and scram. PQencryptPassword could also be marked as deprecated, or let as-is for some time. For \password, we could have another meta-command but that sounds grotty, or just extend \password with a --method argument. Switching the default could happen in another release. A patch among those lines would be a simple, do people feel that this should be part of PG 10? -- Michael
On 03/10/2017 02:43 PM, Michael Paquier wrote: > On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote: >> Should the \password tool in psql inspect password_encryption and act on it >> being 'scram'? > > Not sure if it is wise to change the default fot this release. > >> I didn't see this issue discussed, but the ability to search the archives >> for backslashes is rather limited. > > I'll save you some time: it has not been discussed. Nor has > PQencryptPassword been mentioned. Changing to SCRAM is not that > complicated, just call scram_build_verifier() and you are good to go. > > Instead of changing the default, I think that we should take this > occasion to rename PQencryptPassword to something like > PQhashPassword(), and extend it with a method argument to support both > md5 and scram. PQencryptPassword could also be marked as deprecated, > or let as-is for some time. For \password, we could have another > meta-command but that sounds grotty, or just extend \password with a > --method argument. Switching the default could happen in another > release. > > A patch among those lines would be a simple, do people feel that this > should be part of PG 10? Seems like it should work in an analogous way to CREATE/ALTER ROLE. According to the docs: 8<---- ENCRYPTED UNENCRYPTED These key words control whether the password is stored encrypted in the system catalogs. (If neither is specified, the default behavior is determined by the configuration parameter password_encryption.) If the presented password string is already in MD5-encrypted or SCRAM-encrypted format, then it is stored encrypted as-is, regardless of whether ENCRYPTED or UNENCRYPTED is specified (since the system cannot decrypt the specified encrypted password string). This allows reloading of encrypted passwords during dump/restore. 8<---- So if the password is not already set, \password uses password_encryption to determine which format to use, and if the password is already set, then the current method is assumed. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Sun, Mar 12, 2017 at 5:35 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/10/2017 02:43 PM, Michael Paquier wrote: >> Instead of changing the default, I think that we should take this >> occasion to rename PQencryptPassword to something like >> PQhashPassword(), and extend it with a method argument to support both >> md5 and scram. PQencryptPassword could also be marked as deprecated, >> or let as-is for some time. For \password, we could have another >> meta-command but that sounds grotty, or just extend \password with a >> --method argument. Switching the default could happen in another >> release. >> >> A patch among those lines would be a simple, do people feel that this >> should be part of PG 10? > > Seems like it should work in an analogous way to CREATE/ALTER ROLE. > According to the docs: > > 8<---- > ENCRYPTED > UNENCRYPTED > > These key words control whether the password is stored encrypted in > the system catalogs. (If neither is specified, the default behavior is > determined by the configuration parameter password_encryption.) If the > presented password string is already in MD5-encrypted or SCRAM-encrypted > format, then it is stored encrypted as-is, regardless of whether > ENCRYPTED or UNENCRYPTED is specified (since the system cannot decrypt > the specified encrypted password string). This allows reloading of > encrypted passwords during dump/restore. > 8<---- > > So if the password is not already set, \password uses > password_encryption to determine which format to use, and if the > password is already set, then the current method is assumed. Yeah, the problem here being that this routine does not need a live connection to work, and we surely don't want to make that mandatory, that's why I am suggesting something like the above. Another approach would be to switch to SCRAM once password_encryption does this switch as well... There is no perfect scenario here. -- Michael
On 03/11/2017 02:21 PM, Michael Paquier wrote: > On Sun, Mar 12, 2017 at 5:35 AM, Joe Conway <mail@joeconway.com> wrote: >> So if the password is not already set, \password uses >> password_encryption to determine which format to use, and if the >> password is already set, then the current method is assumed. > > Yeah, the problem here being that this routine does not need a live > connection to work, and we surely don't want to make that mandatory, > that's why I am suggesting something like the above. Another approach > would be to switch to SCRAM once password_encryption does this switch > as well... There is no perfect scenario here. You might extend PQencryptPassword() to take a method. Or create a new function that does? Certainly psql has a connection available to run the ALTER ROLE command that it crafts. I guess a related problem might be, do we have a SQL visible way to determine what method is used by the current password for a given role? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Sun, Mar 12, 2017 at 8:04 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/11/2017 02:21 PM, Michael Paquier wrote: >> On Sun, Mar 12, 2017 at 5:35 AM, Joe Conway <mail@joeconway.com> wrote: >>> So if the password is not already set, \password uses >>> password_encryption to determine which format to use, and if the >>> password is already set, then the current method is assumed. >> >> Yeah, the problem here being that this routine does not need a live >> connection to work, and we surely don't want to make that mandatory, >> that's why I am suggesting something like the above. Another approach >> would be to switch to SCRAM once password_encryption does this switch >> as well... There is no perfect scenario here. > > You might extend PQencryptPassword() to take a method. Or create a new > function that does? Certainly psql has a connection available to run the > ALTER ROLE command that it crafts. Yeah but it can be called as well while the application is calling PQgetResult() and still looping until it gets a NULL result. Not sure if this is a use-case to worry about, but sending a query to the server in PQencryptPassword() could as well break some applications. PQencryptPassword() is used for CREATE/ALTER ROLE commands, so actually wouldn't it make sense to just switch PQencryptPassword to handle SCRAM if at some point we decide to switch the default from md5 to scram? So many questions. > I guess a related problem might be, do we have a SQL visible way to > determine what method is used by the current password for a given role? Nope. We are simply looking at a function doing a lookup at pg_authid and then use get_password_type() to check which type of verifier is used... Or have the type of verifier as a new column of pg_authid, information that could be made visible to any users with column privileges. -- Michael
On 03/11/2017 03:15 PM, Michael Paquier wrote: > Yeah but it can be called as well while the application is calling > PQgetResult() and still looping until it gets a NULL result. Not sure > if this is a use-case to worry about, but sending a query to the > server in PQencryptPassword() could as well break some applications. I was suggesting sending the query outside of PQencryptPassword() in order to determine what method should be passed as a new argument to PQencryptPassword(). > PQencryptPassword() is used for CREATE/ALTER ROLE commands, so > actually wouldn't it make sense to just switch PQencryptPassword to > handle SCRAM if at some point we decide to switch the default from md5 > to scram? So many questions. As long as we support more than one method it would seem to me we need a way to determine which one we want to use and not only default it, don't we? Apologies if this has already been discussed -- I was not able to follow the lengthy threads on SCRAM in any detail. >> I guess a related problem might be, do we have a SQL visible way to >> determine what method is used by the current password for a given role? > > Nope. We are simply looking at a function doing a lookup at pg_authid > and then use get_password_type() to check which type of verifier is > used... Or have the type of verifier as a new column of pg_authid, > information that could be made visible to any users with column > privileges. What happens if the user does not have privs for pg_authid? E.g. if I want to change my own password what happens if the default is one method, and my password uses the other -- now I cannot change my own password using \password? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Sun, Mar 12, 2017 at 9:10 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/11/2017 03:15 PM, Michael Paquier wrote: >> Yeah but it can be called as well while the application is calling >> PQgetResult() and still looping until it gets a NULL result. Not sure >> if this is a use-case to worry about, but sending a query to the >> server in PQencryptPassword() could as well break some applications. > > I was suggesting sending the query outside of PQencryptPassword() in > order to determine what method should be passed as a new argument to > PQencryptPassword(). Why not. Our thoughts don't overlap, I thought about having PQencryptPassword() call itself the server for the value of password_encryption, and force the type depending on what the server answers. >> PQencryptPassword() is used for CREATE/ALTER ROLE commands, so >> actually wouldn't it make sense to just switch PQencryptPassword to >> handle SCRAM if at some point we decide to switch the default from md5 >> to scram? So many questions. > > As long as we support more than one method it would seem to me we need a > way to determine which one we want to use and not only default it, don't > we? Apologies if this has already been discussed -- I was not able to > follow the lengthy threads on SCRAM in any detail. Definitely, the most simple solution would be just to extend PQencryptPassword() with a method value, to allow a user to do what he wants... >>> I guess a related problem might be, do we have a SQL visible way to >>> determine what method is used by the current password for a given role? >> >> Nope. We are simply looking at a function doing a lookup at pg_authid >> and then use get_password_type() to check which type of verifier is >> used... Or have the type of verifier as a new column of pg_authid, >> information that could be made visible to any users with column >> privileges. > > What happens if the user does not have privs for pg_authid? E.g. if I > want to change my own password what happens if the default is one > method, and my password uses the other -- now I cannot change my own > password using \password? You can now. However it would be a problem for a user having a SCRAM verifier using an application that changes the password with PQencryptPassword() as it would change it back to MD5 on an update. Having a RLS on pg_authid to allow a user to look at its own password type is an idea. With multiple verifier types per role such class of bugs can be also completely discarded. -- Michael
On 03/11/2017 11:07 PM, Michael Paquier wrote: > Having a RLS on pg_authid to allow a user to look at its own password > type is an idea. Given that that is not likely at this stage of the dev cycle, what about a special purpose SQL function that returns the password type for the current user? Or is it a security issue of some sort to allow a user to know their own password type? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Fri, Mar 10, 2017 at 5:43 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote: >> Should the \password tool in psql inspect password_encryption and act on it >> being 'scram'? > > Not sure if it is wise to change the default fot this release. Seems like an odd way to phrase it. Aren't we talking about making a feature that worked in previous releases continue to work? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 13, 2017 at 12:48 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/11/2017 11:07 PM, Michael Paquier wrote: >> Having a RLS on pg_authid to allow a user to look at its own password >> type is an idea. > > Given that that is not likely at this stage of the dev cycle, what about > a special purpose SQL function that returns the password type for the > current user? OK, so what about doing the following then: 1) Create pg_role_password_type(oid), taking a role OID in input and returning the type. 2) Extend PQencryptPassword with a method, and document. Plaintext is forbidden. 3) Extend \password in psql with a similar -method=scram|md5 argument, and forbid the use of "plain" format. After thinking about it, extending PQencryptPassword() would lead to future breakage, which makes it clear to not fall into the trap of having password_encryption set to scram in the server's as well as in pg_hba.conf and PQencryptPassword() enforcing things to md5. > Or is it a security issue of some sort to allow a user to > know their own password type? Don't think so. Any user has access to the value of password_encryption. So one can guess what will be the format of a password hashed. -- Michael
On Mon, Mar 13, 2017 at 9:15 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Mar 10, 2017 at 5:43 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote: >>> Should the \password tool in psql inspect password_encryption and act on it >>> being 'scram'? >> >> Not sure if it is wise to change the default fot this release. > > Seems like an odd way to phrase it. Aren't we talking about making a > feature that worked in previous releases continue to work? Considering how fresh scram is, it seems clear to me that we do not want to just switch the default values of password_encryption, the default behaviors of PQencryptPassword() and \password only to scram, but have something else. Actually if we change nothing for default deployments of Postgres using md5, PQencryptPassword() and \password would still work properly. -- Michael
On Fri, Mar 10, 2017 at 2:43 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> Should the \password tool in psql inspect password_encryption and act on it
> being 'scram'?
Not sure if it is wise to change the default fot this release.
I'm not proposing that we change the default value of password_encryption, only that \password respect whatever value it currently finds there. But after thinking about it a bit more, I reached the same conclusion that Joe did, that it should use the same hashing method as the current password does, and only consult password_encryption if there is no password currently set.
A patch among those lines would be a simple, do people feel that this
should be part of PG 10?
I think it is pretty important to have some way of setting the password that doesn't risk it ending up in the server log file, or .psql_history, or having someone shoulder-surf it.
Cheers,
Jeff
On 03/12/2017 08:10 PM, Michael Paquier wrote: > OK, so what about doing the following then: > 1) Create pg_role_password_type(oid), taking a role OID in input and > returning the type. That version would make sense for administrative use. I still think we might want a version of this that takes no argument, works on the current_user, and is executable by anyone. > 2) Extend PQencryptPassword with a method, and document. Plaintext is forbidden. Check, although if "plain" were allowed as a method for the sake of consistency/completeness the function could just immediately return the argument. > 3) Extend \password in psql with a similar -method=scram|md5 argument, > and forbid the use of "plain" format. Not sure why we would forbid "plain" here unless we remove it entirely elsewhere. > After thinking about it, extending PQencryptPassword() would lead to > future breakage, which makes it clear to not fall into the trap of > having password_encryption set to scram in the server's as well as in > pg_hba.conf and PQencryptPassword() enforcing things to md5. I'm not grokking this statement Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Tue, Mar 14, 2017 at 9:54 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/12/2017 08:10 PM, Michael Paquier wrote: >> OK, so what about doing the following then: >> 1) Create pg_role_password_type(oid), taking a role OID in input and >> returning the type. > > That version would make sense for administrative use. I still think we > might want a version of this that takes no argument, works on the > current_user, and is executable by anyone. Sure. >> 2) Extend PQencryptPassword with a method, and document. Plaintext is forbidden. > > Check, although if "plain" were allowed as a method for the sake of > consistency/completeness the function could just immediately return the > argument. > >> 3) Extend \password in psql with a similar -method=scram|md5 argument, >> and forbid the use of "plain" format. > > Not sure why we would forbid "plain" here unless we remove it entirely > elsewhere. OK. I don't mind about those two, as long as documentation is clear enough about what using plain leads to. >> After thinking about it, extending PQencryptPassword() would lead to >> future breakage, which makes it clear to not fall into the trap of >> having password_encryption set to scram in the server's as well as in >> pg_hba.conf and PQencryptPassword() enforcing things to md5. > > I'm not grokking this statement To grok: "To understand profoundly through intuition or empathy.". Learning a new thing everyday. -- Michael
On Sun, Mar 12, 2017 at 11:14 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Mar 13, 2017 at 9:15 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Fri, Mar 10, 2017 at 5:43 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes <jeff.janes@gmail.com> wrote: >>>> Should the \password tool in psql inspect password_encryption and act on it >>>> being 'scram'? >>> >>> Not sure if it is wise to change the default fot this release. >> >> Seems like an odd way to phrase it. Aren't we talking about making a >> feature that worked in previous releases continue to work? > > Considering how fresh scram is, it seems clear to me that we do not > want to just switch the default values of password_encryption, the > default behaviors of PQencryptPassword() and \password only to scram, > but have something else. Actually if we change nothing for default > deployments of Postgres using md5, PQencryptPassword() and \password > would still work properly. I'm not talking about changing the default, just having it be possible to use \password with the new system as it was with the old, whatever exactly we think that means. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not talking about changing the default, just having it be possible
> to use \password with the new system as it was with the old, whatever
> exactly we think that means.
Seems to me the intended behavior of \password is to use the best
available practice. So my guess is that it ought to use SCRAM when
talking to a >= 10.0 server. What the previous password was ought
to be irrelevant, even if it could find that out which it shouldn't
be able to IMO.
regards, tom lane
On Tue, Mar 14, 2017 at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not talking about changing the default, just having it be possible >> to use \password with the new system as it was with the old, whatever >> exactly we think that means. I think that this means looking at password_encryption within PQencryptPassword(), something that could silently break some applications. That's why with Joe we are mentioning upthread to extend PQencryptPassword() with a hashing method, and have a function to allow retrieval of the password type for a given user. > Seems to me the intended behavior of \password is to use the best > available practice. So my guess is that it ought to use SCRAM when > talking to a >= 10.0 server. What the previous password was ought > to be irrelevant, even if it could find that out which it shouldn't > be able to IMO. And in a release or two? SCRAM being a fresh feature, switching the hashing now is not much a conservative approach. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Mar 14, 2017 at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Seems to me the intended behavior of \password is to use the best
>> available practice. So my guess is that it ought to use SCRAM when
>> talking to a >= 10.0 server. What the previous password was ought
>> to be irrelevant, even if it could find that out which it shouldn't
>> be able to IMO.
> And in a release or two? SCRAM being a fresh feature, switching the
> hashing now is not much a conservative approach.
If some other practice becomes better in v12, then we teach it about that
one. It's not like psql hasn't got many other server-version-dependent
behaviors.
Alternatively, if what you mean by that is you don't trust SCRAM at all,
maybe we'd better revert the feature as not being ready for prime time.
regards, tom lane
On Tue, Mar 14, 2017 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: > If some other practice becomes better in v12, then we teach it about that > one. It's not like psql hasn't got many other server-version-dependent > behaviors. Okay. Looking at the code, scram_build_verifier is only available on the backend side, so if we need to rethink it a bit to be able to have libpq build a SCRAM verifier: - The allocation of the verifier buffer needs to happen outside it, and the caller needs to provide an allocated buffer. Its size had better be calculated a macro. - The routine needs to be moved to scram-common.c. - a copy of hex_encode, say pg_hex_encode added in its own file in src/common/, needs to be provided. That's 6 lines of code, we could just have that in scram-common.c. Does that sound correct? > Alternatively, if what you mean by that is you don't trust SCRAM at all, > maybe we'd better revert the feature as not being ready for prime time. FWIW, I am confident about the code. Based on years watching this mailing list, switching long-term behaviors is usually done in a less faster pace, that's the only reason on which my opinion is based. -- Michael
On 03/14/2017 04:47 AM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not talking about changing the default, just having it be possible >> to use \password with the new system as it was with the old, whatever >> exactly we think that means. > > Seems to me the intended behavior of \password is to use the best > available practice. So my guess is that it ought to use SCRAM when > talking to a >= 10.0 server. What the previous password was ought > to be irrelevant, even if it could find that out which it shouldn't > be able to IMO. If the server isn't set up to do SCRAM authentication, i.e. there are no "scram" entries in pg_hba.conf, and you set yourself a SCRAM verifier, you have just locked yourself out of the system. I think that's a non-starter. There needs to be some more intelligence in the decision. It would be a lot more sensible, if there was a way to specify in pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but perhaps we should try to cram that in, after all. - Heikki
On 03/14/2017 03:15 AM, Heikki Linnakangas wrote: > On 03/14/2017 04:47 AM, Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> I'm not talking about changing the default, just having it be possible >>> to use \password with the new system as it was with the old, whatever >>> exactly we think that means. >> >> Seems to me the intended behavior of \password is to use the best >> available practice. So my guess is that it ought to use SCRAM when >> talking to a >= 10.0 server. What the previous password was ought >> to be irrelevant, even if it could find that out which it shouldn't >> be able to IMO. > > If the server isn't set up to do SCRAM authentication, i.e. there are no > "scram" entries in pg_hba.conf, and you set yourself a SCRAM verifier, > you have just locked yourself out of the system. I think that's a > non-starter. There needs to be some more intelligence in the decision. Yes, this was exactly my concern. > It would be a lot more sensible, if there was a way to specify in > pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but > perhaps we should try to cram that in, after all. I was also thinking about that. Basically a primary method and a fallback. If that were the case, a gradual transition could happen, and if we want \password to enforce best practice it would be ok. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
> On 03/14/2017 03:15 AM, Heikki Linnakangas wrote:
>> If the server isn't set up to do SCRAM authentication, i.e. there are no
>> "scram" entries in pg_hba.conf, and you set yourself a SCRAM verifier,
>> you have just locked yourself out of the system. I think that's a
>> non-starter. There needs to be some more intelligence in the decision.
> Yes, this was exactly my concern.
This seems like a serious usability fail.
>> It would be a lot more sensible, if there was a way to specify in
>> pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but
>> perhaps we should try to cram that in, after all.
> I was also thinking about that. Basically a primary method and a
> fallback. If that were the case, a gradual transition could happen, and
> if we want \password to enforce best practice it would be ok.
Why exactly would anyone want "md5 only"? I should think that "scram
only" is a sensible pg_hba setting, if the DBA feels that md5 is too
insecure, but I do not see the point of "md5 only" in 2017. I think
we should just start interpreting that as "md5 or better".
regards, tom lane
On 03/14/2017 08:40 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> On 03/14/2017 03:15 AM, Heikki Linnakangas wrote: >>> It would be a lot more sensible, if there was a way to specify in >>> pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but >>> perhaps we should try to cram that in, after all. > >> I was also thinking about that. Basically a primary method and a >> fallback. If that were the case, a gradual transition could happen, and >> if we want \password to enforce best practice it would be ok. > > Why exactly would anyone want "md5 only"? I should think that "scram > only" is a sensible pg_hba setting, if the DBA feels that md5 is too > insecure, but I do not see the point of "md5 only" in 2017. I think > we should just start interpreting that as "md5 or better". That certainly would work for me. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Tue, Mar 14, 2017 at 3:15 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 03/14/2017 04:47 AM, Tom Lane wrote:Robert Haas <robertmhaas@gmail.com> writes:I'm not talking about changing the default, just having it be possible
to use \password with the new system as it was with the old, whatever
exactly we think that means.
Seems to me the intended behavior of \password is to use the best
available practice. So my guess is that it ought to use SCRAM when
talking to a >= 10.0 server. What the previous password was ought
to be irrelevant, even if it could find that out which it shouldn't
be able to IMO.
If the server isn't set up to do SCRAM authentication, i.e. there are no "scram" entries in pg_hba.conf,
The method is not part of the pg_hba matching algorithm, so either the first match is scram, or it isn't. There is no fallback to later entries, as I understand it.
and you set yourself a SCRAM verifier, you have just locked yourself out of the system. I think that's a non-starter. There needs to be some more intelligence in the decision.
Right. And you can lock yourself out of the system going the other way, as well, setting a md5 password when scram is in pg_hba. Which is how I ended up starting this thread.
It would be a lot more sensible, if there was a way to specify in pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but perhaps we should try to cram that in, after all.
So the backend for the incipient connection would consult the catalog pg_authid before responding back to the client with an authentication method, as opposed to merely pulling it out of pg_hba?
Cheers,
Jeff
On Tue, Mar 14, 2017 at 8:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
> I was also thinking about that. Basically a primary method and a
> fallback. If that were the case, a gradual transition could happen, and
> if we want \password to enforce best practice it would be ok.
Why exactly would anyone want "md5 only"? I should think that "scram
only" is a sensible pg_hba setting, if the DBA feels that md5 is too
insecure, but I do not see the point of "md5 only" in 2017. I think
we should just start interpreting that as "md5 or better".
Without md5-only, a user who uses \password to change their password from a newer client would lock themselves out of connecting again from older clients. As a conscious decision (either of the DBA or the user) that would be OK, but to have it happen by default would be unfortunate.
Cheers,
Jeff
Jeff Janes <jeff.janes@gmail.com> writes:
> On Tue, Mar 14, 2017 at 8:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Why exactly would anyone want "md5 only"? I should think that "scram
>> only" is a sensible pg_hba setting, if the DBA feels that md5 is too
>> insecure, but I do not see the point of "md5 only" in 2017. I think
>> we should just start interpreting that as "md5 or better".
> Without md5-only, a user who uses \password to change their password from a
> newer client would lock themselves out of connecting again from older
> clients. As a conscious decision (either of the DBA or the user) that
> would be OK, but to have it happen by default would be unfortunate.
That's a point, but what it implies is that \password needs some input
from the user about whether to generate a SCRAM or MD5-hashed password.
It would be a fatal error to try to drive that off the auth method
that had been used for the current connection, even if \password had a
way to find that out. By definition, your concern is about clients
other than the current one, which might well be coming in from other
addresses and getting challenges based on other pg_hba entries. So
you can't say that "I came in on a SCRAM connection" is sufficient
reason to generate a SCRAM password.
In short, I don't think that argument refutes my position that "md5"
in pg_hba.conf should be understood as allowing SCRAM passwords too.
regards, tom lane
On Tue, Mar 14, 2017 at 5:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Without md5-only, a user who uses \password to change their password from a >> newer client would lock themselves out of connecting again from older >> clients. As a conscious decision (either of the DBA or the user) that >> would be OK, but to have it happen by default would be unfortunate. > > That's a point, but what it implies is that \password needs some input > from the user about whether to generate a SCRAM or MD5-hashed password. > It would be a fatal error to try to drive that off the auth method > that had been used for the current connection, even if \password had a > way to find that out. By definition, your concern is about clients > other than the current one, which might well be coming in from other > addresses and getting challenges based on other pg_hba entries. So > you can't say that "I came in on a SCRAM connection" is sufficient > reason to generate a SCRAM password. To some extent that seems like a question of system policy. Either the DBA wants users to use SCRAM passwords, or the DBA wants users to use MD5 passwords, or either is permissible. In the last case, the user can do what they like, but it seems like a fairly bad idea from a user perspective to let the user configure a password using a system that will lock them out. We shouldn't assume the user even has any knowledge of what's in pg_hba.conf, or that they would know what those contents meant if they had them. There ought to be something like a PGC_SUSER GUC that sets the kinds of password verifiers that a user is allowed to configure, and maybe \password should default to the first one in the list (but possibly be overridable?). > In short, I don't think that argument refutes my position that "md5" > in pg_hba.conf should be understood as allowing SCRAM passwords too. I'm not sure that's a bad idea, but my first reaction is not to like it. md5 is a funny spelling of md5-or-scram. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 15, 2017 at 6:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jeff Janes <jeff.janes@gmail.com> writes: >> On Tue, Mar 14, 2017 at 8:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Why exactly would anyone want "md5 only"? I should think that "scram >>> only" is a sensible pg_hba setting, if the DBA feels that md5 is too >>> insecure, but I do not see the point of "md5 only" in 2017. I think >>> we should just start interpreting that as "md5 or better". > >> Without md5-only, a user who uses \password to change their password from a >> newer client would lock themselves out of connecting again from older >> clients. As a conscious decision (either of the DBA or the user) that >> would be OK, but to have it happen by default would be unfortunate. > > That's a point, but what it implies is that \password needs some input > from the user about whether to generate a SCRAM or MD5-hashed password. > It would be a fatal error to try to drive that off the auth method > that had been used for the current connection, even if \password had a > way to find that out. By definition, your concern is about clients > other than the current one, which might well be coming in from other > addresses and getting challenges based on other pg_hba entries. So > you can't say that "I came in on a SCRAM connection" is sufficient > reason to generate a SCRAM password. > > In short, I don't think that argument refutes my position that "md5" > in pg_hba.conf should be understood as allowing SCRAM passwords too. I have been hacking my way through this thing, and making scram_build_verifier is requiring a bit more refactoring than I thought first: - stored and server keys are hex-encoded using a backend-only routine. I think that those should be instead base64-encoded using what has already been moved in src/common/. - Callers of scram_build_verifier() need to allocate by themselves the verifier, and feed it to the function similarly to MD5. - Frontend-side random generation function is needed, so I have moved pg_frontend_random() into its own file in src/common/. Attached are four patches: - 0001: Switch server and stored keys to be base64-encoded. - 0002: Move pg_frontend_random() to src/common/ - 0003: Move scram_build_verifier() to src/common/ - 0004: Extend PQencryptPassword with a method argument, able to handle SCRAM, MD5 and cleartext. I have not yet done the psql portion with \password -method (too tired), and in 0004 all the calls of PQencryptPassword use "scram" for the purpose of the demonstration. Even if PQencryptPassword is not extended at the end, patches 0001~0003 are necessary anyway. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 03/15/2017 08:48 AM, Michael Paquier wrote: > I have been hacking my way through this thing, and making > scram_build_verifier is requiring a bit more refactoring than I > thought first: > - stored and server keys are hex-encoded using a backend-only routine. > I think that those should be instead base64-encoded using what has > already been moved in src/common/. Or possibly make the hex routines available in the front end as well instead? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Thu, Mar 16, 2017 at 6:46 AM, Joe Conway <mail@joeconway.com> wrote: > On 03/15/2017 08:48 AM, Michael Paquier wrote: >> I have been hacking my way through this thing, and making >> scram_build_verifier is requiring a bit more refactoring than I >> thought first: >> - stored and server keys are hex-encoded using a backend-only routine. >> I think that those should be instead base64-encoded using what has >> already been moved in src/common/. > > Or possibly make the hex routines available in the front end as well > instead? Yeah, but keeping in mind that src/common/ should be kept as small as necessary, the base64 switch made more sense (hex_encode is really small I know). -- Michael
On 03/14/2017 11:14 PM, Tom Lane wrote: > In short, I don't think that argument refutes my position that "md5" > in pg_hba.conf should be understood as allowing SCRAM passwords too. Yeah, let's do that. Here's a patch. I had some terminology trouble with the docs. What do you call a user that has "md5XXXXX" in pgauthid.rolpassword? What about someone with a SCRAM verifier? I used the terms "those users that have an MD5 hash set in the system catalog", and "users that have set their password as a SCRAM verifier", but it feels awkward. The behavior when a user doesn't exist, or doesn't have a valid password, is a bit subtle. Previously, with 'md5' authentication, we would send the client an MD5 challenge, and fail with "invalid password" error after receiving the response. And with 'scram' authentication, we would perform a dummy authentication exchange, with a made-up salt. This is to avoid revealing to an unauthenticated client whether or not the user existed. With this patch, the dummy authentication logic for 'md5' is a bit more complicated. I made it look at the password_encryption GUC, and send the client a dummy MD5 or SCRAM challenge based on that. The idea is that most users presumably have a password of that type, so we use that method for the dummy authentication, to make it look as "normal" as possible. It's not perfect, if password_encryption is set to 'scram', and you probe for a user that has an MD5 password set, you can tell that it's a valid user from the fact that the server sends an MD5 challenge. In practice, I'm not sure how good this dummy authentication thing really is anyway. Even on older versions, I'd wager a guess that if you tried hard enough, you could tell if a user exists or not based on timing, for example. So I think this is good enough. But it's worth noting and discussing. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 03/16/2017 06:52 AM, Heikki Linnakangas wrote: > On 03/14/2017 11:14 PM, Tom Lane wrote: >> In short, I don't think that argument refutes my position that "md5" >> in pg_hba.conf should be understood as allowing SCRAM passwords too. > > Yeah, let's do that. Here's a patch. > > I had some terminology trouble with the docs. What do you call a user > that has "md5XXXXX" in pgauthid.rolpassword? What about someone with a > SCRAM verifier? I used the terms "those users that have an MD5 hash set > in the system catalog", and "users that have set their password as a > SCRAM verifier", but it feels awkward. maybe something like: "those users with an MD5 hashed password" "those users with a SCRAM verifier hash" > The behavior when a user doesn't exist, or doesn't have a valid > password, is a bit subtle. Previously, with 'md5' authentication, we > would send the client an MD5 challenge, and fail with "invalid password" > error after receiving the response. And with 'scram' authentication, we > would perform a dummy authentication exchange, with a made-up salt. This > is to avoid revealing to an unauthenticated client whether or not the > user existed. > > With this patch, the dummy authentication logic for 'md5' is a bit more > complicated. I made it look at the password_encryption GUC, and send the > client a dummy MD5 or SCRAM challenge based on that. The idea is that > most users presumably have a password of that type, so we use that > method for the dummy authentication, to make it look as "normal" as > possible. It's not perfect, if password_encryption is set to 'scram', > and you probe for a user that has an MD5 password set, you can tell that > it's a valid user from the fact that the server sends an MD5 challenge. Presumably if you are unauthenticated you don't have any way to know what password_encryption is set to, so this seems pretty reasonable. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Thu, Mar 16, 2017 at 10:52 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 03/14/2017 11:14 PM, Tom Lane wrote: >> >> In short, I don't think that argument refutes my position that "md5" >> in pg_hba.conf should be understood as allowing SCRAM passwords too. > > > Yeah, let's do that. Here's a patch. At least this has the merit of making \password simpler from psql without a kind of --method option: if the backend is 9.6 or older, just generate a MD5-hash, and SCRAM-hash for newer versions. PQencryptPassword still needs to be extended so as it accepts a hash method though. > I had some terminology trouble with the docs. What do you call a user that > has "md5XXXXX" in pgauthid.rolpassword? What about someone with a SCRAM > verifier? I used the terms "those users that have an MD5 hash set in the > system catalog", and "users that have set their password as a SCRAM > verifier", but it feels awkward. MD5-hashed values are verifiers as well, the use of the term "password" looks weird to me. > The behavior when a user doesn't exist, or doesn't have a valid password, is > a bit subtle. Previously, with 'md5' authentication, we would send the > client an MD5 challenge, and fail with "invalid password" error after > receiving the response. And with 'scram' authentication, we would perform a > dummy authentication exchange, with a made-up salt. This is to avoid > revealing to an unauthenticated client whether or not the user existed. > > With this patch, the dummy authentication logic for 'md5' is a bit more > complicated. I made it look at the password_encryption GUC, and send the > client a dummy MD5 or SCRAM challenge based on that. The idea is that most > users presumably have a password of that type, so we use that method for the > dummy authentication, to make it look as "normal" as possible. It's not > perfect, if password_encryption is set to 'scram', and you probe for a user > that has an MD5 password set, you can tell that it's a valid user from the > fact that the server sends an MD5 challenge. No objections to that. If password_encryption is set off or plain, it is definitely better to switch to scram as this patch does. > In practice, I'm not sure how good this dummy authentication thing really is > anyway. Even on older versions, I'd wager a guess that if you tried hard > enough, you could tell if a user exists or not based on timing, for example. > So I think this is good enough. But it's worth noting and discussing. Regression tests are proving to be useful here (it would be nice to get those committed first!). I am noticing that this patch breaks connection for users with cleartext or md5-hashed verifier when "password" is used in pg_hba.conf. The case where a user has a scram-hashed verifier when "md5" is used in the matching hba entry works though. The missing piece is visibly in CheckPWChallengeAuth(), which should also be used with uaPassword. -# Most users use SCRAM authentication, but some users use older clients -# that don't support SCRAM authentication, and need to be able to log -# in using MD5 authentication. Such users are put in the @md5users -# group, everyone else must use SCRAM. +# Require SCRAM authentication for most users, but make an exception +# for user 'mike', who uses an older client that doesn't support SCRAM +# authentication.## TYPE DATABASE USER ADDRESS METHOD -host all @md5users .example.com md5 +host all mike .example.com md5 Why not still using @md5users? -- Michael
On Thu, Mar 16, 2017 at 11:38 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Mar 16, 2017 at 10:52 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> On 03/14/2017 11:14 PM, Tom Lane wrote: >>> >>> In short, I don't think that argument refutes my position that "md5" >>> in pg_hba.conf should be understood as allowing SCRAM passwords too. >> >> >> Yeah, let's do that. Here's a patch. > > At least this has the merit of making \password simpler from psql > without a kind of --method option: if the backend is 9.6 or older, > just generate a MD5-hash, and SCRAM-hash for newer versions. > PQencryptPassword still needs to be extended so as it accepts a hash > method though. What if the user doesn't want to switch to SCRAM because they also use some connector that hasn't been updated to support it? I bet there will be a lot of people in that situation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 03/17/2017 02:01 PM, Robert Haas wrote: > On Thu, Mar 16, 2017 at 11:38 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> At least this has the merit of making \password simpler from psql >> without a kind of --method option: if the backend is 9.6 or older, >> just generate a MD5-hash, and SCRAM-hash for newer versions. >> PQencryptPassword still needs to be extended so as it accepts a hash >> method though. > > What if the user doesn't want to switch to SCRAM because they also use > some connector that hasn't been updated to support it? > > I bet there will be a lot of people in that situation. You could use the less secure server-side ALTER USER to set the password in that case. Or use an older client. But I agree, it's a bit weird if we don't allow the user to generate an MD5 hash, if he insists. I think we still need a 'method' option to \password. It would make sense to have \password obey password_encryption GUC. Then \password and ALTER USER would do the same thing, which would be less surprising. Although it's also a bit weird for a GUC to affect client-side behavior, so perhaps better to just document that \password will create a SCRAM verifier, unless you explicitly tell it to create an MD5 hash, and add a 'method' parameter to it. - Heikki
On Fri, Mar 17, 2017 at 8:32 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > It would make sense to have \password obey password_encryption GUC. Then > \password and ALTER USER would do the same thing, which would be less > surprising. Although it's also a bit weird for a GUC to affect client-side > behavior, so perhaps better to just document that \password will create a > SCRAM verifier, unless you explicitly tell it to create an MD5 hash, and add > a 'method' parameter to it. Either of those would be fine with me, but I think we should do one of them. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 17, 2017 at 8:32 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> It would make sense to have \password obey password_encryption GUC. Then
>> \password and ALTER USER would do the same thing, which would be less
>> surprising. Although it's also a bit weird for a GUC to affect client-side
>> behavior, so perhaps better to just document that \password will create a
>> SCRAM verifier, unless you explicitly tell it to create an MD5 hash, and add
>> a 'method' parameter to it.
> Either of those would be fine with me, but I think we should do one of them.
I vote for the second one; seems much less surprising and action-at-a-
distance-y. And I think the entire point of \password is to *not* do
exactly what a bare ALTER USER would do, but to superimpose a layer of
best practice on it. We certainly want to define use of SCRAM as being
best practice.
regards, tom lane
On 03/17/2017 05:38 AM, Michael Paquier wrote: > Regression tests are proving to be useful here (it would be nice to > get those committed first!). I am noticing that this patch breaks > connection for users with cleartext or md5-hashed verifier when > "password" is used in pg_hba.conf. Are you sure? It works for me. Here's a slightly updated patch that includes required changes to the test case (now that those have been committed), and some re-wording in the docs, per Joe's suggestion. All the tests pass here. > -# Most users use SCRAM authentication, but some users use older clients > -# that don't support SCRAM authentication, and need to be able to log > -# in using MD5 authentication. Such users are put in the @md5users > -# group, everyone else must use SCRAM. > +# Require SCRAM authentication for most users, but make an exception > +# for user 'mike', who uses an older client that doesn't support SCRAM > +# authentication. > # > # TYPE DATABASE USER ADDRESS METHOD > -host all @md5users .example.com md5 > +host all mike .example.com md5 > Why not still using @md5users? The old example didn't make much sense, now that md5 means "md5 or scram". Could've still used @md5users, but I think this is more clear. The old explanation was wrong or at least misleading anyway, because @md5users doesn't refer to a group, but a flat file that lists roles. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Wed, Mar 22, 2017 at 8:54 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 03/17/2017 05:38 AM, Michael Paquier wrote: >> >> Regression tests are proving to be useful here (it would be nice to >> get those committed first!). I am noticing that this patch breaks >> connection for users with cleartext or md5-hashed verifier when >> "password" is used in pg_hba.conf. > > Are you sure? It works for me. Hm... All the tests of password are still broken for me on macos, but work on Linux: not ok 4 - authentication success for method password, role scram_role # Failed test 'authentication success for method password, role scram_role' # at t/001_password.pl line 39. # got: '2' # expected: '0' not ok 5 - authentication success for method password, role md5_role # Failed test 'authentication success for method password, role md5_role' # at t/001_password.pl line 39. # got: '2' # expected: '0' not ok 6 - authentication success for method password, role plain_role [... dig ... dig ...] And after a lookup the failure is here: - result = get_role_password(port->user_name, &shadow_pass, logdetail); + shadow_pass = get_role_password(port->user_name, logdetail); if (result == STATUS_OK) result is never setup in this code path, so that may crash. And you need to do something like that, which makes the tests pass here: @@ -721,6 +721,7 @@ CheckPasswordAuth(Port *port, char **logdetail) return STATUS_EOF; /* client wouldn't sendpassword */ shadow_pass = get_role_password(port->user_name, logdetail); + result = shadow_pass != NULL ? STATUS_OK : STATUS_ERROR; if (result == STATUS_OK) result = plain_crypt_verify(port->user_name,shadow_pass, passwd, logdetail); > Here's a slightly updated patch that includes required changes to the test > case (now that those have been committed), and some re-wording in the docs, > per Joe's suggestion. All the tests pass here. + verifier = scram_build_verifier(username, shadow_pass, 0); + + (void) parse_scram_verifier(verifier, &state->salt, &state->iterations, + state->StoredKey, state->ServerKey); + pfree(verifier); Not directly a problem of this patch, but scram_build_verifier can return NULL. >> -# Most users use SCRAM authentication, but some users use older clients >> -# that don't support SCRAM authentication, and need to be able to log >> -# in using MD5 authentication. Such users are put in the @md5users >> -# group, everyone else must use SCRAM. >> +# Require SCRAM authentication for most users, but make an exception >> +# for user 'mike', who uses an older client that doesn't support SCRAM >> +# authentication. >> # >> # TYPE DATABASE USER ADDRESS METHOD >> -host all @md5users .example.com md5 >> +host all mike .example.com md5 >> Why not still using @md5users? > > The old example didn't make much sense, now that md5 means "md5 or scram". > Could've still used @md5users, but I think this is more clear. The old > explanation was wrong or at least misleading anyway, because @md5users > doesn't refer to a group, but a flat file that lists roles. This patch introduces for the first time a non-generic user name in pg_hba.conf, that's why, keeping in mind that users could just copy-paste what is in the docs to make their own file, the approach of using an @ marker looks more generic to me. But I won't insist on this point more. I like the move of removing the status error codes from get_role_password(). With this support grid, only users with MD5-hashed verifiers cannot login when the matching hba entry uses scram. -- Michael
On 03/23/2017 06:41 AM, Michael Paquier wrote: > And after a lookup the failure is here: > - result = get_role_password(port->user_name, &shadow_pass, logdetail); > + shadow_pass = get_role_password(port->user_name, logdetail); > if (result == STATUS_OK) > result is never setup in this code path, so that may crash. Ah, of course. For some reason, I has -Wno-maybe-uninitialized in my configure command line. Without that, gcc even warns about that. Fixed, and pushed. Thanks! - Heikki
On Fri, Mar 24, 2017 at 8:36 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 03/23/2017 06:41 AM, Michael Paquier wrote: >> >> And after a lookup the failure is here: >> - result = get_role_password(port->user_name, &shadow_pass, logdetail); >> + shadow_pass = get_role_password(port->user_name, logdetail); >> if (result == STATUS_OK) >> result is never setup in this code path, so that may crash. > > Ah, of course. For some reason, I has -Wno-maybe-uninitialized in my > configure command line. Without that, gcc even warns about that. > > Fixed, and pushed. Thanks! OK, cool. In order to close this thread, I propose to reuse the patches I sent here to make scram_build_verifier() available to frontends: https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com And on top of it modify \password so as it generates a md5 verifier for pre-9.6 servers and a scram one for post-10 servers by looking at the backend version of the current connection. What do you think? -- Michael
On 03/24/2017 03:02 PM, Michael Paquier wrote: > In order to close this thread, I propose to reuse the patches I sent > here to make scram_build_verifier() available to frontends: > https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com > > And on top of it modify \password so as it generates a md5 verifier > for pre-9.6 servers and a scram one for post-10 servers by looking at > the backend version of the current connection. What do you think? Yep, sounds like a plan. - Heikki
On Fri, Mar 24, 2017 at 10:12 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 03/24/2017 03:02 PM, Michael Paquier wrote: >> >> In order to close this thread, I propose to reuse the patches I sent >> here to make scram_build_verifier() available to frontends: >> >> https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com >> >> And on top of it modify \password so as it generates a md5 verifier >> for pre-9.6 servers and a scram one for post-10 servers by looking at >> the backend version of the current connection. What do you think? > > Yep, sounds like a plan. And attached is a set of rebased patches, with createuser and psql's \password extended to do that. -- Michael
Attachment
- 0001-Use-base64-based-encoding-for-stored-and-server-keys.patch
- 0003-Move-routine-to-build-SCRAM-verifier-into-src-common.patch
- 0004-Extend-PQencryptPassword-with-a-hashing-method.patch
- 0005-Extend-psql-s-password-and-createuser-to-handle-SCRA.patch
- 0002-Refactor-frontend-side-random-number-generation.patch
On Sat, Mar 25, 2017 at 1:10 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Mar 24, 2017 at 10:12 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> On 03/24/2017 03:02 PM, Michael Paquier wrote: >>> >>> In order to close this thread, I propose to reuse the patches I sent >>> here to make scram_build_verifier() available to frontends: >>> >>> https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com >>> >>> And on top of it modify \password so as it generates a md5 verifier >>> for pre-9.6 servers and a scram one for post-10 servers by looking at >>> the backend version of the current connection. What do you think? >> >> Yep, sounds like a plan. > > And attached is a set of rebased patches, with createuser and psql's > \password extended to do that. Heikki, are you going to do something about these? We're running out of time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 04/05/2017 06:53 PM, Robert Haas wrote: > On Sat, Mar 25, 2017 at 1:10 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Fri, Mar 24, 2017 at 10:12 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> On 03/24/2017 03:02 PM, Michael Paquier wrote: >>>> >>>> In order to close this thread, I propose to reuse the patches I sent >>>> here to make scram_build_verifier() available to frontends: >>>> >>>> https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com >>>> >>>> And on top of it modify \password so as it generates a md5 verifier >>>> for pre-9.6 servers and a scram one for post-10 servers by looking at >>>> the backend version of the current connection. What do you think? >>> >>> Yep, sounds like a plan. >> >> And attached is a set of rebased patches, with createuser and psql's >> \password extended to do that. > > Heikki, are you going to do something about these? We're running out of time. Sorry I've been procrastinating. I'm on it now. (We need to do something about this, feature freeze or not..) At a quick glance, moving pg_frontend_random() to src/common looks like a non-starter. It uses pglock_thread() which is internal to libpq, so it won't compile as it is. I think I'm going to change scram_build_verifier() to take a pre-generated salt as argument, to avoid the need for a random number generator in src/common. - Heikki
On Thu, Apr 6, 2017 at 2:11 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > At a quick glance, moving pg_frontend_random() to src/common looks like a > non-starter. It uses pglock_thread() which is internal to libpq, so it won't > compile as it is. I think I'm going to change scram_build_verifier() to take > a pre-generated salt as argument, to avoid the need for a random number > generator in src/common. Oops. Need an updated set of patches? -- Michael
On Thu, Apr 6, 2017 at 8:04 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Apr 6, 2017 at 2:11 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> At a quick glance, moving pg_frontend_random() to src/common looks like a >> non-starter. It uses pglock_thread() which is internal to libpq, so it won't >> compile as it is. I think I'm going to change scram_build_verifier() to take >> a pre-generated salt as argument, to avoid the need for a random number >> generator in src/common. > > Oops. Need an updated set of patches? Attached is an updated set of patches anyway. This is similar to the last set, except that I removed the part where pg_frontend_random() is refactored, extending scram_build_verifier() to use a pre-generated salt. Hope that helps. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- 0001-Use-base64-based-encoding-for-stored-and-server-keys.patch
- 0002-Move-routine-to-build-SCRAM-verifier-into-src-common.patch
- 0003-Refactor-frontend-side-random-number-generation.patch
- 0004-Extend-PQencryptPassword-with-a-hashing-method.patch
- 0005-Extend-psql-s-password-and-createuser-to-handle-SCRA.patch
On Wed, Apr 05, 2017 at 08:11:25PM +0300, Heikki Linnakangas wrote: > On 04/05/2017 06:53 PM, Robert Haas wrote: > >On Sat, Mar 25, 2017 at 1:10 AM, Michael Paquier > ><michael.paquier@gmail.com> wrote: > >>On Fri, Mar 24, 2017 at 10:12 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > >>>On 03/24/2017 03:02 PM, Michael Paquier wrote: > >>>> > >>>>In order to close this thread, I propose to reuse the patches I sent > >>>>here to make scram_build_verifier() available to frontends: > >>>> > >>>>https://www.postgresql.org/message-id/CAB7nPqT4yc3u8wspYkWbG088Ndp6asMH3=Zb___Ck89CTvziYQ@mail.gmail.com > >>>> > >>>>And on top of it modify \password so as it generates a md5 verifier > >>>>for pre-9.6 servers and a scram one for post-10 servers by looking at > >>>>the backend version of the current connection. What do you think? > >>> > >>>Yep, sounds like a plan. > >> > >>And attached is a set of rebased patches, with createuser and psql's > >>\password extended to do that. > > > >Heikki, are you going to do something about these? We're running out of time. > > Sorry I've been procrastinating. I'm on it now. (We need to do something > about this, feature freeze or not..) [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Heikki, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Mon, Apr 10, 2017 at 12:53 PM, Noah Misch <noah@leadboat.com> wrote: > On Wed, Apr 05, 2017 at 08:11:25PM +0300, Heikki Linnakangas wrote: >> >Heikki, are you going to do something about these? We're running out of time. >> >> Sorry I've been procrastinating. I'm on it now. (We need to do something >> about this, feature freeze or not..) As there have been some conflicts because of the commit of SASLprep, here is a rebased set of patches. A couple of things worth noting: - SASLprep does an allocation of the prepared password string. It is definitely better to do all the ground work in pg_saslprep but this costs a free() call with a #ifdef FRONTEND at the end of scram_build_verifier(). - Patch 0005 does that: + /* + * Hash password using SCRAM-SHA-256 when connecting to servers + * newer than Postgres 10, and hash with MD5 otherwise. + */ + if (pset.sversion < 100000) + encrypted_password = PQencryptPassword(pw1, user, "md5"); + else + encrypted_password = PQencryptPassword(pw1, user, "scram"); Actually I am thinking that guessing the hashing function according to the value of password_encryption would make the most sense. Thoughts? -- Michael VMware vCenter server www.vmware.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- 0001-Use-base64-based-encoding-for-stored-and-server-keys.patch
- 0002-Move-routine-to-build-SCRAM-verifier-into-src-common.patch
- 0003-Refactor-frontend-side-random-number-generation.patch
- 0004-Extend-PQencryptPassword-with-a-hashing-method.patch
- 0005-Extend-psql-s-password-and-createuser-to-handle-SCRA.patch
On 04/10/2017 08:42 AM, Michael Paquier wrote: > As there have been some conflicts because of the commit of SASLprep, > here is a rebased set of patches. A couple of things worth noting: > - SASLprep does an allocation of the prepared password string. It is > definitely better to do all the ground work in pg_saslprep but this > costs a free() call with a #ifdef FRONTEND at the end of > scram_build_verifier(). > - Patch 0005 does that: > + /* > + * Hash password using SCRAM-SHA-256 when connecting to servers > + * newer than Postgres 10, and hash with MD5 otherwise. > + */ > + if (pset.sversion < 100000) > + encrypted_password = PQencryptPassword(pw1, user, "md5"); > + else > + encrypted_password = PQencryptPassword(pw1, user, "scram"); > Actually I am thinking that guessing the hashing function according to > the value of password_encryption would make the most sense. Thoughts? Thanks! I've been busy on the other thread on future-proofing the protocol with negotiating the SASL mechanism, I'll come back to this once we get that settled. By the end of the week, I presume. Not sure about the password-encryption thing, there are good arguments for either behavior.. - Heikki
On Tue, Apr 11, 2017 at 10:07:12PM +0300, Heikki Linnakangas wrote: > On 04/10/2017 08:42 AM, Michael Paquier wrote: > >As there have been some conflicts because of the commit of SASLprep, > >here is a rebased set of patches. A couple of things worth noting: > >- SASLprep does an allocation of the prepared password string. It is > >definitely better to do all the ground work in pg_saslprep but this > >costs a free() call with a #ifdef FRONTEND at the end of > >scram_build_verifier(). > >- Patch 0005 does that: > >+ /* > >+ * Hash password using SCRAM-SHA-256 when connecting to servers > >+ * newer than Postgres 10, and hash with MD5 otherwise. > >+ */ > >+ if (pset.sversion < 100000) > >+ encrypted_password = PQencryptPassword(pw1, user, "md5"); > >+ else > >+ encrypted_password = PQencryptPassword(pw1, user, "scram"); > >Actually I am thinking that guessing the hashing function according to > >the value of password_encryption would make the most sense. Thoughts? > > Thanks! I've been busy on the other thread on future-proofing the protocol > with negotiating the SASL mechanism, I'll come back to this once we get that > settled. By the end of the week, I presume. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 14 March 2017 at 15:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I was also thinking about that. Basically a primary method and a >> fallback. If that were the case, a gradual transition could happen, and >> if we want \password to enforce best practice it would be ok. > > Why exactly would anyone want "md5 only"? I should think that "scram > only" is a sensible pg_hba setting, if the DBA feels that md5 is too > insecure, but I do not see the point of "md5 only" in 2017. I think > we should just start interpreting that as "md5 or better". +1 As a potential open item, if we treat "md5" as ">= md5" should we not also treat "password" as ">=password"? It seems strange that we still support "password" and yet tell everyonenot to use it. I'd like PG10 to be the version where I don't have to tell people not to use certain things, hash indexes, "password" etc. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 04/18/2017 11:15 AM, Simon Riggs wrote: > As a potential open item, if we treat "md5" as ">= md5" > should we not also treat "password" as ">=password"? > > It seems strange that we still support "password" and yet tell > everyonenot to use it. > > I'd like PG10 to be the version where I don't have to tell people not > to use certain things, hash indexes, "password" etc. Between md5 and scram, the choice is easy, because a user can only have an MD5 hashed or SCRAM "hashed" password in pg_authid. So you present the client an MD5 challenge or a SCRAM challenge, depending on what the user has in pg_authid, or you error out without even trying. But "password" authentication can be used with any kind of a verifier in pg_authid. "password" authentication can be useful, for example, if a user has a SCRAM verifier in pg_authid, but the client doesn't support SCRAM. You could argue that you shouldn't use it even in that situation, you should upgrade the client, or use SSL certs or an ssh tunnel or something else instead. But that's a very different argument than the one for treating "md5" as ">= md5". Also note that LDAP and RADIUS authentication look identical to "password" authentication, on the wire. The only difference is that instead of checking the password against pg_authid, the server checks it against an LDAP or RADIUS server. - Heikki
On 04/18/2017 08:44 AM, Noah Misch wrote: > On Tue, Apr 11, 2017 at 10:07:12PM +0300, Heikki Linnakangas wrote: >> Thanks! I've been busy on the other thread on future-proofing the protocol >> with negotiating the SASL mechanism, I'll come back to this once we get that >> settled. By the end of the week, I presume. > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com Took a bit longer than I predicted, but the other scram open items have now been resolved, and I'll start reviewing this now. I expect there will be 1-2 iterations on this before it's committed. I'll send another status update by Friday, if this isn't committed by then. - Heikki
On 18 April 2017 at 09:25, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 04/18/2017 11:15 AM, Simon Riggs wrote: >> >> As a potential open item, if we treat "md5" as ">= md5" >> should we not also treat "password" as ">=password"? >> >> It seems strange that we still support "password" and yet tell >> everyonenot to use it. >> >> I'd like PG10 to be the version where I don't have to tell people not >> to use certain things, hash indexes, "password" etc. > > > Between md5 and scram, the choice is easy, because a user can only have an > MD5 hashed or SCRAM "hashed" password in pg_authid. So you present the > client an MD5 challenge or a SCRAM challenge, depending on what the user has > in pg_authid, or you error out without even trying. But "password" > authentication can be used with any kind of a verifier in pg_authid. > "password" authentication can be useful, for example, if a user has a SCRAM > verifier in pg_authid, but the client doesn't support SCRAM. Which would be a little strange and defeat the purpose of SCRAM. > You could argue that you shouldn't use it even in that situation, you should > upgrade the client, or use SSL certs or an ssh tunnel or something else > instead. But that's a very different argument than the one for treating > "md5" as ">= md5". > > Also note that LDAP and RADIUS authentication look identical to "password" > authentication, on the wire. The only difference is that instead of checking > the password against pg_authid, the server checks it against an LDAP or > RADIUS server. So the argument is multiple things are dangerous so we do nothing... We have an opportunity to change things because its PG10, so lets not waste the opportunity. Thanks very much for working on SCRAM, its a good feature. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 04/10/2017 08:42 AM, Michael Paquier wrote: > As there have been some conflicts because of the commit of SASLprep, > here is a rebased set of patches. I've committed a modified version of the first patch, to change the on-disk format to RFC 5803, as mentioned on the other thread (https://www.postgresql.org/message-id/351ba574-85ea-d9b8-9689-8c928dd0955d@iki.fi). I'll continue reviewing the rest of the patch on Monday, but one glaring issue is that we cannot add an argument to the existing libpq PQencryptPassword() function, because that's part of the public API. It would break all applications that use PQencryptPassword(). What we need to do is to add a new function. What should we call that? We don't have a tradition of using "Ex" or such suffix to mark extended versions of existing functions. PQencryptPasswordWithMethod(user, pass, method) ? - Heikki
On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I'll continue reviewing the rest of the patch on Monday, but one glaring > issue is that we cannot add an argument to the existing libpq > PQencryptPassword() function, because that's part of the public API. It > would break all applications that use PQencryptPassword(). > > What we need to do is to add a new function. What should we call that? We > don't have a tradition of using "Ex" or such suffix to mark extended > versions of existing functions. PQencryptPasswordWithMethod(user, pass, > method) ? Do we really want to add a new function or have a hard failure? Any application calling PQencryptPassword may trap itself silently if the server uses scram as hba key or if the default is switched to that, from this point of view extending the function makes sense as well. -- Michael
On Sat, Apr 22, 2017 at 7:20 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Do we really want to add a new function or have a hard failure? Any > application calling PQencryptPassword may trap itself silently if the > server uses scram as hba key or if the default is switched to that, > from this point of view extending the function makes sense as well. Attached is a new patch set, taking into account the new format. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 04/22/2017 01:20 AM, Michael Paquier wrote:
> On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> I'll continue reviewing the rest of the patch on Monday, but one glaring
>> issue is that we cannot add an argument to the existing libpq
>> PQencryptPassword() function, because that's part of the public API. It
>> would break all applications that use PQencryptPassword().
>>
>> What we need to do is to add a new function. What should we call that? We
>> don't have a tradition of using "Ex" or such suffix to mark extended
>> versions of existing functions. PQencryptPasswordWithMethod(user, pass,
>> method) ?
>
> Do we really want to add a new function or have a hard failure? Any
> application calling PQencryptPassword may trap itself silently if the
> server uses scram as hba key or if the default is switched to that,
> from this point of view extending the function makes sense as well.
Yeah, there is that. But we simply cannot change the signature of an
existing function. It would not only produce compile-time errors when
building old applications, which would arguably be a good thing, but it
would also cause old binaries to segfault when dynamically linked with
the new libpq.
I think it's clear that we should have a new function that takes the
algorithm as argument. But there are open decisions on what the old
PQencryptPassword() function should do, and also what the new function
should do by default, if you don't specify an algorithm:
A) Have PQencryptPassword() return an md5 hash.
B) Have PQencryptPassword() return a SCRAM verifier
C) Have PQencryptPassword() return a SCRAM verifier if connected to a
v10 server, and an md5 hash otherwise. This is tricky, because
PQencryptPassword doesn't take a PGconn argument. It could behave like
PQescapeString() does, and choose md5/scram depending on the server
version of the last connection that was established.
For the new function, it's probably best to pass a PGconn argument. That
way we can use the connection to determine the default, and it seems to
be a good idea for future-proofing too. And an extra "options" argument
might be good, while we're at it, to e.g. specify the number of
iterations for SCRAM. So all in all, I propose the documentation for
these functions to be (I chose option C from above for this):
----
char *
PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
const char *method, const char *options)
[this paragraph is the same as current PQencryptPassword()]
This function is intended to be used by client applications that wish to
send commands like ALTER ROLE joe PASSWORD 'pwd'. It is good practice to
not send the original cleartext password in such a command, because it
might be exposed in command logs, activity displays and so on. Instead,
use this function to convert the password to encrypted form before it is
sent.
[end of unchanged part]
This function may execute internal queries to the server to determine
appropriate defaults, using the given connection object. The call can
therefore fail if the connection is busy executing another query, or the
current transaction is aborted.
The return value is a string allocated by malloc, or NULL if out of
memory or other error. On error, a suitable message is stored in the
'conn' object. The caller can assume the string doesn't contain any
special characters that would require escaping. Use PQfreemem to free
the result when done with it.
The function arguments are:
conn Connection object for the database where the password is to be changed.
passwd The new password
user Name of the role whose password is to be changed
method Name of the password encryption method to use. Currently supported
methods are "md5" or "scram-sha-256". If method is NULL, the default for
the current database is used. [i.e. this looks at password_encryption]
options Options specific to the encryption method, or NULL to use the
defaults. (This argument is for future expansion, there are currently no
options, and you should always pass NULL.)
char *
PQencryptPassword(const char *passwd, const char *user)
PQencryptPassword is an older, deprecated version of
PQencryptPasswodConn. The difference is that PQencryptPassword does not
require a connection object. The encryption method will be chosen
depending on the server version of the last established connection, and
built-in default options.
----
Thoughts? Unless someone has better ideas or objections, I'll go
implement that.
- Heikki
On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > algorithm as argument. But there are open decisions on what the old > PQencryptPassword() function should do, and also what the new function > should do by default, if you don't specify an algorithm: > > A) Have PQencryptPassword() return an md5 hash. > > B) Have PQencryptPassword() return a SCRAM verifier > > C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10 > server, and an md5 hash otherwise. This is tricky, because PQencryptPassword > doesn't take a PGconn argument. It could behave like PQescapeString() does, > and choose md5/scram depending on the server version of the last connection > that was established. I vote for A - leave PQencryptPassword() as-is, and deprecate it. Tell people to use the new function going forward. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> A) Have PQencryptPassword() return an md5 hash.
>>
>> B) Have PQencryptPassword() return a SCRAM verifier
>>
>> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
>> server, and an md5 hash otherwise. This is tricky, because PQencryptPassword
>> doesn't take a PGconn argument. It could behave like PQescapeString() does,
>> and choose md5/scram depending on the server version of the last connection
>> that was established.
> I vote for A - leave PQencryptPassword() as-is, and deprecate it.
> Tell people to use the new function going forward.
+1. I never much liked that magic behavior of PQescapeString, and don't
think we should replicate it elsewhere, so I definitely don't like (C).
And I don't think we can do (B) because that will break the functionality
altogether when talking to an older server. That leaves (A) plus invent
a new function.
regards, tom lane
On Tue, Apr 25, 2017 at 8:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 25, 2017 at 11:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> A) Have PQencryptPassword() return an md5 hash.
>>
>> B) Have PQencryptPassword() return a SCRAM verifier
>>
>> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
>> server, and an md5 hash otherwise. This is tricky, because PQencryptPassword
>> doesn't take a PGconn argument. It could behave like PQescapeString() does,
>> and choose md5/scram depending on the server version of the last connection
>> that was established.
> I vote for A - leave PQencryptPassword() as-is, and deprecate it.
> Tell people to use the new function going forward.
+1. I never much liked that magic behavior of PQescapeString, and don't
think we should replicate it elsewhere, so I definitely don't like (C).
And I don't think we can do (B) because that will break the functionality
altogether when talking to an older server. That leaves (A) plus invent
a new function.
+1.
On Wed, Apr 26, 2017 at 12:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Yeah, there is that. But we simply cannot change the signature of an > existing function. It would not only produce compile-time errors when > building old applications, which would arguably be a good thing, but it > would also cause old binaries to segfault when dynamically linked with the > new libpq. Sure. > I think it's clear that we should have a new function that takes the > algorithm as argument. But there are open decisions on what the old > PQencryptPassword() function should do, and also what the new function > should do by default, if you don't specify an algorithm: > > A) Have PQencryptPassword() return an md5 hash. > > B) Have PQencryptPassword() return a SCRAM verifier > > C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10 > server, and an md5 hash otherwise. This is tricky, because PQencryptPassword > doesn't take a PGconn argument. It could behave like PQescapeString() does, > and choose md5/scram depending on the server version of the last connection > that was established. Like the rest I vote for A, and document it as deprecated. > ---- > char * > PQencryptPasswordConn(PGconn *conn, > const char *passwd, > const char *user, > const char *method, > const char *options) > > [...] Good for me, I was first thinking as well about having "default" as keyword, NULL is fine as well. Could it be possible to name the new function as PQhashPassword instead of PQencryptPassword? From the previous threads we agreed that encryption makes no sense in this context. -- Michael
On 04/25/2017 06:26 PM, Heikki Linnakangas wrote: > Thoughts? Unless someone has better ideas or objections, I'll go > implement that. This is what I came up with in the end. Some highlights and differences vs the plan I posted earlier: * If algorithm is not given explicitly, PQencryptPasswordConn() queries "SHOW password_encryption", and uses that. That is documented, and it is also documented that it will *not* issue queries, and hence will not block, if the algorithm is given explicitly. That's an important property for some applications. If you want the default behavior without blocking, query "SHOW password_encryption" yourself, and pass the result as the 'algorithm'. * In the previous draft, I envisioned an algorithm-specific 'options' argument. I left it out, on second thoughts (more on that below). * The old PQencryptPassword() function is unchanged, and always uses md5. Per public opinion. We talked about the alternative where PQencryptPasswordConn() would not look at password_encryption, but would always use the strongest possible algorithm supported by the server. That would avoid querying the server. But then I started thinking how this would work, if we make the number of iterations in the SCRAM verifier configurable in the future. The client would not know the desired number of iterations based only on the server version, it would need to query the server, and we would be back to square one. We could add an "options" argument to PQencryptPasswordConn() that the application could use to pass that information, but documenting how to fetch that information, if you don't want PQencryptPasswordConn() to block, gets tedious, and puts a lot of burden to applications. That is why I left out the "options" argument, after all. I'm now thinking that if we add password hashing options like the iteration count in the future, they will be tacked on to password_encryption. For example, "password_encryption='scram-sha-256, iterations=10000". That way, "password_encryption" will always contain enough information to construct password verifiers. What will happen to existing applications using PQencryptPasswordConn() if a new password_encryption algorithm (or options) is added in the future? With this scheme, the application doesn't need to know what algorithms exist. An application can pass algorithm=NULL, to use the server default, or do "show password_encryption" and pass that, for the non-blocking behavior. If you use an older libpq against a new server, so that libpq doesn't know about the algorithm used by the server, you get an error. For reviewer convenience, I put up the patched docs at http://hlinnaka.iki.fi/temp/scram-wip-docs/libpq-misc.html#libpq-pqencryptpasswordconn. Thoughts? Am I missing anything? As an alternative, I considered making password_encryption GUC_REPORT, so that libpq would always know it without having to issue a query. But it feels wrong to send that option to the client on every connection, when it's only needed in the rare case that you use PQencryptPasswordConn(). And if we added new settings like the iteration count in the future, those would need to be GUC_REPORT too. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Wed, Apr 26, 2017 at 6:22 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 04/25/2017 06:26 PM, Heikki Linnakangas wrote: >> Thoughts? Unless someone has better ideas or objections, I'll go >> implement that. > This is what I came up with in the end. Some highlights and differences vs > the plan I posted earlier: > > * If algorithm is not given explicitly, PQencryptPasswordConn() queries > "SHOW password_encryption", and uses that. That is documented, and it is > also documented that it will *not* issue queries, and hence will not block, > if the algorithm is given explicitly. That's an important property for some > applications. If you want the default behavior without blocking, query "SHOW > password_encryption" yourself, and pass the result as the 'algorithm'. TBH, I'd just require the user to specify the algorithm explicitly. Having it run SHOW on the server seems wonky. It introduces a bunch of failure modes for ... no real benefit, I think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 26, 2017 at 6:22 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> * If algorithm is not given explicitly, PQencryptPasswordConn() queries
>> "SHOW password_encryption", and uses that. That is documented, and it is
>> also documented that it will *not* issue queries, and hence will not block,
>> if the algorithm is given explicitly. That's an important property for some
>> applications. If you want the default behavior without blocking, query "SHOW
>> password_encryption" yourself, and pass the result as the 'algorithm'.
> TBH, I'd just require the user to specify the algorithm explicitly.
> Having it run SHOW on the server seems wonky. It introduces a bunch
> of failure modes for ... no real benefit, I think.
Yeah. Blocking is the least of your worries --- what about being in
a failed transaction, for instance?
However, it's not entirely true that there's no real benefit. If the
client app has to specify the algorithm then client code will need
extension every time we add another algorithm. Maybe that's going to
happen so seldom that it's not a big deal, but it would be nice to
avoid that.
Would it be worth making password_encryption be GUC_REPORT so that
it could be guaranteed available, without a server transaction,
from any valid connection? I'm generally resistant to adding
GUC_REPORT flags, but maybe this is a time for an exception.
regards, tom lane
On Wed, Apr 26, 2017 at 12:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Would it be worth making password_encryption be GUC_REPORT so that > it could be guaranteed available, without a server transaction, > from any valid connection? I'm generally resistant to adding > GUC_REPORT flags, but maybe this is a time for an exception. Well, as Heikki just wrote a few messages upthread: --- As an alternative, I considered making password_encryption GUC_REPORT, so that libpq would always know it without having to issue a query. But it feels wrong to send that option to the client on every connection, when it's only needed in the rare case that you use PQencryptPasswordConn(). And if we added new settings like the iteration count in the future, those would need to be GUC_REPORT too. --- I think those are both good points. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Apr 26, 2017 at 7:22 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> We talked about the alternative where PQencryptPasswordConn() would not look
> at password_encryption, but would always use the strongest possible
> algorithm supported by the server. That would avoid querying the server. But
> then I started thinking how this would work, if we make the number of
> iterations in the SCRAM verifier configurable in the future. The client
> would not know the desired number of iterations based only on the server
> version, it would need to query the server, and we would be back to square
> one. We could add an "options" argument to PQencryptPasswordConn() that the
> application could use to pass that information, but documenting how to fetch
> that information, if you don't want PQencryptPasswordConn() to block, gets
> tedious, and puts a lot of burden to applications. That is why I left out
> the "options" argument, after all.
Fine for me.
> I'm now thinking that if we add password hashing options like the iteration
> count in the future, they will be tacked on to password_encryption. For
> example, "password_encryption='scram-sha-256, iterations=10000". That way,
> "password_encryption" will always contain enough information to construct
> password verifiers.
That's possible as well, adding more GUCs for sub-options of a hashing
algorithm is wrong.
> As an alternative, I considered making password_encryption GUC_REPORT, so
> that libpq would always know it without having to issue a query. But it
> feels wrong to send that option to the client on every connection, when it's
> only needed in the rare case that you use PQencryptPasswordConn(). And if we
> added new settings like the iteration count in the future, those would need
> to be GUC_REPORT too.
Agreed, PQencryptPassword is not that widely used..
Here are some comments.
+ /*
+ * Normalize the password with SASLprep. If that doesn't work, because
+ * the password isn't valid UTF-8 or contains prohibited
characters, just
+ * proceed with the original password. (See comments at top of file.)
+ */
+ rc = pg_saslprep(password, &prep_password);
This comment is not true, comments are at the top of auth-scram.c.
+ * The password should already have been processed with SASLprep, if necessary!
+ *
+ * If iterations is 0, default number of iterations is used. The result is
+ * palloc'd or malloc'd, so caller is responsible for freeing it.
+ */
+char *
+scram_build_verifier(const char *salt, int saltlen, int iterations,
+ const char *password)
+{
+ uint8 storedkeybuf[SCRAM_KEY_LEN];
+ uint8 serverkeybuf[SCRAM_KEY_LEN];
+ char *result;
+ char *p;
+ int maxlen;
I think that it is a mistake to move SASLprep out of
scram_build_verifier, because pre-processing the password is not
necessary, it is normally mandatory. The BE/FE versions that you are
adding also duplicate the calls to pg_saslprep().
Using "encrypt" instead of "hash" in the function name :(
+ else if (strcmp(algorithm, "plain") == 0)
+ {
+ crypt_pwd = strdup(passwd);
+ }
This is not documented, and users should be warned about using it as well.
--
Michael
On Tue, Apr 25, 2017 at 8:56 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 04/22/2017 01:20 AM, Michael Paquier wrote:On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:I'll continue reviewing the rest of the patch on Monday, but one glaring
issue is that we cannot add an argument to the existing libpq
PQencryptPassword() function, because that's part of the public API. It
would break all applications that use PQencryptPassword().
What we need to do is to add a new function. What should we call that? We
don't have a tradition of using "Ex" or such suffix to mark extended
versions of existing functions. PQencryptPasswordWithMethod(user, pass,
method) ?
Do we really want to add a new function or have a hard failure? Any
application calling PQencryptPassword may trap itself silently if the
server uses scram as hba key or if the default is switched to that,
from this point of view extending the function makes sense as well.
Yeah, there is that. But we simply cannot change the signature of an existing function. It would not only produce compile-time errors when building old applications, which would arguably be a good thing, but it would also cause old binaries to segfault when dynamically linked with the new libpq.
I think it's clear that we should have a new function that takes the algorithm as argument. But there are open decisions on what the old PQencryptPassword() function should do, and also what the new function should do by default, if you don't specify an algorithm:
A) Have PQencryptPassword() return an md5 hash.
B) Have PQencryptPassword() return a SCRAM verifier
C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10 server, and an md5 hash otherwise. This is tricky, because PQencryptPassword doesn't take a PGconn argument. It could behave like PQescapeString() does, and choose md5/scram depending on the server version of the last connection that was established.
For the new function, it's probably best to pass a PGconn argument. That way we can use the connection to determine the default, and it seems to be a good idea for future-proofing too. And an extra "options" argument might be good, while we're at it, to e.g. specify the number of iterations for SCRAM. So all in all, I propose the documentation for these functions to be (I chose option C from above for this):
----
char *
PQencryptPasswordConn(PGconn *conn,
const char *passwd,
const char *user,
const char *method,
const char *options)
I was just thinking:
- Do we need to provide the method here?
We have connection object itself, it can decide from the type of connection, which method to be used.
-- Thanks, Ashesh
[this paragraph is the same as current PQencryptPassword()]
This function is intended to be used by client applications that wish to send commands like ALTER ROLE joe PASSWORD 'pwd'. It is good practice to not send the original cleartext password in such a command, because it might be exposed in command logs, activity displays and so on. Instead, use this function to convert the password to encrypted form before it is sent.
[end of unchanged part]
This function may execute internal queries to the server to determine appropriate defaults, using the given connection object. The call can therefore fail if the connection is busy executing another query, or the current transaction is aborted.
The return value is a string allocated by malloc, or NULL if out of memory or other error. On error, a suitable message is stored in the 'conn' object. The caller can assume the string doesn't contain any special characters that would require escaping. Use PQfreemem to free the result when done with it.
The function arguments are:
conn
Connection object for the database where the password is to be changed.
passwd
The new password
user
Name of the role whose password is to be changed
method
Name of the password encryption method to use. Currently supported methods are "md5" or "scram-sha-256". If method is NULL, the default for the current database is used. [i.e. this looks at password_encryption]
options
Options specific to the encryption method, or NULL to use the defaults. (This argument is for future expansion, there are currently no options, and you should always pass NULL.)
char *
PQencryptPassword(const char *passwd, const char *user)
PQencryptPassword is an older, deprecated version of PQencryptPasswodConn. The difference is that PQencryptPassword does not require a connection object. The encryption method will be chosen depending on the server version of the last established connection, and built-in default options.
----
Thoughts? Unless someone has better ideas or objections, I'll go implement that.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 27, 2017 at 1:25 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote: > - Do we need to provide the method here? > We have connection object itself, it can decide from the type of connection, > which method to be used. Providing the method is not mandatory. If you look upthread... If the caller of this routine specified method = NULL, then the value of password_encryption on the server is queried automatically and that will be the method used to hash the password. -- Michael
On Thu, Apr 27, 2017 at 9:57 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Apr 27, 2017 at 1:25 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> - Do we need to provide the method here?
> We have connection object itself, it can decide from the type of connection,
> which method to be used.
Providing the method is not mandatory. If you look upthread... If the
caller of this routine specified method = NULL, then the value of
password_encryption on the server is queried automatically and that
will be the method used to hash the password.
I missed that. Sorry.
Thanks.
--Thanks, Ashesh
--
Michael
On 04/26/2017 07:57 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Apr 26, 2017 at 6:22 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> * If algorithm is not given explicitly, PQencryptPasswordConn() queries >>> "SHOW password_encryption", and uses that. That is documented, and it is >>> also documented that it will *not* issue queries, and hence will not block, >>> if the algorithm is given explicitly. That's an important property for some >>> applications. If you want the default behavior without blocking, query "SHOW >>> password_encryption" yourself, and pass the result as the 'algorithm'. > >> TBH, I'd just require the user to specify the algorithm explicitly. >> Having it run SHOW on the server seems wonky. It introduces a bunch >> of failure modes for ... no real benefit, I think. > > Yeah. Blocking is the least of your worries --- what about being in > a failed transaction, for instance? Well, the "ALTER USER" command that you will surely run next, using the same connection, would fail anyway. I don't think running a query is a problem, as long as it's documented, and there's a documented way to avoid it. You could argue, that since we need to document how to avoid the query and the blocking, we might as well always require the application to run the "show password_encryption" query before calling PQencryptPasswordConn(). But I'd like to offer the convenience for the majority of applications that don't mind blocking. > However, it's not entirely true that there's no real benefit. If the > client app has to specify the algorithm then client code will need > extension every time we add another algorithm. Maybe that's going to > happen so seldom that it's not a big deal, but it would be nice to > avoid that. Yeah, I'd like to be prepared. Hopefully we don't need to add another algorithm any time soon, but maybe we will, or maybe we want to add an option for the SCRAM iteration count, for example. - Heikki
On Fri, Apr 21, 2017 at 11:04:14PM +0300, Heikki Linnakangas wrote: > I'll continue reviewing the rest of the patch on Monday, but [...] This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Moreover, this open item has been in progress for 17 days, materially longer than the 1-2 week RMT non-intervention period. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 04/28/2017 07:49 AM, Noah Misch wrote: > On Fri, Apr 21, 2017 at 11:04:14PM +0300, Heikki Linnakangas wrote: >> I'll continue reviewing the rest of the patch on Monday, but [...] > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Moreover, this open item has been in progress for 17 days, materially > longer than the 1-2 week RMT non-intervention period. Refer to the policy on > open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I just pushed some little cleanups that caught my eye while working on this. I need to rebase the patch set I sent earlier, and fix the little things that Michael pointed out, but that shouldn't take long. I plan to push a fix for this on Tuesday (Monday is a national holiday here). - Heikki
On Thu, Apr 27, 2017 at 3:22 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > You could argue, that since we need to document how to avoid the query and > the blocking, we might as well always require the application to run the > "show password_encryption" query before calling PQencryptPasswordConn(). But > I'd like to offer the convenience for the majority of applications that > don't mind blocking. I still think that's borrowing trouble. It just seems like too critical of a thing to have a default -- if the convenience logic gets it wrong and encrypts the password in a manner not intended by the user, that could (a) amount to a security vulnerability or (b) lock you out of your account. If you ask your significant other "where do you want to go to dinner?" and can't get a clear answer out of them after some period of time, it's probably OK to assume they don't care that much and you can just pick something. If you ask the commander-in-chief "which country should we fire the missiles at?" and you don't get a clear and unambiguous answer, just picking something is not a very good idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 05/01/2017 07:04 PM, Robert Haas wrote: > On Thu, Apr 27, 2017 at 3:22 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> You could argue, that since we need to document how to avoid the query and >> the blocking, we might as well always require the application to run the >> "show password_encryption" query before calling PQencryptPasswordConn(). But >> I'd like to offer the convenience for the majority of applications that >> don't mind blocking. > > I still think that's borrowing trouble. It just seems like too > critical of a thing to have a default -- if the convenience logic gets > it wrong and encrypts the password in a manner not intended by the > user, that could (a) amount to a security vulnerability or (b) lock > you out of your account. That's true for a lot of things. The logic isn't complicated; it runs "SHOW password_encryption", and uses the value of that as the algorithm. There's going to be a default, one way or another. The default is going to come from password_encryption, or it's going to be a hard-coded value or logic based on server-version in PQencryptPasswordConn(). Or it's going to be a hard-coded value or logic implemented in every application that uses PQencryptPasswordConn(). I think looking at password_encryption makes the most sense. The application is not in a good position to make the decision, and forcing the end-user to choose every time they change a password is too onerous. > If you ask your significant other "where do > you want to go to dinner?" and can't get a clear answer out of them > after some period of time, it's probably OK to assume they don't care > that much and you can just pick something. If you ask the > commander-in-chief "which country should we fire the missiles at?" and > you don't get a clear and unambiguous answer, just picking something > is not a very good idea. I don't understand the analogy. If the application explicitly passes an algorithm, we use that. If the application passes NULL, it means "you decide, based on the documented rules". If the application passes "mumble-mumble", you get an error. If the "SHOW password_encryption" query fails or libpq doesn't understand the result, you also get an error. What do you think we should do here, then? Make password_encryption GUC_REPORT? Hard-code an algorithm in every application? Remove the convenience logic from PQencryptionPasswordConn(), and document that for a sensible default, the application should first run "SHOW password_encryption", and use the result of that as the algorithm? Or go with the current patch? - Heikki
On Tue, May 2, 2017 at 3:42 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > There's going to be a default, one way or another. The default is going to > come from password_encryption, or it's going to be a hard-coded value or > logic based on server-version in PQencryptPasswordConn(). Or it's going to > be a hard-coded value or logic implemented in every application that uses > PQencryptPasswordConn(). I think looking at password_encryption makes the > most sense. The application is not in a good position to make the decision, > and forcing the end-user to choose every time they change a password is too > onerous. I think there should be no default, and the caller should have to pass the algorithm explicitly. If they want to determine what default to pass by running 'SHOW password_encryption', that's their choice. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 05/02/2017 07:47 PM, Robert Haas wrote: > On Tue, May 2, 2017 at 3:42 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> There's going to be a default, one way or another. The default is going to >> come from password_encryption, or it's going to be a hard-coded value or >> logic based on server-version in PQencryptPasswordConn(). Or it's going to >> be a hard-coded value or logic implemented in every application that uses >> PQencryptPasswordConn(). I think looking at password_encryption makes the >> most sense. The application is not in a good position to make the decision, >> and forcing the end-user to choose every time they change a password is too >> onerous. > > I think there should be no default, and the caller should have to pass > the algorithm explicitly. If they want to determine what default to > pass by running 'SHOW password_encryption', that's their choice. Ok, gotcha. I disagree, I think we should provide a default. Libpq is in a better position to make a good choice than most applications. I've committed the new PQencryptPasswordConn function, with the default behavior of doing "show password_encryption", and the changes to use it in psql and createuser. This closes the open issue with \password. On 04/27/2017 07:03 AM, Michael Paquier wrote: > I think that it is a mistake to move SASLprep out of > scram_build_verifier, because pre-processing the password is not > necessary, it is normally mandatory. The BE/FE versions that you are > adding also duplicate the calls to pg_saslprep(). I played with that a little bit, but decided to keep pg_saslprep() out of scram_build_verifier() after all. It would seem asymmetric to have scram_build_verifier() call pg_saslprep(), but require callers of scram_SaltedPassword() to call it. So for consistency, I think scram_SaltedPassword() should also call pg_saslprep(). That would complicated the scram_SaltedPassword() function, however. It would need to report an OOM error somehow, for starters. Not an insurmountable issue, of course, but it felt cleaner this way, after all, despite the duplication. > Using "encrypt" instead of "hash" in the function name :( Yeah. For better or worse, I've kept the "encrypt" nomenclature everywhere, for consistency. Thanks! - Heikki
On Wed, May 3, 2017 at 10:26 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
-- On 05/02/2017 07:47 PM, Robert Haas wrote:On Tue, May 2, 2017 at 3:42 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:There's going to be a default, one way or another. The default is going to
come from password_encryption, or it's going to be a hard-coded value or
logic based on server-version in PQencryptPasswordConn(). Or it's going to
be a hard-coded value or logic implemented in every application that uses
PQencryptPasswordConn(). I think looking at password_encryption makes the
most sense. The application is not in a good position to make the decision,
and forcing the end-user to choose every time they change a password is too
onerous.
I think there should be no default, and the caller should have to pass
the algorithm explicitly. If they want to determine what default to
pass by running 'SHOW password_encryption', that's their choice.
Ok, gotcha. I disagree, I think we should provide a default. Libpq is in a better position to make a good choice than most applications.
I've committed the new PQencryptPasswordConn function, with the default behavior of doing "show password_encryption", and the changes to use it in psql and createuser. This closes the open issue with \password.
If we're basically just telling people to call SHOW manually, we might as well do it in the default case. I think the wording you put into the docs there is good, as it tells people exactly what happens and how to reproduce it locally.
For the security perspective, perhaps we should have a link to the part of the docs that discusses the different algorithms?
On Wed, May 3, 2017 at 5:26 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Ok, gotcha. I disagree, I think we should provide a default. Libpq is in a > better position to make a good choice than most applications. > > I've committed the new PQencryptPasswordConn function, with the default > behavior of doing "show password_encryption", and the changes to use it in > psql and createuser. This closes the open issue with \password. Well, there is always the counter argument that applications can check for password_encryption by themselves and complete PQencryptPasswordConn and that would be a couple of extra lines in any applications. But honestly, people will appreciate a way to rely on what the backend uses automatically. > On 04/27/2017 07:03 AM, Michael Paquier wrote: >> I think that it is a mistake to move SASLprep out of >> scram_build_verifier, because pre-processing the password is not >> necessary, it is normally mandatory. The BE/FE versions that you are >> adding also duplicate the calls to pg_saslprep(). > > I played with that a little bit, but decided to keep pg_saslprep() out of > scram_build_verifier() after all. It would seem asymmetric to have > scram_build_verifier() call pg_saslprep(), but require callers of > scram_SaltedPassword() to call it. So for consistency, I think > scram_SaltedPassword() should also call pg_saslprep(). That would > complicated the scram_SaltedPassword() function, however. It would need to > report an OOM error somehow, for starters. Not an insurmountable issue, of > course, but it felt cleaner this way, after all, despite the duplication. Okay, I won't fight on that further. -- Michael