Re: Password identifiers, protocol aging and SCRAM protocol

On Mon, Sep 26, 2016 at 2:15 AM, David Steele <david(at)pgmasters(dot)net> wrote:
> On 9/3/16 8:36 AM, Michael Paquier wrote:
>>
>> Attached is a new series:

Thanks for the review and the comments!

> * [PATCH 1/8] Refactor SHA functions and move them to src/common/
>
> I'd like to see more code comments in sha.c (though I realize this was
> copied directly from pgcrypto.)

OK... I have added some comments for the user-facing routines, as well
as the private routines that are doing step-by-step random
calculations.

> I notice that the copyright from pgcrypto/sha1.c was carried over but
> not the copyright from pgcrypto/sha2.c. I'm no expert on how this
> works, but I believe the copyright from sha2.c must be copied over.

Right, those copyright bits are missing:
- * AUTHOR: Aaron D. Gifford <me(at)aarongifford(dot)com>
[...]
- * Copyright (c) 2000-2001, Aaron D. Gifford
The license block being the same, it seems to me that there is no need
to copy it over. The copyright should be enough.

> Also, are there any plans to expose these functions directly to the user
> without loading pgcrypto? Now that the functionality is in core it
> seems that would be useful. In addition, it would make this patch stand
> on its own rather than just being a building block.

There have been discussions about avoiding enabling those functions by
default in the distribution. We'd rather not do that...

> * [PATCH 2/8] Move encoding routines to src/common/
>
> I wonder if it is confusing to have two of encode.h/encode.c. Perhaps
> they should be renamed to make them distinct?

Yes it may be a good idea to rename that, like encode_utils.[c|h] for
the new files.

> * [PATCH 3/8] Switch password_encryption to a enum
>
> Does not apply on HEAD (98c2d3332):

Interesting, it works for me on da6c4f6.

> For here on I used 39b691f251 for review and testing.
> I seems you are keeping on/off for backwards compatibility, shouldn't
> the default now be "md5"?
>
> -#password_encryption = on
> +#password_encryption = on # on, off, md5 or plain

That sounds like a good idea, so switched this way.

> * [PATCH 4/8] Refactor decision-making of password encryption into a
> single routine
>
> +++ b/src/backend/commands/user.c
> + new_record[Anum_pg_authid_rolpassword - 1] =
> + CStringGetTextDatum(encrypted_passwd);
>
> pfree(encrypted_passwd) here or let it get freed with the context?

Calling encrypt_password did not ensure that the password needs to be
free'd.. So I guess that at the moment I coded that I just relied on
the context. But well reading now let's do this cleanly and have
encrypt_password return a palloc'ed string. That's more consistent.

> * [PATCH 5/8] Create generic routine to fetch password and valid until
> values for a role
>
> Couldn't md5_crypt_verify() be made more general and take the hash type?
> For instance, password_crypt_verify() with the last param as the new
> password type enum.

This would mean incorporating the whole SASL message exchange into
this routine because the password string is part of the scram
initialization context, and it seems to me that it is better to just
do once a lookup at the entry in pg_authid. So we'd finish with a more
confusing code I am afraid. At least that's the conclusion I came up
with when doing that.. md5_crypt_verify does only the work on a
received password.

> * [PATCH 6/8] Support for SCRAM-SHA-256 authentication
>
> +++ b/contrib/passwordcheck/passwordcheck.c
> + case PASSWORD_TYPE_SCRAM:
> + /* unfortunately not much can be done here */
> + break;
>
> Why can't we at least do the same check as md5 to make sure the username
> was not used as the password?

You are right. We could at least check that, so changed the way you suggest.

> +++ b/src/backend/libpq/auth.c
> + * without relying on the length word, but we hardly care about protocol
> + * version or older anymore.)
>
> Do you mean protocol version 2 or older?
>
> +++ b/src/backend/libpq/crypt.c
> return STATUS_ERROR; /* empty password */
> +
>
> Looks like a stray LF.

Fixed.

> +++ b/src/backend/parser/gram.y
> + SAVEPOINT SCHEMA SCRAM SCROLL SEARCH SECOND_P SECURITY SELECT SEQUENCE
>
> Doesn't this belong in patch 7? Even in patch 7 it doesn't appear that
> SCRAM is a keyword since the protocol specified after USING is quoted.

This is some garbage from a past version. Fixed.

> However, it doesn't look like they can be used in conjunction since the
> pg_hba.conf entry must specify either m5 or scram (though the database
> can easily contain a mixture). This would probably make a migration
> very unpleasant.

Yep, it uses a given auth-method once user and database match. This is
partially related to the problem to support multiple password
verifiers per users, which was submitted last CF but got rejected
because of a lack of interest, and removed to simplify this patch. You
need as well to think about other things like password and protocol
aging. But well, it is a problem that we don't have to tackle with
this patch...

> Is there any chance of a mixed mode that will allow new passwords to be
> set as scram while still honoring the old md5 passwords? Or does that
> cause too many complications with the protocol?

Hm. That looks complicated to me. This sounds to me like a retry logic
if for multiple authentication methods, and a different feature. What
you'd be looking for here is a connection parameter to specify a list
of protocols and try them all, no?

And that:
+ * multiple messags sent in both directions. First message is always from

> * [PATCH 7/8] Add clause PASSWORD val USING protocol to CREATE/ALTER ROLE
>
> +++ b/doc/src/sgml/ref/create_role.sgml
> + Sets the role's password using the wanted protocol.
>
> How about "Sets the role's password using the requested procotol."

Done.

> + an unencrypted password. If the presented password string is
> already
> + in MD5-encrypted or SCRAM-encrypted format, then it is stored
> encrypted
> + as-is.
>
> How about, "If the password string is..."

OK.

> On the whole I find this patch set easier to digest than what was
> submitted for 9.6. It is more targeted but still provides very valuable
> functionality.

Thanks.

> I'm a bit concerned that a mixture of md5/scram could cause confusion
> and think this may warrant discussion somewhere in the documentation
> since the idea is for users to migrate from md5 to scram.

We could finish with a red warning in the docs to say that users are
recommended to use SCRAM instead of MD5. Just an idea, perhaps that's
not mandatory for the first shot though.
--
Michael