Password identifiers, protocol aging and SCRAM protocol

Hi, Michael

23.02.2016 10:17, Michael Paquier пишет:

Attached is a set of patches implementing a couple of things that have
been discussed, so let's roll in.

Those 4 patches are aimed at putting in-core basics for the concept I
call password protocol aging, which is a way to allow multiple
password protocols to be defined in Postgres, and aimed at easing
administration as well as retirement of outdated protocols, which is
something that is not doable now in Postgres.

The second set of patch 0005~0008 introduces a new protocol, SCRAM.
9) 0009 is the SCRAM authentication itself....

The theme with password checking is interesting for me, and I can give
review for CF for some features.
I think that review of all suggested features will require a lot of time.
Is it possible to make subset of patches concerning only password
strength and its aging?
The patches you have applied are non-independent. They should be apply
consequentially one by one.
Thus the patch 0009 can't be applied without git error before 0001.
In this conditions all patches were successfully applied and compiled.
All tests successfully passed.

The first 4 patches obviously are the core portion that I would like
to discuss about in this CF, as they put in the base for the rest, and
will surely help Postgres long-term. 0005~0008 are just refactoring
patches, so they are quite simple. 0009 though is quite difficult, and
needs careful review because it manipulates areas of the code where it
is not necessary to be an authenticated user, so if there are bugs in
it it would be possible for example to crash down Postgres just by
sending authentication requests.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

On Fri, Feb 26, 2016 at 1:38 AM, Valery Popov <v.popov@postgrespro.ru> wrote:

Hi, Michael

23.02.2016 10:17, Michael Paquier пишет:

Attached is a set of patches implementing a couple of things that have
been discussed, so let's roll in.

Those 4 patches are aimed at putting in-core basics for the concept I
call password protocol aging, which is a way to allow multiple
password protocols to be defined in Postgres, and aimed at easing
administration as well as retirement of outdated protocols, which is
something that is not doable now in Postgres.

The second set of patch 0005~0008 introduces a new protocol, SCRAM.
9) 0009 is the SCRAM authentication itself....

The theme with password checking is interesting for me, and I can give
review for CF for some features.
I think that review of all suggested features will require a lot of time.
Is it possible to make subset of patches concerning only password strength
and its aging?
The patches you have applied are non-independent. They should be apply
consequentially one by one.
Thus the patch 0009 can't be applied without git error before 0001.
In this conditions all patches were successfully applied and compiled.
All tests successfully passed.

If you want to focus on the password protocol aging, you could just
have a look at 0001~0004.
--
Michael

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

26.02.2016 01:10, Michael Paquier пишет:

On Fri, Feb 26, 2016 at 1:38 AM, Valery Popov <v.popov@postgrespro.ru> wrote:

Hi, Michael

23.02.2016 10:17, Michael Paquier пишет:

Attached is a set of patches implementing a couple of things that have
been discussed, so let's roll in.

Those 4 patches are aimed at putting in-core basics for the concept I
call password protocol aging, which is a way to allow multiple
password protocols to be defined in Postgres, and aimed at easing
administration as well as retirement of outdated protocols, which is
something that is not doable now in Postgres.

The second set of patch 0005~0008 introduces a new protocol, SCRAM.
9) 0009 is the SCRAM authentication itself....

The theme with password checking is interesting for me, and I can give
review for CF for some features.
I think that review of all suggested features will require a lot of time.
Is it possible to make subset of patches concerning only password strength
and its aging?
The patches you have applied are non-independent. They should be apply
consequentially one by one.
Thus the patch 0009 can't be applied without git error before 0001.
In this conditions all patches were successfully applied and compiled.
All tests successfully passed.

If you want to focus on the password protocol aging, you could just
have a look at 0001~0004.

OK, I will review patches 0001-0004, for starting.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

Hi, Michael

23.02.2016 10:17, Michael Paquier пишет:

Attached is a set of patches implementing a couple of things that have
been discussed, so let's roll in.

Those 4 patches are aimed at putting in-core basics for the concept I
call password protocol aging, which is a way to allow multiple
password protocols to be defined in Postgres, and aimed at easing
administration as well as retirement of outdated protocols, which is
something that is not doable now in Postgres.

The second set of patch 0005~0008 introduces a new protocol, SCRAM.
9) 0009 is the SCRAM authentication itself....

