pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
44+ messages / 10 participants
[nested] [flat]
Commits from this thread:
2017-02-01 dbd69118c0 Replace isMD5() with a more future-proof way to check if pw is encrypted.
2017-01-03 31c54096a1 7546c135dc 65a7f190b2 ada2cdb610 470af1f41c 7e3ae54559 Remove bogus notice that older clients might not work with MD5 passwords.
* pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 08:51 Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 2 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-14 08:51 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/09/2016 10:19 AM, Michael Paquier wrote:
> On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"
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:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f
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:<hash>", 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.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 10:15 Michael Paquier <michael.paquier@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 2 replies; 44+ messages in thread
From: Michael Paquier @ 2016-12-14 10:15 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Wed, Dec 14, 2016 at 5:51 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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:<hash>", 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 10:27 Magnus Hagander <magnus@hagander.net>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 2 replies; 44+ messages in thread
From: Magnus Hagander @ 2016-12-14 10:27 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Wed, Dec 14, 2016 at 9:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 12/09/2016 10:19 AM, Michael Paquier wrote:
>
>> On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas <hlinnaka@iki.fi>
>> 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<something>" or "scram-<something>:<something>". 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:<hash>", 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/
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 10:32 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael.paquier@gmail.com>
1 sibling, 0 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-14 10:32 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 11:33 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 2 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-14 11:33 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
-----------------------------------------
password Y 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<something>" or "scram-<something>:<something>". 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$<password>" or
"$scram-sha-256$<iterations>$<salt>$<key>$"?
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 14:52 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Michael Paquier <michael.paquier@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2016-12-14 14:52 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 14:57 Stephen Frost <sfrost@snowman.net>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-14 14:57 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
* Peter Eisentraut (peter.eisentraut@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 18:12 Bruce Momjian <bruce@momjian.us>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 1 reply; 44+ messages in thread
From: Bruce Momjian @ 2016-12-14 18:12 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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 <bruce@momjian.us> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 19:34 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-14 19:34 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; Magnus Hagander <magnus@hagander.net>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 14 December 2016 20:12:05 EET, Bruce Momjian <bruce@momjian.us> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 19:41 Stephen Frost <sfrost@snowman.net>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-14 19:41 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Bruce Momjian <bruce@momjian.us>; Magnus Hagander <magnus@hagander.net>; Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 14 December 2016 20:12:05 EET, Bruce Momjian <bruce@momjian.us> 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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-14 19:58 Joshua D. Drake <jd@commandprompt.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 44+ messages in thread
From: Joshua D. Drake @ 2016-12-14 19:58 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Bruce Momjian <bruce@momjian.us>; Magnus Hagander <magnus@hagander.net>; Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/14/2016 11:41 AM, Stephen Frost wrote:
> * Heikki Linnakangas (hlinnaka@iki.fi) wrote:
>> On 14 December 2016 20:12:05 EET, Bruce Momjian <bruce@momjian.us> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-15 01:00 Michael Paquier <michael.paquier@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2016-12-15 01:00 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
> -----------------------------------------
> password Y 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
-----------------------------------------
password Y 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-15 09:57 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-15 09:57 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/14/2016 04:57 PM, Stephen Frost wrote:
> * Peter Eisentraut (peter.eisentraut@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:<stuff>", 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-15 12:48 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-15 12:48 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/15/2016 03:00 AM, Michael Paquier wrote:
> On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
>> -----------------------------------------
>> password Y 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
> -----------------------------------------
> password Y 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch (invalid/octet-stream, 22.5K) download
From 25670580136d6d0cc5f7b519be2176870bca71a1 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 15 Dec 2016 14:25:54 +0200
Subject: [PATCH 1/1] Use "plain:" prefix for plaintext passwords stored in
pg_authid.
So far, the rule has been that if pg_authid.rolpassword begins with "md5"
and has the right length, it's an MD5 hash, otherwise it's a plaintext
password. That kind of pattern matching gets more awkward when we add
new kinds of verifiers, like the upcoming SCRAM verifiers. To be able to
distinguish different kinds of password hashes, use a "plain:" prefix
for plaintext passwords. With that, every password stored in pg_authid
has either "plain:" or "md5" prefix, and future password schemes can
likewise use different prefixes.
Note that this doesn't change the format of MD5 hashed passwords, which
is what almost everyone uses.
This changes check_password_hook in an incompatible way. The
'password_type' argument is removed, and the hook is expected to call
get_password_type() on the password verifier to determine its type.
Moreover, when a plaintext password is passed to the hook, it will have
the "plain:" prefix. That is a more subtle change!
In the passing, remove the bogus notice from the documentation of CREATE
ROLE, claiming that older clients might not work with MD5 hashed passwords.
That was written when we still had "crypt" authentication, and it was
referring to the fact that an older client might support "crypt"
authentication but not "md5". But we haven't supported "crypt" for years.
(As soon as we add a new authentication mechanism that doesn't work with
MD5 hashes, we'll need a similar notice again, though!)
Discussion: https://www.postgresql.org/message-id/2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi
---
contrib/passwordcheck/passwordcheck.c | 133 +++++++++++------------
doc/src/sgml/catalogs.sgml | 4 +-
doc/src/sgml/ref/create_role.sgml | 6 --
src/backend/commands/user.c | 66 ++++++------
src/backend/libpq/crypt.c | 192 +++++++++++++++++++++++++++-------
src/backend/utils/misc/guc.c | 1 +
src/include/commands/user.h | 13 +--
src/include/common/md5.h | 4 -
src/include/libpq/crypt.h | 20 +++-
9 files changed, 273 insertions(+), 166 deletions(-)
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index a0db89b..3d06964c 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -21,7 +21,7 @@
#endif
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "fmgr.h"
PG_MODULE_MAGIC;
@@ -50,87 +50,78 @@ extern void _PG_init(void);
*/
static void
check_password(const char *username,
- const char *password,
- int password_type,
+ const char *shadow_pass,
Datum validuntil_time,
bool validuntil_null)
{
- int namelen = strlen(username);
- int pwdlen = strlen(password);
- char encrypted[MD5_PASSWD_LEN + 1];
- int i;
- bool pwd_has_letter,
- pwd_has_nonletter;
-
- switch (password_type)
+ if (get_password_type(shadow_pass) != PASSWORD_TYPE_PLAINTEXT)
{
- case PASSWORD_TYPE_MD5:
-
- /*
- * Unfortunately we cannot perform exhaustive checks on encrypted
- * passwords - we are restricted to guessing. (Alternatively, we
- * could insist on the password being presented non-encrypted, but
- * that has its own security disadvantages.)
- *
- * We only check for username = password.
- */
- 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")));
- break;
-
- case PASSWORD_TYPE_PLAINTEXT:
-
+ /*
+ * Unfortunately we cannot perform exhaustive checks on encrypted
+ * passwords - we are restricted to guessing. (Alternatively, we
+ * could insist on the password being presented non-encrypted, but
+ * that has its own security disadvantages.)
+ *
+ * We only check for username = password.
+ */
+ char *logdetail;
+
+ if (plain_crypt_verify(username, shadow_pass, username, &logdetail) == STATUS_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+ }
+ else
+ {
+ /*
+ * For unencrypted passwords we can perform better checks
+ */
+ char *password = get_plain_password(shadow_pass);
+ int pwdlen = strlen(password);
+ int i;
+ bool pwd_has_letter,
+ pwd_has_nonletter;
+
+ /* enforce minimum length */
+ if (pwdlen < MIN_PWD_LENGTH)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is too short")));
+
+ /* check if the password contains the username */
+ if (strstr(password, username))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+
+ /* check if the password contains both letters and non-letters */
+ pwd_has_letter = false;
+ pwd_has_nonletter = false;
+ for (i = 0; i < pwdlen; i++)
+ {
/*
- * For unencrypted passwords we can perform better checks
+ * isalpha() does not work for multibyte encodings but let's
+ * consider non-ASCII characters non-letters
*/
-
- /* enforce minimum length */
- if (pwdlen < MIN_PWD_LENGTH)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is too short")));
-
- /* check if the password contains the username */
- if (strstr(password, username))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password must not contain user name")));
-
- /* check if the password contains both letters and non-letters */
- pwd_has_letter = false;
- pwd_has_nonletter = false;
- for (i = 0; i < pwdlen; i++)
- {
- /*
- * isalpha() does not work for multibyte encodings but let's
- * consider non-ASCII characters non-letters
- */
- if (isalpha((unsigned char) password[i]))
- pwd_has_letter = true;
- else
- pwd_has_nonletter = true;
- }
- if (!pwd_has_letter || !pwd_has_nonletter)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ if (isalpha((unsigned char) password[i]))
+ pwd_has_letter = true;
+ else
+ pwd_has_nonletter = true;
+ }
+ if (!pwd_has_letter || !pwd_has_nonletter)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password must contain both letters and nonletters")));
#ifdef USE_CRACKLIB
- /* call cracklib to check password */
- if (FascistCheck(password, CRACKLIB_DICTPATH))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is easily cracked")));
+ /* call cracklib to check password */
+ if (FascistCheck(password, CRACKLIB_DICTPATH))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is easily cracked")));
#endif
- break;
- default:
- elog(ERROR, "unrecognized password type: %d", password_type);
- break;
+ pfree(password);
}
/* all checks passed, password is ok */
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 11c2019..abf98af 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1320,8 +1320,8 @@
will be of the user's password concatenated to their user name.
For example, if user <literal>joe</> has password <literal>xyzzy</>,
<productname>PostgreSQL</> will store the md5 hash of
- <literal>xyzzyjoe</>. A password that does not follow that
- format is assumed to be unencrypted.
+ <literal>xyzzyjoe</>. If a password is stored unencrypted, the
+ column will begin with the string <literal>plain:</literal>.
</entry>
</row>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8..a3b8ed9 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -235,12 +235,6 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
password string). This allows reloading of encrypted
passwords during dump/restore.
</para>
-
- <para>
- Note that older clients might lack support for the MD5
- authentication mechanism that is needed to work with passwords
- that are stored encrypted.
- </para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index adc6b99..e9061f8 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -29,7 +29,7 @@
#include "commands/dbcommands.h"
#include "commands/seclabel.h"
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "miscadmin.h"
#include "storage/lmgr.h"
#include "utils/acl.h"
@@ -81,7 +81,6 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
ListCell *option;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
bool issuper = false; /* Make the user a superuser? */
bool inherit = true; /* Auto inherit privileges? */
bool createrole = false; /* Can this user create roles? */
@@ -368,11 +367,23 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, stmt->role,
+ password);
+
(*check_password_hook) (stmt->role,
- password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build a tuple to insert
@@ -393,17 +404,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, stmt->role, strlen(stmt->role),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encode the password to the requested format. */
+ password = encode_password(password_type, stmt->role, password);
+ new_record[Anum_pg_authid_rolpassword - 1] =
+ CStringGetTextDatum(password);
}
else
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -506,7 +510,6 @@ AlterRole(AlterRoleStmt *stmt)
char *rolename = NULL;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
int issuper = -1; /* Make the user a superuser? */
int inherit = -1; /* Auto inherit privileges? */
int createrole = -1; /* Can this user create roles? */
@@ -743,11 +746,23 @@ AlterRole(AlterRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, rolename,
+ password);
+
(*check_password_hook) (rolename,
- password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build an updated tuple, perusing the information just obtained
@@ -804,17 +819,8 @@ AlterRole(AlterRoleStmt *stmt)
/* password */
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, rolename, strlen(rolename),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encode the password to the requested format. */
+ password = encode_password(password_type, rolename, password);
new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
}
@@ -1232,7 +1238,7 @@ RenameRole(const char *oldname, const char *newname)
datum = heap_getattr(oldtuple, Anum_pg_authid_rolpassword, dsc, &isnull);
- if (!isnull && isMD5(TextDatumGetCString(datum)))
+ if (!isnull && get_password_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
{
/* MD5 uses the username as salt, so just clear it on a rename */
repl_repl[Anum_pg_authid_rolpassword - 1] = true;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 7e9124f..5df049f 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -1,10 +1,8 @@
/*-------------------------------------------------------------------------
*
* crypt.c
- * Look into the password file and check the encrypted password with
- * the one passed in from the frontend.
- *
- * Original coding by Todd A. Brandys
+ * Functions for dealing with encrypted passwords stored in
+ * pg_authid.rolpassword
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -106,6 +104,98 @@ get_role_password(const char *role, char **shadow_pass, char **logdetail)
}
/*
+ * What kind of a password verifier is 'shadow_pass'?
+ */
+PasswordType
+get_password_type(const char *shadow_pass)
+{
+ if (strncmp(shadow_pass, "plain:", 6) == 0 && strlen(shadow_pass) >= 6)
+ return PASSWORD_TYPE_PLAINTEXT;
+ if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
+ return PASSWORD_TYPE_MD5;
+ return PASSWORD_TYPE_UNKNOWN;
+}
+
+/*
+ * Given a plaintext password verifier, "plain:<password>", return the
+ * actual plaintext password without the prefix. The returned password is
+ * palloc'd.
+ */
+char *
+get_plain_password(const char *shadow_pass)
+{
+ if (strlen(shadow_pass) < 6 || memcmp(shadow_pass, "plain:", 6) != 0)
+ elog(ERROR, "password is not a plaintext password verifier");
+
+ return pstrdup(&shadow_pass[strlen("plain:")]);
+}
+
+/*
+ * Given a user-supplied password, convert it into a verifier of
+ * 'target_type' kind.
+ *
+ * If the password looks like a valid MD5 hash, it is stored as it is.
+ * We cannot reverse the hash, so even if the caller requested a plaintext
+ * plaintext password, the MD5 hash is returned.
+ *
+ * If the password follows the "plain:<password>" format, it is interpreted
+ * as a plaintext password. If an MD5 hash was requested, it is hashed,
+ * otherwise it is returned as it is.
+ */
+char *
+encode_password(PasswordType target_type, const char *role,
+ const char *password)
+{
+ PasswordType guessed_type = get_password_type(password);
+ const char *plain_password;
+ char *encrypted_password;
+
+ switch (target_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ elog(ERROR, "unknown target password type");
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ return psprintf("plain:%s", password);
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ return pstrdup(password);
+
+ case PASSWORD_TYPE_MD5:
+ /* We cannot convert an MD5 hash back to plaintext. Store the hash. */
+ return pstrdup(password);
+ }
+ break;
+
+ case PASSWORD_TYPE_MD5:
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ case PASSWORD_TYPE_PLAINTEXT:
+ encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+
+ /* strip the possible "plain:" prefix, and hash */
+ if (guessed_type == PASSWORD_TYPE_PLAINTEXT)
+ plain_password = &password[strlen("plain:")];
+ else
+ plain_password = password;
+ if (!pg_md5_encrypt(plain_password, role, strlen(role),
+ encrypted_password))
+ elog(ERROR, "password encryption failed");
+ return encrypted_password;
+
+ case PASSWORD_TYPE_MD5:
+ return pstrdup(password);
+ }
+ }
+
+ elog(ERROR, "unrecognized password type conversion");
+}
+
+/*
* Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
*
* 'shadow_pass' is the user's correct password or password hash, as stored
@@ -135,32 +225,40 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
* below: the only possible error is out-of-memory, which is unlikely, and
* if it did happen adding a psprintf call would only make things worse.
*/
- if (isMD5(shadow_pass))
- {
- /* stored password already encrypted, only do salt */
- if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
- return STATUS_ERROR;
- }
- }
- else
+ switch(get_password_type(shadow_pass))
{
- /* stored password is plain, double-encrypt */
- if (!pg_md5_encrypt(shadow_pass,
- role,
- strlen(role),
- crypt_pwd2))
- {
- return STATUS_ERROR;
- }
- if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
+ case PASSWORD_TYPE_MD5:
+ /* stored password already encrypted, only do salt */
+ if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ /* stored password is plain, double-encrypt */
+ if (!pg_md5_encrypt(&shadow_pass[strlen("plain:")],
+ role,
+ strlen(role),
+ crypt_pwd2))
+ {
+ return STATUS_ERROR;
+ }
+ if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ default:
+ /* unknown password hash format. */
+ *logdetail = psprintf(_("User \"%s\" has a password that cannot be used with MD5 authentication."),
+ role);
return STATUS_ERROR;
- }
}
if (strcmp(client_pass, crypt_pwd) == 0)
@@ -198,22 +296,36 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
* the password the client sent, and compare the hashes. Otherwise
* compare the plaintext passwords directly.
*/
- if (isMD5(shadow_pass))
+ switch (get_password_type(shadow_pass))
{
- if (!pg_md5_encrypt(client_pass,
- role,
- strlen(role),
- crypt_client_pass))
- {
+ case PASSWORD_TYPE_MD5:
+ if (!pg_md5_encrypt(client_pass,
+ role,
+ strlen(role),
+ crypt_client_pass))
+ {
+ /*
+ * We do not bother setting logdetail for pg_md5_encrypt failure:
+ * the only possible error is out-of-memory, which is unlikely,
+ * and if it did happen adding a psprintf call would only make
+ * things worse.
+ */
+ return STATUS_ERROR;
+ }
+ client_pass = crypt_client_pass;
+ break;
+ case PASSWORD_TYPE_PLAINTEXT:
+ shadow_pass = &shadow_pass[strlen("plain:")];
+ break;
+
+ default:
/*
- * We do not bother setting logdetail for pg_md5_encrypt failure:
- * the only possible error is out-of-memory, which is unlikely,
- * and if it did happen adding a psprintf call would only make
- * things worse.
+ * This shouldn't happen. Plain "password" authentication should be
+ * possible with any kind of stored password hash.
*/
+ *logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
+ role);
return STATUS_ERROR;
- }
- client_pass = crypt_client_pass;
}
if (strcmp(client_pass, shadow_pass) == 0)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a025117..ab71f47 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -40,6 +40,7 @@
#include "commands/trigger.h"
#include "funcapi.h"
#include "libpq/auth.h"
+#include "libpq/crypt.h"
#include "libpq/be-fsstubs.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 102c2a5..9cb3629 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -15,21 +15,10 @@
#include "nodes/parsenodes.h"
#include "parser/parse_node.h"
-
-/*
- * Types of password, for Password_encryption GUC and the password_type
- * argument of the check-password hook.
- */
-typedef enum PasswordType
-{
- PASSWORD_TYPE_PLAINTEXT = 0,
- PASSWORD_TYPE_MD5
-} PasswordType;
-
extern int Password_encryption; /* GUC */
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *password, int password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index 4a04320..a29731b 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -18,10 +18,6 @@
#define MD5_PASSWD_LEN 35
-#define isMD5(passwd) (strncmp(passwd, "md5", 3) == 0 && \
- strlen(passwd) == MD5_PASSWD_LEN)
-
-
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
extern bool pg_md5_binary(const void *buff, size_t len, void *outbuf);
extern bool pg_md5_encrypt(const char *passwd, const char *salt,
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 229ce76..ed47c05 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,7 +15,25 @@
#include "datatype/timestamp.h"
-extern int get_role_password(const char *role, char **shadow_pass, char **logdetail);
+/*
+ * Types of password.
+ *
+ * This is also used for the password_encryption GUC.
+ */
+typedef enum PasswordType
+{
+ PASSWORD_TYPE_UNKNOWN = -1,
+ PASSWORD_TYPE_PLAINTEXT = 0,
+ PASSWORD_TYPE_MD5
+} PasswordType;
+
+extern PasswordType get_password_type(const char *shadow_pass);
+extern char *get_plain_password(const char *shadow_pass);
+extern char *encode_password(PasswordType target_type, const char *role,
+ const char *password);
+
+extern int get_role_password(const char *role, char **shadow_pass,
+ char **logdetail);
extern int md5_crypt_verify(const char *role, const char *shadow_pass,
const char *client_pass, const char *md5_salt,
--
2.10.2
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-15 13:40 Stephen Frost <sfrost@snowman.net>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 2 replies; 44+ messages in thread
From: Stephen Frost @ 2016-12-15 13:40 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 12/14/2016 04:57 PM, Stephen Frost wrote:
> >* Peter Eisentraut (peter.eisentraut@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:<stuff>", 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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-16 01:31 Michael Paquier <michael.paquier@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2016-12-16 01:31 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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 0x00000000004764d7 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 0x00000000004764d7 in heap_compute_data_size (tupleDesc=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106
#1 0x00000000004781e9 in heap_form_tuple (tupleDescriptor=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:736
#2 0x00000000004784d0 in heap_modify_tuple (tuple=0x277adc8, tupleDesc=0x277f090, replValues=0x7fff1369d030, replIsnull=0x7fff1369d020 "", doReplace=0x7fff1369d010 "")
at heaptuple.c:833
#3 0x0000000000673788 in AlterRole (stmt=0x27a4f78) at user.c:845
#4 0x000000000082aa49 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-16 15:48 Robert Haas <robertmhaas@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 44+ messages in thread
From: Robert Haas @ 2016-12-16 15:48 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Heikki Linnakangas (hlinnaka@iki.fi) wrote:
>> On 12/14/2016 04:57 PM, Stephen Frost wrote:
>> >* Peter Eisentraut (peter.eisentraut@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:<stuff>", 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-16 20:40 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2016-12-16 20:40 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-16 20:42 Stephen Frost <sfrost@snowman.net>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-16 20:42 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
* Peter Eisentraut (peter.eisentraut@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-16 22:30 Michael Paquier <michael.paquier@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 2 replies; 44+ messages in thread
From: Michael Paquier @ 2016-12-16 22:30 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Peter Eisentraut (peter.eisentraut@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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-17 01:23 Stephen Frost <sfrost@snowman.net>
parent: Michael Paquier <michael.paquier@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-17 01:23 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
Michael,
* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Peter Eisentraut (peter.eisentraut@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-17 03:48 Michael Paquier <michael.paquier@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 44+ messages in thread
From: Michael Paquier @ 2016-12-17 03:48 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Sat, Dec 17, 2016 at 10:23 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-17 18:59 Robert Haas <robertmhaas@gmail.com>
parent: Michael Paquier <michael.paquier@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Robert Haas @ 2016-12-17 18:59 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> * Peter Eisentraut (peter.eisentraut@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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-17 22:48 Michael Paquier <michael.paquier@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2016-12-17 22:48 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
> <michael.paquier@gmail.com> 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 ...]
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 03:47 Robert Haas <robertmhaas@gmail.com>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 0 replies; 44+ messages in thread
From: Robert Haas @ 2016-12-20 03:47 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Stephen Frost <sfrost@snowman.net>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Sat, Dec 17, 2016 at 5:48 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
>> <michael.paquier@gmail.com> 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+TgmoY790rphHBogXMbTG6MzSeNdoxdBXebEkAet9ZpZ8gvtw@mail.gmail...
> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 11:37 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-20 11:37 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Stephen Frost <sfrost@snowman.net>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/16/2016 05:48 PM, Robert Haas wrote:
> On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> * Heikki Linnakangas (hlinnaka@iki.fi) wrote:
>>> On 12/14/2016 04:57 PM, Stephen Frost wrote:
>>>> * Peter Eisentraut (peter.eisentraut@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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 12:23 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Heikki Linnakangas @ 2016-12-20 12:23 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 12/16/2016 03:31 AM, Michael Paquier wrote:
> On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p-2.patch (invalid/octet-stream, 22.7K) download
From b4320ff0d1b28a1b501fb77ea8edad5feb60d4b9 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 20 Dec 2016 14:14:09 +0200
Subject: [PATCH 1/1] Use "plain:" prefix for plaintext passwords stored in
pg_authid.
So far, the rule has been that if pg_authid.rolpassword begins with "md5"
and has the right length, it's an MD5 hash, otherwise it's a plaintext
password. That kind of pattern matching gets more awkward when we add
new kinds of verifiers, like the upcoming SCRAM verifiers. To be able to
distinguish different kinds of password hashes, use a "plain:" prefix
for plaintext passwords. With that, every password stored in pg_authid
has either "plain:" or "md5" prefix, and future password schemes can
likewise use different prefixes.
Note that this doesn't change the format of MD5 hashed passwords, which
is what almost everyone uses.
This changes check_password_hook in an incompatible way. The
'password_type' argument is removed, and the hook is expected to call
get_password_type() on the password verifier to determine its type.
Moreover, when a plaintext password is passed to the hook, it will have
the "plain:" prefix. That is a more subtle change!
In the passing, remove the bogus notice from the documentation of CREATE
ROLE, claiming that older clients might not work with MD5 hashed passwords.
That was written when we still had "crypt" authentication, and it was
referring to the fact that an older client might support "crypt"
authentication but not "md5". But we haven't supported "crypt" for years.
(As soon as we add a new authentication mechanism that doesn't work with
MD5 hashes, we'll need a similar notice again, though!)
Discussion: https://www.postgresql.org/message-id/2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi
---
contrib/passwordcheck/passwordcheck.c | 133 +++++++++++------------
doc/src/sgml/catalogs.sgml | 4 +-
doc/src/sgml/ref/create_role.sgml | 6 --
src/backend/commands/user.c | 68 ++++++------
src/backend/libpq/crypt.c | 192 +++++++++++++++++++++++++++-------
src/backend/utils/misc/guc.c | 1 +
src/include/commands/user.h | 13 +--
src/include/common/md5.h | 4 -
src/include/libpq/crypt.h | 20 +++-
9 files changed, 275 insertions(+), 166 deletions(-)
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index a0db89bbbf..3d06964cab 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -21,7 +21,7 @@
#endif
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "fmgr.h"
PG_MODULE_MAGIC;
@@ -50,87 +50,78 @@ extern void _PG_init(void);
*/
static void
check_password(const char *username,
- const char *password,
- int password_type,
+ const char *shadow_pass,
Datum validuntil_time,
bool validuntil_null)
{
- int namelen = strlen(username);
- int pwdlen = strlen(password);
- char encrypted[MD5_PASSWD_LEN + 1];
- int i;
- bool pwd_has_letter,
- pwd_has_nonletter;
-
- switch (password_type)
+ if (get_password_type(shadow_pass) != PASSWORD_TYPE_PLAINTEXT)
{
- case PASSWORD_TYPE_MD5:
-
- /*
- * Unfortunately we cannot perform exhaustive checks on encrypted
- * passwords - we are restricted to guessing. (Alternatively, we
- * could insist on the password being presented non-encrypted, but
- * that has its own security disadvantages.)
- *
- * We only check for username = password.
- */
- 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")));
- break;
-
- case PASSWORD_TYPE_PLAINTEXT:
-
+ /*
+ * Unfortunately we cannot perform exhaustive checks on encrypted
+ * passwords - we are restricted to guessing. (Alternatively, we
+ * could insist on the password being presented non-encrypted, but
+ * that has its own security disadvantages.)
+ *
+ * We only check for username = password.
+ */
+ char *logdetail;
+
+ if (plain_crypt_verify(username, shadow_pass, username, &logdetail) == STATUS_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+ }
+ else
+ {
+ /*
+ * For unencrypted passwords we can perform better checks
+ */
+ char *password = get_plain_password(shadow_pass);
+ int pwdlen = strlen(password);
+ int i;
+ bool pwd_has_letter,
+ pwd_has_nonletter;
+
+ /* enforce minimum length */
+ if (pwdlen < MIN_PWD_LENGTH)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is too short")));
+
+ /* check if the password contains the username */
+ if (strstr(password, username))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+
+ /* check if the password contains both letters and non-letters */
+ pwd_has_letter = false;
+ pwd_has_nonletter = false;
+ for (i = 0; i < pwdlen; i++)
+ {
/*
- * For unencrypted passwords we can perform better checks
+ * isalpha() does not work for multibyte encodings but let's
+ * consider non-ASCII characters non-letters
*/
-
- /* enforce minimum length */
- if (pwdlen < MIN_PWD_LENGTH)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is too short")));
-
- /* check if the password contains the username */
- if (strstr(password, username))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password must not contain user name")));
-
- /* check if the password contains both letters and non-letters */
- pwd_has_letter = false;
- pwd_has_nonletter = false;
- for (i = 0; i < pwdlen; i++)
- {
- /*
- * isalpha() does not work for multibyte encodings but let's
- * consider non-ASCII characters non-letters
- */
- if (isalpha((unsigned char) password[i]))
- pwd_has_letter = true;
- else
- pwd_has_nonletter = true;
- }
- if (!pwd_has_letter || !pwd_has_nonletter)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ if (isalpha((unsigned char) password[i]))
+ pwd_has_letter = true;
+ else
+ pwd_has_nonletter = true;
+ }
+ if (!pwd_has_letter || !pwd_has_nonletter)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password must contain both letters and nonletters")));
#ifdef USE_CRACKLIB
- /* call cracklib to check password */
- if (FascistCheck(password, CRACKLIB_DICTPATH))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is easily cracked")));
+ /* call cracklib to check password */
+ if (FascistCheck(password, CRACKLIB_DICTPATH))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is easily cracked")));
#endif
- break;
- default:
- elog(ERROR, "unrecognized password type: %d", password_type);
- break;
+ pfree(password);
}
/* all checks passed, password is ok */
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 11c2019106..abf98af5ba 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1320,8 +1320,8 @@
will be of the user's password concatenated to their user name.
For example, if user <literal>joe</> has password <literal>xyzzy</>,
<productname>PostgreSQL</> will store the md5 hash of
- <literal>xyzzyjoe</>. A password that does not follow that
- format is assumed to be unencrypted.
+ <literal>xyzzyjoe</>. If a password is stored unencrypted, the
+ column will begin with the string <literal>plain:</literal>.
</entry>
</row>
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 38cd4c8369..a3b8ed9ab4 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -235,12 +235,6 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
password string). This allows reloading of encrypted
passwords during dump/restore.
</para>
-
- <para>
- Note that older clients might lack support for the MD5
- authentication mechanism that is needed to work with passwords
- that are stored encrypted.
- </para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index adc6b99b21..2d513f4041 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -29,7 +29,7 @@
#include "commands/dbcommands.h"
#include "commands/seclabel.h"
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "miscadmin.h"
#include "storage/lmgr.h"
#include "utils/acl.h"
@@ -81,7 +81,6 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
ListCell *option;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
bool issuper = false; /* Make the user a superuser? */
bool inherit = true; /* Auto inherit privileges? */
bool createrole = false; /* Can this user create roles? */
@@ -368,11 +367,23 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, stmt->role,
+ password);
+
(*check_password_hook) (stmt->role,
- password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build a tuple to insert
@@ -393,17 +404,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, stmt->role, strlen(stmt->role),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encode the password to the requested format. */
+ password = encode_password(password_type, stmt->role, password);
+ new_record[Anum_pg_authid_rolpassword - 1] =
+ CStringGetTextDatum(password);
}
else
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -506,7 +510,6 @@ AlterRole(AlterRoleStmt *stmt)
char *rolename = NULL;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
int issuper = -1; /* Make the user a superuser? */
int inherit = -1; /* Auto inherit privileges? */
int createrole = -1; /* Can this user create roles? */
@@ -743,11 +746,23 @@ AlterRole(AlterRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, rolename,
+ password);
+
(*check_password_hook) (rolename,
- password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build an updated tuple, perusing the information just obtained
@@ -804,17 +819,10 @@ AlterRole(AlterRoleStmt *stmt)
/* password */
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, rolename, strlen(rolename),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encode the password to the requested format. */
+ password = encode_password(password_type, rolename, password);
+ new_record[Anum_pg_authid_rolpassword - 1] =
+ CStringGetTextDatum(password);
new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
}
@@ -1232,7 +1240,7 @@ RenameRole(const char *oldname, const char *newname)
datum = heap_getattr(oldtuple, Anum_pg_authid_rolpassword, dsc, &isnull);
- if (!isnull && isMD5(TextDatumGetCString(datum)))
+ if (!isnull && get_password_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
{
/* MD5 uses the username as salt, so just clear it on a rename */
repl_repl[Anum_pg_authid_rolpassword - 1] = true;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 7e9124f39e..5df049fe21 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -1,10 +1,8 @@
/*-------------------------------------------------------------------------
*
* crypt.c
- * Look into the password file and check the encrypted password with
- * the one passed in from the frontend.
- *
- * Original coding by Todd A. Brandys
+ * Functions for dealing with encrypted passwords stored in
+ * pg_authid.rolpassword
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -106,6 +104,98 @@ get_role_password(const char *role, char **shadow_pass, char **logdetail)
}
/*
+ * What kind of a password verifier is 'shadow_pass'?
+ */
+PasswordType
+get_password_type(const char *shadow_pass)
+{
+ if (strncmp(shadow_pass, "plain:", 6) == 0 && strlen(shadow_pass) >= 6)
+ return PASSWORD_TYPE_PLAINTEXT;
+ if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
+ return PASSWORD_TYPE_MD5;
+ return PASSWORD_TYPE_UNKNOWN;
+}
+
+/*
+ * Given a plaintext password verifier, "plain:<password>", return the
+ * actual plaintext password without the prefix. The returned password is
+ * palloc'd.
+ */
+char *
+get_plain_password(const char *shadow_pass)
+{
+ if (strlen(shadow_pass) < 6 || memcmp(shadow_pass, "plain:", 6) != 0)
+ elog(ERROR, "password is not a plaintext password verifier");
+
+ return pstrdup(&shadow_pass[strlen("plain:")]);
+}
+
+/*
+ * Given a user-supplied password, convert it into a verifier of
+ * 'target_type' kind.
+ *
+ * If the password looks like a valid MD5 hash, it is stored as it is.
+ * We cannot reverse the hash, so even if the caller requested a plaintext
+ * plaintext password, the MD5 hash is returned.
+ *
+ * If the password follows the "plain:<password>" format, it is interpreted
+ * as a plaintext password. If an MD5 hash was requested, it is hashed,
+ * otherwise it is returned as it is.
+ */
+char *
+encode_password(PasswordType target_type, const char *role,
+ const char *password)
+{
+ PasswordType guessed_type = get_password_type(password);
+ const char *plain_password;
+ char *encrypted_password;
+
+ switch (target_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ elog(ERROR, "unknown target password type");
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ return psprintf("plain:%s", password);
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ return pstrdup(password);
+
+ case PASSWORD_TYPE_MD5:
+ /* We cannot convert an MD5 hash back to plaintext. Store the hash. */
+ return pstrdup(password);
+ }
+ break;
+
+ case PASSWORD_TYPE_MD5:
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ case PASSWORD_TYPE_PLAINTEXT:
+ encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+
+ /* strip the possible "plain:" prefix, and hash */
+ if (guessed_type == PASSWORD_TYPE_PLAINTEXT)
+ plain_password = &password[strlen("plain:")];
+ else
+ plain_password = password;
+ if (!pg_md5_encrypt(plain_password, role, strlen(role),
+ encrypted_password))
+ elog(ERROR, "password encryption failed");
+ return encrypted_password;
+
+ case PASSWORD_TYPE_MD5:
+ return pstrdup(password);
+ }
+ }
+
+ elog(ERROR, "unrecognized password type conversion");
+}
+
+/*
* Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
*
* 'shadow_pass' is the user's correct password or password hash, as stored
@@ -135,32 +225,40 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
* below: the only possible error is out-of-memory, which is unlikely, and
* if it did happen adding a psprintf call would only make things worse.
*/
- if (isMD5(shadow_pass))
- {
- /* stored password already encrypted, only do salt */
- if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
- return STATUS_ERROR;
- }
- }
- else
+ switch(get_password_type(shadow_pass))
{
- /* stored password is plain, double-encrypt */
- if (!pg_md5_encrypt(shadow_pass,
- role,
- strlen(role),
- crypt_pwd2))
- {
- return STATUS_ERROR;
- }
- if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
+ case PASSWORD_TYPE_MD5:
+ /* stored password already encrypted, only do salt */
+ if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ /* stored password is plain, double-encrypt */
+ if (!pg_md5_encrypt(&shadow_pass[strlen("plain:")],
+ role,
+ strlen(role),
+ crypt_pwd2))
+ {
+ return STATUS_ERROR;
+ }
+ if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ default:
+ /* unknown password hash format. */
+ *logdetail = psprintf(_("User \"%s\" has a password that cannot be used with MD5 authentication."),
+ role);
return STATUS_ERROR;
- }
}
if (strcmp(client_pass, crypt_pwd) == 0)
@@ -198,22 +296,36 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
* the password the client sent, and compare the hashes. Otherwise
* compare the plaintext passwords directly.
*/
- if (isMD5(shadow_pass))
+ switch (get_password_type(shadow_pass))
{
- if (!pg_md5_encrypt(client_pass,
- role,
- strlen(role),
- crypt_client_pass))
- {
+ case PASSWORD_TYPE_MD5:
+ if (!pg_md5_encrypt(client_pass,
+ role,
+ strlen(role),
+ crypt_client_pass))
+ {
+ /*
+ * We do not bother setting logdetail for pg_md5_encrypt failure:
+ * the only possible error is out-of-memory, which is unlikely,
+ * and if it did happen adding a psprintf call would only make
+ * things worse.
+ */
+ return STATUS_ERROR;
+ }
+ client_pass = crypt_client_pass;
+ break;
+ case PASSWORD_TYPE_PLAINTEXT:
+ shadow_pass = &shadow_pass[strlen("plain:")];
+ break;
+
+ default:
/*
- * We do not bother setting logdetail for pg_md5_encrypt failure:
- * the only possible error is out-of-memory, which is unlikely,
- * and if it did happen adding a psprintf call would only make
- * things worse.
+ * This shouldn't happen. Plain "password" authentication should be
+ * possible with any kind of stored password hash.
*/
+ *logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
+ role);
return STATUS_ERROR;
- }
- client_pass = crypt_client_pass;
}
if (strcmp(client_pass, shadow_pass) == 0)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a02511754e..ab71f4759a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -40,6 +40,7 @@
#include "commands/trigger.h"
#include "funcapi.h"
#include "libpq/auth.h"
+#include "libpq/crypt.h"
#include "libpq/be-fsstubs.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 102c2a5861..9cb3629d18 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -15,21 +15,10 @@
#include "nodes/parsenodes.h"
#include "parser/parse_node.h"
-
-/*
- * Types of password, for Password_encryption GUC and the password_type
- * argument of the check-password hook.
- */
-typedef enum PasswordType
-{
- PASSWORD_TYPE_PLAINTEXT = 0,
- PASSWORD_TYPE_MD5
-} PasswordType;
-
extern int Password_encryption; /* GUC */
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *password, int password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index 4a0432076a..a29731b9bc 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -18,10 +18,6 @@
#define MD5_PASSWD_LEN 35
-#define isMD5(passwd) (strncmp(passwd, "md5", 3) == 0 && \
- strlen(passwd) == MD5_PASSWD_LEN)
-
-
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
extern bool pg_md5_binary(const void *buff, size_t len, void *outbuf);
extern bool pg_md5_encrypt(const char *passwd, const char *salt,
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 229ce76b61..ed47c0549f 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,7 +15,25 @@
#include "datatype/timestamp.h"
-extern int get_role_password(const char *role, char **shadow_pass, char **logdetail);
+/*
+ * Types of password.
+ *
+ * This is also used for the password_encryption GUC.
+ */
+typedef enum PasswordType
+{
+ PASSWORD_TYPE_UNKNOWN = -1,
+ PASSWORD_TYPE_PLAINTEXT = 0,
+ PASSWORD_TYPE_MD5
+} PasswordType;
+
+extern PasswordType get_password_type(const char *shadow_pass);
+extern char *get_plain_password(const char *shadow_pass);
+extern char *encode_password(PasswordType target_type, const char *role,
+ const char *password);
+
+extern int get_role_password(const char *role, char **shadow_pass,
+ char **logdetail);
extern int md5_crypt_verify(const char *role, const char *shadow_pass,
const char *client_pass, const char *md5_salt,
--
2.11.0
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 12:56 Robert Haas <robertmhaas@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 0 replies; 44+ messages in thread
From: Robert Haas @ 2016-12-20 12:56 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Stephen Frost <sfrost@snowman.net>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Tue, Dec 20, 2016 at 6:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 13:34 Stephen Frost <sfrost@snowman.net>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-20 13:34 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
Heikki,
* Heikki Linnakangas (hlinnaka@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 16:08 David Fetter <david@fetter.org>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 2 replies; 44+ messages in thread
From: David Fetter @ 2016-12-20 16:08 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> Heikki,
>
> * Heikki Linnakangas (hlinnaka@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 <david(at)fetter(dot)org> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 21:30 Michael Paquier <michael.paquier@gmail.com>
parent: David Fetter <david@fetter.org>
1 sibling, 0 replies; 44+ messages in thread
From: Michael Paquier @ 2016-12-20 21:30 UTC (permalink / raw)
To: David Fetter <david@fetter.org>; +Cc: Stephen Frost <sfrost@snowman.net>; Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Wed, Dec 21, 2016 at 1:08 AM, David Fetter <david@fetter.org> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 23:14 Stephen Frost <sfrost@snowman.net>
parent: David Fetter <david@fetter.org>
1 sibling, 1 reply; 44+ messages in thread
From: Stephen Frost @ 2016-12-20 23:14 UTC (permalink / raw)
To: David Fetter <david@fetter.org>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
David,
* David Fetter (david@fetter.org) wrote:
> On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > * Heikki Linnakangas (hlinnaka@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-20 23:29 David Fetter <david@fetter.org>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 44+ messages in thread
From: David Fetter @ 2016-12-20 23:29 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote:
> David,
>
> * David Fetter (david@fetter.org) wrote:
> > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > > * Heikki Linnakangas (hlinnaka@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 <david(at)fetter(dot)org> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-21 00:54 Stephen Frost <sfrost@snowman.net>
parent: David Fetter <david@fetter.org>
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Frost @ 2016-12-21 00:54 UTC (permalink / raw)
To: David Fetter <david@fetter.org>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; Julian Markwort <julian.markwort@uni-muenster.de>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
David,
* David Fetter (david@fetter.org) wrote:
> On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote:
> > * David Fetter (david@fetter.org) wrote:
> > > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > > > * Heikki Linnakangas (hlinnaka@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
Attachments:
signature.asc (application/pgp-signature, 818B) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2016-12-21 02:09 Michael Paquier <michael.paquier@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 44+ messages in thread
From: Michael Paquier @ 2016-12-21 02:09 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Tue, Dec 20, 2016 at 9:23 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
009_authentication.pl (application/x-perl, 2.1K) download
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-03 12:11 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 0 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2017-01-03 12:11 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-03 14:09 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 3 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2017-01-03 14:09 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments: download all (2 files, .tar.gz)
0001-Replace-isMD5-with-a-more-future-proof-way-to-check-.patch (invalid/octet-stream, 18.9K) download
From 2abe24b64311cfcf22e3891a08f947507206b1e0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 3 Jan 2017 13:57:52 +0200
Subject: [PATCH 1/2] Replace isMD5() with a more future-proof way to check if
pw is encrypted.
The rule has been that if pg_authid.rolpassword begins with "md5" and has
the right length, it's an MD5 hash, otherwise it's a plaintext password.
The idiom has been to use isMD5() to check for that, but that gets awkward,
when we add new kinds of verifiers, like the verifiers for SCRAM
authentication, in the pending patch set. Replace isMD5() with a new
get_password_type() function, so that when new verifier types are added, we
don't need to remember to modify every place that currently calls isMD5(),
to also recognize the new kinds of verifiers.
This doesn't change the representation of passwords in the catalogs. There
was a long discussion on pgsql-hackers on adding a "plain:" prefix to
plaintext passwords, but this patch doesn't do that yet.
For the benefit of passwordcheck contrib module, and any 3rd party modules
using password_check_hook, add a new function called "plain_crypt_verify",
which can be used to check if a password matches. This allows conveniently
guessing or "brute-forcing" the password, whether it's stored as an MD5
hash, or in the future, as some other kind of a hash or verifier.
In the passing, remove the bogus notice from the documentation of CREATE
ROLE, claiming that older clients might not work with MD5 hashed passwords.
That was written when we still had "crypt" authentication, and it was
referring to the fact that an older client might support "crypt"
authentication but not "md5". But we haven't supported "crypt" for years.
(As soon as we add a new authentication mechanism that doesn't work with
MD5 hashes, we'll need a similar notice again, though!)
Discussion: https://www.postgresql.org/message-id/2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi
---
contrib/passwordcheck/passwordcheck.c | 134 ++++++++++++++---------------
src/backend/commands/user.c | 40 +++------
src/backend/libpq/crypt.c | 155 +++++++++++++++++++++++++---------
src/include/commands/user.h | 17 +---
src/include/common/md5.h | 4 -
src/include/libpq/crypt.h | 18 +++-
6 files changed, 210 insertions(+), 158 deletions(-)
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index a0db89bbbf..03dfdfd0e1 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -21,7 +21,7 @@
#endif
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "fmgr.h"
PG_MODULE_MAGIC;
@@ -50,87 +50,77 @@ extern void _PG_init(void);
*/
static void
check_password(const char *username,
- const char *password,
- int password_type,
+ const char *shadow_pass,
+ PasswordType password_type,
Datum validuntil_time,
bool validuntil_null)
{
- int namelen = strlen(username);
- int pwdlen = strlen(password);
- char encrypted[MD5_PASSWD_LEN + 1];
- int i;
- bool pwd_has_letter,
- pwd_has_nonletter;
-
- switch (password_type)
+ if (password_type != PASSWORD_TYPE_PLAINTEXT)
{
- case PASSWORD_TYPE_MD5:
-
- /*
- * Unfortunately we cannot perform exhaustive checks on encrypted
- * passwords - we are restricted to guessing. (Alternatively, we
- * could insist on the password being presented non-encrypted, but
- * that has its own security disadvantages.)
- *
- * We only check for username = password.
- */
- 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")));
- break;
-
- case PASSWORD_TYPE_PLAINTEXT:
-
+ /*
+ * Unfortunately we cannot perform exhaustive checks on encrypted
+ * passwords - we are restricted to guessing. (Alternatively, we
+ * could insist on the password being presented non-encrypted, but
+ * that has its own security disadvantages.)
+ *
+ * We only check for username = password.
+ */
+ char *logdetail;
+
+ if (plain_crypt_verify(username, shadow_pass, username, &logdetail) == STATUS_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+ }
+ else
+ {
+ /*
+ * For unencrypted passwords we can perform better checks
+ */
+ const char *password = shadow_pass;
+ int pwdlen = strlen(password);
+ int i;
+ bool pwd_has_letter,
+ pwd_has_nonletter;
+
+ /* enforce minimum length */
+ if (pwdlen < MIN_PWD_LENGTH)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is too short")));
+
+ /* check if the password contains the username */
+ if (strstr(password, username))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password must not contain user name")));
+
+ /* check if the password contains both letters and non-letters */
+ pwd_has_letter = false;
+ pwd_has_nonletter = false;
+ for (i = 0; i < pwdlen; i++)
+ {
/*
- * For unencrypted passwords we can perform better checks
+ * isalpha() does not work for multibyte encodings but let's
+ * consider non-ASCII characters non-letters
*/
-
- /* enforce minimum length */
- if (pwdlen < MIN_PWD_LENGTH)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is too short")));
-
- /* check if the password contains the username */
- if (strstr(password, username))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password must not contain user name")));
-
- /* check if the password contains both letters and non-letters */
- pwd_has_letter = false;
- pwd_has_nonletter = false;
- for (i = 0; i < pwdlen; i++)
- {
- /*
- * isalpha() does not work for multibyte encodings but let's
- * consider non-ASCII characters non-letters
- */
- if (isalpha((unsigned char) password[i]))
- pwd_has_letter = true;
- else
- pwd_has_nonletter = true;
- }
- if (!pwd_has_letter || !pwd_has_nonletter)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ if (isalpha((unsigned char) password[i]))
+ pwd_has_letter = true;
+ else
+ pwd_has_nonletter = true;
+ }
+ if (!pwd_has_letter || !pwd_has_nonletter)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password must contain both letters and nonletters")));
#ifdef USE_CRACKLIB
- /* call cracklib to check password */
- if (FascistCheck(password, CRACKLIB_DICTPATH))
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password is easily cracked")));
+ /* call cracklib to check password */
+ if (FascistCheck(password, CRACKLIB_DICTPATH))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("password is easily cracked")));
#endif
- break;
-
- default:
- elog(ERROR, "unrecognized password type: %d", password_type);
- break;
}
/* all checks passed, password is ok */
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index a3521e7757..48969d3a47 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -29,7 +29,7 @@
#include "commands/dbcommands.h"
#include "commands/seclabel.h"
#include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
#include "miscadmin.h"
#include "storage/lmgr.h"
#include "utils/acl.h"
@@ -81,7 +81,6 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
ListCell *option;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
bool issuper = false; /* Make the user a superuser? */
bool inherit = true; /* Auto inherit privileges? */
bool createrole = false; /* Can this user create roles? */
@@ -370,7 +369,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (stmt->role,
password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ get_password_type(password),
validUntil_datum,
validUntil_null);
@@ -393,17 +392,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, stmt->role, strlen(stmt->role),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encrypt the password to the requested format. */
+ password = encrypt_password(password_type, stmt->role, password);
+ new_record[Anum_pg_authid_rolpassword - 1] =
+ CStringGetTextDatum(password);
}
else
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -506,7 +498,6 @@ AlterRole(AlterRoleStmt *stmt)
char *rolename = NULL;
char *password = NULL; /* user password */
int password_type = Password_encryption;
- char encrypted_password[MD5_PASSWD_LEN + 1];
int issuper = -1; /* Make the user a superuser? */
int inherit = -1; /* Auto inherit privileges? */
int createrole = -1; /* Can this user create roles? */
@@ -745,7 +736,7 @@ AlterRole(AlterRoleStmt *stmt)
if (check_password_hook && password)
(*check_password_hook) (rolename,
password,
- isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+ get_password_type(password),
validUntil_datum,
validUntil_null);
@@ -804,17 +795,10 @@ AlterRole(AlterRoleStmt *stmt)
/* password */
if (password)
{
- if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(password);
- else
- {
- if (!pg_md5_encrypt(password, rolename, strlen(rolename),
- encrypted_password))
- elog(ERROR, "password encryption failed");
- new_record[Anum_pg_authid_rolpassword - 1] =
- CStringGetTextDatum(encrypted_password);
- }
+ /* Encrypt the password to the requested format. */
+ password = encrypt_password(password_type, rolename, password);
+ new_record[Anum_pg_authid_rolpassword - 1] =
+ CStringGetTextDatum(password);
new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
}
@@ -1232,7 +1216,7 @@ RenameRole(const char *oldname, const char *newname)
datum = heap_getattr(oldtuple, Anum_pg_authid_rolpassword, dsc, &isnull);
- if (!isnull && isMD5(TextDatumGetCString(datum)))
+ if (!isnull && get_password_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
{
/* MD5 uses the username as salt, so just clear it on a rename */
repl_repl[Anum_pg_authid_rolpassword - 1] = true;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 7e9124f39e..5ec5951ece 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -1,10 +1,8 @@
/*-------------------------------------------------------------------------
*
* crypt.c
- * Look into the password file and check the encrypted password with
- * the one passed in from the frontend.
- *
- * Original coding by Todd A. Brandys
+ * Functions for dealing with encrypted passwords stored in
+ * pg_authid.rolpassword.
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -106,6 +104,61 @@ get_role_password(const char *role, char **shadow_pass, char **logdetail)
}
/*
+ * What kind of a password verifier is 'shadow_pass'?
+ */
+PasswordType
+get_password_type(const char *shadow_pass)
+{
+ if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
+ return PASSWORD_TYPE_MD5;
+ return PASSWORD_TYPE_PLAINTEXT;
+}
+
+/*
+ * Given a user-supplied password, convert it into a verifier of
+ * 'target_type' kind.
+ *
+ * If the password looks like a valid MD5 hash, it is stored as it is.
+ * We cannot reverse the hash, so even if the caller requested a plaintext
+ * plaintext password, the MD5 hash is returned.
+ */
+char *
+encrypt_password(PasswordType target_type, const char *role,
+ const char *password)
+{
+ PasswordType guessed_type = get_password_type(password);
+ char *encrypted_password;
+
+ switch (target_type)
+ {
+ case PASSWORD_TYPE_PLAINTEXT:
+ /*
+ * We cannot convert an MD5 hash back to plaintext,
+ * so no need to what what the input was. Just store
+ * it as it is.
+ */
+ return pstrdup(password);
+
+ case PASSWORD_TYPE_MD5:
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_PLAINTEXT:
+ encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+
+ if (!pg_md5_encrypt(password, role, strlen(role),
+ encrypted_password))
+ elog(ERROR, "password encryption failed");
+ return encrypted_password;
+
+ case PASSWORD_TYPE_MD5:
+ return pstrdup(password);
+ }
+ }
+
+ elog(ERROR, "unrecognized password type conversion");
+}
+
+/*
* Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
*
* 'shadow_pass' is the user's correct password or password hash, as stored
@@ -135,32 +188,40 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
* below: the only possible error is out-of-memory, which is unlikely, and
* if it did happen adding a psprintf call would only make things worse.
*/
- if (isMD5(shadow_pass))
- {
- /* stored password already encrypted, only do salt */
- if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
- return STATUS_ERROR;
- }
- }
- else
+ switch(get_password_type(shadow_pass))
{
- /* stored password is plain, double-encrypt */
- if (!pg_md5_encrypt(shadow_pass,
- role,
- strlen(role),
- crypt_pwd2))
- {
- return STATUS_ERROR;
- }
- if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
- md5_salt, md5_salt_len,
- crypt_pwd))
- {
+ case PASSWORD_TYPE_MD5:
+ /* stored password already encrypted, only do salt */
+ if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ /* stored password is plain, double-encrypt */
+ if (!pg_md5_encrypt(&shadow_pass[strlen("plain:")],
+ role,
+ strlen(role),
+ crypt_pwd2))
+ {
+ return STATUS_ERROR;
+ }
+ if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+ md5_salt, md5_salt_len,
+ crypt_pwd))
+ {
+ return STATUS_ERROR;
+ }
+ break;
+
+ default:
+ /* unknown password hash format. */
+ *logdetail = psprintf(_("User \"%s\" has a password that cannot be used with MD5 authentication."),
+ role);
return STATUS_ERROR;
- }
}
if (strcmp(client_pass, crypt_pwd) == 0)
@@ -198,22 +259,36 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
* the password the client sent, and compare the hashes. Otherwise
* compare the plaintext passwords directly.
*/
- if (isMD5(shadow_pass))
+ switch (get_password_type(shadow_pass))
{
- if (!pg_md5_encrypt(client_pass,
- role,
- strlen(role),
- crypt_client_pass))
- {
+ case PASSWORD_TYPE_MD5:
+ if (!pg_md5_encrypt(client_pass,
+ role,
+ strlen(role),
+ crypt_client_pass))
+ {
+ /*
+ * We do not bother setting logdetail for pg_md5_encrypt failure:
+ * the only possible error is out-of-memory, which is unlikely,
+ * and if it did happen adding a psprintf call would only make
+ * things worse.
+ */
+ return STATUS_ERROR;
+ }
+ client_pass = crypt_client_pass;
+ break;
+ case PASSWORD_TYPE_PLAINTEXT:
+ shadow_pass = &shadow_pass[strlen("plain:")];
+ break;
+
+ default:
/*
- * We do not bother setting logdetail for pg_md5_encrypt failure:
- * the only possible error is out-of-memory, which is unlikely,
- * and if it did happen adding a psprintf call would only make
- * things worse.
+ * This shouldn't happen. Plain "password" authentication should be
+ * possible with any kind of stored password hash.
*/
+ *logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
+ role);
return STATUS_ERROR;
- }
- client_pass = crypt_client_pass;
}
if (strcmp(client_pass, shadow_pass) == 0)
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 102c2a5861..08037e0f81 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -12,24 +12,15 @@
#define USER_H
#include "catalog/objectaddress.h"
+#include "libpq/crypt.h"
#include "nodes/parsenodes.h"
#include "parser/parse_node.h"
-
-/*
- * Types of password, for Password_encryption GUC and the password_type
- * argument of the check-password hook.
- */
-typedef enum PasswordType
-{
- PASSWORD_TYPE_PLAINTEXT = 0,
- PASSWORD_TYPE_MD5
-} PasswordType;
-
-extern int Password_encryption; /* GUC */
+/* GUC. Is actually of type PasswordType. */
+extern int Password_encryption;
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *password, int password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index 4a0432076a..a29731b9bc 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -18,10 +18,6 @@
#define MD5_PASSWD_LEN 35
-#define isMD5(passwd) (strncmp(passwd, "md5", 3) == 0 && \
- strlen(passwd) == MD5_PASSWD_LEN)
-
-
extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
extern bool pg_md5_binary(const void *buff, size_t len, void *outbuf);
extern bool pg_md5_encrypt(const char *passwd, const char *salt,
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 229ce76b61..338be28b9d 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,7 +15,23 @@
#include "datatype/timestamp.h"
-extern int get_role_password(const char *role, char **shadow_pass, char **logdetail);
+/*
+ * Types of password.
+ *
+ * This is also used for the password_encryption GUC.
+ */
+typedef enum PasswordType
+{
+ PASSWORD_TYPE_PLAINTEXT = 0,
+ PASSWORD_TYPE_MD5
+} PasswordType;
+
+extern PasswordType get_password_type(const char *shadow_pass);
+extern char *encrypt_password(PasswordType target_type, const char *role,
+ const char *password);
+
+extern int get_role_password(const char *role, char **shadow_pass,
+ char **logdetail);
extern int md5_crypt_verify(const char *role, const char *shadow_pass,
const char *client_pass, const char *md5_salt,
--
2.11.0
0002-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch (invalid/octet-stream, 9.0K) download
From 5bbda2ed5c1e8fc06c2a6761e4924224ecb34adf Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 3 Jan 2017 13:37:49 +0200
Subject: [PATCH 2/2] Use "plain:" prefix for plaintext passwords stored in
pg_authid.
So far, the rule has been that if pg_authid.rolpassword begins with "md5"
and has the right length, it's an MD5 hash, otherwise it's a plaintext
password. That kind of pattern matching gets more awkward when we add
new kinds of verifiers, like the upcoming SCRAM verifiers. To be able to
distinguish different kinds of password hashes, use a "plain:" prefix
for plaintext passwords. With that, every password stored in pg_authid
has either "plain:" or "md5" prefix, and future password schemes can
likewise use different prefixes.
Note that this doesn't change the format of MD5 hashed passwords, which
is what almost everyone uses.
This changes check_password_hook in an incompatible way. The
'password_type' argument is removed, and the hook is expected to call
get_password_type() on the password verifier to determine its type.
Moreover, when a plaintext password is passed to the hook, it will have
the "plain:" prefix. That is a more subtle change!
Discussion: https://www.postgresql.org/message-id/2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi
---
contrib/passwordcheck/passwordcheck.c | 7 +++--
doc/src/sgml/catalogs.sgml | 4 +--
src/backend/commands/user.c | 32 ++++++++++++++++++---
src/backend/libpq/crypt.c | 53 +++++++++++++++++++++++++++++------
src/include/commands/user.h | 2 +-
src/include/libpq/crypt.h | 1 +
6 files changed, 81 insertions(+), 18 deletions(-)
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index 03dfdfd0e1..3d06964cab 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -51,11 +51,10 @@ extern void _PG_init(void);
static void
check_password(const char *username,
const char *shadow_pass,
- PasswordType password_type,
Datum validuntil_time,
bool validuntil_null)
{
- if (password_type != PASSWORD_TYPE_PLAINTEXT)
+ if (get_password_type(shadow_pass) != PASSWORD_TYPE_PLAINTEXT)
{
/*
* Unfortunately we cannot perform exhaustive checks on encrypted
@@ -77,7 +76,7 @@ check_password(const char *username,
/*
* For unencrypted passwords we can perform better checks
*/
- const char *password = shadow_pass;
+ char *password = get_plain_password(shadow_pass);
int pwdlen = strlen(password);
int i;
bool pwd_has_letter,
@@ -121,6 +120,8 @@ check_password(const char *username,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password is easily cracked")));
#endif
+
+ pfree(password);
}
/* all checks passed, password is ok */
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 493050618d..5df31797e6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1325,8 +1325,8 @@
will be of the user's password concatenated to their user name.
For example, if user <literal>joe</> has password <literal>xyzzy</>,
<productname>PostgreSQL</> will store the md5 hash of
- <literal>xyzzyjoe</>. A password that does not follow that
- format is assumed to be unencrypted.
+ <literal>xyzzyjoe</>. If a password is stored unencrypted, the
+ column will begin with the string <literal>plain:</literal>.
</entry>
</row>
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 48969d3a47..6811f0fa3d 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -367,11 +367,23 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encrypt_password(PASSWORD_TYPE_PLAINTEXT, stmt->role,
+ password);
+
(*check_password_hook) (stmt->role,
- password,
- get_password_type(password),
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build a tuple to insert
@@ -734,11 +746,23 @@ AlterRole(AlterRoleStmt *stmt)
* Call the password checking hook if there is one defined
*/
if (check_password_hook && password)
+ {
+ char *plain_password;
+
+ /*
+ * We prefer to pass a plaintext verifier to the password hook, if
+ * possible, even if it's eventually stored in a hashed format, so
+ * that the hook can perform more extensive tests on it.
+ */
+ plain_password = encrypt_password(PASSWORD_TYPE_PLAINTEXT, rolename,
+ password);
+
(*check_password_hook) (rolename,
- password,
- get_password_type(password),
+ plain_password,
validUntil_datum,
validUntil_null);
+ pfree(plain_password);
+ }
/*
* Build an updated tuple, perusing the information just obtained
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 5ec5951ece..57806a38e8 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -109,9 +109,25 @@ get_role_password(const char *role, char **shadow_pass, char **logdetail)
PasswordType
get_password_type(const char *shadow_pass)
{
+ if (strncmp(shadow_pass, "plain:", 6) == 0 && strlen(shadow_pass) >= 6)
+ return PASSWORD_TYPE_PLAINTEXT;
if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
return PASSWORD_TYPE_MD5;
- return PASSWORD_TYPE_PLAINTEXT;
+ return PASSWORD_TYPE_UNKNOWN;
+}
+
+/*
+ * Given a plaintext password verifier, "plain:<password>", return the
+ * actual plaintext password without the prefix. The returned password is
+ * palloc'd.
+ */
+char *
+get_plain_password(const char *shadow_pass)
+{
+ if (strlen(shadow_pass) < 6 || memcmp(shadow_pass, "plain:", 6) != 0)
+ elog(ERROR, "password is not a plaintext password verifier");
+
+ return pstrdup(&shadow_pass[strlen("plain:")]);
}
/*
@@ -121,31 +137,52 @@ get_password_type(const char *shadow_pass)
* If the password looks like a valid MD5 hash, it is stored as it is.
* We cannot reverse the hash, so even if the caller requested a plaintext
* plaintext password, the MD5 hash is returned.
+ *
+ * If the password follows the "plain:<password>" format, it is interpreted
+ * as a plaintext password. If an MD5 hash was requested, it is hashed,
+ * otherwise it is returned as it is.
*/
char *
encrypt_password(PasswordType target_type, const char *role,
const char *password)
{
PasswordType guessed_type = get_password_type(password);
+ const char *plain_password;
char *encrypted_password;
switch (target_type)
{
+ case PASSWORD_TYPE_UNKNOWN:
+ elog(ERROR, "unknown target password type");
+
case PASSWORD_TYPE_PLAINTEXT:
- /*
- * We cannot convert an MD5 hash back to plaintext,
- * so no need to what what the input was. Just store
- * it as it is.
- */
- return pstrdup(password);
+ switch (guessed_type)
+ {
+ case PASSWORD_TYPE_UNKNOWN:
+ return psprintf("plain:%s", password);
+
+ case PASSWORD_TYPE_PLAINTEXT:
+ return pstrdup(password);
+
+ case PASSWORD_TYPE_MD5:
+ /* We cannot convert an MD5 hash back to plaintext. Store the hash. */
+ return pstrdup(password);
+ }
+ break;
case PASSWORD_TYPE_MD5:
switch (guessed_type)
{
+ case PASSWORD_TYPE_UNKNOWN:
case PASSWORD_TYPE_PLAINTEXT:
encrypted_password = palloc(MD5_PASSWD_LEN + 1);
- if (!pg_md5_encrypt(password, role, strlen(role),
+ /* strip the possible "plain:" prefix, and hash */
+ if (guessed_type == PASSWORD_TYPE_PLAINTEXT)
+ plain_password = &password[strlen("plain:")];
+ else
+ plain_password = password;
+ if (!pg_md5_encrypt(plain_password, role, strlen(role),
encrypted_password))
elog(ERROR, "password encryption failed");
return encrypted_password;
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 08037e0f81..c1c4d431d4 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -20,7 +20,7 @@
extern int Password_encryption;
/* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, Datum validuntil_time, bool validuntil_null);
extern PGDLLIMPORT check_password_hook_type check_password_hook;
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 338be28b9d..4c94054718 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -22,6 +22,7 @@
*/
typedef enum PasswordType
{
+ PASSWORD_TYPE_UNKNOWN = -1,
PASSWORD_TYPE_PLAINTEXT = 0,
PASSWORD_TYPE_MD5
} PasswordType;
--
2.11.0
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-04 00:45 Michael Paquier <michael.paquier@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 0 replies; 44+ messages in thread
From: Michael Paquier @ 2017-01-04 00:45 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Tue, Jan 3, 2017 at 11:09 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-05 13:31 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2017-01-05 13:31 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-05 13:51 Michael Paquier <michael.paquier@gmail.com>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 0 replies; 44+ messages in thread
From: Michael Paquier @ 2017-01-05 13:51 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On Thu, Jan 5, 2017 at 10:31 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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-268609a4821f@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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-01-17 21:51 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 1 reply; 44+ messages in thread
From: Peter Eisentraut @ 2017-01-17 21:51 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-02-01 11:13 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 44+ messages in thread
From: Heikki Linnakangas @ 2017-02-01 11:13 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; +Cc: Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-02-02 03:50 David Rowley <david.rowley@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 44+ messages in thread
From: David Rowley @ 2017-02-02 03:50 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 2 February 2017 at 00:13, Heikki Linnakangas <hlinnaka@iki.fi> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
encrypt_password_warning_fix.patch (application/octet-stream, 426B) download
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 893ce6e..e7dd212 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -160,6 +160,7 @@ encrypt_password(PasswordType target_type, const char *role,
* handle every combination of source and target password types.
*/
elog(ERROR, "cannot encrypt password to requested type");
+ return NULL; /* keep compiler quiet */
}
/*
* Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
@ 2017-02-02 08:45 Heikki Linnakangas <hlinnaka@iki.fi>
parent: David Rowley <david.rowley@2ndquadrant.com>
0 siblings, 0 replies; 44+ messages in thread
From: Heikki Linnakangas @ 2017-02-02 08:45 UTC (permalink / raw)
To: David Rowley <david.rowley@2ndquadrant.com>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Magnus Hagander <magnus@hagander.net>; Robert Haas <robertmhaas@gmail.com>; Andres Freund <andres@anarazel.de>; David Steele <david@pgmasters.net>; Tom Lane <tgl@sss.pgh.pa.us>; David Fetter <david@fetter.org>; Alvaro Herrera <alvherre@2ndquadrant.com>; Julian Markwort <julian.markwort@uni-muenster.de>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; Valery Popov <v.popov@postgrespro.ru>
On 02/02/2017 05:50 AM, David Rowley wrote:
> On 2 February 2017 at 00:13, Heikki Linnakangas <hlinnaka@iki.fi> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
end of thread, other threads:[~2017-02-02 08:45 UTC | newest]
Thread overview: 44+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14 08:51 pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol) Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-14 10:15 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-14 10:32 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-14 14:52 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2016-12-14 14:57 ` Stephen Frost <sfrost@snowman.net>
2016-12-15 09:57 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-15 13:40 ` Stephen Frost <sfrost@snowman.net>
2016-12-16 15:48 ` Robert Haas <robertmhaas@gmail.com>
2016-12-20 11:37 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-20 12:56 ` Robert Haas <robertmhaas@gmail.com>
2016-12-20 13:34 ` Stephen Frost <sfrost@snowman.net>
2016-12-20 16:08 ` David Fetter <david@fetter.org>
2016-12-20 21:30 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-20 23:14 ` Stephen Frost <sfrost@snowman.net>
2016-12-20 23:29 ` David Fetter <david@fetter.org>
2016-12-21 00:54 ` Stephen Frost <sfrost@snowman.net>
2016-12-16 20:40 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2016-12-16 20:42 ` Stephen Frost <sfrost@snowman.net>
2016-12-16 22:30 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-17 01:23 ` Stephen Frost <sfrost@snowman.net>
2016-12-17 03:48 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-17 18:59 ` Robert Haas <robertmhaas@gmail.com>
2016-12-17 22:48 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-20 03:47 ` Robert Haas <robertmhaas@gmail.com>
2016-12-14 10:27 ` Magnus Hagander <magnus@hagander.net>
2016-12-14 11:33 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-15 01:00 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-15 12:48 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-16 01:31 ` Michael Paquier <michael.paquier@gmail.com>
2016-12-20 12:23 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-21 02:09 ` Michael Paquier <michael.paquier@gmail.com>
2017-01-03 14:09 ` Heikki Linnakangas <hlinnaka@iki.fi>
2017-01-04 00:45 ` Michael Paquier <michael.paquier@gmail.com>
2017-01-05 13:31 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2017-01-05 13:51 ` Michael Paquier <michael.paquier@gmail.com>
2017-01-17 21:51 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2017-02-01 11:13 ` Heikki Linnakangas <hlinnaka@iki.fi>
2017-02-02 03:50 ` David Rowley <david.rowley@2ndquadrant.com>
2017-02-02 08:45 ` Heikki Linnakangas <hlinnaka@iki.fi>
2017-01-03 12:11 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-14 18:12 ` Bruce Momjian <bruce@momjian.us>
2016-12-14 19:34 ` Heikki Linnakangas <hlinnaka@iki.fi>
2016-12-14 19:41 ` Stephen Frost <sfrost@snowman.net>
2016-12-14 19:58 ` Joshua D. Drake <jd@commandprompt.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox