Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 02/02/2017 05:50 AM, David Rowley wrote:

On 2 February 2017 at 00:13, Heikki Linnakangas  wrote:

Ok, I'll drop the second patch for now. I committed the first patch after
fixing the things you and Michael pointed out. Thanks for the review!


dbd69118 caused small compiler warning for me.

The attached fixed it.


Fixed, thanks!

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[David Rowley]

On 2 February 2017 at 00:13, Heikki Linnakangas  wrote:
> Ok, I'll drop the second patch for now. I committed the first patch after
> fixing the things you and Michael pointed out. Thanks for the review!

dbd69118 caused small compiler warning for me.

The attached fixed it.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


encrypt_password_warning_fix.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 01/17/2017 11:51 PM, Peter Eisentraut wrote:

On 1/3/17 9:09 AM, Heikki Linnakangas wrote:

Since not everyone agrees with this approach, I split this patch into
two. The first patch refactors things, replacing the isMD5() function
with get_password_type(), without changing the representation of
pg_authid.rolpassword. That is hopefully uncontroversial.


I have checked these patches.

The refactoring in the first patch seems sensible.  As Michael pointed
out, there is still a reference to "plain:" in the first patch.


Fixed.


The commit message needs to be updated, because the function
plain_crypt_verify() was already added in a previous patch.


Fixed.


I'm not fond of this kind of coding

password = encrypt_password(password_type, stmt->role, password);

where the 'password' variable has a different meaning before and after.


Added a new local variable to avoid the confusion.


This error message might be a mistake:

elog(ERROR, "unrecognized password type conversion");


I rephrased the error as "cannot encrypt password to requested type", 
and added a comment explaining that it cannot happen. I hope that 
helped, I'm not sure why you thought it might've been a mistake.



I think some pieces from the second patch could be included in the first
patch, e.g., the parts for passwordcheck.c and user.c.


I refrained from doing that for now. It would've changed the 
passwordcheck hook API in an incompatible way. Breaking the API 
explicitly would be a good thing, if we added the "plain:" prefix, 
because modules would need to deal with the prefix anyway. But until we 
do that, better to not break the API for no good reason.



And the second
patch adds the "plain:" prefix, which not everyone agrees on.


The code also gets a little bit dubious, as it introduces an "unknown"
password type, which is sometimes treated as plaintext and sometimes as
an error.  I think this is going be messy.

I would skip this patch for now at least.  Too much controversy, and we
don't know how the rest of the patches for this feature will look like
to be able to know if it's worth it.


Ok, I'll drop the second patch for now. I committed the first patch 
after fixing the things you and Michael pointed out. Thanks for the review!


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Peter Eisentraut]

On 1/3/17 9:09 AM, Heikki Linnakangas wrote:
> Since not everyone agrees with this approach, I split this patch into 
> two. The first patch refactors things, replacing the isMD5() function 
> with get_password_type(), without changing the representation of 
> pg_authid.rolpassword. That is hopefully uncontroversial.

I have checked these patches.

The refactoring in the first patch seems sensible.  As Michael pointed
out, there is still a reference to "plain:" in the first patch.

The commit message needs to be updated, because the function
plain_crypt_verify() was already added in a previous patch.

I'm not fond of this kind of coding

password = encrypt_password(password_type, stmt->role, password);

where the 'password' variable has a different meaning before and after.

This error message might be a mistake:

elog(ERROR, "unrecognized password type conversion");

I think some pieces from the second patch could be included in the first
patch, e.g., the parts for passwordcheck.c and user.c.

> And the second 
> patch adds the "plain:" prefix, which not everyone agrees on.

The code also gets a little bit dubious, as it introduces an "unknown"
password type, which is sometimes treated as plaintext and sometimes as
an error.  I think this is going be messy.

I would skip this patch for now at least.  Too much controversy, and we
don't know how the rest of the patches for this feature will look like
to be able to know if it's worth it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Thu, Jan 5, 2017 at 10:31 PM, Peter Eisentraut
 wrote:
> On 1/3/17 9:09 AM, Heikki Linnakangas wrote:
>> Since not everyone agrees with this approach, I split this patch into
>> two. The first patch refactors things, replacing the isMD5() function
>> with get_password_type(), without changing the representation of
>> pg_authid.rolpassword. That is hopefully uncontroversial. And the second
>> patch adds the "plain:" prefix, which not everyone agrees on.
>>
>> Barring objections I'm going to at least commit the first patch. I think
>> we should commit the second one too, but it's not as critical, and the
>> first patch matters more for the SCRAM patch, too.
>
> Is there currently anything to review here for the commit fest?

The patches sent here make sense as part of the SCRAM set:
https://www.postgresql.org/message-id/6831df67-7641-1a66-4985-268609a48...@iki.fi
I was just waiting for Heikki to fix the split of the patches before
moving on with an extra lookup though.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Peter Eisentraut]

On 1/3/17 9:09 AM, Heikki Linnakangas wrote:
> Since not everyone agrees with this approach, I split this patch into 
> two. The first patch refactors things, replacing the isMD5() function 
> with get_password_type(), without changing the representation of 
> pg_authid.rolpassword. That is hopefully uncontroversial. And the second 
> patch adds the "plain:" prefix, which not everyone agrees on.
> 
> Barring objections I'm going to at least commit the first patch. I think 
> we should commit the second one too, but it's not as critical, and the 
> first patch matters more for the SCRAM patch, too.

Is there currently anything to review here for the commit fest?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Tue, Jan 3, 2017 at 11:09 PM, Heikki Linnakangas  wrote:
> Since not everyone agrees with this approach, I split this patch into two.
> The first patch refactors things, replacing the isMD5() function with
> get_password_type(), without changing the representation of
> pg_authid.rolpassword. That is hopefully uncontroversial. And the second
> patch adds the "plain:" prefix, which not everyone agrees on.
>
> Barring objections I'm going to at least commit the first patch. I think we
> should commit the second one too, but it's not as critical, and the first
> patch matters more for the SCRAM patch, too.

The split does not look correct to me. 0001 has references to the
prefix "plain:".
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/21/2016 04:09 AM, Michael Paquier wrote:

Thanks for having a look! Attached is a new version, with that bug fixed.


I have been able more advanced testing without the crash and things
seem to work properly. The attached set of tests is also able to pass
for all the combinations of hba configurations and password formats.
And looking at the code I don't have more comments.


Thanks!

Since not everyone agrees with this approach, I split this patch into 
two. The first patch refactors things, replacing the isMD5() function 
with get_password_type(), without changing the representation of 
pg_authid.rolpassword. That is hopefully uncontroversial. And the second 
patch adds the "plain:" prefix, which not everyone agrees on.


Barring objections I'm going to at least commit the first patch. I think 
we should commit the second one too, but it's not as critical, and the 
first patch matters more for the SCRAM patch, too.


- Heikki



0001-Replace-isMD5-with-a-more-future-proof-way-to-check-.patch
Description: invalid/octet-stream


0002-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/14/2016 01:33 PM, Heikki Linnakangas wrote:

I just noticed that the manual for CREATE ROLE says:


Note that older clients might lack support for the MD5 authentication
mechanism that is needed to work with passwords that are stored
encrypted.


That's is incorrect. The alternative to MD5 authentication is plain
'password' authentication, and that works just fine with MD5-hashed
passwords. I think that sentence is a leftover from when we still
supported "crypt" authentication (so I actually get to blame you for
that ;-), commit 53a5026b). Back then, it was true that if an MD5 hash
was stored in pg_authid, you couldn't do "crypt" authentication. That
might have left old clients out in the cold.

Now that we're getting SCRAM authentication, we'll need a similar notice
there again, for the incompatibility of a SCRAM verifier with MDD5
authentication and vice versa.


I went ahead and removed the current bogus notice from the docs. We 
might need to put back something like it, with the SCRAM patch, but it 
needs to be rewritten anyway.


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Tue, Dec 20, 2016 at 9:23 PM, Heikki Linnakangas  wrote:
> On 12/16/2016 03:31 AM, Michael Paquier wrote:
> Actually, it does still perform that check. There's a new function,
> plain_crypt_verify, that passwordcheck uses now. plain_crypt_verify() is
> intended to work with any future hash formats we might introduce in the
> future (including SCRAM), so that passwordcheck doesn't need to know about
> all the hash formats.

Bah. I have misread the first version of the patch, and it is indeed
keeping the username checks. Now that things don't crash that behaves
as expected:
=# load 'passwordcheck';
LOAD
=# alter role mpaquier password 'mpaquier';
ERROR:  22023: password must not contain user name
LOCATION:  check_password, passwordcheck.c:101
=# alter role mpaquier password 'md58349d3a1bc8f4f7399b1ff9dea493b15';
ERROR:  22023: password must not contain user name
LOCATION:  check_password, passwordcheck.c:82
With the patch:

>> +case PASSWORD_TYPE_PLAINTEXT:
>> +shadow_pass = _pass[strlen("plain:")];
>> +break;
>> It would be a good idea to have a generic routine able to get the plain
>> password value. In short I think that we should reduce the amount of
>> locations where "plain:" prefix is hardcoded.
>
> There is such a function included in the patch, get_plain_password(char
> *shadow_pass), actually. Contrib/passwordcheck uses it. I figured that in
> crypt.c itself, it's OK to do the above directly, but get_plain_password()
> is intended to be used elsewhere.

The idea would be to have the function not return an allocated string,
just a position to it. That would be useful in plain_crypt_verify()
for example, for a total of 4 places, including get_plain_password()
where the new string allocation is done. Well, it's not like this
prefix "plain:" would change anyway in the future nor that it is going
to spread much.

> Thanks for having a look! Attached is a new version, with that bug fixed.

I have been able more advanced testing without the crash and things
seem to work properly. The attached set of tests is also able to pass
for all the combinations of hba configurations and password formats.
And looking at the code I don't have more comments.
-- 
Michael


009_authentication.pl
Description: Perl program

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

David,

* David Fetter (da...@fetter.org) wrote:
> On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote:
> > * David Fetter (da...@fetter.org) wrote:
> > > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > > > * Heikki Linnakangas (hlinn...@iki.fi) wrote:
> > > > > Even if you have a separate "verifier type" column, it's not fully
> > > > > normalized, because there's still a dependency between the
> > > > > verifier and verifier type columns. You will always need to look
> > > > > at the verifier type to make sense of the verifier itself.
> > > > 
> > > > That's true- but you don't need to look at the verifier, or even
> > > > have *access* to the verifier, to look at the verifier type.
> > > 
> > > Would a view that shows only what's to the left of the first semicolon
> > > suit this purpose?
> > 
> > Obviously a (security barrier...) view or a (security definer) function
> > could be used, but I don't believe either is actually a good idea.
> 
> Would you be so kind as to help me understand what's wrong with that idea?

For starters, it doubles-down on the assumption that we'll always be
happy with that particular separator and implies to anyone watching that
they'll be able to trust it.  Further, it's additional complication
which, at least to my eyes, is entirely in the wrong direction.

We could push everything in pg_authid into a single colon-separated text
field and call it simpler because we don't have to deal with those silly
column things, and we'd have something a lot closer to a unix passwd
file too!, but it wouldn't make it a terribly smart thing to do.  We
aren't a bunch of individual C programs having to parse out things out
of flat text files, after all.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[David Fetter]

On Tue, Dec 20, 2016 at 06:14:40PM -0500, Stephen Frost wrote:
> David,
> 
> * David Fetter (da...@fetter.org) wrote:
> > On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > > * Heikki Linnakangas (hlinn...@iki.fi) wrote:
> > > > Even if you have a separate "verifier type" column, it's not fully
> > > > normalized, because there's still a dependency between the
> > > > verifier and verifier type columns. You will always need to look
> > > > at the verifier type to make sense of the verifier itself.
> > > 
> > > That's true- but you don't need to look at the verifier, or even
> > > have *access* to the verifier, to look at the verifier type.
> > 
> > Would a view that shows only what's to the left of the first semicolon
> > suit this purpose?
> 
> Obviously a (security barrier...) view or a (security definer) function
> could be used, but I don't believe either is actually a good idea.

Would you be so kind as to help me understand what's wrong with that idea?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

David,

* David Fetter (da...@fetter.org) wrote:
> On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> > * Heikki Linnakangas (hlinn...@iki.fi) wrote:
> > > Even if you have a separate "verifier type" column, it's not fully
> > > normalized, because there's still a dependency between the
> > > verifier and verifier type columns. You will always need to look
> > > at the verifier type to make sense of the verifier itself.
> > 
> > That's true- but you don't need to look at the verifier, or even
> > have *access* to the verifier, to look at the verifier type.
> 
> Would a view that shows only what's to the left of the first semicolon
> suit this purpose?

Obviously a (security barrier...) view or a (security definer) function
could be used, but I don't believe either is actually a good idea.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Wed, Dec 21, 2016 at 1:08 AM, David Fetter  wrote:
> Would a view that shows only what's to the left of the first semicolon
> suit this purpose?

Of course it would, you would just need to make the routines now
checking the shape of MD5 and SCRAM identifiers available at SQL level
and feed the strings into them. Now I am not sure that it's worth
having a new superuser view for that. pg_roles and pg_shadow hide the
information about verifiers.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[David Fetter]