The theme with password checking is interesting for me, and I can give
review for CF for some features.
I think that review of all suggested features will require a lot of
time.
Is it possible to make subset of patches concerning only password
strength
and its aging?
The patches you have applied are non-independent. They should be apply
consequentially one by one.
Thus the patch 0009 can't be applied without git error before 0001.
In this conditions all patches were successfully applied and compiled.
All tests successfully passed.

If you want to focus on the password protocol aging, you could just
have a look at 0001~0004.

OK, I will review patches 0001-0004, for starting.

Below are the results of compiling and testing.
============================
I've got the last version of sources from
git://git.postgresql.org/git/postgresql.git.

vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git branch
* master

Then I've applied patches 0001-0004 with two warnings:
vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git apply
0001-Add-facility-to-store-multiple-password-verifiers.patch
0001-Add-facility-to-store-multiple-password-verifiers.patch:2547:
trailing whitespace.
warning: 1 line adds whitespace errors.
vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git apply
0002-Introduce-password_protocols.patch
vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git apply
0003-Add-pg_auth_verifiers_sanitize.patch
0003-Add-pg_auth_verifiers_sanitize.patch:87: indent with spaces.
if (!superuser())
warning: 1 line adds whitespace errors.
vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git apply
0004-Remove-password-verifiers-for-unsupported-protocols-.patch
The compilation with option ./configure --enable-debug --enable-nls
--enable-cassert --enable-tap-tests --with-perl
was successful.
Regression tests and all TAP-tests also passed successfully.

Also I've applied patches 0005-0008 into clean sources directory with no
warnings.
vpopov@vpopov-Ubuntu:~/Projects/pwdtest2/postgresql$ git apply
0005-Move-sha1.c-to-src-common.patch
vpopov@vpopov-Ubuntu:~/Projects/pwdtest2/postgresql$ git apply
0006-Refactor-sendAuthRequest.patch
vpopov@vpopov-Ubuntu:~/Projects/pwdtest2/postgresql$ git apply
0007-Refactor-RandomSalt-to-handle-salts-of-different-len.patch
vpopov@vpopov-Ubuntu:~/Projects/pwdtest2/postgresql$ git apply
0008-Move-encoding-routines-to-src-common.patch
The compilation with option ./configure --enable-debug --enable-nls
--enable-cassert --enable-tap-tests --with-perl
was successful.
Regression and the TAP-tests also passed successfully.

The patch 0009 depends on all previous patches 0001-0008: first we need
to apply patches 0001-0008, then 0009.
Then, all patches were successfully compiled.
All test passed.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

On Mon, Feb 29, 2016 at 8:43 PM, Valery Popov <v.popov@postgrespro.ru> wrote:

vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git branch

Thanks for the input!

0001-Add-facility-to-store-multiple-password-verifiers.patch:2547: trailing
whitespace.
warning: 1 line adds whitespace errors.
0003-Add-pg_auth_verifiers_sanitize.patch:87: indent with spaces.
if (!superuser())
warning: 1 line adds whitespace errors.

Argh, yes. Those two ones have slipped though my successive rebases I
think. Will fix in my tree, I don't think that it is worth sending
again the whole series just for that 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

On 1 March 2016 at 06:34, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Feb 29, 2016 at 8:43 PM, Valery Popov <v.popov@postgrespro.ru>
wrote:

vpopov@vpopov-Ubuntu:~/Projects/pwdtest/postgresql$ git branch

Thanks for the input!

0001-Add-facility-to-store-multiple-password-verifiers.patch:2547:

trailing

whitespace.
warning: 1 line adds whitespace errors.
0003-Add-pg_auth_verifiers_sanitize.patch:87: indent with spaces.
if (!superuser())
warning: 1 line adds whitespace errors.

Argh, yes. Those two ones have slipped though my successive rebases I
think. Will fix in my tree, I don't think that it is worth sending
again the whole series just for that 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

Hi, Michael

Few questions about the documentation.

config.sgml:1200

<listitem>
<para>
Specifies a comma-separated list of supported password formats by
the server. Supported formats are currently <literal>plain</> and
<literal>md5</>.
</para>

<para>
When a password is specified in <xref linkend="sql-createuser"> or
<xref linkend="sql-alterrole">, this parameter determines if the
password specified is authorized to be stored or not, returning
an error message to caller if it is not.
</para>

<para>
The default is <literal>plain,md5,scram</>, meaning that

MD5-encrypted

passwords, plain passwords, and SCRAM-encrypted passwords are

accepted.

</para>
</listitem>

