Re: WIP: SCRAM authentication

On Fri, Aug 7, 2015 at 4:22 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> I have as well done the following things:
> - Added PASSWORD VERIFIER (md5 = 'hoge', plain = 'hoge') which is used
> as well by pg_dump all to be able to specify password verifiers one by
> one.
> - password check hook has been reworked as mentioned to be able to
> manage a list of password verifiers instead of a single entry.
> contrib/passwordcheck has been updated as well.
> - Added regression tests testing UNENCRYPTED/ENCRYPTED, PASSWORD
> VERIFIER, PASSWORD, etc.
> - The patch does not break backward compatibility regarding CREATE
> ROLE and ALTER ROLE.
> - password_encryption has been changed to a list with comma-separated
> elements, for now the possible elements are 'md5' and 'plain'. This
> breaks backward compatibility, so if we care about it we should
> consider having a new GUC password_encryption_list or similar. Default
> is md5, default that does not change backward compatibility.
> - Added documentation.
> - pg_shadow has been updated, switching to an array with
> method:password as elements.
>
>> I'll have a look more in-depth at the scram patch as well.
>
> The SCRAM patch (0002~0004) has been rebased to use the new facility.
> I have as well fixed a syscache leak... But I haven't been able to
> enter much in the details yet.
>
> The patch 0001 to add the multiple verifier facility is in a state
> good enough to get some review, so I am registering an entry for it in
> the next CF. And I'll continue the work on the SCRAM portion next
> week, with hopefully a version ready for CF submission.

Using rfc5802 as reference (http://tools.ietf.org/html/rfc5802), we
use "scram" as a name for the authentication method, like in
pg_hba.conf, but the correct name would be "scram-sha-1". Does it
really matter? We may want to implement other scram protocols using
different hash function than sha1, no? (in case channel binding is
supported, -plus can be appended).

There are still a list of TODO items in the patch that need to be
addressed and I have spotted a couple of bugs:
- the salt used when building the SCRAM verifier is for now the MD5
salt. We definitely need to use a longer one than MD5. but how much?
For now I have bumped it to 10.
- Regarding that (got a lesson from Tom a couple of months ago):
+# FIXME: any reason not to link with libpgcommon?
+OBJS += scram-common.o sha1.o
libpq cannot be linked with libpgcommon, because libpgcommon may not
be compiled to be position-independent code (depends on platform).
- When client receives the last message from server, it did not fetch
the server proof and did not check for it. I fixed that by
implementing what was needed.
- No randomness was used when generating the frontend nonce. I
switched to a call for random() without setting up a seed, which I
think could be improved with for example something based on
gettimeofday(). Thoughts about that are welcome. I think this could be
a separate patch as well.
- fe-connect.c is broken in PQconnectPoll because of this code block
like MD5 authentication:
+ /* Get additional payload, if any */
+ if (msgLength > 4)
+ {
+ int llen =
msgLength - 4;
+ [...]
A correct fix seems to me to use this code path only for AUTH_REQ_SASL
and AUTH_REQ_SASL_CONT.
- The definition of nonce and salt length used by both the frontend
and backend looks better placed in common/scram-common.h.
- For now the password is not normalized with SASLprep. Do you think
that's necessary as a first shot in the authorization protocol?

Other things:
- SASL is a superset of protocols and SCRAM-SHA1 is one of them. We
may want to split the basic stuff of SASL and SCRAM-SHA1 into two
different patches, though having SASL alone does not make much sense
as without any protocols implemented we would just have some dead code
around until a protocol is implemented. We also refer to scram in
pg_hba.conf, but that's not completely correct to me, we may want to
use scram_sha1 actually, particularly if we implement new SASL methods
based on SCRAM, like for example SCRAM-SHA256.
- RandomSalt() in postmaster.c could be refactored such as it gets the
salt length as well in input. I implemented that as a separate patch
attached.
- SCRAM authentication uses the routines for encoding/decoding in
base64 in both frontend and backend, which are located in encode.c as
static routines on HEAD. It looks to me that it is a bad idea to
duplicate it in both frontend and backend as the scram patch does now.
It also does not seem to be a problem to move them in src/common/,
though the error handling needs to be reworked as those routines use
some elog calls which need to be removed. And actually, it seems to me
that it would be better to make a larger move and move all the
encoding/decoding routines of encode.c to libpgcommon (I mean hex,
escape *and* base64). I have done nothing about that yet, and thoughts
are welcome. This should be a separate patch as well that SCRAM relies
on.

For now, I am attaching a new series of patches, and the SCRAM
authentication is still using the new catalog pg_auth_verifiers.
Switching to a one-verifier-per-role approach or similar does not seem
to be a huge task to me.

Regards,
--
Michael