On Tue, Dec 20, 2016 at 08:34:19AM -0500, Stephen Frost wrote:
> Heikki,
> 
> * Heikki Linnakangas (hlinn...@iki.fi) wrote:
> > Even if you have a separate "verifier type" column, it's not fully
> > normalized, because there's still a dependency between the
> > verifier and verifier type columns. You will always need to look
> > at the verifier type to make sense of the verifier itself.
> 
> That's true- but you don't need to look at the verifier, or even
> have *access* to the verifier, to look at the verifier type.

Would a view that shows only what's to the left of the first semicolon
suit this purpose?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

Heikki,

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> Even if you have a separate "verifier type" column, it's not fully
> normalized, because there's still a dependency between the verifier
> and verifier type columns. You will always need to look at the
> verifier type to make sense of the verifier itself.

That's true- but you don't need to look at the verifier, or even have
*access* to the verifier, to look at the verifier type.  That is
actually very useful when you start thinking about the downstream side
of this- what about the monitoring tool which will want to check and
make sure there are only certain verifier types being used?  It'll have
to be a superuser, or have access to some superuser security defined
function, and that really sucks.  I'm not saying that we would
necessairly want the verifier type to be publicly visible, but being
able to see it without being a superuser would be good, imv.

> It's more convenient to carry the type information with the verifier
> itself, in backend code, in pg_dump, etc. Sure, you could have a
> separate "transfer" text format that has the prefix, and strip it
> out when the datum enters the system. But it is even simpler to have
> only one format, with the prefix, and use that everywhere.

It's more convenient when you need to look at both- it's not more
convenient when you only wish to look at the verifier type.  Further, it
means that we have to have a construct that assumes things about the
verifier type and verifier- what if a verifier type came along that used
a colon?  We'd have to do some special magic to handle that correctly,
and that just sucks, and anyone who is writing code to generically deal
with these fields will end up writing that same code (or forgetting to,
and not handling the case correctly).

> It might make sense to add a separate column, to e.g. make it easier
> to e.g. query for users that have an MD5 verifier. You could do
> "WHERE rolverifiertype = 'md5'", instead of "WHERE rolpassword LIKE
> 'md5%'". It's not a big difference, though. But even if we did that,
> I would still love to have the type information *also* included with
> the verifier itself, for convenience. And if we include it in the
> verifier itself, adding a separate type column seems more trouble
> than it's worth.

I don't agree that it's "not a big difference."  As I argue above- your
approach also assumes that anyone who would like to investigate the
verifier type should have access to the verifier itself, which I do not
agree with.  I also have a hard time buying the argument that it's
really so much more convenient to have the verifier type included in the
same string as the verifier that we should duplicate that information
and then run the risk that we end up with the two not matching or that
we won't ever run into complications down the road when our chosen
separator causes us difficulties.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Tue, Dec 20, 2016 at 6:37 AM, Heikki Linnakangas  wrote:
> It's more convenient to carry the type information with the verifier itself,
> in backend code, in pg_dump, etc. Sure, you could have a separate "transfer"
> text format that has the prefix, and strip it out when the datum enters the
> system. But it is even simpler to have only one format, with the prefix, and
> use that everywhere.

I see your point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/16/2016 03:31 AM, Michael Paquier wrote:

On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas  wrote:

The only way to distinguish, is to know about every verifier kind there is,
and check whether rolpassword looks valid as anything else than a plaintext
password. And we already got tripped by a bug-of-omission on that once. If
we add more verifier formats in the future, it's bound to happen again.
Let's nip that source of bugs in the bud. Attached is a patch to implement
what I have in mind.


OK, I had a look at the patch proposed.

-if (!pg_md5_encrypt(username, username, namelen, encrypted))
-elog(ERROR, "password encryption failed");
-if (strcmp(password, encrypted) == 0)
-ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password must not contain user name")));

This patch removes the only possible check for MD5 hashes that it has
never been done in passwordcheck. It may be fine to remove it, but I would
think that it is a good source of example regarding what could be done with
MD5 hashes, though limited. So it seems to me that this check should involve
as well pg_md5_encrypt on the username and compare if with the MD5 hash
given by the caller.


Actually, it does still perform that check. There's a new function, 
plain_crypt_verify, that passwordcheck uses now. plain_crypt_verify() is 
intended to work with any future hash formats we might introduce in the 
future (including SCRAM), so that passwordcheck doesn't need to know 
about all the hash formats.



A simple ALTER USER role PASSWORD 'foo' causes a crash:


Ah, fixed.


+case PASSWORD_TYPE_PLAINTEXT:
+shadow_pass = _pass[strlen("plain:")];
+break;
It would be a good idea to have a generic routine able to get the plain
password value. In short I think that we should reduce the amount of
locations where "plain:" prefix is hardcoded.


There is such a function included in the patch, get_plain_password(char 
*shadow_pass), actually. Contrib/passwordcheck uses it. I figured that 
in crypt.c itself, it's OK to do the above directly, but 
get_plain_password() is intended to be used elsewhere.


Thanks for having a look! Attached is a new version, with that bug fixed.

- Heikki



0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p-2.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/16/2016 05:48 PM, Robert Haas wrote:

On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost  wrote:

* Heikki Linnakangas (hlinn...@iki.fi) wrote:

On 12/14/2016 04:57 PM, Stephen Frost wrote:

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:

On 12/14/16 5:15 AM, Michael Paquier wrote:

I would be tempted to suggest adding the verifier type as a new column
of pg_authid


Yes please.


This discussion seems to continue to come up and I don't entirely
understand why we keep trying to shove more things into pg_authid, or
worse, into rolpassword.


I understand the relational beauty of having a separate column for
the verifier type, but I don't think it would be practical.


I disagree.


Me, too.  I think the idea of moving everything into a separate table
that allows multiple verifiers is probably not a good thing to do just
right now, because that introduces a bunch of additional issues above
and beyond what we need to do to get SCRAM implemented.  There are
administration and policy decisions to be made there that we should
not conflate with SCRAM proper.

However, Heikki's proposal seems to be that it's reasonable to force
rolpassword to be of the form 'type:verifier' in all cases but not
reasonable to have separate columns for type and verifier.  Eh?


I fear we'll just have to agree to disagree here, but I'll try to 
explain myself one more time.


Even if you have a separate "verifier type" column, it's not fully 
normalized, because there's still a dependency between the verifier and 
verifier type columns. You will always need to look at the verifier type 
to make sense of the verifier itself.


It's more convenient to carry the type information with the verifier 
itself, in backend code, in pg_dump, etc. Sure, you could have a 
separate "transfer" text format that has the prefix, and strip it out 
when the datum enters the system. But it is even simpler to have only 
one format, with the prefix, and use that everywhere.


It might make sense to add a separate column, to e.g. make it easier to 
e.g. query for users that have an MD5 verifier. You could do "WHERE 
rolverifiertype = 'md5'", instead of "WHERE rolpassword LIKE 'md5%'". 
It's not a big difference, though. But even if we did that, I would 
still love to have the type information *also* included with the 
verifier itself, for convenience. And if we include it in the verifier 
itself, adding a separate type column seems more trouble than it's worth.


For comparison, imagine that we added a column to pg_authid for a 
picture of the user, stored as a bytea. The picture can be in JPEG or 
PNG format. Looking at the first few bytes of the image, you can tell 
which one it is. Would it make sense to add a separate "type" column, to 
tell what format the image is in? I think it would be more convenient 
and robust to rely on the first bytes of the image data instead.


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Sat, Dec 17, 2016 at 5:48 PM, Michael Paquier
 wrote:
> On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas  wrote:
>> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
>>  wrote:
>>> From the discussions of last year on -hackers, it was decided to *not*
>>> have an additional column per complains from a couple of hackers
>>> (Robert you were in this set at this point), ...
>>
>> Hmm, I don't recall taking that position, but then there are a lot of
>> things that I ought to recall and don't.  (Ask my wife!)
>
> [... digging objects of the past ...]
> From the past thread:
> https://www.postgresql.org/message-id/ca+tgmoy790rphhbogxmbtg6mzsendoxdbxebekaet9zpz8g...@mail.gmail.com
> The complain is directed directly to multiple verifiers per users
> though, not to have the type in a separate column.

Yes, I rather like the separate column.  But since Heikki is doing the
work (or if he is) I'm not going to gripe too much.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sun, Dec 18, 2016 at 3:59 AM, Robert Haas  wrote:
> On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
>  wrote:
>> From the discussions of last year on -hackers, it was decided to *not*
>> have an additional column per complains from a couple of hackers
>> (Robert you were in this set at this point), ...
>
> Hmm, I don't recall taking that position, but then there are a lot of
> things that I ought to recall and don't.  (Ask my wife!)

[... digging objects of the past ...]
>From the past thread:
https://www.postgresql.org/message-id/ca+tgmoy790rphhbogxmbtg6mzsendoxdbxebekaet9zpz8g...@mail.gmail.com
The complain is directed directly to multiple verifiers per users
though, not to have the type in a separate column.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Fri, Dec 16, 2016 at 5:30 PM, Michael Paquier
 wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
>> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>>> On 12/15/16 8:40 AM, Stephen Frost wrote:
>>> > I don't follow why we can't change the syntax for CREATE USER to allow
>>> > specifying the verifier type independently.
>>>
>>> That's what the last patch set I looked at actually does.
>>
>> Well, same here, but it was quite a while ago and things have progressed
>> since then wrt SCRAM, as I understand it...
>
> From the discussions of last year on -hackers, it was decided to *not*
> have an additional column per complains from a couple of hackers
> (Robert you were in this set at this point), ...

Hmm, I don't recall taking that position, but then there are a lot of
things that I ought to recall and don't.  (Ask my wife!)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sat, Dec 17, 2016 at 10:23 AM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> (Robert you were in this set at this point), and the same thing was
>> concluded during the informal lunch meeting at PGcon. The point is,
>> the existing SCRAM patch set can survive without touching at *all* the
>> format of pg_authid. We could block SCRAM authentication when
>> "password" is used in pg_hba.conf and as well as when "scram" is used
>> with a plain password stored in pg_authid. Or look at the format of
>> the string in the catalog if "password" is defined and decide the
>> authentication protocol to follow based on that.
>
> As I mentioned up-thread, moving forward with minimal changes to get
> SCRAM in certainly makes sense, but I do think we should be open to
> (and, ideally, encouraging people to work towards) having a seperate
> table for verifiers with independent columns for type and verifier.

Definitely, and you know my position on the matter or I would not have
written last year's patch series. Both things are just orthogonal IMO
at this point. And it would be good to focus just on one problem at
the moment to get it out.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> On 12/15/16 8:40 AM, Stephen Frost wrote:
> >> > I don't follow why we can't change the syntax for CREATE USER to allow
> >> > specifying the verifier type independently.
> >>
> >> That's what the last patch set I looked at actually does.
> >
> > Well, same here, but it was quite a while ago and things have progressed
> > since then wrt SCRAM, as I understand it...
> 
> From the discussions of last year on -hackers, it was decided to *not*
> have an additional column per complains from a couple of hackers

It seems that, at best, we didn't have consensus on it.  Hopefully we
are moving in a direction of consensus.

> (Robert you were in this set at this point), and the same thing was
> concluded during the informal lunch meeting at PGcon. The point is,
> the existing SCRAM patch set can survive without touching at *all* the
> format of pg_authid. We could block SCRAM authentication when
> "password" is used in pg_hba.conf and as well as when "scram" is used
> with a plain password stored in pg_authid. Or look at the format of
> the string in the catalog if "password" is defined and decide the
> authentication protocol to follow based on that.

As I mentioned up-thread, moving forward with minimal changes to get
SCRAM in certainly makes sense, but I do think we should be open to
(and, ideally, encouraging people to work towards) having a seperate
table for verifiers with independent columns for type and verifier.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Sat, Dec 17, 2016 at 5:42 AM, Stephen Frost  wrote:
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 12/15/16 8:40 AM, Stephen Frost wrote:
>> > I don't follow why we can't change the syntax for CREATE USER to allow
>> > specifying the verifier type independently.
>>
>> That's what the last patch set I looked at actually does.
>
> Well, same here, but it was quite a while ago and things have progressed
> since then wrt SCRAM, as I understand it...

>From the discussions of last year on -hackers, it was decided to *not*
have an additional column per complains from a couple of hackers
(Robert you were in this set at this point), and the same thing was
concluded during the informal lunch meeting at PGcon. The point is,
the existing SCRAM patch set can survive without touching at *all* the
format of pg_authid. We could block SCRAM authentication when
"password" is used in pg_hba.conf and as well as when "scram" is used
with a plain password stored in pg_authid. Or look at the format of
the string in the catalog if "password" is defined and decide the
authentication protocol to follow based on that.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 12/15/16 8:40 AM, Stephen Frost wrote:
> > I don't follow why we can't change the syntax for CREATE USER to allow
> > specifying the verifier type independently.
> 
> That's what the last patch set I looked at actually does.

Well, same here, but it was quite a while ago and things have progressed
since then wrt SCRAM, as I understand it...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Peter Eisentraut]

On 12/15/16 8:40 AM, Stephen Frost wrote:
> I don't follow why we can't change the syntax for CREATE USER to allow
> specifying the verifier type independently.

That's what the last patch set I looked at actually does.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Robert Haas]

On Thu, Dec 15, 2016 at 8:40 AM, Stephen Frost  wrote:
> * Heikki Linnakangas (hlinn...@iki.fi) wrote:
>> On 12/14/2016 04:57 PM, Stephen Frost wrote:
>> >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> >>On 12/14/16 5:15 AM, Michael Paquier wrote:
>> >>>I would be tempted to suggest adding the verifier type as a new column
>> >>>of pg_authid
>> >>
>> >>Yes please.
>> >
>> >This discussion seems to continue to come up and I don't entirely
>> >understand why we keep trying to shove more things into pg_authid, or
>> >worse, into rolpassword.
>>
>> I understand the relational beauty of having a separate column for
>> the verifier type, but I don't think it would be practical.
>
> I disagree.