The default value contains "scram". Shouldn't be here also:

Specifies a comma-separated list of supported password formats by
the server. Supported formats are currently <literal>plain</>,
<literal>md5</> and <literal>scram</>.

Or I missed something?

And one more:

config.sgml:1284

<para>
<varname>db_user_namespace</> causes the client's and
server's user name representation to differ.
Authentication checks are always done with the server's user name
so authentication methods must be configured for the
server's user name, not the client's. Because
<literal>md5</> uses the user name as salt on both the
client and server, <literal>md5</> cannot be used with
<varname>db_user_namespace</>.
</para>

Looks like the same (pls, correct me if I'm wrong) is applicable for
"scram" as I see from the code below. Shouldn't be "scram" mentioned here
also? Here's the code:

diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 28f9fb5..df0cc1d 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1184,6 +1184,19 @@ parse_hba_line(List *line, int line_num, char

*raw_line)

}
parsedline->auth_method = uaMD5;
}
+ else if (strcmp(token->string, "scram") == 0)
+ {
+ if (Db_user_namespace)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("SCRAM authentication is not supported when \"db_user_namespace\"

is enabled"),

On Wed, Mar 2, 2016 at 4:05 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

[...]

Thanks for the review.

The default value contains "scram". Shouldn't be here also:

Specifies a comma-separated list of supported password formats by
the server. Supported formats are currently <literal>plain</>,
<literal>md5</> and <literal>scram</>.

Or I missed something?

Ah, I see. That's in the documentation of password_protocols. Yes
scram should be listed there as well. That should be fixed in 0009.

<para>
<varname>db_user_namespace</> causes the client's and
server's user name representation to differ.
Authentication checks are always done with the server's user name
so authentication methods must be configured for the
server's user name, not the client's. Because
<literal>md5</> uses the user name as salt on both the
client and server, <literal>md5</> cannot be used with
<varname>db_user_namespace</>.
</para>

Looks like the same (pls, correct me if I'm wrong) is applicable for "scram"
as I see from the code below. Shouldn't be "scram" mentioned here also?

Oops. Good catch. Yes it should be mentioned as part of the SCRAM patch (0009).
--
Michael

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

<para>
<varname>db_user_namespace</> causes the client's and
server's user name representation to differ.
Authentication checks are always done with the server's user name
so authentication methods must be configured for the
server's user name, not the client's. Because
<literal>md5</> uses the user name as salt on both the
client and server, <literal>md5</> cannot be used with
<varname>db_user_namespace</>.
</para>

Also in doc/src/sgml/ref/create_role.sgml is should be instead of
<term>PASSWORD VERIFIERS ( <replaceable
class="PARAMETER">verifier_type</replaceable> = '<replaceable
class="PARAMETER">password</replaceable>'</term>
like this
<term><literal>PASSWORD VERIFIERS</> ( <replaceable
class="PARAMETER">verifier_type</replaceable> = '<replaceable
class="PARAMETER">password</replaceable>'</term>-- Regards, Valery Popov
Postgres Professional http://www.postgrespro.com The Russian Postgres
Company

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

On Wed, Mar 2, 2016 at 5:43 PM, Valery Popov <v.popov@postgrespro.ru> wrote:

<para>
<varname>db_user_namespace</> causes the client's and
server's user name representation to differ.
Authentication checks are always done with the server's user name
so authentication methods must be configured for the
server's user name, not the client's. Because
<literal>md5</> uses the user name as salt on both the
client and server, <literal>md5</> cannot be used with
<varname>db_user_namespace</>.
</para>

Also in doc/src/sgml/ref/create_role.sgml is should be instead of
<term>PASSWORD VERIFIERS ( <replaceable
class="PARAMETER">verifier_type</replaceable> = '<replaceable
class="PARAMETER">password</replaceable>'</term>
like this
<term><literal>PASSWORD VERIFIERS</> ( <replaceable
class="PARAMETER">verifier_type</replaceable> = '<replaceable
class="PARAMETER">password</replaceable>'</term>

So the <literal> markup is missing. Thanks. I am taking note of 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

This is a review of "Password identifiers, protocol aging and SCRAM
protocol" patches
/messages/by-id/CAB7nPqSMXU35g=W9X74HVeQp0uvgJxvYOuA4A-A3M+0wfEBv-w@mail.gmail.com

Contents & Purpose
--------------------------
There was a discussion dedicated to SCRAM:
/messages/by-id/55192AFE.6080106@iki.fi

This set of patches implements the following:
- Introduce in Postgres an extensible password aging facility, by having
a new concept of 1 user/multiple password verifier, one password
verifier per protocol.
- Give to system administrators tools to decide unsupported protocols,
and have pg_upgrade use that
- Introduce new password protocols for Postgres, aimed at replacing
existing, say limited ones.

This set of patches consists of 9 separate patches.
Description of each patch is well described in initial thread email and
in comments.
The first set of patches 0001-0008 adds facility to store multiple
password verifiers,
CREATE ROLE and ALTER ROLE are extended with PASSWORD VERIFIERS, new
superuser GUC parameters which specifies a list of supported password
protocols in Postgres backend, added pg_auth_verifiers_sanitize
function, removed password verifiers for unsupported protocols in
pg_upgrade, and more features.
The second set of patch 0005~0008 introduces a new protocol, SCRAM, and
0009 is SCRAM itself.

Initial Run
-------------
Included in the patches are:
- source code
- regression tests
- documentation
The source code is well commented.
The patches are in context diff format and were applied correctly to
HEAD (there were 2 warnings, and it was fixed by author).
There were several markup warnings, should be fixed by author.
Regression tests pass successfully, without errors. It seems that the
patches work as expected.
The patch 0009 depends on all previous patches 0001-0008: first we need
to apply patches 0001-0008, then 0009.

Performance
-----------
I have not tested possible performance issues yet.

Conclusion
--------------
I think introduced features are useful and I vote for commit +1.

On 03/02/2016 02:55 PM, Michael Paquier wrote:

On Wed, Mar 2, 2016 at 5:43 PM, Valery Popov <v.popov@postgrespro.ru> wrote:
So the <literal> markup is missing. Thanks. I am taking note of it.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

On 2/23/16 2:17 AM, Michael Paquier wrote:

As a continuation of the thread firstly dedicated to SCRAM:
/messages/by-id/55192AFE.6080106@iki.fi
Here is a new thread aimed at gathering all the ideas of this previous
thread and aimed at clarifying a bit what has been discussed until now
regarding password protocols, verifiers, and SCRAM itself.

It looks like this patch set is a bit out of date.

When applying 0004:

$ git apply
../other/0004-Remove-password-verifiers-for-unsupported-protocols-.patch
error: patch failed: src/bin/pg_upgrade/pg_upgrade.c:262
error: src/bin/pg_upgrade/pg_upgrade.c: patch does not apply

Then I tried to build with just 0001-0003:

cd /postgres/src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3318
3319
3320
3321
3322
make[3]: *** [postgres.bki] Error 1

Could you provide an updated set of patches for review? Meanwhile I am
marking this as "waiting for author".

Thanks,
--
-David
david@pgmasters.net

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

On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david@pgmasters.net> wrote:

On 2/23/16 2:17 AM, Michael Paquier wrote:

As a continuation of the thread firstly dedicated to SCRAM:
/messages/by-id/55192AFE.6080106@iki.fi
Here is a new thread aimed at gathering all the ideas of this previous
thread and aimed at clarifying a bit what has been discussed until now
regarding password protocols, verifiers, and SCRAM itself.

It looks like this patch set is a bit out of date.

When applying 0004:

$ git apply
../other/0004-Remove-password-verifiers-for-unsupported-protocols-.patch
error: patch failed: src/bin/pg_upgrade/pg_upgrade.c:262
error: src/bin/pg_upgrade/pg_upgrade.c: patch does not apply

Then I tried to build with just 0001-0003:

cd /postgres/src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3318
3319
3320
3321
3322
make[3]: *** [postgres.bki] Error 1

Could you provide an updated set of patches for review? Meanwhile I am
marking this as "waiting for author".

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.
--
Michael

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

On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david@pgmasters.net> wrote:

Could you provide an updated set of patches for review? Meanwhile I am
marking this as "waiting for author".

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.

And here they are. I have addressed the documentation and the
whitespaces reported up to now at the same time.
--
Michael

Hi, All

On 03/15/2016 02:07 AM, Michael Paquier wrote:

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.
And here they are. I have addressed the documentation and the
whitespaces reported up to now at the same time.

I've applied all of 0001-0009 patches from the new set with no any
warnings to today's master branch.
Then compiled with configure options:
./configure --enable-debug --enable-nls --enable-cassert
--enable-tap-tests --with-perl
All regression tests passed successfully.
make check-world passed successfully.
make installcheck-world failed on several contrib modules:
dblink, file_fdw, hstore, pgcrypto, pgstattuple, postgres_fdw,
tablefunc. The tests results are attached.
Documentation looks good.
Where may be a problem with make check-world and make installcheck-world
results?

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

On Tue, Mar 15, 2016 at 3:46 PM, Valery Popov wrote:

make installcheck-world failed on several contrib modules:
dblink, file_fdw, hstore, pgcrypto, pgstattuple, postgres_fdw, tablefunc.
The tests results are attached.
Documentation looks good.
Where may be a problem with make check-world and make installcheck-world
results?

I cannot reproduce this, and my guess is that the binaries of those
contrib/ modules are not up to date for the installed instance of
Postgres you are running the tests on. Particularly I find this
portion doubtful:

SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
! server closed the connection unexpectedly
! This probably means the server terminated abnormally
! before or while processing the request.
! connection to server was lost

The set of patches I am proposing here does not go through those code
paths, and this is likely an aggregate failure.
--
Michael

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

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:

On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david@pgmasters.net> wrote:

Could you provide an updated set of patches for review? Meanwhile I am
marking this as "waiting for author".

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.

And here they are. I have addressed the documentation and the
whitespaces reported up to now at the same time.

For this first review I would like to focus on the user visible changes
introduced in 0001-0002.

First I created two new users with each type of supported verifier:

postgres=# create user test with password 'test';
CREATE ROLE
postgres=# create user testu with unencrypted password 'testu'
valid until '2017-01-01';
CREATE ROLE

1) I see that rolvaliduntil is still in pg_authid:

postgres=# select oid, rolname, rolvaliduntil from pg_authid;

oid | rolname | rolvaliduntil
-------+---------+------------------------
10 | vagrant |
16387 | test |
16388 | testu | 2017-01-01 00:00:00+00

I think that's OK if we now define it to be "role validity" (it's still
password validity in the patched docs). I would also like to see a
validuntil column in pg_auth_verifiers so we can track password
expiration for each verifier separately. For now I think it's enough to
copy the same validity both places since there can only be one verifier.

2) I don't think the column naming in pg_auth_verifiers is consistent
with other catalogs:

postgres=# select * from pg_auth_verifiers;

roleid | verimet | verival
--------+---------+-------------------------------------
16387 | m | md505a671c66aefea124cc08b76ea6d30bb
16388 | p | testu

System catalogs generally use a 3 character prefix so I would expect the
columns to be (if we pick avr as a prefix):

avrrole
avrmethod
avrverifier
avrvaliduntil

I'm not a big fan in abbreviating too much so you can see I've expanded
the names a bit.

3) rolpassword is still in pg_shadow even though it is not useful anymore:

postgres=# select usename, passwd, valuntil from pg_shadow;

usename | passwd | valuntil
---------+----------+------------------------
vagrant | ******** |
test | ******** |
testu | ******** | 2017-01-01 00:00:00+00

If anyone is actually using this column in a meaningful way they are in
for a nasty surprise when trying use the value in passwd as a verifier.
I would prefer to drop the column entirely and produce a clear error.

Perhaps a better option would be to drop pg_shadow entirely since it
seems to have no further purpose in life.

Thanks,
--
-David
david@pgmasters.net

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

Hi!

On 03/15/2016 06:59 PM, Michael Paquier wrote:

The set of patches I am proposing here does not go through those code
paths, and this is likely an aggregate failure.

Michael, you were right. It was incorrect installation of contrib binaries.
Now all tests pass OK, both check-world and installcheck-world,
Thanks.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

On Tue, Mar 15, 2016 at 6:38 PM, David Steele <david@pgmasters.net> wrote:

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:

On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Mar 14, 2016 at 4:32 PM, David Steele <david@pgmasters.net> wrote:

Could you provide an updated set of patches for review? Meanwhile I am
marking this as "waiting for author".

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.

And here they are. I have addressed the documentation and the
whitespaces reported up to now at the same time.

For this first review I would like to focus on the user visible changes
introduced in 0001-0002.

Thanks for the input!

1) I see that rolvaliduntil is still in pg_authid:
I think that's OK if we now define it to be "role validity" (it's still
password validity in the patched docs). I would also like to see a
validuntil column in pg_auth_verifiers so we can track password
expiration for each verifier separately. For now I think it's enough to
copy the same validity both places since there can only be one verifier.

FWIW, this is an intentional change, and my goal is to focus on only
the protocol aging for now. We will need to move rolvaliduntil to
pg_auth_verifiers if we want to allow rolling updates of password
verifiers for a given role, but that's a different patch, and we need
to think about the SQL interface carefully. This infrastructure makes
the move easier by the way to do that, and honestly I don't really see
what we gain now by copying the same value to two different system
catalogs.

2) I don't think the column naming in pg_auth_verifiers is consistent
with other catalogs:
postgres=# select * from pg_auth_verifiers;
roleid | verimet | verival
--------+---------+-------------------------------------
16387 | m | md505a671c66aefea124cc08b76ea6d30bb
16388 | p | testu

