Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
This is a review of "Password identifiers, protocol aging and SCRAM protocol" patches http://www.postgresql.org/message-id/CAB7nPqSMXU35g=w9x74hveqp0uvgjxvyoua4a-a3m+0wfeb...@mail.gmail.com Contents & Purpose -- There was a discussion dedicated to SCRAM: http://www.postgresql.org/message-id/55192afe.6080...@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 wrote: So the 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
Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
On Wed, Mar 2, 2016 at 5:43 PM, Valery Popov wrote: > >>> >>> 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 >>> md5 uses the user name as salt on both the >>> client and server, md5 cannot be used with >>> db_user_namespace. >>> > > Also in doc/src/sgml/ref/create_role.sgml is should be instead of > PASSWORD VERIFIERS ( class="PARAMETER">verifier_type = ' class="PARAMETER">password' > like this > PASSWORD VERIFIERS ( class="PARAMETER">verifier_type = ' class="PARAMETER">password' So the 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
Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
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 md5 uses the user name as salt on both the client and server, md5 cannot be used with db_user_namespace. Also in doc/src/sgml/ref/create_role.sgml is should be instead of PASSWORD VERIFIERS ( class="PARAMETER">verifier_type = 'class="PARAMETER">password' like this PASSWORD VERIFIERS ( class="PARAMETER">verifier_type = 'class="PARAMETER">password'-- 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
Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
On Wed, Mar 2, 2016 at 4:05 AM, Dmitry Dolgov <9erthali...@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 plain, >>md5 and 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. >> >>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 >>md5 uses the user name as salt on both the >>client and server, md5 cannot be used with >>db_user_namespace. >> > > 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
Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
On 1 March 2016 at 06:34, Michael Paquier wrote:
> On Mon, Feb 29, 2016 at 8:43 PM, Valery Popov
> 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
>
>
>Specifies a comma-separated list of supported password formats by
>the server. Supported formats are currently plain and
>md5.
>
>
>
>When a password is specified in or
>, this parameter determines if the
>password specified is authorized to be stored or not, returning
>an error message to caller if it is not.
>
>
>
>The default is plain,md5,scram, meaning that
MD5-encrypted
>passwords, plain passwords, and SCRAM-encrypted passwords are
accepted.
>
>
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 plain,
>md5 and scram.
Or I missed something?
And one more:
config.sgml:1284
>
>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
>md5 uses the user name as salt on both the
>client and server, md5 cannot be used with
>db_user_namespace.
>
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"),
>+ errcontext("line %d of configuration file \"%s\"",
>+ line_num, HbaFileName)));
>+ return NULL;
>+ }
>+ parsedline->auth_method = uaSASL;
>+ }
> else if (strcmp(token->string, "pam") == 0)
> #ifdef USE_PAM
> parsedline->auth_method = uaPAM;
Re: [HACKERS][REVIEW]: Password identifiers, protocol aging and SCRAM protocol
On Mon, Feb 29, 2016 at 8:43 PM, Valery Popov 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
Re: [HACKERS][REVIEW]: 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. 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