Me, too.  I think the idea of moving everything into a separate table
that allows multiple verifiers is probably not a good thing to do just
right now, because that introduces a bunch of additional issues above
and beyond what we need to do to get SCRAM implemented.  There are
administration and policy decisions to be made there that we should
not conflate with SCRAM proper.

However, Heikki's proposal seems to be that it's reasonable to force
rolpassword to be of the form 'type:verifier' in all cases but not
reasonable to have separate columns for type and verifier.  Eh?

>> For
>> starters, we'd still like to have a self-identifying string format
>> like "scram-sha-256:", so that you can conveniently pass the
>> verifier as a string to CREATE USER.
>
> I don't follow why we can't change the syntax for CREATE USER to allow
> specifying the verifier type independently.  Generally speaking, I don't
> expect *users* to be providing actual encoded *verifiers* very often, so
> it seems like a bit of extra syntax that pg_dump has to use isn't that
> big of a deal.

We don't have to change the CREATE USER syntax at all.  It could just
split on the first colon and put the two halves of the string in
different places.  Of course, changing the syntax might be a good idea
anyway -- or not --- but the point is, right now, when you look at
rolpassword, there's not a clear rule for what kind of thing you've
got in there.  That's absolutely terrible design and has got to be
fixed.  Heikki's proposal of prefixing every entry with a type and a
':' will solve that problem and I'm not going to roll over in my grave
if we do it that way, but there is such a thing as normalization and
that technique could be applied here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas  wrote:
> The only way to distinguish, is to know about every verifier kind there is,
> and check whether rolpassword looks valid as anything else than a plaintext
> password. And we already got tripped by a bug-of-omission on that once. If
> we add more verifier formats in the future, it's bound to happen again.
> Let's nip that source of bugs in the bud. Attached is a patch to implement
> what I have in mind.

OK, I had a look at the patch proposed.

-if (!pg_md5_encrypt(username, username, namelen, encrypted))
-elog(ERROR, "password encryption failed");
-if (strcmp(password, encrypted) == 0)
-ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("password must not contain user name")));

This patch removes the only possible check for MD5 hashes that it has
never been done in passwordcheck. It may be fine to remove it, but I would
think that it is a good source of example regarding what could be done with
MD5 hashes, though limited. So it seems to me that this check should involve
as well pg_md5_encrypt on the username and compare if with the MD5 hash
given by the caller. The new code is being careful about trying to pass
down a plain password, but it is possible to load MD5 hashes directly as
well, aka pg_dumpall.

A simple ALTER USER role PASSWORD 'foo' causes a crash:
#0  0x004764d7 in heap_compute_data_size (tupleDesc=0x277f090, 
values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106
106 VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
(gdb) bt
#0  0x004764d7 in heap_compute_data_size (tupleDesc=0x277f090, 
values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106
#1  0x004781e9 in heap_form_tuple (tupleDescriptor=0x277f090, 
values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:736
#2  0x004784d0 in heap_modify_tuple (tuple=0x277adc8, 
tupleDesc=0x277f090, replValues=0x7fff1369d030, replIsnull=0x7fff1369d020 "", 
doReplace=0x7fff1369d010 "")
at heaptuple.c:833
#3  0x00673788 in AlterRole (stmt=0x27a4f78) at user.c:845
#4  0x0082aa49 in standard_ProcessUtility (parsetree=0x27a4f78, 
queryString=0x27a43e8 "alter role ioltas password 'toto';", 
context=PROCESS_UTILITY_TOPLEVEL,
params=0x0, dest=0x27a5300, completionTag=0x7fff1369d5b0 "") at 
utility.c:711

+case PASSWORD_TYPE_PLAINTEXT:
+shadow_pass = _pass[strlen("plain:")];
+break;
It would be a good idea to have a generic routine able to get the plain
password value. In short I think that we should reduce the amount of
locations where "plain:" prefix is hardcoded.

> Alternatively, you could argue that we should forbid storing passwords in
> plaintext altogether. I'm OK with that, too, if that's what people prefer.
> Then you cannot have a user that can log in with both MD5 and SCRAM
> authentication, but it's certainly more secure, and it's easier to document.

At the end this may prove to be a bad idea for some developers. In local
deployments when working on a backend application with Postgres as backend,
it is actually useful to have plain passwords. At least I have found that
useful in some stuff I did many years ago.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> On 12/14/2016 04:57 PM, Stephen Frost wrote:
> >* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >>On 12/14/16 5:15 AM, Michael Paquier wrote:
> >>>I would be tempted to suggest adding the verifier type as a new column
> >>>of pg_authid
> >>
> >>Yes please.
> >
> >This discussion seems to continue to come up and I don't entirely
> >understand why we keep trying to shove more things into pg_authid, or
> >worse, into rolpassword.
> 
> I understand the relational beauty of having a separate column for
> the verifier type, but I don't think it would be practical.

I disagree.

> For
> starters, we'd still like to have a self-identifying string format
> like "scram-sha-256:", so that you can conveniently pass the
> verifier as a string to CREATE USER.

I don't follow why we can't change the syntax for CREATE USER to allow
specifying the verifier type independently.  Generally speaking, I don't
expect *users* to be providing actual encoded *verifiers* very often, so
it seems like a bit of extra syntax that pg_dump has to use isn't that
big of a deal.

> I think it'll be much better to
> stick to one format, than try to split the verifier into type and
> the string, when it enters the catalog table.

Apparently, multiple people disagree with this approach.  I don't think
history is really on your side here either.

> >We should have an independent table for the verifiers, which has a
> >different column for the verifier type, and either starts off supporting
> >multiple verifiers per role or at least gives us the ability to add that
> >easily later.  We should also move rolvaliduntil to that new table.
> 
> I agree we'll probably need a new table for verifiers. Or turn
> rolpassword into an array or something. We discussed that before,
> however, and it didn't really go anywhere, so right now I'd like to
> get SCRAM in with minimal changes to the rest of the system. There
> is a lot of room for improvement once it's in.

Using an array strikes me as an absolutely terrible idea- how are you
going to handle having different valid_until times then?

I do agree with trying to get SCRAM in without changing too much of the
rest of the system, but I wanted to make it clear that it's the only
point that I agree with for continuing down this path and that we should
absolutely be looking to change the CREATE USER syntax to specify the
verifier independently, plan to use a different table for the verifiers
with an independent column for the verifier type, support multiple
verifiers per role, etc, in the (hopefully very near...) future.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/15/2016 03:00 AM, Michael Paquier wrote:

