On Wed, Apr 26, 2017 at 7:22 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote: > We talked about the alternative where PQencryptPasswordConn() would not look > at password_encryption, but would always use the strongest possible > algorithm supported by the server. That would avoid querying the server. But > then I started thinking how this would work, if we make the number of > iterations in the SCRAM verifier configurable in the future. The client > would not know the desired number of iterations based only on the server > version, it would need to query the server, and we would be back to square > one. We could add an "options" argument to PQencryptPasswordConn() that the > application could use to pass that information, but documenting how to fetch > that information, if you don't want PQencryptPasswordConn() to block, gets > tedious, and puts a lot of burden to applications. That is why I left out > the "options" argument, after all.
Fine for me. > I'm now thinking that if we add password hashing options like the iteration > count in the future, they will be tacked on to password_encryption. For > example, "password_encryption='scram-sha-256, iterations=10000". That way, > "password_encryption" will always contain enough information to construct > password verifiers. That's possible as well, adding more GUCs for sub-options of a hashing algorithm is wrong. > As an alternative, I considered making password_encryption GUC_REPORT, so > that libpq would always know it without having to issue a query. But it > feels wrong to send that option to the client on every connection, when it's > only needed in the rare case that you use PQencryptPasswordConn(). And if we > added new settings like the iteration count in the future, those would need > to be GUC_REPORT too. Agreed, PQencryptPassword is not that widely used.. Here are some comments. + /* + * Normalize the password with SASLprep. If that doesn't work, because + * the password isn't valid UTF-8 or contains prohibited characters, just + * proceed with the original password. (See comments at top of file.) + */ + rc = pg_saslprep(password, &prep_password); This comment is not true, comments are at the top of auth-scram.c. + * The password should already have been processed with SASLprep, if necessary! + * + * If iterations is 0, default number of iterations is used. The result is + * palloc'd or malloc'd, so caller is responsible for freeing it. + */ +char * +scram_build_verifier(const char *salt, int saltlen, int iterations, + const char *password) +{ + uint8 storedkeybuf[SCRAM_KEY_LEN]; + uint8 serverkeybuf[SCRAM_KEY_LEN]; + char *result; + char *p; + int maxlen; I think that it is a mistake to move SASLprep out of scram_build_verifier, because pre-processing the password is not necessary, it is normally mandatory. The BE/FE versions that you are adding also duplicate the calls to pg_saslprep(). Using "encrypt" instead of "hash" in the function name :( + else if (strcmp(algorithm, "plain") == 0) + { + crypt_pwd = strdup(passwd); + } This is not documented, and users should be warned about using it as well. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers