Re: WIP: SCRAM authentication

On Mon, Mar 30, 2015 at 7:52 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> There have been numerous threads on replacing our MD5 authentication
> method, so I started hacking on that to see what it might look like. Just
> to be clear, this is 9.6 material. Attached is a WIP patch series that adds
> support for SCRAM. There's no need to look at the details yet, but it
> demonstrates what the protocol changes and the code structure would be like.
>
> I'm not wedded to SCRAM - SRP or JPAKE or something else might be better.
> But replacing the algorithm, or adding more of them, should be
> straightforward with this.
>

Agreed. We need such a facility.

> There is no negotiation of the authentication mechanism. SCRAM is just
> added as a new one, alongside all the existing ones. If the server requests
> SCRAM authentication, but the client doesn't support it, the attempt will
> fail. We might want to do something about that, to make the transition
> easier, but it's an orthogonal feature and not absolutely required.
>
> There are four patches in the series. The first two are just refactoring:
> moving the SHA-1 implementation from pgcrypto to src/common, and some
> refactoring in src/backend/auth.c that IMHO would make sense anyway.
>

The two first patches of the series look good to me.

> Patches three and four are the interesting ones:
>

I have not looked in details yet at number implementing SCRAM.

> 3. Allow storing multiple verifiers in pg_authid
> ------------------------------------------------
>
> Replace the pg_authid.rolpassword text field with an array, and rename it
> to 'rolverifiers'. This allows storing multiple password hashes: an MD5
> hash for MD5 authentication, and a SCRAM salt and stored key for SCRAM
> authentication, etc. Each element in the array is a string that begins with
> the method's name. For example "md5:<MD5 hash>", or "password:<plaintext>".
>
> For dump/reload, and for clients that wish to create the hashes in the
> client-side, there is a new option to CREATE/ALTER USER commands: PASSWORD
> VERIFIERS '{ ... }', that allows replacing the array.
>
> The old "ENCRYPTED/UNENCRYPTED PASSWORD 'foo'" options are still supported
> for backwards-compatibility, but it's not clear what it should mean now.
>
> TODO:
>
> * Password-checking hook needs to be redesigned, to allow for more kinds
> of hashes.
>
> * With "CREATE USER PASSWORD 'foo'", which hashes/verifiers should be
> generated by default? We currently have a boolean password_encryption
> setting for that. Needs to be a list.
>

I have been looking more in depths at this one, which adds essential
infrastructure to support multiple authentication hashes for more
protocols. Here are some comments:
- Docs are missing (not a big issue for a WIP)
- Instead of an array that has an identified embedded, let's add a new
catalog pg_authid_hashes that stores all the hashes for a user (idea by
Heikki):
-- hashrol, role Oid associated with the hash
-- hashmet, hash method
-- hashval, value of the hash
- New password-checking hook (contrib/passwordcheck will need a refresh).
As of now, we have that:
void (*check_password_hook_type)
(const char *username,
const char *password,
int password_type,
Datum validuntil_time,
bool validuntil_null);
We need to switch to something that checks a list of hashes:
void (*check_password_hook_type)
(const char *username,
list *passwd,
Datum validuntil_time,
bool validuntil_null);
passwd is a structure containing the password type and the hash value.
Password type can then be "plain" (or password to match pg_hba.conf) or
"md5" for now.
- When password_encryption is switched to a list, true means md5, and false
means plain. At the addition of SCRAM, we could think harder the default
value, "true" may be worth meaning "md5,scram".
- For CREATE ROLE/ALTER ROLE, it is necessary to be able to define the list
of hashes that need to be generated, with something like that for example:
[ ENCRYPTED [(md5[, scram])] | UNENCRYPTED ] PASSWORD 'password'
When UNENCRYPTED is used, we could simply store the password as plain. When
only ENCRYPTED is used, we store it for all the methods available, except
"plain". ENCRYPTED and plain are not allowed combinations.
- Also, do we really want an option at SQL level to allow storing custom
hashes generated on client side as a first step? We could have something
like WITH (md5 = 'blah', scram = 'blah2') appended after PASSWORD for
example.
- rolpassword is removed from pg_authid.

I am willing to write a patch for the next CF following more or less those
lines, depending of course on the outcome of the discussion we can have
here, so feel free to comment.

I'll have a look more in-depth at the scram patch as well.
Regards,
--
Michael