On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas  wrote:

But, a password stored in plaintext works with either MD5 or SCRAM, or any
future authentication mechanism. So as soon as we have SCRAM authentication,
it becomes somewhat useful again.

In a nutshell:

auth / stored   MD5 SCRAM   plaintext
-
passwordY   Y   Y
md5 Y   N   Y
scram   N   Y   Y

If a password is stored in plaintext, it can be used with any authentication
mechanism. And the plaintext 'password' authentication mechanism works with
any kind of a stored password. But an MD5 hash cannot be used with SCRAM
authentication, or vice versa.


So.. I have been thinking about this portion of the thread. And what I
find the most scary is not the fact that we use plain passwords for
SCRAM authentication, it is the fact that we would need to do a
catalog lookup earlier in the connection workflow to decide what is
the connection protocol to use depending on the username provided in
the startup packet if the pg_hba.conf entry matching the user and
database names uses "password".


I don't see why we would need to do a catalog lookup any earlier. With 
"password" authentication, the server can simply request the client to 
send its password. When it receives it, it performs the catalog lookup 
to get pg_authid.rolpassword. If it's in plaintext, just compare it, if 
it's an MD5 hash, hash the client's password and compare, and if it's a 
SCRAM verifier, build a verifier with the same salt and iteration count 
and compare.



And, honestly, why do we actually need to have a support table that
spread? SCRAM is designed to be secure, so it seems to me that it
would on the contrary a bad idea to encourage the use of plain
passwords if we actually think that they should never be used (they
are actually useful for located, development instances, not production
ones).


I agree we should not encourage bad password practices. But as long as 
we support passwords to be stored in plaintext at all, it makes no sense 
to not allow them to be used with SCRAM. The fact that you can use a 
password stored in plaintext with both MD5 and SCRAM is literally the 
only reason you would store a password in plaintext, so if we don't want 
to allow that, we should disallow storing passwords in plaintext altogether.



So what I would suggest would be to have a support table like
that:
auth / stored   MD5 SCRAM   plaintext
-
passwordY   Y   N
md5 Y   N   Y
scram   N   N   Y


I was using 'Y' to indicate that the combination works, and 'N' to 
indicate that it does not. Assuming you're using the same notation, the 
above doesn't make any sense.



So here is an idea for things to do now:
1) do not change the format of the existing passwords
2) do not change pg_authid
3) block access to instances if "password" or "md5" are used in
pg_hba.conf if the user have a SCRAM verifier.
4) block access if "scram" is used and if user has a plain or md5 verifier.
5) Allow access if "scram" is used and if user has a SCRAM verifier.
We had a similar discussion regarding verifier/password formats last
year but that did not end well. It would be sad to fall back again
into this discussion and get no result. If somebody wants to support
access to SCRAM with plain password entries, why not. But that would
gain a -1 from me regarding the earlier lookup of pg_authid needed to
do the decision making on the protocol to use. And I think that we
want SCRAM to be designed to be a maximum stable and secure.


The bottom line is that at the moment, when plaintext passwords are 
stored as is, without any indicator that it's a plaintext password, it's 
ambiguous whether a password is a SCRAM verifier, or if it's a plaintext 
password that just happens to begin with the word "scram:". That is 
completely unrelated to which combinations of stored passwords and 
authentication mechanisms we actually support or allow to work.


The only way to distinguish, is to know about every verifier kind there 
is, and check whether rolpassword looks valid as anything else than a 
plaintext password. And we already got tripped by a bug-of-omission on 
that once. If we add more verifier formats in the future, it's bound to 
happen again. Let's nip that source of bugs in the bud. Attached is a 
patch to implement what I have in mind.


Alternatively, you could argue that we should forbid storing passwords 
in plaintext altogether. I'm OK with that, too, if that's what people 
prefer. Then you cannot have a user that can log in with both MD5 and 
SCRAM authentication, but it's certainly more secure, and it's easier to 
document.


- Heikki



0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers 

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/14/2016 04:57 PM, Stephen Frost wrote:

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:

On 12/14/16 5:15 AM, Michael Paquier wrote:

I would be tempted to suggest adding the verifier type as a new column
of pg_authid


Yes please.


This discussion seems to continue to come up and I don't entirely
understand why we keep trying to shove more things into pg_authid, or
worse, into rolpassword.


I understand the relational beauty of having a separate column for the 
verifier type, but I don't think it would be practical. For starters, 
we'd still like to have a self-identifying string format like 
"scram-sha-256:", so that you can conveniently pass the verifier 
as a string to CREATE USER. I think it'll be much better to stick to one 
format, than try to split the verifier into type and the string, when it 
enters the catalog table.



We should have an independent table for the verifiers, which has a
different column for the verifier type, and either starts off supporting
multiple verifiers per role or at least gives us the ability to add that
easily later.  We should also move rolvaliduntil to that new table.


I agree we'll probably need a new table for verifiers. Or turn 
rolpassword into an array or something. We discussed that before, 
however, and it didn't really go anywhere, so right now I'd like to get 
SCRAM in with minimal changes to the rest of the system. There is a lot 
of room for improvement once it's in.


- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Wed, Dec 14, 2016 at 8:33 PM, Heikki Linnakangas  wrote:
> But, a password stored in plaintext works with either MD5 or SCRAM, or any
> future authentication mechanism. So as soon as we have SCRAM authentication,
> it becomes somewhat useful again.
>
> In a nutshell:
>
> auth / stored   MD5 SCRAM   plaintext
> -
> passwordY   Y   Y
> md5 Y   N   Y
> scram   N   Y   Y
>
> If a password is stored in plaintext, it can be used with any authentication
> mechanism. And the plaintext 'password' authentication mechanism works with
> any kind of a stored password. But an MD5 hash cannot be used with SCRAM
> authentication, or vice versa.

So.. I have been thinking about this portion of the thread. And what I
find the most scary is not the fact that we use plain passwords for
SCRAM authentication, it is the fact that we would need to do a
catalog lookup earlier in the connection workflow to decide what is
the connection protocol to use depending on the username provided in
the startup packet if the pg_hba.conf entry matching the user and
database names uses "password".

And, honestly, why do we actually need to have a support table that
spread? SCRAM is designed to be secure, so it seems to me that it
would on the contrary a bad idea to encourage the use of plain
passwords if we actually think that they should never be used (they
are actually useful for located, development instances, not production
ones). So what I would suggest would be to have a support table like
that:
auth / stored   MD5 SCRAM   plaintext
-
passwordY   Y   N
md5 Y   N   Y
scram   N   N   Y

