Re: scram and \password

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