From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Cc: | Noah Misch <noah(at)leadboat(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Joe Conway <mail(at)joeconway(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: scram and \password |
Date: | 2017-04-27 04:03:04 |
Message-ID: | CAB7nPqQYuivTYp_0PKrtcFhTXExM1GMvvChSV5uji69FdaH2VQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2017-04-27 04:05:07 | Re: [PostgreSQL 10] default of hot_standby should be "on"? |
Previous Message | Amit Langote | 2017-04-27 03:36:21 | Crash when partition column specified twice |