So here is an idea for things to do now:
1) do not change the format of the existing passwords
2) do not change pg_authid
3) block access to instances if "password" or "md5" are used in
pg_hba.conf if the user have a SCRAM verifier.
4) block access if "scram" is used and if user has a plain or md5 verifier.
5) Allow access if "scram" is used and if user has a SCRAM verifier.
We had a similar discussion regarding verifier/password formats last
year but that did not end well. It would be sad to fall back again
into this discussion and get no result. If somebody wants to support
access to SCRAM with plain password entries, why not. But that would
gain a -1 from me regarding the earlier lookup of pg_authid needed to
do the decision making on the protocol to use. And I think that we
want SCRAM to be designed to be a maximum stable and secure.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Joshua D. Drake]

On 12/14/2016 11:41 AM, Stephen Frost wrote:

* Heikki Linnakangas (hlinn...@iki.fi) wrote:

On 14 December 2016 20:12:05 EET, Bruce Momjian  wrote:

On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:



Storing plaintext passwords has been bad form for just about forever and
I wouldn't be sad to see our support of it go.  At the least, as was
discussed somewhere, but I'm not sure where it ended up, we should give
administrators the ability to control what ways a password can be
stored.  In particular, once a user has migrated all of their users to
SCRAM, they should be able to say "don't let new passwords be in any
format other than SCRAM-SHA-256".


It isn't as bad as it used to be. I remember with PASSWORD was the 
default. I agree that we should be able to set a policy that says, "we 
only allow X for password storage".


JD




Thanks!

Stephen




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

* Heikki Linnakangas (hlinn...@iki.fi) wrote:
> On 14 December 2016 20:12:05 EET, Bruce Momjian  wrote:
> >On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:
> >> I would so like to just drop support for plain passwords completely
> >:) But
> >> there's a backwards compatibility issue to think about of course.
> >> 
> >> But -- is there any actual usecase for them anymore?
> >
> >I thought we recommended 'password' for SSL connections because if you
> >use MD5 passwords the password text layout is known and that simplifies
> >cryptanalysis.
> 
> No, that makes no sense. And whether you use 'password' or 'md5' 
> authentication is a different question than whether you store passwords in 
> plaintext or as md5 hashes. Magnus was asking whether it ever makes sense to 
> *store* passwords in plaintext.

Right.

> Since you brought it up, there is a legitimate argument to be made that 
> 'password' authentication is more secure than 'md5', when SSL is used. 
> Namely, if an attacker can acquire contents of pg_authid e.g. by stealing a 
> backup tape, with 'md5' authentication he can log in as any user, using just 
> the stolen hashes. But with 'password', he needs to reverse the hash first. 
> It's not a great difference, but it's something.

Tunnelled passwords which are stored as hashes is also well understood
and comparable to SSH with passwords in /etc/passwd.

Storing plaintext passwords has been bad form for just about forever and
I wouldn't be sad to see our support of it go.  At the least, as was
discussed somewhere, but I'm not sure where it ended up, we should give
administrators the ability to control what ways a password can be
stored.  In particular, once a user has migrated all of their users to
SCRAM, they should be able to say "don't let new passwords be in any
format other than SCRAM-SHA-256".

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 14 December 2016 20:12:05 EET, Bruce Momjian  wrote:
>On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:
>> I would so like to just drop support for plain passwords completely
>:) But
>> there's a backwards compatibility issue to think about of course.
>> 
>> But -- is there any actual usecase for them anymore?
>
>I thought we recommended 'password' for SSL connections because if you
>use MD5 passwords the password text layout is known and that simplifies
>cryptanalysis.

No, that makes no sense. And whether you use 'password' or 'md5' authentication 
is a different question than whether you store passwords in plaintext or as md5 
hashes. Magnus was asking whether it ever makes sense to *store* passwords in 
plaintext.

Since you brought it up, there is a legitimate argument to be made that 
'password' authentication is more secure than 'md5', when SSL is used. Namely, 
if an attacker can acquire contents of pg_authid e.g. by stealing a backup 
tape, with 'md5' authentication he can log in as any user, using just the 
stolen hashes. But with 'password', he needs to reverse the hash first. It's 
not a great difference, but it's something.

 - Heikki


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Bruce Momjian]

On Wed, Dec 14, 2016 at 11:27:15AM +0100, Magnus Hagander wrote:
> I would so like to just drop support for plain passwords completely :) But
> there's a backwards compatibility issue to think about of course.
> 
> But -- is there any actual usecase for them anymore?

I thought we recommended 'password' for SSL connections because if you
use MD5 passwords the password text layout is known and that simplifies
cryptanalysis.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 12/14/16 5:15 AM, Michael Paquier wrote:
> > I would be tempted to suggest adding the verifier type as a new column
> > of pg_authid
> 
> Yes please.

This discussion seems to continue to come up and I don't entirely
understand why we keep trying to shove more things into pg_authid, or
worse, into rolpassword.

We should have an independent table for the verifiers, which has a
different column for the verifier type, and either starts off supporting
multiple verifiers per role or at least gives us the ability to add that
easily later.  We should also move rolvaliduntil to that new table.

No, I am specifically *not* concerned with "backwards compatibility" of
that table- we continually add to it and change it and applications
which are so closely tied to PG that they look at pg_authid need to be
updated with nearly every release anyway.  What we *do* need to make
sure we get correct is what pg_dump/pg_upgrade do, but that's entirely
within our control to manage and shouldn't be that much of an issue to
implement.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Peter Eisentraut]

On 12/14/16 5:15 AM, Michael Paquier wrote:
> I would be tempted to suggest adding the verifier type as a new column
> of pg_authid

Yes please.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/14/2016 12:27 PM, Magnus Hagander wrote:

I would so like to just drop support for plain passwords completely :) But
there's a backwards compatibility issue to think about of course.

But -- is there any actual usecase for them anymore?


Hmm. At the moment, I don't think there is.

But, a password stored in plaintext works with either MD5 or SCRAM, or 
any future authentication mechanism. So as soon as we have SCRAM 
authentication, it becomes somewhat useful again.


In a nutshell:

auth / stored   MD5 SCRAM   plaintext
-
passwordY   Y   Y
md5 Y   N   Y
scram   N   Y   Y

If a password is stored in plaintext, it can be used with any 
authentication mechanism. And the plaintext 'password' authentication 
mechanism works with any kind of a stored password. But an MD5 hash 
cannot be used with SCRAM authentication, or vice versa.



I just noticed that the manual for CREATE ROLE says:


Note that older clients might lack support for the MD5 authentication
mechanism that is needed to work with passwords that are stored
encrypted.


That's is incorrect. The alternative to MD5 authentication is plain 
'password' authentication, and that works just fine with MD5-hashed 
passwords. I think that sentence is a leftover from when we still 
supported "crypt" authentication (so I actually get to blame you for 
that ;-), commit 53a5026b). Back then, it was true that if an MD5 hash 
was stored in pg_authid, you couldn't do "crypt" authentication. That 
might have left old clients out in the cold.