System catalogs generally use a 3 character prefix so I would expect the
columns to be (if we pick avr as a prefix):

OK, this makes sense.

avrrole
avrmethod
avrverifier

Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
I kind of like those ones, more than with "avr" as prefix actually.
Other ideas are of course welcome.

I'm not a big fan in abbreviating too much so you can see I've expanded
the names a bit.

Sure.

3) rolpassword is still in pg_shadow even though it is not useful anymore:
postgres=# select usename, passwd, valuntil from pg_shadow;

usename | passwd | valuntil
---------+----------+------------------------
vagrant | ******** |
test | ******** |
testu | ******** | 2017-01-01 00:00:00+00

If anyone is actually using this column in a meaningful way they are in
for a nasty surprise when trying use the value in passwd as a verifier.
I would prefer to drop the column entirely and produce a clear error.

Perhaps a better option would be to drop pg_shadow entirely since it
seems to have no further purpose in life.

We discussed that on the previous thread and the conclusion was to
keep pg_shadow, but to clobber the password value with "***",
explaining this choice:
/messages/by-id/6174.1455501497@sss.pgh.pa.us
--
Michael

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

On 3/16/16 9:00 AM, Michael Paquier wrote:

On Tue, Mar 15, 2016 at 6:38 PM, David Steele <david@pgmasters.net> wrote:

