Re: [HACKERS] scram and \password - Mailing list pgsql-hackers

On Wed, Apr 26, 2017 at 7:22 PM, Heikki Linnakangas <hlinnaka@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