Now that we're getting SCRAM authentication, we'll need a similar notice 
there again, for the incompatibility of a SCRAM verifier with MDD5 
authentication and vice versa.




If not, another option could be to just specifically check that it's *not*
"md5" or "scram-:". That would invalidate
plaintext passwords that have those texts in them of course, but what's the
likelyhood of that in reality?


Hmm, we have dismissed that risk for the MD5 hashes (and we also have a 
length check for them), but as we get new hash formats, the risk 
increases. Someone might well want to use "plain:of:jars" as password. 
Perhaps we should use a more complicated pattern.


I googled around for how others store SCRAM and other password hashes. 
Many other systems seem to have similar naming schemes. The closest 
thing to a standard I could find was:


https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md

Perhaps we should also use something like "$plain$" or 
"$scram-sha-256"?


There's also https://tools.ietf.org/html/rfc5803, which specifies how to 
store SCRAM verifiers in LDAP. I don't understand enough of LDAP to 
understand what those actually look like, though, and there were no 
examples in the RFC.


I wonder if we should also worry about storing multiple verifiers in 
rolpassword? We don't support that now, but we might in the future. It 
might come handy, if you could easily store multiple hashes in a single 
string, separated by commas for example.


- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Heikki Linnakangas]

On 12/14/2016 12:15 PM, Michael Paquier wrote:

This work is definitely something that should be done before anything
else. Need a patch or are you on it?


I'm on it..

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Magnus Hagander]

On Wed, Dec 14, 2016 at 9:51 AM, Heikki Linnakangas  wrote:

> On 12/09/2016 10:19 AM, Michael Paquier wrote:
>
>> On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas 
>> wrote:
>>
>>> Couple of things I should write down before I forget:
>>>
>>> 1. It's a bit cumbersome that the scram verifiers stored in
>>> pg_authid.rolpassword don't have any clear indication that they're scram
>>> verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I
>>> think
>>> we should use a "scram-sha-256:" for scram verifiers.
>>>
>>
>> scram-sha-256 would make the most sense to me.
>>
>> Actually, I think it'd be awfully nice to also prefix plaintext passwords
>>> with "plain:", but I'm not sure it's worth breaking the compatibility, if
>>> there are tools out there that peek into rolpassword. Thoughts?
>>>
>>
>> pgbouncer is the only thing coming up in mind. It looks at pg_shadow
>> for password values. pg_dump'ing data from pre-10 instances will also
>> need to adapt. I see tricky the compatibility with the exiting CREATE
>> USER PASSWORD command though, so I am wondering if that's worth the
>> complication.
>>
>> 2. It's currently not possible to use the plaintext "password"
>>> authentication method, for a user that has a SCRAM verifier in
>>> rolpassword.
>>> That seems like an oversight. We can't do MD5 authentication with a SCRAM
>>> verifier, but "password" we could.
>>>
>>
>> Yeah, that should be possible...
>>
>
> The tip of the work branch can now do SCRAM authentication, when a user
> has a plaintext password in pg_authid.rolpassword. The reverse doesn't
> work, however: you cannot do plain "password" authentication, when the user
> has a SCRAM verifier in pg_authid.rolpassword. It gets worse: plain
> "password" authentication doesn't check if the string stored in
> pg_authid.rolpassword is a SCRAM authenticator, and treats it as a
> plaintext password, so you can do this:
>
> PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1
> a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d7131
> 49c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"  psql
> postgres  -h localhost -U scram_user
>
> I think we're going to have a more bugs like this, if we don't start to
> explicitly label plaintext passwords as such.
>
> So, let's add "plain:" prefix to plaintext passwords, in
> pg_authid.rolpassword. With that, these would be valid values in
> pg_authid.rolpassword:
>
> plain:foo
> md55a962ce7a24371a10e85627a484cac28
> scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b1
> 9715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56
> bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f
>

I would so like to just drop support for plain passwords completely :) But
there's a backwards compatibility issue to think about of course.

But -- is there any actual usecase for them anymore?

If not, another option could be to just specifically check that it's *not*
"md5" or "scram-:". That would invalidate
plaintext passwords that have those texts in them of course, but what's the
likelyhood of that in reality?

Though I guess that might at least in theory be more bug-prone, so going
with a "plain:" prefix seems like a good idea as well.



> But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:"
> would be invalid. You shouldn't have invalid values in the column, but if
> you do, all the authentication mechanisms would reject it.
>
> It would be nice to also change the format of MD5 passwords to have a
> colon, as in "md5:", but that's probably not worth breaking
> compatibility for. Almost no-one stores passwords in plaintext, so changing
> the format of that wouldn't affect many people, but there might well be
> tools out there that peek into MD5 hashes.


There are definitely tools that do that, so +1 on leaving that alone.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: pg_authid.rolpassword format (was Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol)

[Michael Paquier]

On Wed, Dec 14, 2016 at 5:51 PM, Heikki Linnakangas  wrote:
> The tip of the work branch can now do SCRAM authentication, when a user has
> a plaintext password in pg_authid.rolpassword. The reverse doesn't work,
> however: you cannot do plain "password" authentication, when the user has a
> SCRAM verifier in pg_authid.rolpassword. It gets worse: plain "password"
> authentication doesn't check if the string stored in pg_authid.rolpassword
> is a SCRAM authenticator, and treats it as a plaintext password, so you can
> do this:
>
> PGPASSWORD="scram-sha-256:mDBuqO1mEekieg==:4096:17dc259499c1a184c26ee5b19715173d9354195f510b4d3af8be585acb39ae33:d3d713149c6becbbe56bae259aafe4e95b79ab7e3b50f2fbd850ea7d7b7c114f"
> psql postgres  -h localhost -U scram_user

This one's fun.

> I think we're going to have a more bugs like this, if we don't start to
> explicitly label plaintext passwords as such.
>
> So, let's add "plain:" prefix to plaintext passwords, in
> pg_authid.rolpassword. With that, these would be valid values in
> pg_authid.rolpassword:
>
> [...]
>
> But anything that doesn't begin with "plain:", "md5", or "scram-sha-256:"
> would be invalid. You shouldn't have invalid values in the column, but if
> you do, all the authentication mechanisms would reject it.

I would be tempted to suggest adding the verifier type as a new column
of pg_authid, but as CREATE USER PASSWORD accepts strings with md5
prefix as-is for ages using the "plain:" prefix is definitely a better
plan. My opinion on the matter has changed compared to a couple of
months back.

> It would be nice to also change the format of MD5 passwords to have a colon,
> as in "md5:", but that's probably not worth breaking compatibility
> for. Almost no-one stores passwords in plaintext, so changing the format of
> that wouldn't affect many people, but there might well be tools out there
> that peek into MD5 hashes.

Yes, let's not take this road.

This work is definitely something that should be done before anything
else. Need a patch or are you on it?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers