Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 02/02/2017 05:50 AM, David Rowley wrote: On 2 February 2017 at 00:13, Heikki Linnakangas wrote: Ok, I'll drop the second patch for now. I committed the first patch after fixing the things you and Michael pointed out. Thanks for the review! dbd69118 caused small compiler warning for me. The attached fixed it. Fixed, thanks! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 2 February 2017 at 00:13, Heikki Linnakangas wrote: > Ok, I'll drop the second patch for now. I committed the first patch after > fixing the things you and Michael pointed out. Thanks for the review! dbd69118 caused small compiler warning for me. The attached fixed it. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services encrypt_password_warning_fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 01/17/2017 11:51 PM, Peter Eisentraut wrote: On 1/3/17 9:09 AM, Heikki Linnakangas wrote: Since not everyone agrees with this approach, I split this patch into two. The first patch refactors things, replacing the isMD5() function with get_password_type(), without changing the representation of pg_authid.rolpassword. That is hopefully uncontroversial. I have checked these patches. The refactoring in the first patch seems sensible. As Michael pointed out, there is still a reference to "plain:" in the first patch. Fixed. The commit message needs to be updated, because the function plain_crypt_verify() was already added in a previous patch. Fixed. I'm not fond of this kind of coding password = encrypt_password(password_type, stmt->role, password); where the 'password' variable has a different meaning before and after. Added a new local variable to avoid the confusion. This error message might be a mistake: elog(ERROR, "unrecognized password type conversion"); I rephrased the error as "cannot encrypt password to requested type", and added a comment explaining that it cannot happen. I hope that helped, I'm not sure why you thought it might've been a mistake. I think some pieces from the second patch could be included in the first patch, e.g., the parts for passwordcheck.c and user.c. I refrained from doing that for now. It would've changed the passwordcheck hook API in an incompatible way. Breaking the API explicitly would be a good thing, if we added the "plain:" prefix, because modules would need to deal with the prefix anyway. But until we do that, better to not break the API for no good reason. And the second patch adds the "plain:" prefix, which not everyone agrees on. The code also gets a little bit dubious, as it introduces an "unknown" password type, which is sometimes treated as plaintext and sometimes as an error. I think this is going be messy. I would skip this patch for now at least. Too much controversy, and we don't know how the rest of the patches for this feature will look like to be able to know if it's worth it. Ok, I'll drop the second patch for now. I committed the first patch after fixing the things you and Michael pointed out. Thanks for the review! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 1/3/17 9:09 AM, Heikki Linnakangas wrote: > Since not everyone agrees with this approach, I split this patch into > two. The first patch refactors things, replacing the isMD5() function > with get_password_type(), without changing the representation of > pg_authid.rolpassword. That is hopefully uncontroversial. I have checked these patches. The refactoring in the first patch seems sensible. As Michael pointed out, there is still a reference to "plain:" in the first patch. The commit message needs to be updated, because the function plain_crypt_verify() was already added in a previous patch. I'm not fond of this kind of coding password = encrypt_password(password_type, stmt->role, password); where the 'password' variable has a different meaning before and after. This error message might be a mistake: elog(ERROR, "unrecognized password type conversion"); I think some pieces from the second patch could be included in the first patch, e.g., the parts for passwordcheck.c and user.c. > And the second > patch adds the "plain:" prefix, which not everyone agrees on. The code also gets a little bit dubious, as it introduces an "unknown" password type, which is sometimes treated as plaintext and sometimes as an error. I think this is going be messy. I would skip this patch for now at least. Too much controversy, and we don't know how the rest of the patches for this feature will look like to be able to know if it's worth it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Thu, Jan 5, 2017 at 10:31 PM, Peter Eisentraut wrote: > On 1/3/17 9:09 AM, Heikki Linnakangas wrote: >> Since not everyone agrees with this approach, I split this patch into >> two. The first patch refactors things, replacing the isMD5() function >> with get_password_type(), without changing the representation of >> pg_authid.rolpassword. That is hopefully uncontroversial. And the second >> patch adds the "plain:" prefix, which not everyone agrees on. >> >> Barring objections I'm going to at least commit the first patch. I think >> we should commit the second one too, but it's not as critical, and the >> first patch matters more for the SCRAM patch, too. > > Is there currently anything to review here for the commit fest? The patches sent here make sense as part of the SCRAM set: https://www.postgresql.org/message-id/6831df67-7641-1a66-4985-268609a48...@iki.fi I was just waiting for Heikki to fix the split of the patches before moving on with an extra lookup though. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 1/3/17 9:09 AM, Heikki Linnakangas wrote: > Since not everyone agrees with this approach, I split this patch into > two. The first patch refactors things, replacing the isMD5() function > with get_password_type(), without changing the representation of > pg_authid.rolpassword. That is hopefully uncontroversial. And the second > patch adds the "plain:" prefix, which not everyone agrees on. > > Barring objections I'm going to at least commit the first patch. I think > we should commit the second one too, but it's not as critical, and the > first patch matters more for the SCRAM patch, too. Is there currently anything to review here for the commit fest? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Tue, Jan 3, 2017 at 11:09 PM, Heikki Linnakangas wrote: > Since not everyone agrees with this approach, I split this patch into two. > The first patch refactors things, replacing the isMD5() function with > get_password_type(), without changing the representation of > pg_authid.rolpassword. That is hopefully uncontroversial. And the second > patch adds the "plain:" prefix, which not everyone agrees on. > > Barring objections I'm going to at least commit the first patch. I think we > should commit the second one too, but it's not as critical, and the first > patch matters more for the SCRAM patch, too. The split does not look correct to me. 0001 has references to the prefix "plain:". -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/21/2016 04:09 AM, Michael Paquier wrote: Thanks for having a look! Attached is a new version, with that bug fixed. I have been able more advanced testing without the crash and things seem to work properly. The attached set of tests is also able to pass for all the combinations of hba configurations and password formats. And looking at the code I don't have more comments. Thanks! Since not everyone agrees with this approach, I split this patch into two. The first patch refactors things, replacing the isMD5() function with get_password_type(), without changing the representation of pg_authid.rolpassword. That is hopefully uncontroversial. And the second patch adds the "plain:" prefix, which not everyone agrees on. Barring objections I'm going to at least commit the first patch. I think we should commit the second one too, but it's not as critical, and the first patch matters more for the SCRAM patch, too. - Heikki 0001-Replace-isMD5-with-a-more-future-proof-way-to-check-.patch Description: invalid/octet-stream 0002-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/2016 01:33 PM, Heikki Linnakangas wrote: I just noticed that the manual for CREATE ROLE says: Note that older clients might lack support for the MD5 authentication mechanism that is needed to work with passwords that are stored encrypted. That's is incorrect. The alternative to MD5 authentication is plain 'password' authentication, and that works just fine with MD5-hashed passwords. I think that sentence is a leftover from when we still supported "crypt" authentication (so I actually get to blame you for that ;-), commit 53a5026b). Back then, it was true that if an MD5 hash was stored in pg_authid, you couldn't do "crypt" authentication. That might have left old clients out in the cold. Now that we're getting SCRAM authentication, we'll need a similar notice there again, for the incompatibility of a SCRAM verifier with MDD5 authentication and vice versa. I went ahead and removed the current bogus notice from the docs. We might need to put back something like it, with the SCRAM patch, but it needs to be rewritten anyway. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Tue, Dec 20, 2016 at 9:23 PM, Heikki Linnakangas wrote: > On 12/16/2016 03:31 AM, Michael Paquier wrote: > Actually, it does still perform that check. There's a new function, > plain_crypt_verify, that passwordcheck uses now. plain_crypt_verify() is > intended to work with any future hash formats we might introduce in the > future (including SCRAM), so that passwordcheck doesn't need to know about > all the hash formats. Bah. I have misread the first version of the patch, and it is indeed keeping the username checks. Now that things don't crash that behaves as expected: =# load 'passwordcheck'; LOAD =# alter role mpaquier password 'mpaquier'; ERROR: 22023: password must not contain user name LOCATION: check_password, passwordcheck.c:101 =# alter role mpaquier password 'md58349d3a1bc8f4f7399b1ff9dea493b15'; ERROR: 22023: password must not contain user name LOCATION: check_password, passwordcheck.c:82 With the patch: >> +case PASSWORD_TYPE_PLAINTEXT: >> +shadow_pass = &shadow_pass[strlen("plain:")]; >> +break; >> It would be a good idea to have a generic routine able to get the plain >> password value. In short I think that we should reduce the amount of >> locations where "plain:" prefix is hardcoded. > > There is such a function included in the patch, get_plain_password(char > *shadow_pass), actually. Contrib/passwordcheck uses it. I figured that in > crypt.c itself, it's OK to do the above directly, but get_plain_password() > is intended to be used elsewhere. The idea would be to have the function not return an allocated string, just a position to it. That would be useful in plain_crypt_verify() for example, for a total of 4 places, including get_plain_password() where the new string allocation is done. Well, it's not like this prefix "plain:" would change anyway in the future nor that it is going to spread much. > Thanks for having a look! Attached is a new version, with that bug fixed. I have been able more advanced testing without the crash and things seem to work properly. The attached set of tests is also able to pass for all the combinations of hba configurations and password formats. And looking at the code I don't have more comments. -- Michael 009_authentication.pl Description: Perl program -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
David, * David Fetter (da...@fetter.org) wrote: > On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote: > > * David Fetter (da...@fetter.org) wrote: > > > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote: > > > > * Heikki Linnakangas (hlinn...@iki.fi) wrote: > > > > > Even if you have a separate "verifier type" column, it's not fully > > > > > normalized, because there's still a dependency between the > > > > > verifier and verifier type columns. You will always need to look > > > > > at the verifier type to make sense of the verifier itself. > > > > > > > > That's true- but you don't need to look at the verifier, or even > > > > have *access* to the verifier, to look at the verifier type. > > > > > > Would a view that shows only what's to the left of the first semicolon > > > suit this purpose? > > > > Obviously a (security barrier...) view or a (security definer) function > > could be used, but I don't believe either is actually a good idea. > > Would you be so kind as to help me understand what's wrong with that idea? For starters, it doubles-down on the assumption that we'll always be happy with that particular separator and implies to anyone watching that they'll be able to trust it. Further, it's additional complication which, at least to my eyes, is entirely in the wrong direction. We could push everything in pg_authid into a single colon-separated text field and call it simpler because we don't have to deal with those silly column things, and we'd have something a lot closer to a unix passwd file too!, but it wouldn't make it a terribly smart thing to do. We aren't a bunch of individual C programs having to parse out things out of flat text files, after all. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote: > David, > > * David Fetter (da...@fetter.org) wrote: > > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote: > > > * Heikki Linnakangas (hlinn...@iki.fi) wrote: > > > > Even if you have a separate "verifier type" column, it's not fully > > > > normalized, because there's still a dependency between the > > > > verifier and verifier type columns. You will always need to look > > > > at the verifier type to make sense of the verifier itself. > > > > > > That's true- but you don't need to look at the verifier, or even > > > have *access* to the verifier, to look at the verifier type. > > > > Would a view that shows only what's to the left of the first semicolon > > suit this purpose? > > Obviously a (security barrier...) view or a (security definer) function > could be used, but I don't believe either is actually a good idea. Would you be so kind as to help me understand what's wrong with that idea? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
David, * David Fetter (da...@fetter.org) wrote: > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote: > > * Heikki Linnakangas (hlinn...@iki.fi) wrote: > > > Even if you have a separate "verifier type" column, it's not fully > > > normalized, because there's still a dependency between the > > > verifier and verifier type columns. You will always need to look > > > at the verifier type to make sense of the verifier itself. > > > > That's true- but you don't need to look at the verifier, or even > > have *access* to the verifier, to look at the verifier type. > > Would a view that shows only what's to the left of the first semicolon > suit this purpose? Obviously a (security barrier...) view or a (security definer) function could be used, but I don't believe either is actually a good idea. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Wed, Dec 21, 2016 at 1:08 AM, David Fetter wrote: > Would a view that shows only what's to the left of the first semicolon > suit this purpose? Of course it would, you would just need to make the routines now checking the shape of MD5 and SCRAM identifiers available at SQL level and feed the strings into them. Now I am not sure that it's worth having a new superuser view for that. pg_roles and pg_shadow hide the information about verifiers. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote: > Heikki, > > * Heikki Linnakangas (hlinn...@iki.fi) wrote: > > Even if you have a separate "verifier type" column, it's not fully > > normalized, because there's still a dependency between the > > verifier and verifier type columns. You will always need to look > > at the verifier type to make sense of the verifier itself. > > That's true- but you don't need to look at the verifier, or even > have *access* to the verifier, to look at the verifier type. Would a view that shows only what's to the left of the first semicolon suit this purpose? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
Heikki, * Heikki Linnakangas (hlinn...@iki.fi) wrote: > Even if you have a separate "verifier type" column, it's not fully > normalized, because there's still a dependency between the verifier > and verifier type columns. You will always need to look at the > verifier type to make sense of the verifier itself. That's true- but you don't need to look at the verifier, or even have *access* to the verifier, to look at the verifier type. That is actually very useful when you start thinking about the downstream side of this- what about the monitoring tool which will want to check and make sure there are only certain verifier types being used? It'll have to be a superuser, or have access to some superuser security defined function, and that really sucks. I'm not saying that we would necessairly want the verifier type to be publicly visible, but being able to see it without being a superuser would be good, imv. > It's more convenient to carry the type information with the verifier > itself, in backend code, in pg_dump, etc. Sure, you could have a > separate "transfer" text format that has the prefix, and strip it > out when the datum enters the system. But it is even simpler to have > only one format, with the prefix, and use that everywhere. It's more convenient when you need to look at both- it's not more convenient when you only wish to look at the verifier type. Further, it means that we have to have a construct that assumes things about the verifier type and verifier- what if a verifier type came along that used a colon? We'd have to do some special magic to handle that correctly, and that just sucks, and anyone who is writing code to generically deal with these fields will end up writing that same code (or forgetting to, and not handling the case correctly). > It might make sense to add a separate column, to e.g. make it easier > to e.g. query for users that have an MD5 verifier. You could do > "WHERE rolverifiertype = 'md5'", instead of "WHERE rolpassword LIKE > 'md5%'". It's not a big difference, though. But even if we did that, > I would still love to have the type information *also* included with > the verifier itself, for convenience. And if we include it in the > verifier itself, adding a separate type column seems more trouble > than it's worth. I don't agree that it's "not a big difference." As I argue above- your approach also assumes that anyone who would like to investigate the verifier type should have access to the verifier itself, which I do not agree with. I also have a hard time buying the argument that it's really so much more convenient to have the verifier type included in the same string as the verifier that we should duplicate that information and then run the risk that we end up with the two not matching or that we won't ever run into complications down the road when our chosen separator causes us difficulties. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Tue, Dec 20, 2016 at 6:37 AM, Heikki Linnakangas wrote: > It's more convenient to carry the type information with the verifier itself, > in backend code, in pg_dump, etc. Sure, you could have a separate "transfer" > text format that has the prefix, and strip it out when the datum enters the > system. But it is even simpler to have only one format, with the prefix, and > use that everywhere. I see your point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/16/2016 03:31 AM, Michael Paquier wrote: On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas wrote: The only way to distinguish, is to know about every verifier kind there is, and check whether rolpassword looks valid as anything else than a plaintext password. And we already got tripped by a bug-of-omission on that once. If we add more verifier formats in the future, it's bound to happen again. Let's nip that source of bugs in the bud. Attached is a patch to implement what I have in mind. OK, I had a look at the patch proposed. -if (!pg_md5_encrypt(username, username, namelen, encrypted)) -elog(ERROR, "password encryption failed"); -if (strcmp(password, encrypted) == 0) -ereport(ERROR, -(errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("password must not contain user name"))); This patch removes the only possible check for MD5 hashes that it has never been done in passwordcheck. It may be fine to remove it, but I would think that it is a good source of example regarding what could be done with MD5 hashes, though limited. So it seems to me that this check should involve as well pg_md5_encrypt on the username and compare if with the MD5 hash given by the caller. Actually, it does still perform that check. There's a new function, plain_crypt_verify, that passwordcheck uses now. plain_crypt_verify() is intended to work with any future hash formats we might introduce in the future (including SCRAM), so that passwordcheck doesn't need to know about all the hash formats. A simple ALTER USER role PASSWORD 'foo' causes a crash: Ah, fixed. +case PASSWORD_TYPE_PLAINTEXT: +shadow_pass = &shadow_pass[strlen("plain:")]; +break; It would be a good idea to have a generic routine able to get the plain password value. In short I think that we should reduce the amount of locations where "plain:" prefix is hardcoded. There is such a function included in the patch, get_plain_password(char *shadow_pass), actually. Contrib/passwordcheck uses it. I figured that in crypt.c itself, it's OK to do the above directly, but get_plain_password() is intended to be used elsewhere. Thanks for having a look! Attached is a new version, with that bug fixed. - Heikki 0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p-2.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/16/2016 05:48 PM, Robert Haas wrote: On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost wrote: * Heikki Linnakangas (hlinn...@iki.fi) wrote: On 12/14/2016 04:57 PM, Stephen Frost wrote: * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: On 12/14/16 5:15 AM, Michael Paquier wrote: I would be tempted to suggest adding the verifier type as a new column of pg_authid Yes please. This discussion seems to continue to come up and I don't entirely understand why we keep trying to shove more things into pg_authid, or worse, into rolpassword. I understand the relational beauty of having a separate column for the verifier type, but I don't think it would be practical. I disagree. Me, too. I think the idea of moving everything into a separate table that allows multiple verifiers is probably not a good thing to do just right now, because that introduces a bunch of additional issues above and beyond what we need to do to get SCRAM implemented. There are administration and policy decisions to be made there that we should not conflate with SCRAM proper. However, Heikki's proposal seems to be that it's reasonable to force rolpassword to be of the form 'type:verifier' in all cases but not reasonable to have separate columns for type and verifier. Eh? I fear we'll just have to agree to disagree here, but I'll try to explain myself one more time. Even if you have a separate "verifier type" column, it's not fully normalized, because there's still a dependency between the verifier and verifier type columns. You will always need to look at the verifier type to make sense of the verifier itself. It's more convenient to carry the type information with the verifier itself, in backend code, in pg_dump, etc. Sure, you could have a separate "transfer" text format that has the prefix, and strip it out when the datum enters the system. But it is even simpler to have only one format, with the prefix, and use that everywhere. It might make sense to add a separate column, to e.g. make it easier to e.g. query for users that have an MD5 verifier. You could do "WHERE rolverifiertype = 'md5'", instead of "WHERE rolpassword LIKE 'md5%'". It's not a big difference, though. But even if we did that, I would still love to have the type information *also* included with the verifier itself, for convenience. And if we include it in the verifier itself, adding a separate type column seems more trouble than it's worth. For comparison, imagine that we added a column to pg_authid for a picture of the user, stored as a bytea. The picture can be in JPEG or PNG format. Looking at the first few bytes of the image, you can tell which one it is. Would it make sense to add a separate "type" column, to tell what format the image is in? I think it would be more convenient and robust to rely on the first bytes of the image data instead. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Sat, Dec 17, 2016 at 5:48 PM, Michael Paquier wrote: > On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas wrote: >> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier >> wrote: >>> From the discussions of last year on -hackers, it was decided to *not* >>> have an additional column per complains from a couple of hackers >>> (Robert you were in this set at this point), ... >> >> Hmm, I don't recall taking that position, but then there are a lot of >> things that I ought to recall and don't. (Ask my wife!) > > [... digging objects of the past ...] > From the past thread: > https://www.postgresql.org/message-id/ca+tgmoy790rphhbogxmbtg6mzsendoxdbxebekaet9zpz8g...@mail.gmail.com > The complain is directed directly to multiple verifiers per users > though, not to have the type in a separate column. Yes, I rather like the separate column. But since Heikki is doing the work (or if he is) I'm not going to gripe too much. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas wrote: > On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier > wrote: >> From the discussions of last year on -hackers, it was decided to *not* >> have an additional column per complains from a couple of hackers >> (Robert you were in this set at this point), ... > > Hmm, I don't recall taking that position, but then there are a lot of > things that I ought to recall and don't. (Ask my wife!) [... digging objects of the past ...] >From the past thread: https://www.postgresql.org/message-id/ca+tgmoy790rphhbogxmbtg6mzsendoxdbxebekaet9zpz8g...@mail.gmail.com The complain is directed directly to multiple verifiers per users though, not to have the type in a separate column. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier wrote: > On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost wrote: >> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: >>> On 12/15/16 8:40 AM, Stephen Frost wrote: >>> > I don't follow why we can't change the syntax for CREATE USER to allow >>> > specifying the verifier type independently. >>> >>> That's what the last patch set I looked at actually does. >> >> Well, same here, but it was quite a while ago and things have progressed >> since then wrt SCRAM, as I understand it... > > From the discussions of last year on -hackers, it was decided to *not* > have an additional column per complains from a couple of hackers > (Robert you were in this set at this point), ... Hmm, I don't recall taking that position, but then there are a lot of things that I ought to recall and don't. (Ask my wife!) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Sat, Dec 17, 2016 at 10:23 AM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> (Robert you were in this set at this point), and the same thing was >> concluded during the informal lunch meeting at PGcon. The point is, >> the existing SCRAM patch set can survive without touching at *all* the >> format of pg_authid. We could block SCRAM authentication when >> "password" is used in pg_hba.conf and as well as when "scram" is used >> with a plain password stored in pg_authid. Or look at the format of >> the string in the catalog if "password" is defined and decide the >> authentication protocol to follow based on that. > > As I mentioned up-thread, moving forward with minimal changes to get > SCRAM in certainly makes sense, but I do think we should be open to > (and, ideally, encouraging people to work towards) having a seperate > table for verifiers with independent columns for type and verifier. Definitely, and you know my position on the matter or I would not have written last year's patch series. Both things are just orthogonal IMO at this point. And it would be good to focus just on one problem at the moment to get it out. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost wrote: > > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > >> On 12/15/16 8:40 AM, Stephen Frost wrote: > >> > I don't follow why we can't change the syntax for CREATE USER to allow > >> > specifying the verifier type independently. > >> > >> That's what the last patch set I looked at actually does. > > > > Well, same here, but it was quite a while ago and things have progressed > > since then wrt SCRAM, as I understand it... > > From the discussions of last year on -hackers, it was decided to *not* > have an additional column per complains from a couple of hackers It seems that, at best, we didn't have consensus on it. Hopefully we are moving in a direction of consensus. > (Robert you were in this set at this point), and the same thing was > concluded during the informal lunch meeting at PGcon. The point is, > the existing SCRAM patch set can survive without touching at *all* the > format of pg_authid. We could block SCRAM authentication when > "password" is used in pg_hba.conf and as well as when "scram" is used > with a plain password stored in pg_authid. Or look at the format of > the string in the catalog if "password" is defined and decide the > authentication protocol to follow based on that. As I mentioned up-thread, moving forward with minimal changes to get SCRAM in certainly makes sense, but I do think we should be open to (and, ideally, encouraging people to work towards) having a seperate table for verifiers with independent columns for type and verifier. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost wrote: > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: >> On 12/15/16 8:40 AM, Stephen Frost wrote: >> > I don't follow why we can't change the syntax for CREATE USER to allow >> > specifying the verifier type independently. >> >> That's what the last patch set I looked at actually does. > > Well, same here, but it was quite a while ago and things have progressed > since then wrt SCRAM, as I understand it... >From the discussions of last year on -hackers, it was decided to *not* have an additional column per complains from a couple of hackers (Robert you were in this set at this point), and the same thing was concluded during the informal lunch meeting at PGcon. The point is, the existing SCRAM patch set can survive without touching at *all* the format of pg_authid. We could block SCRAM authentication when "password" is used in pg_hba.conf and as well as when "scram" is used with a plain password stored in pg_authid. Or look at the format of the string in the catalog if "password" is defined and decide the authentication protocol to follow based on that. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > On 12/15/16 8:40 AM, Stephen Frost wrote: > > I don't follow why we can't change the syntax for CREATE USER to allow > > specifying the verifier type independently. > > That's what the last patch set I looked at actually does. Well, same here, but it was quite a while ago and things have progressed since then wrt SCRAM, as I understand it... Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/15/16 8:40 AM, Stephen Frost wrote: > I don't follow why we can't change the syntax for CREATE USER to allow > specifying the verifier type independently. That's what the last patch set I looked at actually does. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost wrote: > * Heikki Linnakangas (hlinn...@iki.fi) wrote: >> On 12/14/2016 04:57 PM, Stephen Frost wrote: >> >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: >> >>On 12/14/16 5:15 AM, Michael Paquier wrote: >> >>>I would be tempted to suggest adding the verifier type as a new column >> >>>of pg_authid >> >> >> >>Yes please. >> > >> >This discussion seems to continue to come up and I don't entirely >> >understand why we keep trying to shove more things into pg_authid, or >> >worse, into rolpassword. >> >> I understand the relational beauty of having a separate column for >> the verifier type, but I don't think it would be practical. > > I disagree. Me, too. I think the idea of moving everything into a separate table that allows multiple verifiers is probably not a good thing to do just right now, because that introduces a bunch of additional issues above and beyond what we need to do to get SCRAM implemented. There are administration and policy decisions to be made there that we should not conflate with SCRAM proper. However, Heikki's proposal seems to be that it's reasonable to force rolpassword to be of the form 'type:verifier' in all cases but not reasonable to have separate columns for type and verifier. Eh? >> For >> starters, we'd still like to have a self-identifying string format >> like "scram-sha-256:", so that you can conveniently pass the >> verifier as a string to CREATE USER. > > I don't follow why we can't change the syntax for CREATE USER to allow > specifying the verifier type independently. Generally speaking, I don't > expect *users* to be providing actual encoded *verifiers* very often, so > it seems like a bit of extra syntax that pg_dump has to use isn't that > big of a deal. We don't have to change the CREATE USER syntax at all. It could just split on the first colon and put the two halves of the string in different places. Of course, changing the syntax might be a good idea anyway -- or not --- but the point is, right now, when you look at rolpassword, there's not a clear rule for what kind of thing you've got in there. That's absolutely terrible design and has got to be fixed. Heikki's proposal of prefixing every entry with a type and a ':' will solve that problem and I'm not going to roll over in my grave if we do it that way, but there is such a thing as normalization and that technique could be applied here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas wrote: > The only way to distinguish, is to know about every verifier kind there is, > and check whether rolpassword looks valid as anything else than a plaintext > password. And we already got tripped by a bug-of-omission on that once. If > we add more verifier formats in the future, it's bound to happen again. > Let's nip that source of bugs in the bud. Attached is a patch to implement > what I have in mind. OK, I had a look at the patch proposed. -if (!pg_md5_encrypt(username, username, namelen, encrypted)) -elog(ERROR, "password encryption failed"); -if (strcmp(password, encrypted) == 0) -ereport(ERROR, -(errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("password must not contain user name"))); This patch removes the only possible check for MD5 hashes that it has never been done in passwordcheck. It may be fine to remove it, but I would think that it is a good source of example regarding what could be done with MD5 hashes, though limited. So it seems to me that this check should involve as well pg_md5_encrypt on the username and compare if with the MD5 hash given by the caller. The new code is being careful about trying to pass down a plain password, but it is possible to load MD5 hashes directly as well, aka pg_dumpall. A simple ALTER USER role PASSWORD 'foo' causes a crash: #0 0x004764d7 in heap_compute_data_size (tupleDesc=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106 106 VARATT_CAN_MAKE_SHORT(DatumGetPointer(val))) (gdb) bt #0 0x004764d7 in heap_compute_data_size (tupleDesc=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106 #1 0x004781e9 in heap_form_tuple (tupleDescriptor=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:736 #2 0x004784d0 in heap_modify_tuple (tuple=0x277adc8, tupleDesc=0x277f090, replValues=0x7fff1369d030, replIsnull=0x7fff1369d020 "", doReplace=0x7fff1369d010 "") at heaptuple.c:833 #3 0x00673788 in AlterRole (stmt=0x27a4f78) at user.c:845 #4 0x0082aa49 in standard_ProcessUtility (parsetree=0x27a4f78, queryString=0x27a43e8 "alter role ioltas password 'toto';", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, dest=0x27a5300, completionTag=0x7fff1369d5b0 "") at utility.c:711 +case PASSWORD_TYPE_PLAINTEXT: +shadow_pass = &shadow_pass[strlen("plain:")]; +break; It would be a good idea to have a generic routine able to get the plain password value. In short I think that we should reduce the amount of locations where "plain:" prefix is hardcoded. > Alternatively, you could argue that we should forbid storing passwords in > plaintext altogether. I'm OK with that, too, if that's what people prefer. > Then you cannot have a user that can log in with both MD5 and SCRAM > authentication, but it's certainly more secure, and it's easier to document. At the end this may prove to be a bad idea for some developers. In local deployments when working on a backend application with Postgres as backend, it is actually useful to have plain passwords. At least I have found that useful in some stuff I did many years ago. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 12/14/2016 04:57 PM, Stephen Frost wrote: > >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > >>On 12/14/16 5:15 AM, Michael Paquier wrote: > >>>I would be tempted to suggest adding the verifier type as a new column > >>>of pg_authid > >> > >>Yes please. > > > >This discussion seems to continue to come up and I don't entirely > >understand why we keep trying to shove more things into pg_authid, or > >worse, into rolpassword. > > I understand the relational beauty of having a separate column for > the verifier type, but I don't think it would be practical. I disagree. > For > starters, we'd still like to have a self-identifying string format > like "scram-sha-256:", so that you can conveniently pass the > verifier as a string to CREATE USER. I don't follow why we can't change the syntax for CREATE USER to allow specifying the verifier type independently. Generally speaking, I don't expect *users* to be providing actual encoded *verifiers* very often, so it seems like a bit of extra syntax that pg_dump has to use isn't that big of a deal. > I think it'll be much better to > stick to one format, than try to split the verifier into type and > the string, when it enters the catalog table. Apparently, multiple people disagree with this approach. I don't think history is really on your side here either. > >We should have an independent table for the verifiers, which has a > >different column for the verifier type, and either starts off supporting > >multiple verifiers per role or at least gives us the ability to add that > >easily later. We should also move rolvaliduntil to that new table. > > I agree we'll probably need a new table for verifiers. Or turn > rolpassword into an array or something. We discussed that before, > however, and it didn't really go anywhere, so right now I'd like to > get SCRAM in with minimal changes to the rest of the system. There > is a lot of room for improvement once it's in. Using an array strikes me as an absolutely terrible idea- how are you going to handle having different valid_until times then? I do agree with trying to get SCRAM in without changing too much of the rest of the system, but I wanted to make it clear that it's the only point that I agree with for continuing down this path and that we should absolutely be looking to change the CREATE USER syntax to specify the verifier independently, plan to use a different table for the verifiers with an independent column for the verifier type, support multiple verifiers per role, etc, in the (hopefully very near...) future. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/15/2016 03:00 AM, Michael Paquier wrote: On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas wrote: But, a password stored in plaintext works with either MD5 or SCRAM, or any future authentication mechanism. So as soon as we have SCRAM authentication, it becomes somewhat useful again. In a nutshell: auth / stored MD5 SCRAM plaintext - passwordY Y Y md5 Y N Y scram N Y Y If a password is stored in plaintext, it can be used with any authentication mechanism. And the plaintext 'password' authentication mechanism works with any kind of a stored password. But an MD5 hash cannot be used with SCRAM authentication, or vice versa. So.. I have been thinking about this portion of the thread. And what I find the most scary is not the fact that we use plain passwords for SCRAM authentication, it is the fact that we would need to do a catalog lookup earlier in the connection workflow to decide what is the connection protocol to use depending on the username provided in the startup packet if the pg_hba.conf entry matching the user and database names uses "password". I don't see why we would need to do a catalog lookup any earlier. With "password" authentication, the server can simply request the client to send its password. When it receives it, it performs the catalog lookup to get pg_authid.rolpassword. If it's in plaintext, just compare it, if it's an MD5 hash, hash the client's password and compare, and if it's a SCRAM verifier, build a verifier with the same salt and iteration count and compare. And, honestly, why do we actually need to have a support table that spread? SCRAM is designed to be secure, so it seems to me that it would on the contrary a bad idea to encourage the use of plain passwords if we actually think that they should never be used (they are actually useful for located, development instances, not production ones). I agree we should not encourage bad password practices. But as long as we support passwords to be stored in plaintext at all, it makes no sense to not allow them to be used with SCRAM. The fact that you can use a password stored in plaintext with both MD5 and SCRAM is literally the only reason you would store a password in plaintext, so if we don't want to allow that, we should disallow storing passwords in plaintext altogether. So what I would suggest would be to have a support table like that: auth / stored MD5 SCRAM plaintext - passwordY Y N md5 Y N Y scram N N Y I was using 'Y' to indicate that the combination works, and 'N' to indicate that it does not. Assuming you're using the same notation, the above doesn't make any sense. So here is an idea for things to do now: 1) do not change the format of the existing passwords 2) do not change pg_authid 3) block access to instances if "password" or "md5" are used in pg_hba.conf if the user have a SCRAM verifier. 4) block access if "scram" is used and if user has a plain or md5 verifier. 5) Allow access if "scram" is used and if user has a SCRAM verifier. We had a similar discussion regarding verifier/password formats last year but that did not end well. It would be sad to fall back again into this discussion and get no result. If somebody wants to support access to SCRAM with plain password entries, why not. But that would gain a -1 from me regarding the earlier lookup of pg_authid needed to do the decision making on the protocol to use. And I think that we want SCRAM to be designed to be a maximum stable and secure. The bottom line is that at the moment, when plaintext passwords are stored as is, without any indicator that it's a plaintext password, it's ambiguous whether a password is a SCRAM verifier, or if it's a plaintext password that just happens to begin with the word "scram:". That is completely unrelated to which combinations of stored passwords and authentication mechanisms we actually support or allow to work. The only way to distinguish, is to know about every verifier kind there is, and check whether rolpassword looks valid as anything else than a plaintext password. And we already got tripped by a bug-of-omission on that once. If we add more verifier formats in the future, it's bound to happen again. Let's nip that source of bugs in the bud. Attached is a patch to implement what I have in mind. Alternatively, you could argue that we should forbid storing passwords in plaintext altogether. I'm OK with that, too, if that's what people prefer. Then you cannot have a user that can log in with both MD5 and SCRAM authentication, but it's certainly more secure, and it's easier to document. - Heikki 0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-h
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/2016 04:57 PM, Stephen Frost wrote: * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: On 12/14/16 5:15 AM, Michael Paquier wrote: I would be tempted to suggest adding the verifier type as a new column of pg_authid Yes please. This discussion seems to continue to come up and I don't entirely understand why we keep trying to shove more things into pg_authid, or worse, into rolpassword. I understand the relational beauty of having a separate column for the verifier type, but I don't think it would be practical. For starters, we'd still like to have a self-identifying string format like "scram-sha-256:", so that you can conveniently pass the verifier as a string to CREATE USER. I think it'll be much better to stick to one format, than try to split the verifier into type and the string, when it enters the catalog table. We should have an independent table for the verifiers, which has a different column for the verifier type, and either starts off supporting multiple verifiers per role or at least gives us the ability to add that easily later. We should also move rolvaliduntil to that new table. I agree we'll probably need a new table for verifiers. Or turn rolpassword into an array or something. We discussed that before, however, and it didn't really go anywhere, so right now I'd like to get SCRAM in with minimal changes to the rest of the system. There is a lot of room for improvement once it's in. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas wrote: > But, a password stored in plaintext works with either MD5 or SCRAM, or any > future authentication mechanism. So as soon as we have SCRAM authentication, > it becomes somewhat useful again. > > In a nutshell: > > auth / stored MD5 SCRAM plaintext > - > passwordY Y Y > md5 Y N Y > scram N Y Y > > If a password is stored in plaintext, it can be used with any authentication > mechanism. And the plaintext 'password' authentication mechanism works with > any kind of a stored password. But an MD5 hash cannot be used with SCRAM > authentication, or vice versa. So.. I have been thinking about this portion of the thread. And what I find the most scary is not the fact that we use plain passwords for SCRAM authentication, it is the fact that we would need to do a catalog lookup earlier in the connection workflow to decide what is the connection protocol to use depending on the username provided in the startup packet if the pg_hba.conf entry matching the user and database names uses "password". And, honestly, why do we actually need to have a support table that spread? SCRAM is designed to be secure, so it seems to me that it would on the contrary a bad idea to encourage the use of plain passwords if we actually think that they should never be used (they are actually useful for located, development instances, not production ones). So what I would suggest would be to have a support table like that: auth / stored MD5 SCRAM plaintext - passwordY Y N md5 Y N Y scram N N Y So here is an idea for things to do now: 1) do not change the format of the existing passwords 2) do not change pg_authid 3) block access to instances if "password" or "md5" are used in pg_hba.conf if the user have a SCRAM verifier. 4) block access if "scram" is used and if user has a plain or md5 verifier. 5) Allow access if "scram" is used and if user has a SCRAM verifier. We had a similar discussion regarding verifier/password formats last year but that did not end well. It would be sad to fall back again into this discussion and get no result. If somebody wants to support access to SCRAM with plain password entries, why not. But that would gain a -1 from me regarding the earlier lookup of pg_authid needed to do the decision making on the protocol to use. And I think that we want SCRAM to be designed to be a maximum stable and secure. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/2016 11:41 AM, Stephen Frost wrote: * Heikki Linnakangas (hlinn...@iki.fi) wrote: On 14 December 2016 20:12:05 EET, Bruce Momjian wrote: On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote: Storing plaintext passwords has been bad form for just about forever and I wouldn't be sad to see our support of it go. At the least, as was discussed somewhere, but I'm not sure where it ended up, we should give administrators the ability to control what ways a password can be stored. In particular, once a user has migrated all of their users to SCRAM, they should be able to say "don't let new passwords be in any format other than SCRAM-SHA-256". It isn't as bad as it used to be. I remember with PASSWORD was the default. I agree that we should be able to set a policy that says, "we only allow X for password storage". JD Thanks! Stephen -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 14 December 2016 20:12:05 EET, Bruce Momjian wrote: > >On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote: > >> I would so like to just drop support for plain passwords completely > >:) But > >> there's a backwards compatibility issue to think about of course. > >> > >> But -- is there any actual usecase for them anymore? > > > >I thought we recommended 'password' for SSL connections because if you > >use MD5 passwords the password text layout is known and that simplifies > >cryptanalysis. > > No, that makes no sense. And whether you use 'password' or 'md5' > authentication is a different question than whether you store passwords in > plaintext or as md5 hashes. Magnus was asking whether it ever makes sense to > *store* passwords in plaintext. Right. > Since you brought it up, there is a legitimate argument to be made that > 'password' authentication is more secure than 'md5', when SSL is used. > Namely, if an attacker can acquire contents of pg_authid e.g. by stealing a > backup tape, with 'md5' authentication he can log in as any user, using just > the stolen hashes. But with 'password', he needs to reverse the hash first. > It's not a great difference, but it's something. Tunnelled passwords which are stored as hashes is also well understood and comparable to SSH with passwords in /etc/passwd. Storing plaintext passwords has been bad form for just about forever and I wouldn't be sad to see our support of it go. At the least, as was discussed somewhere, but I'm not sure where it ended up, we should give administrators the ability to control what ways a password can be stored. In particular, once a user has migrated all of their users to SCRAM, they should be able to say "don't let new passwords be in any format other than SCRAM-SHA-256". Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 14 December 2016 20:12:05 EET, Bruce Momjian wrote: >On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote: >> I would so like to just drop support for plain passwords completely >:) But >> there's a backwards compatibility issue to think about of course. >> >> But -- is there any actual usecase for them anymore? > >I thought we recommended 'password' for SSL connections because if you >use MD5 passwords the password text layout is known and that simplifies >cryptanalysis. No, that makes no sense. And whether you use 'password' or 'md5' authentication is a different question than whether you store passwords in plaintext or as md5 hashes. Magnus was asking whether it ever makes sense to *store* passwords in plaintext. Since you brought it up, there is a legitimate argument to be made that 'password' authentication is more secure than 'md5', when SSL is used. Namely, if an attacker can acquire contents of pg_authid e.g. by stealing a backup tape, with 'md5' authentication he can log in as any user, using just the stolen hashes. But with 'password', he needs to reverse the hash first. It's not a great difference, but it's something. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote: > I would so like to just drop support for plain passwords completely :) But > there's a backwards compatibility issue to think about of course. > > But -- is there any actual usecase for them anymore? I thought we recommended 'password' for SSL connections because if you use MD5 passwords the password text layout is known and that simplifies cryptanalysis. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > On 12/14/16 5:15 AM, Michael Paquier wrote: > > I would be tempted to suggest adding the verifier type as a new column > > of pg_authid > > Yes please. This discussion seems to continue to come up and I don't entirely understand why we keep trying to shove more things into pg_authid, or worse, into rolpassword. We should have an independent table for the verifiers, which has a different column for the verifier type, and either starts off supporting multiple verifiers per role or at least gives us the ability to add that easily later. We should also move rolvaliduntil to that new table. No, I am specifically *not* concerned with "backwards compatibility" of that table- we continually add to it and change it and applications which are so closely tied to PG that they look at pg_authid need to be updated with nearly every release anyway. What we *do* need to make sure we get correct is what pg_dump/pg_upgrade do, but that's entirely within our control to manage and shouldn't be that much of an issue to implement. Thanks! Stephen signature.asc Description: Digital signature
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/16 5:15 AM, Michael Paquier wrote: > I would be tempted to suggest adding the verifier type as a new column > of pg_authid Yes please. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/2016 12:27 PM, Magnus Hagander wrote: I would so like to just drop support for plain passwords completely :) But there's a backwards compatibility issue to think about of course. But -- is there any actual usecase for them anymore? Hmm. At the moment, I don't think there is. But, a password stored in plaintext works with either MD5 or SCRAM, or any future authentication mechanism. So as soon as we have SCRAM authentication, it becomes somewhat useful again. In a nutshell: auth / stored MD5 SCRAM plaintext - passwordY Y Y md5 Y N Y scram N Y Y If a password is stored in plaintext, it can be used with any authentication mechanism. And the plaintext 'password' authentication mechanism works with any kind of a stored password. But an MD5 hash cannot be used with SCRAM authentication, or vice versa. I just noticed that the manual for CREATE ROLE says: Note that older clients might lack support for the MD5 authentication mechanism that is needed to work with passwords that are stored encrypted. That's is incorrect. The alternative to MD5 authentication is plain 'password' authentication, and that works just fine with MD5-hashed passwords. I think that sentence is a leftover from when we still supported "crypt" authentication (so I actually get to blame you for that ;-), commit 53a5026b). Back then, it was true that if an MD5 hash was stored in pg_authid, you couldn't do "crypt" authentication. That might have left old clients out in the cold. Now that we're getting SCRAM authentication, we'll need a similar notice there again, for the incompatibility of a SCRAM verifier with MDD5 authentication and vice versa. If not, another option could be to just specifically check that it's *not* "md5" or "scram-:". That would invalidate plaintext passwords that have those texts in them of course, but what's the likelyhood of that in reality? Hmm, we have dismissed that risk for the MD5 hashes (and we also have a length check for them), but as we get new hash formats, the risk increases. Someone might well want to use "plain:of:jars" as password. Perhaps we should use a more complicated pattern. I googled around for how others store SCRAM and other password hashes. Many other systems seem to have similar naming schemes. The closest thing to a standard I could find was: https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md Perhaps we should also use something like "$plain$" or "$scram-sha-256"? There's also https://tools.ietf.org/html/rfc5803, which specifies how to store SCRAM verifiers in LDAP. I don't understand enough of LDAP to understand what those actually look like, though, and there were no examples in the RFC. I wonder if we should also worry about storing multiple verifiers in rolpassword? We don't support that now, but we might in the future. It might come handy, if you could easily store multiple hashes in a single string, separated by commas for example. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On 12/14/2016 12:15 PM, Michael Paquier wrote: This work is definitely something that should be done before anything else. Need a patch or are you on it? I'm on it.. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Wed, Dec 14, 2016 at 9:51 AM, Heikki Linnakangas wrote: > On 12/09/2016 10:19 AM, Michael Paquier wrote: > >> On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas >> wrote: >> >>> Couple of things I should write down before I forget: >>> >>> 1. It's a bit cumbersome that the scram verifiers stored in >>> pg_authid.rolpassword don't have any clear indication that they're scram >>> verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I >>> think >>> we should use a "scram-sha-256:" for scram verifiers. >>> >> >> scram-sha-256 would make the most sense to me. >> >> Actually, I think it'd be awfully nice to also prefix plaintext passwords >>> with "plain:", but I'm not sure it's worth breaking the compatibility, if >>> there are tools out there that peek into rolpassword. Thoughts? >>> >> >> pgbouncer is the only thing coming up in mind. It looks at pg_shadow >> for password values. pg_dump'ing data from pre-10 instances will also >> need to adapt. I see tricky the compatibility with the exiting CREATE >> USER PASSWORD command though, so I am wondering if that's worth the >> complication. >> >> 2. It's currently not possible to use the plaintext "password" >>> authentication method, for a user that has a SCRAM verifier in >>> rolpassword. >>> That seems like an oversight. We can't do MD5 authentication with a SCRAM >>> verifier, but "password" we could. >>> >> >> Yeah, that should be possible... >> > > The tip of the work branch can now do SCRAM authentication, when a user > has a plaintext password in pg_authid.rolpassword. The reverse doesn't > work, however: you cannot do plain "password" authentication, when the user > has a SCRAM verifier in pg_authid.rolpassword. It gets worse: plain > "password" authentication doesn't check if the string stored in > pg_authid.rolpassword is a SCRAM authenticator, and treats it as a > plaintext password, so you can do this: > > PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1 > a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d7131 > 49c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f" psql > postgres -h localhost -U scram_user > > I think we're going to have a more bugs like this, if we don't start to > explicitly label plaintext passwords as such. > > So, let's add "plain:" prefix to plaintext passwords, in > pg_authid.rolpassword. With that, these would be valid values in > pg_authid.rolpassword: > > plain:foo > md55a962ce7a24371a10e85627a484cac28 > scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b1 > 9715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56 > bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f > I would so like to just drop support for plain passwords completely :) But there's a backwards compatibility issue to think about of course. But -- is there any actual usecase for them anymore? If not, another option could be to just specifically check that it's *not* "md5" or "scram-:". That would invalidate plaintext passwords that have those texts in them of course, but what's the likelyhood of that in reality? Though I guess that might at least in theory be more bug-prone, so going with a "plain:" prefix seems like a good idea as well. > But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:" > would be invalid. You shouldn't have invalid values in the column, but if > you do, all the authentication mechanisms would reject it. > > It would be nice to also change the format of MD5 passwords to have a > colon, as in "md5:", but that's probably not worth breaking > compatibility for. Almost no-one stores passwords in plaintext, so changing > the format of that wouldn't affect many people, but there might well be > tools out there that peek into MD5 hashes. There are definitely tools that do that, so +1 on leaving that alone. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)
On Wed, Dec 14, 2016 at 5:51 PM, Heikki Linnakangas wrote: > The tip of the work branch can now do SCRAM authentication, when a user has > a plaintext password in pg_authid.rolpassword. The reverse doesn't work, > however: you cannot do plain "password" authentication, when the user has a > SCRAM verifier in pg_authid.rolpassword. It gets worse: plain "password" > authentication doesn't check if the string stored in pg_authid.rolpassword > is a SCRAM authenticator, and treats it as a plaintext password, so you can > do this: > > PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f" > psql postgres -h localhost -U scram_user This one's fun. > I think we're going to have a more bugs like this, if we don't start to > explicitly label plaintext passwords as such. > > So, let's add "plain:" prefix to plaintext passwords, in > pg_authid.rolpassword. With that, these would be valid values in > pg_authid.rolpassword: > > [...] > > But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:" > would be invalid. You shouldn't have invalid values in the column, but if > you do, all the authentication mechanisms would reject it. I would be tempted to suggest adding the verifier type as a new column of pg_authid, but as CREATE USER PASSWORD accepts strings with md5 prefix as-is for ages using the "plain:" prefix is definitely a better plan. My opinion on the matter has changed compared to a couple of months back. > It would be nice to also change the format of MD5 passwords to have a colon, > as in "md5:", but that's probably not worth breaking compatibility > for. Almost no-one stores passwords in plaintext, so changing the format of > that wouldn't affect many people, but there might well be tools out there > that peek into MD5 hashes. Yes, let's not take this road. This work is definitely something that should be done before anything else. Need a patch or are you on it? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers