Thread: [HACKERS] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

Hi all,

As discussed on the thread dedicated to SCRAM
(https://www.postgresql.org/message-id/243d8c11-6149-a4bb-0909-136992f74b23@iki.fi),
here is a separate thread dedicated to the following extension for
CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').

Now that password_encryption has been extended with a new value
'scram', it is a bit bothersome for the user to create roles using
different methods because password_encryption would need to be set
first:
=# SET password_encryption = 'scram';
SET
=# CREATE ROLE foorole PASSWORD 'foopass';
CREATE ROLE
=# SET password_encryption = 'md5';
SET
=# CREATE ROLE foorole2 PASSWORD 'foopass';
CREATE ROLE

What I am proposing with the patch attached is to add a new clause
(grammar is an idea from Robert), to do the same in a single command:
=# CREATE ROLE foorole3 PASSWORD ('foo' USING 'scram');
CREATE ROLE
=# CREATE ROLE foorole4 PASSWORD ('foo' USING 'md5');
CREATE ROLE
This way there is no need to enforce password_encryption prior to
define a new password. Note that like the existing clauses, this is
permissive. In short, if the value is already MD5-encrypted or
SCRAM-encrypted, then the type of the parsed value is enforced
compared to what is defined as method for this USING clause, which is
useful for bumping data.

As this needs clarification before Postgres 10, I am adding a bullet
in the TODO items. This would prove to be useful if more protocols are
added in the future.

Thoughts?
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:
> here is a separate thread dedicated to the following extension for
> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').

The parentheses seem weird ... do we really need those?
        regards, tom lane

On Wed, Mar 8, 2017 at 1:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> here is a separate thread dedicated to the following extension for
>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>
> The parentheses seem weird ... do we really need those?

A grammar without parenthesis works as well, and I am open to suggestions.
-- 
Michael

On Wed, Mar 8, 2017 at 2:32 PM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> One quick comments:
>
> +            | PASSWORD '(' Sconst USING Sconst ')'
> +                {
> +                    $$ = makeDefElem("methodPassword",
> +                                     (Node *)list_make2(makeString($3),
> +                                                        makeString($5)),
> +                                     @1);
> +                }
>
> methodPassword looks bit weird, can we change it to passwordMethod
> or pwdEncryptMethod ?

Using "Password" in suffix looks still better to me though for
consistency with the other ones.
-- 
Michael

On 03/07/2017 08:29 PM, Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> here is a separate thread dedicated to the following extension for
>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>
> The parentheses seem weird ... do we really need those?

+1

> +        If you do not plan to use password authentication you can omit this
> +        option. The methods supported are <literal>md5</> to enforce a password
> +        to be MD5-encrypted, <literal>scram</> for a SCRAM-encrypted password
> +        and <literal>plain</> for an unencrypted password.  If the password

Can we please stop calling this encryption? What is being done is a form
of cryptographic hashing, not encryption.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Thu, Mar 9, 2017 at 1:17 AM, Joe Conway <mail@joeconway.com> wrote:
> On 03/07/2017 08:29 PM, Tom Lane wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>> here is a separate thread dedicated to the following extension for
>>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>>
>> The parentheses seem weird ... do we really need those?
>
> +1

Seeing 3 opinions in favor of that, let's do so then. I have updated
the patch to not use parenthesis.

>> +        If you do not plan to use password authentication you can omit this
>> +        option. The methods supported are <literal>md5</> to enforce a password
>> +        to be MD5-encrypted, <literal>scram</> for a SCRAM-encrypted password
>> +        and <literal>plain</> for an unencrypted password.  If the password
>
> Can we please stop calling this encryption? What is being done is a form
> of cryptographic hashing, not encryption.

Yes, I agree with that for MD5, and after looking around I can see
(like here http://prosody.im/doc/plain_or_hashed) as well that
SCRAM-hashed is used. Now, there are as well references to the salt,
like in protocol.sgml:
"The salt to use when encrypting the password."
Joe, do you think that in this case using the term "hashing" would be
more appropriate? I would think so as we use it to hash the password.

The patch attached removes the parenthesis for this grammar, and uses
"hashed" instead of "encrypted" for the new documentation. For the
existing documentation, perhaps we had better just spawn a new thread,
but I am unsure of all the details yet. Opinions welcome.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Mar 9, 2017 at 7:59 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Yes, I agree with that for MD5, and after looking around I can see
> (like here http://prosody.im/doc/plain_or_hashed) as well that
> SCRAM-hashed is used. Now, there are as well references to the salt,
> like in protocol.sgml:
> "The salt to use when encrypting the password."
> Joe, do you think that in this case using the term "hashing" would be
> more appropriate? I would think so as we use it to hash the password.

I'm not Joe, but I think that would be more appropriate.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 03/09/2017 06:34 AM, Robert Haas wrote:
> On Thu, Mar 9, 2017 at 7:59 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Yes, I agree with that for MD5, and after looking around I can see
>> (like here http://prosody.im/doc/plain_or_hashed) as well that
>> SCRAM-hashed is used. Now, there are as well references to the salt,
>> like in protocol.sgml:
>> "The salt to use when encrypting the password."
>> Joe, do you think that in this case using the term "hashing" would be
>> more appropriate? I would think so as we use it to hash the password.
>
> I'm not Joe, but I think that would be more appropriate.


I am Joe and I agree ;-)


--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Fri, Mar 10, 2017 at 12:00 AM, Joe Conway <mail@joeconway.com> wrote:
> On 03/09/2017 06:34 AM, Robert Haas wrote:
>> On Thu, Mar 9, 2017 at 7:59 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> Yes, I agree with that for MD5, and after looking around I can see
>>> (like here http://prosody.im/doc/plain_or_hashed) as well that
>>> SCRAM-hashed is used. Now, there are as well references to the salt,
>>> like in protocol.sgml:
>>> "The salt to use when encrypting the password."
>>> Joe, do you think that in this case using the term "hashing" would be
>>> more appropriate? I would think so as we use it to hash the password.
>>
>> I'm not Joe, but I think that would be more appropriate.
>
> I am Joe and I agree ;-)

OK I'll spawn a new thread on the matter.
-- 
Michael

On Fri, Mar 17, 2017 at 2:30 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Thu, Mar 9, 2017 at 4:59 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Thu, Mar 9, 2017 at 1:17 AM, Joe Conway <mail@joeconway.com> wrote:
>> > On 03/07/2017 08:29 PM, Tom Lane wrote:
>> >> Michael Paquier <michael.paquier@gmail.com> writes:
>> >>> here is a separate thread dedicated to the following extension for
>> >>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>> >>
>> >> The parentheses seem weird ... do we really need those?
>> >
>> > +1
>>
>> Seeing 3 opinions in favor of that, let's do so then. I have updated
>> the patch to not use parenthesis.
>
> The regression tests only exercise the CREATE ROLE...USING version, not the
> ALTER ROLE...USING version.

Done.

> +        and <literal>plain</> for an non-hashed password.  If the password
> +        string is already in MD5-hashed or SCRAM-hashed, then it is
> +        stored hashed as-is.
>
> In the last line, I think "stored as-is" sounds better.

Okay.

> Other than that, it looks good to me.

Thanks for the review. Attached is an updated patch.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 17, 2017 at 10:10:59AM +0900, Michael Paquier wrote:
> On Fri, Mar 17, 2017 at 2:30 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> > On Thu, Mar 9, 2017 at 4:59 AM, Michael Paquier <michael.paquier@gmail.com>
> > wrote:
> >>
> >> On Thu, Mar 9, 2017 at 1:17 AM, Joe Conway <mail@joeconway.com> wrote:
> >> > On 03/07/2017 08:29 PM, Tom Lane wrote:
> >> >> Michael Paquier <michael.paquier@gmail.com> writes:
> >> >>> here is a separate thread dedicated to the following extension for
> >> >>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
> >> >>
> >> >> The parentheses seem weird ... do we really need those?
> >> >
> >> > +1
> >>
> >> Seeing 3 opinions in favor of that, let's do so then. I have updated
> >> the patch to not use parenthesis.
> >
> > The regression tests only exercise the CREATE ROLE...USING version, not the
> > ALTER ROLE...USING version.
> 
> Done.
> 
> > +        and <literal>plain</> for an non-hashed password.  If the password
> > +        string is already in MD5-hashed or SCRAM-hashed, then it is
> > +        stored hashed as-is.
> >
> > In the last line, I think "stored as-is" sounds better.
> 
> Okay.
> 
> > Other than that, it looks good to me.
> 
> Thanks for the review. Attached is an updated patch.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Heikki,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 04/06/2017 08:33 AM, Noah Misch wrote:
> On Fri, Mar 17, 2017 at 10:10:59AM +0900, Michael Paquier wrote:
>> On Fri, Mar 17, 2017 at 2:30 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>>> On Thu, Mar 9, 2017 at 4:59 AM, Michael Paquier <michael.paquier@gmail.com>
>>> wrote:
>>>>
>>>> On Thu, Mar 9, 2017 at 1:17 AM, Joe Conway <mail@joeconway.com> wrote:
>>>>> On 03/07/2017 08:29 PM, Tom Lane wrote:
>>>>>> Michael Paquier <michael.paquier@gmail.com> writes:
>>>>>>> here is a separate thread dedicated to the following extension for
>>>>>>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>>>>>>
>>>>>> The parentheses seem weird ... do we really need those?
>>>>>
>>>>> +1
>>>>
>>>> Seeing 3 opinions in favor of that, let's do so then. I have updated
>>>> the patch to not use parenthesis.
>>>
>>> The regression tests only exercise the CREATE ROLE...USING version, not the
>>> ALTER ROLE...USING version.
>>
>> Done.
>>
>>> +        and <literal>plain</> for an non-hashed password.  If the password
>>> +        string is already in MD5-hashed or SCRAM-hashed, then it is
>>> +        stored hashed as-is.
>>>
>>> In the last line, I think "stored as-is" sounds better.
>>
>> Okay.
>>
>>> Other than that, it looks good to me.
>>
>> Thanks for the review. Attached is an updated patch.
>
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.

This isn't critical, SCRAM is fully functional without this new syntax. 
I think we should drop this, and revisit for Postgres 11, if it feels 
like we still want this then. I'll remove this from the open items list.

- Heikki