1) I see that rolvaliduntil is still in pg_authid:
I think that's OK if we now define it to be "role validity" (it's still
password validity in the patched docs). I would also like to see a
validuntil column in pg_auth_verifiers so we can track password
expiration for each verifier separately. For now I think it's enough to
copy the same validity both places since there can only be one verifier.

FWIW, this is an intentional change, and my goal is to focus on only
the protocol aging for now. We will need to move rolvaliduntil to
pg_auth_verifiers if we want to allow rolling updates of password
verifiers for a given role, but that's a different patch, and we need
to think about the SQL interface carefully. This infrastructure makes
the move easier by the way to do that, and honestly I don't really see
what we gain now by copying the same value to two different system
catalogs.

Here's my thinking. If validuntil is moved to pg_auth_verifiers now
then people can start using it there. That will make it less traumatic
when/if validuntil in pg_authid is removed later. The field in
pg_authid could be deprecated in this release to let people know not to
use it.

Or, as I suggested it could be recast as role validity, which right now
happens to be the same as password validity.

2) I don't think the column naming in pg_auth_verifiers is consistent
with other catalogs:
postgres=# select * from pg_auth_verifiers;
roleid | verimet | verival
--------+---------+-------------------------------------
16387 | m | md505a671c66aefea124cc08b76ea6d30bb
16388 | p | testu

System catalogs generally use a 3 character prefix so I would expect the
columns to be (if we pick avr as a prefix):

OK, this makes sense.

avrrole
avrmethod
avrverifier

Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
I kind of like those ones, more than with "avr" as prefix actually.
Other ideas are of course welcome.

ver is fine as a prefix.

3) rolpassword is still in pg_shadow even though it is not useful anymore:
postgres=# select usename, passwd, valuntil from pg_shadow;

usename | passwd | valuntil
---------+----------+------------------------
vagrant | ******** |
test | ******** |
testu | ******** | 2017-01-01 00:00:00+00

If anyone is actually using this column in a meaningful way they are in
for a nasty surprise when trying use the value in passwd as a verifier.
I would prefer to drop the column entirely and produce a clear error.

Perhaps a better option would be to drop pg_shadow entirely since it
seems to have no further purpose in life.

We discussed that on the previous thread and the conclusion was to
keep pg_shadow, but to clobber the password value with "***",
explaining this choice:
/messages/by-id/6174.1455501497@sss.pgh.pa.us

Ah, I missed that one.

--
-David
david@pgmasters.net

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