Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 05/02/2017 09:57 PM, Robert Haas wrote:

Does db_user_namespace work with SCRAM?


Yes. Haven't tested it, come to think of it, but it should work.

- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Robert Haas]

On Tue, Apr 18, 2017 at 7:58 AM, Magnus Hagander  wrote:
>> Yeah, that would be reasonable. It can't be called just "password",
>> though, because there's no way to implement "password-or-md5-or-scram" in a
>> sensible way (see my reply to Simon at [1]). Unless we remove the support
>> for what "password" does today altogether, and redefine "password" to mean
>> just "md5-or-beyond". Which might not be a bad idea, but that's a separate
>> discussion.
>
> It is an interesting one though. "password" today is really only useful in
> the case of db_user_namespace=on, right? Given the very few people I think
> are using that feature, it wouldn't be unreasonable to rename it to
> something more closely related to that.

I think it would be nice to have something with the same functionality
as db_user_namespace that smells less like a giant hack.

Does db_user_namespace work with SCRAM?

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


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

Re: [HACKERS] SCRAM authentication, take three

[Magnus Hagander]

On Tue, Apr 18, 2017 at 1:52 PM, Heikki Linnakangas  wrote:

> On 04/14/2017 10:33 PM, Peter Eisentraut wrote:
>
>> On 4/11/17 01:10, Heikki Linnakangas wrote:
>>
>>> That question won't arise in practice. Firstly, if the server can do
>>> scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
>>> there's a change in the way the channel binding works, such that the
>>> scram-sha-512-plus variant needs a newer version of OpenSSL or
>>> something. Secondly, the user's pg_authid row will contain a
>>> SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate
>>> which one to use.
>>>
>>
>> Right.  So putting the actual password method in pg_hba.conf isn't going
>> to be useful very often.
>>
>> I think the most practical thing that the user wants in pg_hba.conf is
>> "best password method supported by what is in pg_authid".  This is
>> currently spelled "md5", which is of course pretty weird.  And it will
>> become weirder over time.
>>
>> I think we want to have a new keyword in pg_hba.conf for that, one which
>> does not indicate any particular algorithm or method (so not "scram" or
>> "sasl").
>>
>> We could use "password".  If we think that "md5" can mean md5-or-beyond,
>> then maybe "password" can mean password-or-md5-or-beyond.
>>
>> Or otherwise a completely new word.
>>
>> We also want to give users/admins a way to phase out old methods or set
>> some policy.  We could either make a global GUC setting
>> password_methods='md5 scram-sha-256' and/or make that an option in
>> pg_hba.conf past the method field.
>>
>
> Yeah, that would be reasonable. It can't be called just "password",
> though, because there's no way to implement "password-or-md5-or-scram" in a
> sensible way (see my reply to Simon at [1]). Unless we remove the support
> for what "password" does today altogether, and redefine "password" to mean
> just "md5-or-beyond". Which might not be a bad idea, but that's a separate
> discussion.
>

It is an interesting one though. "password" today is really only useful in
the case of db_user_namespace=on, right? Given the very few people I think
are using that feature, it wouldn't be unreasonable to rename it to
something more closely related to that.

However, that would also leave us in the position to explain "before 10,
avoid using password because it stores in clear text. after 10, we
recommend you use password". Reusing something that's existed before and
not really been secure for something that would be a good choice in the
future seems like a bad idea.

But we can also implement this functionality but under a differet name.
Like just "hashed" or something, which would mean md5-or-scram?



> In any case, I think we would probably still need more fine-grained
> control, too, so we would still need to have "scram-sha-256" as a method
> you can specify directly in pg_hba.conf. So I consider this as a separate,
> new, feature that we can add in the future, if it seems worth the effort.
>

Yes, I think wherever we go we don't want to loose the fine-grained
control. But some people will be happier for not having to use it.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/14/2017 10:33 PM, Peter Eisentraut wrote:

On 4/11/17 01:10, Heikki Linnakangas wrote:

That question won't arise in practice. Firstly, if the server can do
scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
there's a change in the way the channel binding works, such that the
scram-sha-512-plus variant needs a newer version of OpenSSL or
something. Secondly, the user's pg_authid row will contain a
SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate
which one to use.


Right.  So putting the actual password method in pg_hba.conf isn't going
to be useful very often.

I think the most practical thing that the user wants in pg_hba.conf is
"best password method supported by what is in pg_authid".  This is
currently spelled "md5", which is of course pretty weird.  And it will
become weirder over time.

I think we want to have a new keyword in pg_hba.conf for that, one which
does not indicate any particular algorithm or method (so not "scram" or
"sasl").

We could use "password".  If we think that "md5" can mean md5-or-beyond,
then maybe "password" can mean password-or-md5-or-beyond.

Or otherwise a completely new word.

We also want to give users/admins a way to phase out old methods or set
some policy.  We could either make a global GUC setting
password_methods='md5 scram-sha-256' and/or make that an option in
pg_hba.conf past the method field.


Yeah, that would be reasonable. It can't be called just "password", 
though, because there's no way to implement "password-or-md5-or-scram" 
in a sensible way (see my reply to Simon at [1]). Unless we remove the 
support for what "password" does today altogether, and redefine 
"password" to mean just "md5-or-beyond". Which might not be a bad idea, 
but that's a separate discussion.


In any case, I think we would probably still need more fine-grained 
control, too, so we would still need to have "scram-sha-256" as a method 
you can specify directly in pg_hba.conf. So I consider this as a 
separate, new, feature that we can add in the future, if it seems worth 
the effort.


I've committed a simple renaming of "scram" to "scram-sha-256", as the 
pg_hba.conf and password_encryption option. I think that will do for v10.


[1] 
https://www.postgresql.org/message-id/fa6cec54-4fa9-756d-53be-a5ba3d03d...@iki.fi


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 16 April 2017 07:14:21 EEST, Noah Misch  wrote:
>This PostgreSQL 10 open item is past due for your status update. 
>Kindly send
>a status update within 24 hours, and include a date for your subsequent
>status
>update.  

I will pick this up on Tuesday. The consensus seems to be to change the option 
to "scram-sha-256", so I'll do that.

- Heikki


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

Re: [HACKERS] SCRAM authentication, take three

[Noah Misch]

On Wed, Apr 12, 2017 at 02:33:27AM -0400, Noah Misch wrote:
> On Tue, Apr 11, 2017 at 08:10:23AM +0300, Heikki Linnakangas wrote:
> > On 04/11/2017 04:52 AM, Peter Eisentraut wrote:
> > >On 4/10/17 04:27, Heikki Linnakangas wrote:
> > >>One thing to consider is that we just made the decision that "md5"
> > >>actually means "md5 or scram-sha-256". Extrapolating from that, I think
> > >>we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus"
> > >>(i.e. the channel-bonding variant) in the future. And if we get support
> > >>for scram-sha-512, "scram-sha-256" would presumably allow that too.
> > >
> > >But how would you choose between scram-sha-256-plus and scram-sha-512?
> > 
> > Good question. We would need to decide the order of preference for those.
> > 
> > That question won't arise in practice. Firstly, if the server can do
> > scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
> > there's a change in the way the channel binding works, such that the
> > scram-sha-512-plus variant needs a newer version of OpenSSL or something.
> > Secondly, the user's pg_authid row will contain a SCRAM-SHA-256 or
> > SCRAM-SHA-512 verifier, not both, so that will dictate which one to use.
> 
> [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

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] SCRAM authentication, take three

[Peter Eisentraut]

On 4/11/17 01:10, Heikki Linnakangas wrote:
> That question won't arise in practice. Firstly, if the server can do 
> scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless 
> there's a change in the way the channel binding works, such that the 
> scram-sha-512-plus variant needs a newer version of OpenSSL or 
> something. Secondly, the user's pg_authid row will contain a 
> SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate 
> which one to use.

Right.  So putting the actual password method in pg_hba.conf isn't going
to be useful very often.

I think the most practical thing that the user wants in pg_hba.conf is
"best password method supported by what is in pg_authid".  This is
currently spelled "md5", which is of course pretty weird.  And it will
become weirder over time.

I think we want to have a new keyword in pg_hba.conf for that, one which
does not indicate any particular algorithm or method (so not "scram" or
"sasl").

We could use "password".  If we think that "md5" can mean md5-or-beyond,
then maybe "password" can mean password-or-md5-or-beyond.

Or otherwise a completely new word.

We also want to give users/admins a way to phase out old methods or set
some policy.  We could either make a global GUC setting
password_methods='md5 scram-sha-256' and/or make that an option in
pg_hba.conf past the method field.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Noah Misch]

On Tue, Apr 11, 2017 at 08:10:23AM +0300, Heikki Linnakangas wrote:
> On 04/11/2017 04:52 AM, Peter Eisentraut wrote:
> >On 4/10/17 04:27, Heikki Linnakangas wrote:
> >>One thing to consider is that we just made the decision that "md5"
> >>actually means "md5 or scram-sha-256". Extrapolating from that, I think
> >>we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus"
> >>(i.e. the channel-bonding variant) in the future. And if we get support
> >>for scram-sha-512, "scram-sha-256" would presumably allow that too.
> >
> >But how would you choose between scram-sha-256-plus and scram-sha-512?
> 
> Good question. We would need to decide the order of preference for those.
> 
> That question won't arise in practice. Firstly, if the server can do
> scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
> there's a change in the way the channel binding works, such that the
> scram-sha-512-plus variant needs a newer version of OpenSSL or something.
> Secondly, the user's pg_authid row will contain a SCRAM-SHA-256 or
> SCRAM-SHA-512 verifier, not both, so that will dictate which one to use.

[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


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

Re: [HACKERS] SCRAM authentication, take three

[Bruce Momjian]

On Tue, Apr 11, 2017 at 08:10:23AM +0300, Heikki Linnakangas wrote:
> On 04/11/2017 04:52 AM, Peter Eisentraut wrote:
> Good question. We would need to decide the order of preference for those.
> 
> That question won't arise in practice. Firstly, if the server can do
> scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless
> there's a change in the way the channel binding works, such that the
> scram-sha-512-plus variant needs a newer version of OpenSSL or something.
> Secondly, the user's pg_authid row will contain a SCRAM-SHA-256 or
> SCRAM-SHA-512 verifier, not both, so that will dictate which one to use.

It seems openssl has had to deal with cipher preferences for years and
invented ssl_ciphers.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/11/2017 04:52 AM, Peter Eisentraut wrote:

On 4/10/17 04:27, Heikki Linnakangas wrote:

One thing to consider is that we just made the decision that "md5"
actually means "md5 or scram-sha-256". Extrapolating from that, I think
we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus"
(i.e. the channel-bonding variant) in the future. And if we get support
for scram-sha-512, "scram-sha-256" would presumably allow that too.


But how would you choose between scram-sha-256-plus and scram-sha-512?


Good question. We would need to decide the order of preference for those.

That question won't arise in practice. Firstly, if the server can do 
scram-sha-256-plus, it presumably can also do scram-sha-512-plus. Unless 
there's a change in the way the channel binding works, such that the 
scram-sha-512-plus variant needs a newer version of OpenSSL or 
something. Secondly, the user's pg_authid row will contain a 
SCRAM-SHA-256 or SCRAM-SHA-512 verifier, not both, so that will dictate 
which one to use.


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Peter Eisentraut]

On 4/10/17 04:27, Heikki Linnakangas wrote:
> One thing to consider is that we just made the decision that "md5" 
> actually means "md5 or scram-sha-256". Extrapolating from that, I think 
> we'll want "scram-sha-256" to mean "scram-sha-256 or scram-sha-256-plus" 
> (i.e. the channel-bonding variant) in the future. And if we get support 
> for scram-sha-512, "scram-sha-256" would presumably allow that too.

But how would you choose between scram-sha-256-plus and scram-sha-512?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Peter Eisentraut]

On 4/9/17 19:19, Noah Misch wrote:
> These are the two chief approaches I'm seeing:
> 
> 1. scram-sha-256, scram-sha-256-plus, and successors will be their own
>pg_hba.conf authentication methods.  Until and unless someone implements an
>ability to name multiple methods per HBA line, you must choose exactly one
>SASL method.  The concrete work for v10 would be merely renaming "scram" to
>"scram-sha-256".

I like that.

> 2. Create a multiplexed authentication method like "sasl" or "scram" (not to
>be confused with today's "scram" method, which denotes SCRAM-SHA-256
>precisely).  The DBA permits concrete methods like scram-sha-256 via HBA
>option.  Absent that option, the system could default to a reasonable list.

The problem with that approach is that you would then eventually need
yet another place like pg_hba.conf to configure which SASL mechanisms to
use under which circumstances.  pg_hba.conf is already that place for
the Legacy Authentication and Security Layer, so it could be that place
for SASL as well.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/10/2017 02:19 AM, Noah Misch wrote:

On Fri, Apr 07, 2017 at 10:28:59AM +0300, Heikki Linnakangas wrote:

On 04/07/2017 08:21 AM, Noah Misch wrote:

Michael shared[1] better pg_hba.conf syntax on 2016-11-05.  I agreed[2] with
his framing of the problem and provided two syntax alternatives, on
2017-01-18.  Michael implemented[3] a variation of one of those on 2017-02-20,
which you declined in your 2017-03-07 commit with just the explanation quoted
above.  I say Michael came up with something better five months ago.


OK. My feeling is that we should have a relatively short and
easy-to-pronounce name for it. People editing pg_hba.conf with a text editor
will need to type in the keyword, and "scram" is a lot easier to remember
than "scram-sha-256". The word will also be used in conversations, "hey,
Noah, can you add example.com to the hba file, with scram, please?" If md5
had a more difficult name, I think we would've come up with a shorthand for
it back in the day, too.

I might be wrong, of course. I don't set up PostgreSQL installations for a
living, so I might be out of touch of what's important.


Likewise, but I've never seen pg_hba.conf edits become a large slice of
PostgreSQL DBA work.  Whereas experts can appreciate terse query syntax,
pg_hba.conf syntax gains little from terseness.


In [1], Michael wrote:

There is also the channel binding to think about... So we could have a
list of keywords perhaps associated with SASL? Imagine for example:
sasl$algo,$channel_binding
Giving potentially:
saslscram_sha256
saslscram_sha256,channel
saslscram_sha512
saslscram_sha512,channel
In the case of the patch of this thread just the first entry would
make sense, once channel binding support is added a second
keyword/option could be added. And there are of course other methods
that could replace SCRAM..


It should also be possible to somehow specify "use channel binding, if the
client supports it".

I don't think "sasl" is interesting to a user, it's the actual mechanisms
(e.g "scram-sha256") that matter. So I'd suggest that we allow a list of
algorithms in the method field. If we go with the longer "scram-sha-256"
name, it would look like this:

# TYPE  DATABASEUSERADDRESS METHOD
hostall all example.com scram-sha-256-plus,
scram-sha-256


I agree "sasl" focuses on a less-interesting aspect of the authentication
method.  Allowing multiple methods per HBA line may be the better answer, so
long as the policy questions it raises aren't too thorny.  Do you allow any
combination of methods or limit it somehow (e.g. only SASL methods)?  If the
same option pertains to two methods, do we provide a way to set the option on
just one method?  Those don't seem too challenging, though.


The problem again is that those names are quite long. Is that OK?


Yes.

With this, you could have a meta-method name (e.g. "default") that stands for
all methods generally considered safe.  Compare SSL default cipher lists.


In [2], you wrote:

The latest versions document this precisely, but I agree with Peter's concern
about plain "scram".  Suppose it's 2025 and PostgreSQL support SASL mechanisms
OAUTHBEARER, SCRAM-SHA-256, SCRAM-SHA-256-PLUS, and SCRAM-SHA3-512.  What
should the pg_hba.conf options look like at that time?  I don't think having a
single "scram" option fits in such a world.  I see two strategies that fit:

1. Single "sasl" option, with a GUC, similar to ssl_ciphers, controlling the
  mechanisms to offer.
2. Separate options "scram_sha_256", "scram_sha3_512", "oauthbearer", etc.


The example I gave above is like option 2. The problem with option 1 is that
different SASL mechanisms can have very different properties. You might want
to allow "NTLM" from a trusted network, but require "OTP" from the public
internet, for example. So I don't think a single GUC would be flexible
enough.

That said, a GUC with a more narrow scope might be OK. If we called the
method in pg_hba.conf "secure_password", and had a GUC to list which
password-based mechanisms are considered secure, that would be OK. But I
think we'd still need a separate pg_hba.conf method name for OAUTHBEARER,
for example.


Michael replied to my message with the idea to use a mechanism= HBA option.
That's better than a GUC.


PS. If we go with the full names, I think it should "scram-sha-256", rather
than "scram_sha_256", because the official name uses dashes rather than
underscores.


Agreed, I don't remember why I wrote underscores.  One could argue on that
basis for using uppercase, but I won't.


These are the two chief approaches I'm seeing:

1. scram-sha-256, scram-sha-256-plus, and successors will be their own
   pg_hba.conf authentication methods.  Until and unless someone implements an
   ability to name multiple methods per HBA line, you must choose exactly one
   SASL method.  The concrete work for v10 would be merely renaming "scram" to
   "scram-sha-256".

2. 

Re: [HACKERS] SCRAM authentication, take three

[Craig Ringer]

On 10 April 2017 at 13:57, Craig Ringer  wrote:
> On 10 April 2017 at 12:34, Michael Paquier  wrote:
>
>> Attached is a patch to hopefully make the discussion progress. I
>> simply propose to use sasl as a keyword for pg_hba.conf, on the basis
>> that SASL is the protocol used, and scram is a mechanism used to
>> achieve the SASL exchange. We can always come up with a set of options
>> and aliases later, I am actually open to have more fancy extra options
>> in pg_hba.conf.
>
> I'd really like to see this approach proceed.
>
> pg_hba.conf isn't the most user-friendly thing in the world, and seems
> to be one of the top sources of confusion for new users. Simple is
> good here IMO.
>
> Let users specify 'scram' and negotiate.

sasl, rather.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Craig Ringer]

On 10 April 2017 at 12:34, Michael Paquier  wrote:

> Attached is a patch to hopefully make the discussion progress. I
> simply propose to use sasl as a keyword for pg_hba.conf, on the basis
> that SASL is the protocol used, and scram is a mechanism used to
> achieve the SASL exchange. We can always come up with a set of options
> and aliases later, I am actually open to have more fancy extra options
> in pg_hba.conf.

I'd really like to see this approach proceed.

pg_hba.conf isn't the most user-friendly thing in the world, and seems
to be one of the top sources of confusion for new users. Simple is
good here IMO.

Let users specify 'scram' and negotiate.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Sat, Apr 8, 2017 at 9:28 AM, Robert Haas  wrote:
> On Fri, Apr 7, 2017 at 6:32 PM, Michael Paquier
>  wrote:
>> On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas  wrote:
>>> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas  wrote:
 I think the "SCRAM" part is more important than "SHA-256", so -1 on that.
>>>
>>> I agree.  The point here isn't that we're using a better hashing
>>> method, even if a lot of people *think* that's the point.  The point
>>> is we're using a modern algorithm that has nice properties like "you
>>> can't impersonate the client by steeling the verifier, or even by
>>> snooping the exchange".
>>>
>>> But "sasl" might be even better.
>>
>> FWIW, my opinion has not changed much on the matter, I would still
>> favor "sasl" as the keyword used in pg_hba.conf. What has changed in
>> my mind though is that defining no mechanisms with an additional
>> option mean that all possible choices are sent to the client. But if
>> you define a list of mechanisms, then we'll just send back to the
>> client the specified list as a possible choice of exchange mechanism:
>> host all all blah.com sasl mechanism=scram-sha-256-plus
>> Here for example the user would not be allowed to use SCRAM-SHA-256,
>> just SCRAM with channel binding.
>>
>> Such an option makes sense once we add support for one more mechanism
>> in SASL, like channel binding, but that's by far a generic approach
>> that can serve us for years to come, and by admitting that nothing
>> listed means all possible options we don't need any immediate action.
>
> Yes, that all seems quite sensible.  What exactly is the counter-argument?

I am unsure what that argument is either by reading the thread again.

Attached is a patch to hopefully make the discussion progress. I
simply propose to use sasl as a keyword for pg_hba.conf, on the basis
that SASL is the protocol used, and scram is a mechanism used to
achieve the SASL exchange. We can always come up with a set of options
and aliases later, I am actually open to have more fancy extra options
in pg_hba.conf.

Here is my original proposal:
sasl mechanism=scram-sha-256-plus,scram-sha-256

But we could have something like that as well:
sasl mechanism=scram-plus,scram
sasl mechanism=scram channel_binding=on/off

A problem with the last one is that it is not possible to control
channel binding per mechanism, but that could be discussed later on
once support for channel binding is added. For now I would just favor
the most extensive approach. No need to work later on with some
aliases in pg_hba.conf either.
-- 
Michael


reshape-sasl-hba.patch
Description: Binary data

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

Re: [HACKERS] SCRAM authentication, take three

[Noah Misch]

On Fri, Apr 07, 2017 at 10:28:59AM +0300, Heikki Linnakangas wrote:
> On 04/07/2017 08:21 AM, Noah Misch wrote:
> >Michael shared[1] better pg_hba.conf syntax on 2016-11-05.  I agreed[2] with
> >his framing of the problem and provided two syntax alternatives, on
> >2017-01-18.  Michael implemented[3] a variation of one of those on 
> >2017-02-20,
> >which you declined in your 2017-03-07 commit with just the explanation quoted
> >above.  I say Michael came up with something better five months ago.
> 
> OK. My feeling is that we should have a relatively short and
> easy-to-pronounce name for it. People editing pg_hba.conf with a text editor
> will need to type in the keyword, and "scram" is a lot easier to remember
> than "scram-sha-256". The word will also be used in conversations, "hey,
> Noah, can you add example.com to the hba file, with scram, please?" If md5
> had a more difficult name, I think we would've come up with a shorthand for
> it back in the day, too.
> 
> I might be wrong, of course. I don't set up PostgreSQL installations for a
> living, so I might be out of touch of what's important.

Likewise, but I've never seen pg_hba.conf edits become a large slice of
PostgreSQL DBA work.  Whereas experts can appreciate terse query syntax,
pg_hba.conf syntax gains little from terseness.

> In [1], Michael wrote:
> >There is also the channel binding to think about... So we could have a
> >list of keywords perhaps associated with SASL? Imagine for example:
> >sasl$algo,$channel_binding
> >Giving potentially:
> >saslscram_sha256
> >saslscram_sha256,channel
> >saslscram_sha512
> >saslscram_sha512,channel
> >In the case of the patch of this thread just the first entry would
> >make sense, once channel binding support is added a second
> >keyword/option could be added. And there are of course other methods
> >that could replace SCRAM..
> 
> It should also be possible to somehow specify "use channel binding, if the
> client supports it".
> 
> I don't think "sasl" is interesting to a user, it's the actual mechanisms
> (e.g "scram-sha256") that matter. So I'd suggest that we allow a list of
> algorithms in the method field. If we go with the longer "scram-sha-256"
> name, it would look like this:
> 
> # TYPE  DATABASEUSERADDRESS METHOD
> host  all all example.com scram-sha-256-plus,
> scram-sha-256

I agree "sasl" focuses on a less-interesting aspect of the authentication
method.  Allowing multiple methods per HBA line may be the better answer, so
long as the policy questions it raises aren't too thorny.  Do you allow any
combination of methods or limit it somehow (e.g. only SASL methods)?  If the
same option pertains to two methods, do we provide a way to set the option on
just one method?  Those don't seem too challenging, though.

> The problem again is that those names are quite long. Is that OK?

Yes.

With this, you could have a meta-method name (e.g. "default") that stands for
all methods generally considered safe.  Compare SSL default cipher lists.

> In [2], you wrote:
> >The latest versions document this precisely, but I agree with Peter's concern
> >about plain "scram".  Suppose it's 2025 and PostgreSQL support SASL 
> >mechanisms
> >OAUTHBEARER, SCRAM-SHA-256, SCRAM-SHA-256-PLUS, and SCRAM-SHA3-512.  What
> >should the pg_hba.conf options look like at that time?  I don't think having 
> >a
> >single "scram" option fits in such a world.  I see two strategies that fit:
> >
> >1. Single "sasl" option, with a GUC, similar to ssl_ciphers, controlling the
> >   mechanisms to offer.
> >2. Separate options "scram_sha_256", "scram_sha3_512", "oauthbearer", etc.
> 
> The example I gave above is like option 2. The problem with option 1 is that
> different SASL mechanisms can have very different properties. You might want
> to allow "NTLM" from a trusted network, but require "OTP" from the public
> internet, for example. So I don't think a single GUC would be flexible
> enough.
> 
> That said, a GUC with a more narrow scope might be OK. If we called the
> method in pg_hba.conf "secure_password", and had a GUC to list which
> password-based mechanisms are considered secure, that would be OK. But I
> think we'd still need a separate pg_hba.conf method name for OAUTHBEARER,
> for example.

Michael replied to my message with the idea to use a mechanism= HBA option.
That's better than a GUC.

> PS. If we go with the full names, I think it should "scram-sha-256", rather
> than "scram_sha_256", because the official name uses dashes rather than
> underscores.

Agreed, I don't remember why I wrote underscores.  One could argue on that
basis for using uppercase, but I won't.


These are the two chief approaches I'm seeing:

1. scram-sha-256, scram-sha-256-plus, and successors will be their own
   pg_hba.conf authentication methods.  Until and unless someone implements an
   ability to name multiple methods per HBA line, you must choose 

Re: [HACKERS] SCRAM authentication, take three

[Robert Haas]

On Fri, Apr 7, 2017 at 6:32 PM, Michael Paquier
 wrote:
> On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas  wrote:
>> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas  wrote:
>>> I think the "SCRAM" part is more important than "SHA-256", so -1 on that.
>>
>> I agree.  The point here isn't that we're using a better hashing
>> method, even if a lot of people *think* that's the point.  The point
>> is we're using a modern algorithm that has nice properties like "you
>> can't impersonate the client by steeling the verifier, or even by
>> snooping the exchange".
>>
>> But "sasl" might be even better.
>
> FWIW, my opinion has not changed much on the matter, I would still
> favor "sasl" as the keyword used in pg_hba.conf. What has changed in
> my mind though is that defining no mechanisms with an additional
> option mean that all possible choices are sent to the client. But if
> you define a list of mechanisms, then we'll just send back to the
> client the specified list as a possible choice of exchange mechanism:
> host all all blah.com sasl mechanism=scram-sha-256-plus
> Here for example the user would not be allowed to use SCRAM-SHA-256,
> just SCRAM with channel binding.
>
> Such an option makes sense once we add support for one more mechanism
> in SASL, like channel binding, but that's by far a generic approach
> that can serve us for years to come, and by admitting that nothing
> listed means all possible options we don't need any immediate action.

Yes, that all seems quite sensible.  What exactly is the counter-argument?

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


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas  wrote:
> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas  wrote:
>> I think the "SCRAM" part is more important than "SHA-256", so -1 on that.
>
> I agree.  The point here isn't that we're using a better hashing
> method, even if a lot of people *think* that's the point.  The point
> is we're using a modern algorithm that has nice properties like "you
> can't impersonate the client by steeling the verifier, or even by
> snooping the exchange".
>
> But "sasl" might be even better.

FWIW, my opinion has not changed much on the matter, I would still
favor "sasl" as the keyword used in pg_hba.conf. What has changed in
my mind though is that defining no mechanisms with an additional
option mean that all possible choices are sent to the client. But if
you define a list of mechanisms, then we'll just send back to the
client the specified list as a possible choice of exchange mechanism:
host all all blah.com sasl mechanism=scram-sha-256-plus
Here for example the user would not be allowed to use SCRAM-SHA-256,
just SCRAM with channel binding.

Such an option makes sense once we add support for one more mechanism
in SASL, like channel binding, but that's by far a generic approach
that can serve us for years to come, and by admitting that nothing
listed means all possible options we don't need any immediate action.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Robert Haas]

On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas  wrote:
> I think the "SCRAM" part is more important than "SHA-256", so -1 on that.

I agree.  The point here isn't that we're using a better hashing
method, even if a lot of people *think* that's the point.  The point
is we're using a modern algorithm that has nice properties like "you
can't impersonate the client by steeling the verifier, or even by
snooping the exchange".

But "sasl" might be even better.

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


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

Re: [HACKERS] SCRAM authentication, take three

[Álvaro Hernández Tortosa]

On 07/04/17 11:05, Magnus Hagander wrote:
On Fri, Apr 7, 2017 at 9:59 AM, Heikki Linnakangas > wrote:


On 04/07/2017 10:38 AM, Magnus Hagander wrote:

So here's a wild idea. What if we just call it "sha256"? Does
the user
actually care about it being scram, or is scram just an
implementation
detail for them? That way when the next one shows up, it'll be
sha512 or
whatever. It happens to use scram under the hood, but does the
user have to
or does the user want to care about that?

(One could argue the same way that the user shouldn't have to
or want to
care about the hashing algorithm -- but if that's the case
then we should
only have one entry, it would be "scram", and the system would
decide from
there. And I think this discussion already indicates we don't
think this is
enough)


I think the "SCRAM" part is more important than "SHA-256", so -1
on that.


If that is the important part, then I agree :) I am not entirely sure 
that the scram part *is* more important though.


I agree it is much more important. Needed, I'd say. "SHA-256" could 
refer to other mechanisms that just simply hash the value (maybe with a 
salt, or not) with that hash algorithm. SCRAM is a different beast, with 
much more functionality than that. So yes, it matters a lot :)




I think most users will be a lot more comfortable with "sha256" than 
"scram" though. But I guess that says using scram-sha-256 is the 
correct way.


I don't like UPPERCASE, but the RFC links to the IANA registry 
where SCRAM methods are all uppercase and with dashes: SCRAM-SHA-256 and 
SCRAM-SHA-256-PLUS. I'd use those names, they are standardized.





The main against using just "scram" is that it's misleading,
because we implement SCRAM-SHA-256, rather than SCRAM-SHA-1, which
was the first SCRAM mechanism, commonly called just SCRAM. As long
as that's the only SCRAM variant we have, that's not too bad, but
it becomes more confusing if we ever implement SCRAM-SHA-512 or
SCRAM-something-else in the future. That's the point Noah made,
and it's a fair point, but the question is whether we consider
that to be more important than having a short name for what we
have now.


Yeah, I agree we should be prepared for the future. And having "scram" 
and "scram-sha-512" would definitely fall under confusing.


The channel binding aspect is actually more important to think
about right
now, as that we will hopefully implement in the next release
or two.

In [1], Michael wrote:

There is also the channel binding to think about... So we
could have a
list of keywords perhaps associated with SASL? Imagine for
example:
sasl$algo,$channel_binding
Giving potentially:
saslscram_sha256
saslscram_sha256,channel
saslscram_sha512
saslscram_sha512,channel
In the case of the patch of this thread just the first
entry would
make sense, once channel binding support is added a second
keyword/option could be added. And there are of course
other methods
that could replace SCRAM..


It should also be possible to somehow specify "use channel
binding, if the
client supports it".


Is that really a type of authentication? We already hvae the idea of
authentication method options, used for most other things except
md5 which
doesn't have any. So it could be "sha256 channelbind=on", "sha256
channelbind=off" or "sha256 channelbind=negotiate" or something
like that?


> Technically, the channel-binding variant is a separate SASL 
mechanism, i.e. it has a separate name, SCRAM-SHA-256-PLUS. I'm not 
sure if > users/admins think of it that way.



I bet they don't.


Probably. But let's not underestimate channel binding: it is the 
"greatest" feature of SCRAM, and where security really shines. I'd 
encourage the use of channel binding and would definitely make it explicit.


As for the options, there's no way to negotiate, the client picks. 
It could still be three-valued: on, off, only-channel-binding (or 
however you want to call them). That will only change what mechanisms 
the server will be advertising to clients.




Álvaro



--

Álvaro Hernández Tortosa


---
<8K>data

Re: [HACKERS] SCRAM authentication, take three

[Magnus Hagander]

On Fri, Apr 7, 2017 at 9:59 AM, Heikki Linnakangas  wrote:

> On 04/07/2017 10:38 AM, Magnus Hagander wrote:
>
>> So here's a wild idea. What if we just call it "sha256"? Does the user
>> actually care about it being scram, or is scram just an implementation
>> detail for them? That way when the next one shows up, it'll be sha512 or
>> whatever. It happens to use scram under the hood, but does the user have
>> to
>> or does the user want to care about that?
>>
>> (One could argue the same way that the user shouldn't have to or want to
>> care about the hashing algorithm -- but if that's the case then we should
>> only have one entry, it would be "scram", and the system would decide from
>> there. And I think this discussion already indicates we don't think this
>> is
>> enough)
>>
>
> I think the "SCRAM" part is more important than "SHA-256", so -1 on that.
>

If that is the important part, then I agree :) I am not entirely sure that
the scram part *is* more important though.

I think most users will be a lot more comfortable with "sha256" than
"scram" though. But I guess that says using scram-sha-256 is the correct
way.



> The main against using just "scram" is that it's misleading, because we
> implement SCRAM-SHA-256, rather than SCRAM-SHA-1, which was the first SCRAM
> mechanism, commonly called just SCRAM. As long as that's the only SCRAM
> variant we have, that's not too bad, but it becomes more confusing if we
> ever implement SCRAM-SHA-512 or SCRAM-something-else in the future. That's
> the point Noah made, and it's a fair point, but the question is whether we
> consider that to be more important than having a short name for what we
> have now.


Yeah, I agree we should be prepared for the future. And having "scram" and
"scram-sha-512" would definitely fall under confusing.


The channel binding aspect is actually more important to think about right
>> now, as that we will hopefully implement in the next release or two.
>>
>> In [1], Michael wrote:
>>
>> There is also the channel binding to think about... So we could have a
>>> list of keywords perhaps associated with SASL? Imagine for example:
>>> sasl$algo,$channel_binding
>>> Giving potentially:
>>> saslscram_sha256
>>> saslscram_sha256,channel
>>> saslscram_sha512
>>> saslscram_sha512,channel
>>> In the case of the patch of this thread just the first entry would
>>> make sense, once channel binding support is added a second
>>> keyword/option could be added. And there are of course other methods
>>> that could replace SCRAM..
>>>
>>
>> It should also be possible to somehow specify "use channel binding, if the
>> client supports it".
>>
>
> Is that really a type of authentication? We already hvae the idea of
> authentication method options, used for most other things except md5 which
> doesn't have any. So it could be "sha256 channelbind=on", "sha256
> channelbind=off" or "sha256 channelbind=negotiate" or something like that?
>

> Technically, the channel-binding variant is a separate SASL mechanism,
i.e. it has a separate name, SCRAM-SHA-256-PLUS. I'm not sure if >
users/admins think of it that way.


I bet they don't.



I don't think "sasl" is interesting to a user, it's the actual mechanisms
>> (e.g "scram-sha256") that matter. So I'd suggest that we allow a list of
>> algorithms in the method field. If we go with the longer "scram-sha-256"
>> name, it would look like this:
>>
>> # TYPE  DATABASEUSERADDRESS METHOD
>> hostall all example.com scram-sha-256-plus,
>> scram-sha-256
>>
>> The problem again is that those names are quite long. Is that OK?
>>
>
> Not sure if it would be doable in the code, but we could also have:
> host all all example.com scram method=sha256plus,sha256
>
> or something like that. Which would fit within the current syntax of the
> file. But I think it might not be enough, because then you couldn't have
> two entries with different scram methods for the same combination of the
> other fields -- the hba *matching* doesn't look at the options fields.
>


> You can't have two entries with the same type+database+user+address
combination, period. (Or if you do, the second one is ignored.)

That's exactly my point.

//Magnus

Re: [HACKERS] SCRAM authentication, take three

[Craig Ringer]

On 7 April 2017 at 15:59, Heikki Linnakangas  wrote:
> On 04/07/2017 10:38 AM, Magnus Hagander wrote:

>> Not sure if it would be doable in the code, but we could also have:
>> host all all example.com scram method=sha256plus,sha256
>>
>> or something like that. Which would fit within the current syntax of the
>> file. But I think it might not be enough, because then you couldn't have
>> two entries with different scram methods for the same combination of the
>> other fields -- the hba *matching* doesn't look at the options fields.
>
> You can't have two entries with the same type+database+user+address
> combination, period. (Or if you do, the second one is ignored.)

So we need a methods= list for users who want to constrain the allowed
methods, accepting a list of methods. This is just how things like SSH
work; e.g. ssh_config might contain

Ciphers aes128-cbc,3des-cbc

if you feel like using the old dodgy stuff today.

If the user doesn't supply a methods= list, they get a full list of
supported methods by the server to choose from in the 'B' message, and
can auth with any one of them.

I'm aware there are some compat concerns there, but existing clients
will already have no idea what the scram method is, so now's our
chance to lock it in as containing a *list* of permitted methods. Even
though to start with it it's hard coded.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/07/2017 10:38 AM, Magnus Hagander wrote:

So here's a wild idea. What if we just call it "sha256"? Does the user
actually care about it being scram, or is scram just an implementation
detail for them? That way when the next one shows up, it'll be sha512 or
whatever. It happens to use scram under the hood, but does the user have to
or does the user want to care about that?

(One could argue the same way that the user shouldn't have to or want to
care about the hashing algorithm -- but if that's the case then we should
only have one entry, it would be "scram", and the system would decide from
there. And I think this discussion already indicates we don't think this is
enough)


I think the "SCRAM" part is more important than "SHA-256", so -1 on that.

The main against using just "scram" is that it's misleading, because we 
implement SCRAM-SHA-256, rather than SCRAM-SHA-1, which was the first 
SCRAM mechanism, commonly called just SCRAM. As long as that's the only 
SCRAM variant we have, that's not too bad, but it becomes more confusing 
if we ever implement SCRAM-SHA-512 or SCRAM-something-else in the 
future. That's the point Noah made, and it's a fair point, but the 
question is whether we consider that to be more important than having a 
short name for what we have now.



The channel binding aspect is actually more important to think about right
now, as that we will hopefully implement in the next release or two.

In [1], Michael wrote:


There is also the channel binding to think about... So we could have a
list of keywords perhaps associated with SASL? Imagine for example:
sasl$algo,$channel_binding
Giving potentially:
saslscram_sha256
saslscram_sha256,channel
saslscram_sha512
saslscram_sha512,channel
In the case of the patch of this thread just the first entry would
make sense, once channel binding support is added a second
keyword/option could be added. And there are of course other methods
that could replace SCRAM..


It should also be possible to somehow specify "use channel binding, if the
client supports it".


Is that really a type of authentication? We already hvae the idea of
authentication method options, used for most other things except md5 which
doesn't have any. So it could be "sha256 channelbind=on", "sha256
channelbind=off" or "sha256 channelbind=negotiate" or something like that?


Technically, the channel-binding variant is a separate SASL mechanism, 
i.e. it has a separate name, SCRAM-SHA-256-PLUS. I'm not sure if 
users/admins think of it that way.



I don't think "sasl" is interesting to a user, it's the actual mechanisms
(e.g "scram-sha256") that matter. So I'd suggest that we allow a list of
algorithms in the method field. If we go with the longer "scram-sha-256"
name, it would look like this:

# TYPE  DATABASEUSERADDRESS METHOD
hostall all example.com scram-sha-256-plus,
scram-sha-256

The problem again is that those names are quite long. Is that OK?


Not sure if it would be doable in the code, but we could also have:
host all all example.com scram method=sha256plus,sha256

or something like that. Which would fit within the current syntax of the
file. But I think it might not be enough, because then you couldn't have
two entries with different scram methods for the same combination of the
other fields -- the hba *matching* doesn't look at the options fields.


You can't have two entries with the same type+database+user+address 
combination, period. (Or if you do, the second one is ignored.)


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Magnus Hagander]

Jumping late into this one, apologies if these opinions have already been
up and discarded.

On Fri, Apr 7, 2017 at 9:28 AM, Heikki Linnakangas  wrote:

> On 04/07/2017 08:21 AM, Noah Misch wrote:
>
>> On Thu, Apr 06, 2017 at 09:46:29PM +0300, Heikki Linnakangas wrote:
>>
>>> On 04/06/2017 08:36 AM, Noah Misch wrote:
>>>
 On Tue, Mar 07, 2017 at 02:36:13PM +0200, Heikki Linnakangas wrote:

> I didn't include the last-minute changes to the way you specify this in
> pg_hba.conf. So it's still just "scram". I agree in general that we
> should
> think about how to extend that too, but I think the proposed syntax was
> overly verbose for what we actually support right now. Let's discuss
> that as
> a separate thread, as well.
>

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

 The above-described topic is currently a PostgreSQL 10 open item.

>>>
>>> I don't think we will come up with anything better than what we have
>>> now, so
>>> I have removed this from the open items list.
>>>
>>
>> Michael shared[1] better pg_hba.conf syntax on 2016-11-05.  I agreed[2]
>> with
>> his framing of the problem and provided two syntax alternatives, on
>> 2017-01-18.  Michael implemented[3] a variation of one of those on
>> 2017-02-20,
>> which you declined in your 2017-03-07 commit with just the explanation
>> quoted
>> above.  I say Michael came up with something better five months ago.
>>
>
> OK. My feeling is that we should have a relatively short and
> easy-to-pronounce name for it. People editing pg_hba.conf with a text
> editor will need to type in the keyword, and "scram" is a lot easier to
> remember than "scram-sha-256". The word will also be used in conversations,
> "hey, Noah, can you add example.com to the hba file, with scram, please?"
> If md5 had a more difficult name, I think we would've come up with a
> shorthand for it back in the day, too.
>
> I might be wrong, of course. I don't set up PostgreSQL installations for a
> living, so I might be out of touch of what's important.
>
> Reserving, as HEAD does today, keyword "scram" to mean "type of SCRAM we
>> introduced first" will look ugly in 2027.  Cryptographic hash functions
>> have a
>> short shelf life compared to PostgreSQL.
>>
>
> I don't think that's such a big deal. Firstly, I don't think it would be
> too bad for "scram" to mean "the type of SCRAM we introduced first".
> Secondly, we can add an alias later, if we add support for a new mechanism
> in the SCRAM family.
>
> Our MD5 authentication method was introduced in 2001, I expect
> SCRAM-SHA-256 to also last about 15 years before we consider replacing it.
> Note that the big problem with our MD5 authentication is not actually the
> hash algorithm. There are still no practical pre-image attacks on MD5, even
> though it's thoroughly broken for collisions. If we had "SCRAM-MD5", it
> would still be OK. So I'd hazard a guess that whatever will eventually
> replace SCRAM-SHA-256, will not be SCRAM with a different hash algorithm,
> but something else entirely.
>

So here's a wild idea. What if we just call it "sha256"? Does the user
actually care about it being scram, or is scram just an implementation
detail for them? That way when the next one shows up, it'll be sha512 or
whatever. It happens to use scram under the hood, but does the user have to
or does the user want to care about that?

(One could argue the same way that the user shouldn't have to or want to
care about the hashing algorithm -- but if that's the case then we should
only have one entry, it would be "scram", and the system would decide from
there. And I think this discussion already indicates we don't think this is
enough)




>
> The channel binding aspect is actually more important to think about right
> now, as that we will hopefully implement in the next release or two.
>
> In [1], Michael wrote:
>
>> There is also the channel binding to think about... So we could have a
>> list of keywords perhaps associated with SASL? Imagine for example:
>> sasl$algo,$channel_binding
>> Giving potentially:
>> saslscram_sha256
>> saslscram_sha256,channel
>> saslscram_sha512
>> saslscram_sha512,channel
>> In the case of the patch of this thread just the first entry would
>> make sense, once channel binding support is added a second
>> keyword/option could be added. And there are of course other methods
>> that could replace SCRAM..
>>
>
> It should also be possible to somehow specify "use channel binding, if the
> client supports it".
>

Is that really a type of authentication? We already hvae the idea of
authentication method options, used for most other things except md5 which
doesn't have any. So it could be "sha256 channelbind=on", "sha256
channelbind=off" or "sha256 channelbind=negotiate" or something like that?



> I don't think "sasl" is interesting to a user, it's the actual mechanisms
> (e.g "scram-sha256") that 

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/07/2017 08:21 AM, Noah Misch wrote:

On Thu, Apr 06, 2017 at 09:46:29PM +0300, Heikki Linnakangas wrote:

On 04/06/2017 08:36 AM, Noah Misch wrote:

On Tue, Mar 07, 2017 at 02:36:13PM +0200, Heikki Linnakangas wrote:

I didn't include the last-minute changes to the way you specify this in
pg_hba.conf. So it's still just "scram". I agree in general that we should
think about how to extend that too, but I think the proposed syntax was
overly verbose for what we actually support right now. Let's discuss that as
a separate thread, as well.


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

The above-described topic is currently a PostgreSQL 10 open item.


I don't think we will come up with anything better than what we have now, so
I have removed this from the open items list.


Michael shared[1] better pg_hba.conf syntax on 2016-11-05.  I agreed[2] with
his framing of the problem and provided two syntax alternatives, on
2017-01-18.  Michael implemented[3] a variation of one of those on 2017-02-20,
which you declined in your 2017-03-07 commit with just the explanation quoted
above.  I say Michael came up with something better five months ago.


OK. My feeling is that we should have a relatively short and 
easy-to-pronounce name for it. People editing pg_hba.conf with a text 
editor will need to type in the keyword, and "scram" is a lot easier to 
remember than "scram-sha-256". The word will also be used in 
conversations, "hey, Noah, can you add example.com to the hba file, with 
scram, please?" If md5 had a more difficult name, I think we would've 
come up with a shorthand for it back in the day, too.


I might be wrong, of course. I don't set up PostgreSQL installations for 
a living, so I might be out of touch of what's important.



Reserving, as HEAD does today, keyword "scram" to mean "type of SCRAM we
introduced first" will look ugly in 2027.  Cryptographic hash functions have a
short shelf life compared to PostgreSQL.


I don't think that's such a big deal. Firstly, I don't think it would be 
too bad for "scram" to mean "the type of SCRAM we introduced first". 
Secondly, we can add an alias later, if we add support for a new 
mechanism in the SCRAM family.


Our MD5 authentication method was introduced in 2001, I expect 
SCRAM-SHA-256 to also last about 15 years before we consider replacing 
it. Note that the big problem with our MD5 authentication is not 
actually the hash algorithm. There are still no practical pre-image 
attacks on MD5, even though it's thoroughly broken for collisions. If we 
had "SCRAM-MD5", it would still be OK. So I'd hazard a guess that 
whatever will eventually replace SCRAM-SHA-256, will not be SCRAM with a 
different hash algorithm, but something else entirely.


The channel binding aspect is actually more important to think about 
right now, as that we will hopefully implement in the next release or two.


In [1], Michael wrote:

There is also the channel binding to think about... So we could have a
list of keywords perhaps associated with SASL? Imagine for example:
sasl$algo,$channel_binding
Giving potentially:
saslscram_sha256
saslscram_sha256,channel
saslscram_sha512
saslscram_sha512,channel
In the case of the patch of this thread just the first entry would
make sense, once channel binding support is added a second
keyword/option could be added. And there are of course other methods
that could replace SCRAM..


It should also be possible to somehow specify "use channel binding, if 
the client supports it".


I don't think "sasl" is interesting to a user, it's the actual 
mechanisms (e.g "scram-sha256") that matter. So I'd suggest that we 
allow a list of algorithms in the method field. If we go with the longer 
"scram-sha-256" name, it would look like this:


# TYPE  DATABASEUSERADDRESS METHOD
host	all all example.com 
scram-sha-256-plus, scram-sha-256


The problem again is that those names are quite long. Is that OK?

In [2], you wrote:

The latest versions document this precisely, but I agree with Peter's concern
about plain "scram".  Suppose it's 2025 and PostgreSQL support SASL mechanisms
OAUTHBEARER, SCRAM-SHA-256, SCRAM-SHA-256-PLUS, and SCRAM-SHA3-512.  What
should the pg_hba.conf options look like at that time?  I don't think having a
single "scram" option fits in such a world.  I see two strategies that fit:

1. Single "sasl" option, with a GUC, similar to ssl_ciphers, controlling the
   mechanisms to offer.
2. Separate options "scram_sha_256", "scram_sha3_512", "oauthbearer", etc.


The example I gave above is like option 2. The problem with option 1 is 
that different SASL mechanisms can have very different properties. You 
might want to allow "NTLM" from a trusted network, but require "OTP" 
from the public internet, for example. So I don't think a single GUC 
would be flexible enough.


That said, a GUC with a more narrow scope might be OK. If we 

Re: [HACKERS] SCRAM authentication, take three

[Noah Misch]

On Thu, Apr 06, 2017 at 09:46:29PM +0300, Heikki Linnakangas wrote:
> On 04/06/2017 08:36 AM, Noah Misch wrote:
> >On Tue, Mar 07, 2017 at 02:36:13PM +0200, Heikki Linnakangas wrote:
> >>I didn't include the last-minute changes to the way you specify this in
> >>pg_hba.conf. So it's still just "scram". I agree in general that we should
> >>think about how to extend that too, but I think the proposed syntax was
> >>overly verbose for what we actually support right now. Let's discuss that as
> >>a separate thread, as well.
> >
> >[Action required within three days.  This is a generic notification.]
> >
> >The above-described topic is currently a PostgreSQL 10 open item.
> 
> I don't think we will come up with anything better than what we have now, so
> I have removed this from the open items list.

Michael shared[1] better pg_hba.conf syntax on 2016-11-05.  I agreed[2] with
his framing of the problem and provided two syntax alternatives, on
2017-01-18.  Michael implemented[3] a variation of one of those on 2017-02-20,
which you declined in your 2017-03-07 commit with just the explanation quoted
above.  I say Michael came up with something better five months ago.
Reserving, as HEAD does today, keyword "scram" to mean "type of SCRAM we
introduced first" will look ugly in 2027.  Cryptographic hash functions have a
short shelf life compared to PostgreSQL.

nm

[1] 
https://www.postgresql.org/message-id/CAB7nPqS99Z31f7jhoYYMoBDbuZSQRpn+HQzByA=ewfmdywc...@mail.gmail.com
[2] https://www.postgresql.org/message-id/20170118052356.GA5952@gust
[3] 
https://www.postgresql.org/message-id/cab7npqsalxkoohbk3ugbf+kfq4pqgtgjk_os68f3nkxghdo...@mail.gmail.com


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 04/06/2017 08:36 AM, Noah Misch wrote:

On Tue, Mar 07, 2017 at 02:36:13PM +0200, Heikki Linnakangas wrote:

I didn't include the last-minute changes to the way you specify this in
pg_hba.conf. So it's still just "scram". I agree in general that we should
think about how to extend that too, but I think the proposed syntax was
overly verbose for what we actually support right now. Let's discuss that as
a separate thread, as well.


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

The above-described topic is currently a PostgreSQL 10 open item.


I don't think we will come up with anything better than what we have 
now, so I have removed this from the open items list.


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Noah Misch]

On Tue, Mar 07, 2017 at 02:36:13PM +0200, Heikki Linnakangas wrote:
> I didn't include the last-minute changes to the way you specify this in
> pg_hba.conf. So it's still just "scram". I agree in general that we should
> think about how to extend that too, but I think the proposed syntax was
> overly verbose for what we actually support right now. Let's discuss that as
> a separate thread, as well.

[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


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

Re: [HACKERS] SCRAM authentication, take three

[Magnus Hagander]

On Tue, Mar 7, 2017 at 4:36 AM, Heikki Linnakangas  wrote:

> On 03/02/2017 08:50 AM, Michael Paquier wrote:
>
>> Attached is a new patch set. I have combined SASLprep with the rest
>> and fixed some conflicts. At the same time when going through NFKC
>> this morning I have noticed that the implementation was doing the
>> canonical decomposition and reordered the characters using the
>> combining classes, but the string recomposition was still missing.
>> This is addressed in this patch set, and well as on my dev tree:
>> https://github.com/michaelpq/postgres/tree/scram
>>
>
> I've now committed the bulk of these patches. Many thanks to everyone
> involved, and especially you, Michael, for your persistence!
>

+1!


Currently, the AuthenticationSASL protocol message specifies the mechanism
> that the client must use, but as future-proofing, it'd probably be best to
> redefine that to be a list of mechanisms, and let the client choose among
> those. That's not a show-stopper, but would be good to get that right in
> version 10, so that clients can prepare for that, even if we only support
> one mechanism ATM.
>

+1, and let's make sure we get it into 10. We don't want to be stuck with
something without flexibility -- then we're going to have to do the whole
"is it time yet" dance again. It would be especially bad since the
underlying protocol is pluggable.

This seems like an obvious place, but are there any other places where we
should also consider something like that for compatibility?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Tue, Mar 7, 2017 at 9:36 PM, Heikki Linnakangas  wrote:
> I've now committed the bulk of these patches. Many thanks to everyone
> involved, and especially you, Michael, for your persistence!

Thanks!

> There are a bunch of loose ends, like the SASLprep thing. But the core of
> this patch has been unchanged for some time now, so it's time to move ahead.

This is mandatory for Postgres 10. Among all the other subjects this
is high on the list.

> I left out the new CREATE/ALTER USER ... PASSWORD (... USING '')
> syntax, after all. I think that's still a good idea, but it turned out to be
> largely orthogonal, and this patch was large enough without it. Let's
> discuss that separately, in another thread.

Without that, we are left with only password_encryption as an option
to create a verifier. I am not sure if people would be fine with this
limitation in PG 10. I'll start a thread tomorrow so let's see.

> Currently, the AuthenticationSASL protocol message specifies the mechanism
> that the client must use, but as future-proofing, it'd probably be best to
> redefine that to be a list of mechanisms, and let the client choose among
> those. That's not a show-stopper, but would be good to get that right in
> version 10, so that clients can prepare for that, even if we only support
> one mechanism ATM.

Yep.

> I didn't include the last-minute changes to the way you specify this in
> pg_hba.conf. So it's still just "scram". I agree in general that we should
> think about how to extend that too, but I think the proposed syntax was
> overly verbose for what we actually support right now. Let's discuss that as
> a separate thread, as well.

Fine for me.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 03/02/2017 08:50 AM, Michael Paquier wrote:

Attached is a new patch set. I have combined SASLprep with the rest
and fixed some conflicts. At the same time when going through NFKC
this morning I have noticed that the implementation was doing the
canonical decomposition and reordered the characters using the
combining classes, but the string recomposition was still missing.
This is addressed in this patch set, and well as on my dev tree:
https://github.com/michaelpq/postgres/tree/scram


I've now committed the bulk of these patches. Many thanks to everyone 
involved, and especially you, Michael, for your persistence!


There are a bunch of loose ends, like the SASLprep thing. But the core 
of this patch has been unchanged for some time now, so it's time to move 
ahead.


I left out the new CREATE/ALTER USER ... PASSWORD (... USING '') 
syntax, after all. I think that's still a good idea, but it turned out 
to be largely orthogonal, and this patch was large enough without it. 
Let's discuss that separately, in another thread.


Currently, the AuthenticationSASL protocol message specifies the 
mechanism that the client must use, but as future-proofing, it'd 
probably be best to redefine that to be a list of mechanisms, and let 
the client choose among those. That's not a show-stopper, but would be 
good to get that right in version 10, so that clients can prepare for 
that, even if we only support one mechanism ATM.


I didn't include the last-minute changes to the way you specify this in 
pg_hba.conf. So it's still just "scram". I agree in general that we 
should think about how to extend that too, but I think the proposed 
syntax was overly verbose for what we actually support right now. Let's 
discuss that as a separate thread, as well.


I didn't commit the TAP authentication tests yet. I just didn't have the 
energy to review it all in one go - I will revisit that in the next few 
days.


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 02/20/2017 01:51 PM, Aleksander Alekseev wrote:

Currently I don't see any significant flaws in these patches. However I
would like to verify that implemented algorithms are compatible with
algorithms implemented by third party.


Yes, that's very important.


For instance, for user 'eax' and password 'pass' I got the following
record in pg_authid:

```
scram-sha-256:
xtznkRN/nc/1DQ==:
4096:
2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
```

Let's say I would like to implement SCRAM in pure Python, for instance
add it to pg8000 driver. Firstly I need to know how to generate server
key and client key having only user name and password. Somehow like
this:

```
 >>> import base64
 >>> import hashlib
 >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
 ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
```

Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
implementation just in front of me but unfortunately I'm still having
problems calculating exactly the same server key and client key. It makes
me worry whether PostgreSQL implementation is OK.

Could you please give an example of how to do it?


RFC5802 describes the protocol in detail:


 SaltedPassword  := Hi(Normalize(password), salt, i)
 ClientKey   := HMAC(SaltedPassword, "Client Key")
 StoredKey   := H(ClientKey)
 AuthMessage := client-first-message-bare + "," +
server-first-message + "," +
client-final-message-without-proof
 ClientSignature := HMAC(StoredKey, AuthMessage)
 ClientProof := ClientKey XOR ClientSignature
 ServerKey   := HMAC(SaltedPassword, "Server Key")
 ServerSignature := HMAC(ServerKey, AuthMessage)


You've calculated SaltedPassword correctly with your Python snippet. To 
derive ClientKey from it, you need to pass it to the HMAC() function. In 
python, that'd be hmac.new(SaltedPassword, "Client Key", 
hashlib.sha256). For example:


```
import base64
import hashlib
import hmac

salt = base64.b64decode(b'xtznkRN/nc/1DQ==');
SaltedPassword = hashlib.pbkdf2_hmac('sha256', b'pass',
 salt, 4096);
ClientKey = hmac.new(SaltedPassword, "Client Key", 
hashlib.sha256).hexdigest()

print 'SaltedPassword: ' + base64.b16encode(SaltedPassword)
print 'ClientKey; ' + ClientKey
```

This prints:

SaltedPassword: 
14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3

ClientKey; 5b681195146a2027cb028a921bd0a89ff858b74bd2b38ed8b42561c28b1e369f

Which matches what the libpq implementation calculated. For constructing 
the whole client-final-message, you'll also need to calculate 
ClientSignature and ClientProof, which depend on the nonces, and is 
therefore different on every authentication exchange.


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Mon, Mar 6, 2017 at 11:50 AM, Michael Paquier
 wrote:
> On Fri, Mar 3, 2017 at 2:43 PM, Michael Paquier
>  wrote:
>> I am attaching 0009 and 0010 that address those problems (pushed on
>> github as well) that can be applied on top of the latest set.
>
> While doing more tests with my module able to do SASLprep, I have
> noticed that calculations related to Hangul characters were incorrect:
>  /* Constants for calculations wih Hangul characters */
> -#define SBASE  0xAC00
> -#define LBASE  0x1100
> -#define VBASE  0x1161
> -#define TBASE  0x11A7
> +#define SBASE  0xEAB080/* U+AC00 */
> +#define LBASE  0xE18480/* U+1100 */
> +#define VBASE  0xE185A1/* U+1161 */
> +#define TBASE  0xE186A7/* U+11A7 */

Here is as well an extra patch with this set of fixes, to be applied
on top of the rest. Those are on my github as well, that's for the
archive's sake, and that's better than sending a full set of patches
again.
-- 
Michael


0011-Set-of-fixes-for-SASLprep.patch
Description: Binary data

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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Fri, Mar 3, 2017 at 2:43 PM, Michael Paquier
 wrote:
> I am attaching 0009 and 0010 that address those problems (pushed on
> github as well) that can be applied on top of the latest set.

While doing more tests with my module able to do SASLprep, I have
noticed that calculations related to Hangul characters were incorrect:
 /* Constants for calculations wih Hangul characters */
-#define SBASE  0xAC00
-#define LBASE  0x1100
-#define VBASE  0x1161
-#define TBASE  0x11A7
+#define SBASE  0xEAB080/* U+AC00 */
+#define LBASE  0xE18480/* U+1100 */
+#define VBASE  0xE185A1/* U+1161 */
+#define TBASE  0xE186A7/* U+11A7 */

Once the following is applied things get better:
-- Test for U+FB01, Latin Small Ligature Fi
=# select array_to_utf8(pg_sasl_prepare(utf8_to_array('fi')));
 array_to_utf8
---
 fi
(1 row)
-- Test for U+1E9B, Latin Small Letter Long S with Dot Above
-- This one was failing previously.
=# select array_to_utf8(pg_sasl_prepare(utf8_to_array('ẛ')));
 array_to_utf8
---
 ṡ
(1 row)
-- Test for U+2075, superscript 5
=# select array_to_utf8(pg_sasl_prepare(utf8_to_array('⁵')));
 array_to_utf8
---
 5
(1 row)
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Thu, Mar 2, 2017 at 9:13 PM, Kyotaro HORIGUCHI
 wrote:
> I'm studying the normalization of Unicode so I apologize possible
> stupidity in advance.
>
> I looked into this and have some comments. Sorry for the random
> order.

Thanks, this needs a lot of background and I am glad that somebody is
taking the time to look at what I am doing here.

> 
> Composition version should be written some where.

Sure.

> 
> Perhaps one of the most important things is that the code exactly
> reflects the TR. pg_utf8_check_string returns true for ASCII
> strings but the TR says that
>
> | Text containing only ASCII characters (U+ to U+007F) is left
> | unaffected by all of the normalization forms. This is
> | particularly important for programming languages
>
> And running SASLprepare for apparent ASCII string (which is the
> most case) is a waste of CPU cycles.

Yeah, that's true. We could just for example check in
pg_utf8_check_string() if the length gathered matches strlen(source)
as only ASCII are 1-byte long.

> 
> From the point of view of reflectivity (please someone teach me an
> appropreate wording for this..), basically the code had better to
> be a copy of the reference code as long as no performance
> degradation occurs. Hangul part of get_decomposed_size(and
> decompose_code, recompose_code) uses different naming from the
> TR. hindex should be sindex and t should be tindex. Magic numbers
> should have names in the TR.
>
> * A bit later, I noticed that these are copies of charlint. If so
>   I want a description about that.)

Yeah, their stuff works quite nicely.

> 
> The following comment is equivalent to "reordering in canonical
> order". But the definition of "decomposition" includes this
> step. (I'm not sure if it needs rewriting, since I acutually
> could understand that.)
>
>> /*
>>  * Now that the decomposition is done, apply the combining class
>>  * for each multibyte character.
>>  */

I have reworked a bit this one:
/*
-* Now that the decomposition is done, apply the combining class
-* for each multibyte character.
+* Now end the decomposition by applying the combining class for
+* each multibyte character.
 */

> 
>>* make the allocation of the recomposed string based on that assumption.
>>*/
>>   recomp_chars = (pg_wchar *) malloc(decomp_size * sizeof(int));
>>   lastClass = -1; /* this eliminates a special check */
>
> utf_sasl_prepare uses variable names with two naming
> conventions. Is there any reason for the two?

OK, did some adjustments here.

> 
>> starterCh = recomp_chars[0] = decomp_chars[0];
>
> starterCh reads as "starter channel" why not "starter_char"?

Starter character of the current set, which is a character with a
combining class of 0.

> 
>>   else if (start >= SBASE && start < (SBASE + SCOUNT) &&
>>((start - SBASE) % TCOUNT) == 0 &&
>>code >= TBASE && code <= (TBASE + TCOUNT))
>
> "code <= (TBASE + TCOUNT)" somewhat smells. Then I found the
> original code for the current implementation in charlint and it
> seems correct to me. Some description about the difference
> between them is needed.

Right. I have updated all those things to use constants instead of
hardcoded values.

> 
> In use_sasl_prepare, the recompose part is the copy of charlint
> but it seems a bit inefficient. It calls recompose_code
> unconditionally but it is required only for the case of
> "lastClass < chClass".
>
> Something like this. (This still calls recompose_code for the
> case that ch is the same position with starterChar so there still
> be room for more improvement.)

Agreed.

> 
> If I read the TR correctly, "6 Composition Exclusion Table" says
> that there some characters not to be composed. But I don't find
> the corresponding code. (or comments)

Ah, right! There are 100 characters that enter in this category, and
all of them have a combining class of 0, so it is as simple as
removing them from the tables generated.

I am attaching 0009 and 0010 that address those problems (pushed on
github as well) that can be applied on top of the latest set.
-- 
Michael


0009-Set-of-fixes-for-SASLprep.patch
Description: Binary data


0010-Consider-characters-excluded-from-composition-in-con.patch
Description: Binary data

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

Re: [HACKERS] SCRAM authentication, take three

[Kyotaro HORIGUCHI]

I'm studying the normalization of Unicode so I apologize possible
stupidity in advance.

At Thu, 2 Mar 2017 15:50:34 +0900, Michael Paquier  
wrote in 
> On Tue, Feb 21, 2017 at 9:53 AM, Michael Paquier
>  wrote:
> > On Mon, Feb 20, 2017 at 9:41 PM, Aleksander Alekseev
> >  wrote:
> >>> Speaking about flaws, it looks like there is a memory leak in
> >>> array_to_utf procedure - result is allocated twice.
> >
> > Pushed a fix for this one on my branch.
> >
> >> And a few more things I've noticed after a closer look:
> >>
> >> * build_client_first_message does not free `state->client_nonce` if
> >>   second malloc (for `buf`) fails
> >> * same for `state->client_first_message_bare`
> >> * ... and most other procedures declared in fe-auth-scram.c file
> >>   (see malloc and strdup calls)
> >
> > You are visibly missing pg_fe_scram_free().
> >
> >> * scram_Normalize doesn't check malloc return value
> >
> > Yes, I am aware of this one. This makes the interface utterly ugly
> > though because an error log message needs to be handled across many
> > API layers. We could just assume anything returning NULL is equivalent
> > to an OOM and nothing else though.
> 
> Attached is a new patch set. I have combined SASLprep with the rest
> and fixed some conflicts. At the same time when going through NFKC
> this morning I have noticed that the implementation was doing the
> canonical decomposition and reordered the characters using the
> combining classes, but the string recomposition was still missing.
> This is addressed in this patch set, and well as on my dev tree:
> https://github.com/michaelpq/postgres/tree/scram

I looked into this and have some comments. Sorry for the random
order.


Composition version should be written some where.


Perhaps one of the most important things is that the code exactly
reflects the TR. pg_utf8_check_string returns true for ASCII
strings but the TR says that

| Text containing only ASCII characters (U+ to U+007F) is left
| unaffected by all of the normalization forms. This is
| particularly important for programming languages


And running SASLprepare for apparent ASCII string (which is the
most case) is a waste of CPU cycles.


>From the point of view of reflectivity(please someone teach me an
appropreate wording for this..), basically the code had better to
be a copy of the reference code as long as no performance
degradation occurs. Hangul part of get_decomposed_size(and
decompose_code, recompose_code) uses different naming from the
TR. hindex should be sindex and t should be tindex. Magic numbers
should have names in the TR. 

* A bit later, I noticed that these are copies of charlint. If so
  I want a description about that.)


The following comment is equivalent to "reordering in canonical
order". But the definition of "decomposition" includes this
step. (I'm not sure if it needs rewriting, since I acutually
could understand that.)

> /*
>  * Now that the decomposition is done, apply the combining class
>  * for each multibyte character.
>  */


utf_sasl_prepare does canonical ordering in a bit different way
than the TR. Totally it should make a sequence of characters
starts with combining class = 0 and in the order of combining
class. The code does stable bubble sort within each combining
character and it seems to work as the same way. (In short, no
probelm found here.)


>* make the allocation of the recomposed string based on that assumption.
>*/
>   recomp_chars = (pg_wchar *) malloc(decomp_size * sizeof(int));
>   lastClass = -1; /* this eliminates a special check */

utf_sasl_prepare uses variable names with two naming
conventions. Is there any reason for the two?


> starterCh = recomp_chars[0] = decomp_chars[0];

starterCh reads as "starter channel" why not "starter_char"?


Other than the magic numbers, I don't think that the following is
not a good expression.

>   else if (start >= 0xAC00 && start < 0xD7A4 &&
>!((start - 0xAC00) % 28) &&
>code >= 0x11A8 && code < 0x11C3)

"!((start - 0xAC00) % 28)" is a similar of !strcmp(a, b) and it
is confusing. It would be better to be "((start - 0xAC00) % 28) == 0".

The last sub-condition "code >= 0x11A8 && code < 0x11C3"
corresnponds to "(0 <= TIndex && TIndex <= TCount)". TIndex here
is (code - 0x11a7) and TCount = 28 so this two are not identical.

Totally the condition should be like the following.

>   else if (start >= 0xAC00 && start < 0xD7A4 &&
>((start - 0xAC00) % 28) == 0 &&
>code >= 0x11A7 && code <= 0x11C3)

The more preferable form is 

>   else if (start >= SBASE && start < (SBASE + SCOUNT) &&
>((start - SBASE) % TCOUNT) == 0 &&
>code >= TBASE && code <= (TBASE + TCOUNT))

"code <= (TBASE + TCOUNT)" somewhat smells. Then I found the
original code 

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Mon, Feb 20, 2017 at 9:41 PM, Aleksander Alekseev
 wrote:
>> Speaking about flaws, it looks like there is a memory leak in
>> array_to_utf procedure - result is allocated twice.

Pushed a fix for this one on my branch.

> And a few more things I've noticed after a closer look:
>
> * build_client_first_message does not free `state->client_nonce` if
>   second malloc (for `buf`) fails
> * same for `state->client_first_message_bare`
> * ... and most other procedures declared in fe-auth-scram.c file
>   (see malloc and strdup calls)

You are visibly missing pg_fe_scram_free().

> * scram_Normalize doesn't check malloc return value

Yes, I am aware of this one. This makes the interface utterly ugly
though because an error log message needs to be handled across many
API layers. We could just assume anything returning NULL is equivalent
to an OOM and nothing else though.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

And a few more things I've noticed after a closer look:

* build_client_first_message does not free `state->client_nonce` if
  second malloc (for `buf`) fails
* same for `state->client_first_message_bare`
* ... and most other procedures declared in fe-auth-scram.c file
  (see malloc and strdup calls)
* scram_Normalize doesn't check malloc return value

Sorry for lots of emails.

On Mon, Feb 20, 2017 at 03:15:14PM +0300, Aleksander Alekseev wrote:
> Speaking about flaws, it looks like there is a memory leak in
> array_to_utf procedure - result is allocated twice.
> 
> On Mon, Feb 20, 2017 at 02:51:13PM +0300, Aleksander Alekseev wrote:
> > Hi!
> > 
> > Currently I don't see any significant flaws in these patches. However I
> > would like to verify that implemented algorithms are compatible with
> > algorithms implemented by third party.
> > 
> > For instance, for user 'eax' and password 'pass' I got the following
> > record in pg_authid:
> > 
> > ```
> > scram-sha-256:
> > xtznkRN/nc/1DQ==:
> > 4096:
> > 2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
> > 5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
> > ```
> > 
> > Let's say I would like to implement SCRAM in pure Python, for instance
> > add it to pg8000 driver. Firstly I need to know how to generate server
> > key and client key having only user name and password. Somehow like
> > this:
> > 
> > ```
> >  >>> import base64
> >  >>> import hashlib
> >  >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
> >  ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
> > b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
> > ```
> > 
> > Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
> > SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
> > implementation just in front of me but unfortunately I'm still having
> > problems calculating exactly the same server key and client key. It makes
> > me worry whether PostgreSQL implementation is OK.
> > 
> > Could you please give an example of how to do it?
> > 
> > On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> > > On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
> > >  wrote:
> > > > There is something that I think is still unwelcome in this patch: the
> > > > interface in pg_hba.conf. I mentioned that in the previous thread but
> > > > now if you want to match a user and a database with a scram password
> > > > you need to do that with the current set of patches:
> > > > local $dbname $user scram
> > > > That's not really portable as SCRAM is one protocol in the SASL
> > > > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > > > change pg_hba.conf to be as follows:
> > > > local $dbname $user sasl protocol=scram_sha_256
> > > > This is extensible for the future, and protocol is a mandatory option
> > > > that would have now just a single value: scram_sha_256. Heikki,
> > > > others, are you fine with that?
> > > 
> > > I have implemented that as 0009 which is attached, and need to be
> > > applied on the rest of upthread. I am not sure if we want to make the
> > > case where no protocol is specified map to everything. This would be a
> > > tricky support for users in the future if new authentication
> > > mechanisms for SASL are added in the future.
> > > 
> > > Another issue that I have is: do we really want to have
> > > password_encryption being set to "scram" for verifiers of
> > > SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> > > Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> > > 
> > > At the same time, attached is a new version of 0008 that implements
> > > SASLprep, I have stabilized the beast after fixing some allocation
> > > calculations when converting the decomposed pg_wchar array back to a
> > > utf8 string.
> > > -- 
> > > Michael
> > 
> > -- 
> > Best regards,
> > Aleksander Alekseev
> 
> 
> 
> -- 
> Best regards,
> Aleksander Alekseev



-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Speaking about flaws, it looks like there is a memory leak in
array_to_utf procedure - result is allocated twice.

On Mon, Feb 20, 2017 at 02:51:13PM +0300, Aleksander Alekseev wrote:
> Hi!
> 
> Currently I don't see any significant flaws in these patches. However I
> would like to verify that implemented algorithms are compatible with
> algorithms implemented by third party.
> 
> For instance, for user 'eax' and password 'pass' I got the following
> record in pg_authid:
> 
> ```
> scram-sha-256:
> xtznkRN/nc/1DQ==:
> 4096:
> 2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
> 5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
> ```
> 
> Let's say I would like to implement SCRAM in pure Python, for instance
> add it to pg8000 driver. Firstly I need to know how to generate server
> key and client key having only user name and password. Somehow like
> this:
> 
> ```
>  >>> import base64
>  >>> import hashlib
>  >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
>  ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
> b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
> ```
> 
> Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
> SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
> implementation just in front of me but unfortunately I'm still having
> problems calculating exactly the same server key and client key. It makes
> me worry whether PostgreSQL implementation is OK.
> 
> Could you please give an example of how to do it?
> 
> On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> > On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
> >  wrote:
> > > There is something that I think is still unwelcome in this patch: the
> > > interface in pg_hba.conf. I mentioned that in the previous thread but
> > > now if you want to match a user and a database with a scram password
> > > you need to do that with the current set of patches:
> > > local $dbname $user scram
> > > That's not really portable as SCRAM is one protocol in the SASL
> > > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > > change pg_hba.conf to be as follows:
> > > local $dbname $user sasl protocol=scram_sha_256
> > > This is extensible for the future, and protocol is a mandatory option
> > > that would have now just a single value: scram_sha_256. Heikki,
> > > others, are you fine with that?
> > 
> > I have implemented that as 0009 which is attached, and need to be
> > applied on the rest of upthread. I am not sure if we want to make the
> > case where no protocol is specified map to everything. This would be a
> > tricky support for users in the future if new authentication
> > mechanisms for SASL are added in the future.
> > 
> > Another issue that I have is: do we really want to have
> > password_encryption being set to "scram" for verifiers of
> > SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> > Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> > 
> > At the same time, attached is a new version of 0008 that implements
> > SASLprep, I have stabilized the beast after fixing some allocation
> > calculations when converting the decomposed pg_wchar array back to a
> > utf8 string.
> > -- 
> > Michael
> 
> -- 
> Best regards,
> Aleksander Alekseev



-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Hi!

Currently I don't see any significant flaws in these patches. However I
would like to verify that implemented algorithms are compatible with
algorithms implemented by third party.

For instance, for user 'eax' and password 'pass' I got the following
record in pg_authid:

```
scram-sha-256:
xtznkRN/nc/1DQ==:
4096:
2387c124a3139a276b848c910f43ece05dd670d0977ace4f20d724af522312e4:
5e3bdd6584880198b0375acabd8754c460af2389499f71a756660a10a8aaa843
```

Let's say I would like to implement SCRAM in pure Python, for instance
add it to pg8000 driver. Firstly I need to know how to generate server
key and client key having only user name and password. Somehow like
this:

```
 >>> import base64
 >>> import hashlib
 >>> base64.b16encode(hashlib.pbkdf2_hmac('sha256', b'pass',
 ...base64.b64decode(b'xtznkRN/nc/1DQ=='), 4096))
b'14B90CFCF690120399A0E6D30C60DD9D9D221CD9C2E31EA0A00514C41823E6C3'
```

Naturally it doesn't work because both SCRAM_SERVER_KEY_NAME and
SCRAM_CLIENT_KEY_NAME should also be involved. I see PostgreSQL
implementation just in front of me but unfortunately I'm still having
problems calculating exactly the same server key and client key. It makes
me worry whether PostgreSQL implementation is OK.

Could you please give an example of how to do it?

On Mon, Feb 20, 2017 at 03:29:19PM +0900, Michael Paquier wrote:
> On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
>  wrote:
> > There is something that I think is still unwelcome in this patch: the
> > interface in pg_hba.conf. I mentioned that in the previous thread but
> > now if you want to match a user and a database with a scram password
> > you need to do that with the current set of patches:
> > local $dbname $user scram
> > That's not really portable as SCRAM is one protocol in the SASL
> > family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> > change pg_hba.conf to be as follows:
> > local $dbname $user sasl protocol=scram_sha_256
> > This is extensible for the future, and protocol is a mandatory option
> > that would have now just a single value: scram_sha_256. Heikki,
> > others, are you fine with that?
> 
> I have implemented that as 0009 which is attached, and need to be
> applied on the rest of upthread. I am not sure if we want to make the
> case where no protocol is specified map to everything. This would be a
> tricky support for users in the future if new authentication
> mechanisms for SASL are added in the future.
> 
> Another issue that I have is: do we really want to have
> password_encryption being set to "scram" for verifiers of
> SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
> Who knows, perhaps there could be in a couple of years a SHA-SHA-512..
> 
> At the same time, attached is a new version of 0008 that implements
> SASLprep, I have stabilized the beast after fixing some allocation
> calculations when converting the decomposed pg_wchar array back to a
> utf8 string.
> -- 
> Michael

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
 wrote:
> There is something that I think is still unwelcome in this patch: the
> interface in pg_hba.conf. I mentioned that in the previous thread but
> now if you want to match a user and a database with a scram password
> you need to do that with the current set of patches:
> local $dbname $user scram
> That's not really portable as SCRAM is one protocol in the SASL
> family, and even worse in our case we use SCRAM-SHA-256. I'd like to
> change pg_hba.conf to be as follows:
> local $dbname $user sasl protocol=scram_sha_256
> This is extensible for the future, and protocol is a mandatory option
> that would have now just a single value: scram_sha_256. Heikki,
> others, are you fine with that?

I have implemented that as 0009 which is attached, and need to be
applied on the rest of upthread. I am not sure if we want to make the
case where no protocol is specified map to everything. This would be a
tricky support for users in the future if new authentication
mechanisms for SASL are added in the future.

Another issue that I have is: do we really want to have
password_encryption being set to "scram" for verifiers of
SCRAM-SHA-256? I would think that scram_sha_256 makes the most sense.
Who knows, perhaps there could be in a couple of years a SHA-SHA-512..

At the same time, attached is a new version of 0008 that implements
SASLprep, I have stabilized the beast after fixing some allocation
calculations when converting the decomposed pg_wchar array back to a
utf8 string.
-- 
Michael


0009-Make-hba-configuration-for-SASL-more-extensible.patch
Description: Binary data


0008-Implement-SASLprep-aka-NFKC-for-SCRAM-authentication.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Wed, Feb 15, 2017 at 8:28 PM, Heikki Linnakangas  wrote:
> On 02/07/2017 04:20 AM, Michael Paquier wrote:
>> --- a/src/backend/utils/errcodes.txt
>> +++ b/src/backend/utils/errcodes.txt
>> @@ -247,6 +247,7 @@ Section: Class 28 - Invalid Authorization
>> Specification
>>
>>  28000EERRCODE_INVALID_AUTHORIZATION_SPECIFICATION
>> invalid_authorization_specification
>>  28P01EERRCODE_INVALID_PASSWORD
>> invalid_password
>> +28P01EERRCODE_INVALID_NONCE
>> invalid_nonce
>>
>
> Having two error codes with the same SQLSTATE is not cool, and tripped the
> assertion in PL/python. I removed the new error code, it was only used in
> one place, and ERRCODE_PROTOCOL_VIOLATIOn was more appropriate there anyway.
>
> Attached is a new set of patches, with that fixed. Thanks for the report
> Aleksander!

There is something that I think is still unwelcome in this patch: the
interface in pg_hba.conf. I mentioned that in the previous thread but
now if you want to match a user and a database with a scram password
you need to do that with the current set of patches:
local $dbname $user scram
That's not really portable as SCRAM is one protocol in the SASL
family, and even worse in our case we use SCRAM-SHA-256. I'd like to
change pg_hhba.conf to be as follows:
local $dbname $user sasl protocol=scram_sha_256
This is extensible for the future, and protocol is a mandatory option
that would have now just a single value: scram_sha_256. Heikki,
others, are you fine with that?
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Sun, Feb 19, 2017 at 6:55 PM, Robert Haas  wrote:
> Gosh, this SCRAM stuff seems to be taking us pretty deeply into
> dealing with encoding details which apparently we haven't formerly
> needed to worry about.  That is a little surprising and maybe
> something we should try to avoid?

The RFC of SCRAM, RFC5802 is clear on the matter
(https://tools.ietf.org/html/rfc5802), SASLprep needs NFKC (RFC4013
here, the worst in the set https://tools.ietf.org/html/rfc4013) if we
want our implementation to be compatible with any other Postgres
driver that implement things at protocol level without libpq. I think
that JDBC is one of those things. So I am afraid we cannot avoid it if
we want SCRAM.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Robert Haas]

On Fri, Feb 17, 2017 at 7:29 PM, Michael Paquier
 wrote:
> On Wed, Feb 15, 2017 at 9:27 PM, Michael Paquier
>  wrote:
>> On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas  wrote:
>>> On 02/09/2017 09:33 AM, Michael Paquier wrote:
 Now regarding the shape of the implementation for SCRAM, we need one
 thing: a set of routines in src/common/ to build decompositions for a
 given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
 decomposition and the reordering. The extension attached roughly
 implements that. What we can actually do as well is have in contrib/ a
 module that does NFK[C|D] using the base APIs in src/common/. Using
 arrays of pg_wchar (integers) to manipulate the characters, we can
 validate and have a set of regression tests that do *not* have to
 print non-ASCII characters.
>>>
>>>
>>> A contrib module or built-in extra functions to deal with Unicode characters
>>> might be handy for a lot of things. But I'd leave that out for now, to keep
>>> this patch minimal.
>>
>> No problem from me. I'll get something for SASLprep in the shape of
>> something like the above. It should not take me long.
>
> OK, attached is a patch that implements SASLprep that needs to be
> applied on top of the other ones. When working on the table reduction,
> the worst size was at 2.4MB. After removing all the characters with a
> class of 0 and no decomposition, I am able to get that down to 570kB.
> After splitting the decompositions by size into their own tables, it
> got down to 120kB, which is even nicer. One thing that I forgot
> previously was the handling of the decomposition of Hangul characters
> (Korean stuff) which is algorithmic, so you actually do not need a
> table for them. The algorithm is here for the curious =>
> http://unicode.org/reports/tr15/tr15-18.html#Hangul.
>
> The patch includes the conversion tables, which is why it is large,
> and the perl script that I used to generate it. It has been pushed as
> well on my github branch. The basics are here I think, still this
> portion really needs a careful review. I have done some basic tests
> and things are basically working, but I have been able to break things
> pretty easily when using some exotic characters. The conversion tables
> look correct, I have tested it using my module which implements NFKC
> (https://github.com/michaelpq/pg_plugins/tree/master/pg_sasl_prepare),
> still much refinement needs to be done.

Gosh, this SCRAM stuff seems to be taking us pretty deeply into
dealing with encoding details which apparently we haven't formerly
needed to worry about.  That is a little surprising and maybe
something we should try to avoid?

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


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Wed, Feb 15, 2017 at 9:27 PM, Michael Paquier
 wrote:
> On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas  wrote:
>> On 02/09/2017 09:33 AM, Michael Paquier wrote:
>>> Now regarding the shape of the implementation for SCRAM, we need one
>>> thing: a set of routines in src/common/ to build decompositions for a
>>> given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
>>> decomposition and the reordering. The extension attached roughly
>>> implements that. What we can actually do as well is have in contrib/ a
>>> module that does NFK[C|D] using the base APIs in src/common/. Using
>>> arrays of pg_wchar (integers) to manipulate the characters, we can
>>> validate and have a set of regression tests that do *not* have to
>>> print non-ASCII characters.
>>
>>
>> A contrib module or built-in extra functions to deal with Unicode characters
>> might be handy for a lot of things. But I'd leave that out for now, to keep
>> this patch minimal.
>
> No problem from me. I'll get something for SASLprep in the shape of
> something like the above. It should not take me long.

OK, attached is a patch that implements SASLprep that needs to be
applied on top of the other ones. When working on the table reduction,
the worst size was at 2.4MB. After removing all the characters with a
class of 0 and no decomposition, I am able to get that down to 570kB.
After splitting the decompositions by size into their own tables, it
got down to 120kB, which is even nicer. One thing that I forgot
previously was the handling of the decomposition of Hangul characters
(Korean stuff) which is algorithmic, so you actually do not need a
table for them. The algorithm is here for the curious =>
http://unicode.org/reports/tr15/tr15-18.html#Hangul.

The patch includes the conversion tables, which is why it is large,
and the perl script that I used to generate it. It has been pushed as
well on my github branch. The basics are here I think, still this
portion really needs a careful review. I have done some basic tests
and things are basically working, but I have been able to break things
pretty easily when using some exotic characters. The conversion tables
look correct, I have tested it using my module which implements NFKC
(https://github.com/michaelpq/pg_plugins/tree/master/pg_sasl_prepare),
still much refinement needs to be done.
-- 
Michael


0008-Implement-SASLprep-aka-NFKC-for-SCRAM-authentication.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Wed, Feb 15, 2017 at 8:28 PM, Heikki Linnakangas  wrote:
> Ah, found it. It was because of this change:
>
> Having two error codes with the same SQLSTATE is not cool, and tripped the
> assertion in PL/python. I removed the new error code, it was only used in
> one place, and ERRCODE_PROTOCOL_VIOLATIOn was more appropriate there anyway.

It seems that this one is on me. Thanks for looking at it.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas  wrote:
> On 02/09/2017 09:33 AM, Michael Paquier wrote:
>> Now regarding the shape of the implementation for SCRAM, we need one
>> thing: a set of routines in src/common/ to build decompositions for a
>> given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
>> decomposition and the reordering. The extension attached roughly
>> implements that. What we can actually do as well is have in contrib/ a
>> module that does NFK[C|D] using the base APIs in src/common/. Using
>> arrays of pg_wchar (integers) to manipulate the characters, we can
>> validate and have a set of regression tests that do *not* have to
>> print non-ASCII characters.
>
>
> A contrib module or built-in extra functions to deal with Unicode characters
> might be handy for a lot of things. But I'd leave that out for now, to keep
> this patch minimal.

No problem from me. I'll get something for SASLprep in the shape of
something like the above. It should not take me long.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 02/07/2017 04:20 AM, Michael Paquier wrote:

On Tue, Feb 7, 2017 at 3:12 AM, Aleksander Alekseev
 wrote:

No, I'm afraid `make distclean` doesn't help. I've re-checked twice.


Hm. I can see the failure on macos and python2 builds as well with the
set of patches applied. And the master branch is working properly.
This needs some investigation.


Ah, found it. It was because of this change:


--- a/src/backend/utils/errcodes.txt
+++ b/src/backend/utils/errcodes.txt
@@ -247,6 +247,7 @@ Section: Class 28 - Invalid Authorization Specification

 28000EERRCODE_INVALID_AUTHORIZATION_SPECIFICATION
invalid_authorization_specification
 28P01EERRCODE_INVALID_PASSWORD   
invalid_password
+28P01EERRCODE_INVALID_NONCE  
invalid_nonce



Having two error codes with the same SQLSTATE is not cool, and tripped 
the assertion in PL/python. I removed the new error code, it was only 
used in one place, and ERRCODE_PROTOCOL_VIOLATIOn was more appropriate 
there anyway.


Attached is a new set of patches, with that fixed. Thanks for the report 
Aleksander!


- Heikki



0001-Refactor-SHA2-functions-and-move-them-to-src-common.patch.gz
Description: application/gzip


0002-Add-encoding-routines-for-base64-without-whitespace-.patch.gz
Description: application/gzip


0003-Add-clause-PASSWORD-val-USING-protocol-to-CREATE-ALT.patch.gz
Description: application/gzip


0004-Support-for-SCRAM-SHA-256-authentication-RFC-5802-an.patch.gz
Description: application/gzip


0005-Add-regression-tests-for-passwords.patch.gz
Description: application/gzip


0006-Add-TAP-tests-for-authentication-methods.patch.gz
Description: application/gzip


0007-Introduce-WIP-for-UTF-8-normalization.patch.gz
Description: application/gzip

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

Re: [HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

On 02/09/2017 09:33 AM, Michael Paquier wrote:

On Tue, Feb 7, 2017 at 11:28 AM, Michael Paquier
 wrote:

Yes, I am actively working on this one now. I am trying to come up
first with something in the shape of an extension to begin with, and
get a patch out of it. That will be more simple for testing. For now
the work that really remains in the patches attached on this thread is
to get the internal work done, all the UTF8-related routines being
already present in scram-common.c to work on the strings.


It took me a couple of days... And attached is the prototype
implementing SASLprep(), or NFKC if you want for UTF-8 strings.


Cool!


Now using this module I have arrived to the following conclusions to
put to a minimum the size of the conversion tables, without much
impacting lookup performance:
- There are 24k characters with a combining class of 0 and no
decomposition for a total of 30k characters, those need to be dropped
from the conversion table.
- Most characters have a single, or double decomposition, one has a
decomposition of 18 characters. So we need to create two sets of
conversion tables:
-- A base table, with the character number (4 bytes), the combining
class (1 byte) and the size of the decomposition (1 byte).
-- A set of decomposition tables, classified by size.
So when decomposing a character, we check first the size of the
decomposition, then get the set from the correct table.


Sounds good.


Now regarding the shape of the implementation for SCRAM, we need one
thing: a set of routines in src/common/ to build decompositions for a
given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
decomposition and the reordering. The extension attached roughly
implements that. What we can actually do as well is have in contrib/ a
module that does NFK[C|D] using the base APIs in src/common/. Using
arrays of pg_wchar (integers) to manipulate the characters, we can
validate and have a set of regression tests that do *not* have to
print non-ASCII characters.


A contrib module or built-in extra functions to deal with Unicode 
characters might be handy for a lot of things. But I'd leave that out 
for now, to keep this patch minimal.


- Heikki



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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Tue, Feb 7, 2017 at 11:28 AM, Michael Paquier
 wrote:
> Yes, I am actively working on this one now. I am trying to come up
> first with something in the shape of an extension to begin with, and
> get a patch out of it. That will be more simple for testing. For now
> the work that really remains in the patches attached on this thread is
> to get the internal work done, all the UTF8-related routines being
> already present in scram-common.c to work on the strings.

It took me a couple of days... And attached is the prototype
implementing SASLprep(), or NFKC if you want for UTF-8 strings. This
extension is pretty useful to check the validity of any normalization
forms defined in the Unicode specs, and is as well on my github:
https://github.com/michaelpq/pg_plugins/tree/master/pg_sasl_prepare

In short, at build what this extension does is fetching
UnicodeData.txt, and it builds a conversion table including the fields
necessary for NFKC:
- The utf code of a character.
- The combining class of the character.
- The decomposition sets of characters.
I am aware of the fact that the implemention of this extension is the
worst possible, there are many bytes wasted, and the resulting shared
library gets at 2.4MB. Now as an example of how normalization forms
work that's a great study, and with that I am now aware of what needs
to be done to reduce the size of the conversion tables.

This extension has two conversion functions for UTF-8 string <=>
integer array (as UTF-8 characters are on 4 bytes with pg_wchar):
=# SELECT array_to_utf8('{50064}');
 array_to_utf8
---
 Ð
(1 row)
=# SELECT utf8_to_array('ÐÐÐ');
utf8_to_array
-
 {50064,50064,50064}
(1 row)

Then using an integer array in input SASLprep() can be done using
pg_sasl_prepare(), which returns to caller a decomposed recursively
set, with reordering done using the combining class integer array from
the conversion table generated wiht UnicodeData.txt. Lookups at the
conversion tables are done using bsearch(), so that's, at least I
guess, fast.

I have implemented as well a function to query the whole conversion as
a SRF (character number, combining class and decomposition), which is
useful for analysis:
=# select count(*) from utf8_conv_table();
 count
---
 30590
(1 row)

Now using this module I have arrived to the following conclusions to
put to a minimum the size of the conversion tables, without much
impacting lookup performance:
- There are 24k characters with a combining class of 0 and no
decomposition for a total of 30k characters, those need to be dropped
from the conversion table.
- Most characters have a single, or double decomposition, one has a
decomposition of 18 characters. So we need to create two sets of
conversion tables:
-- A base table, with the character number (4 bytes), the combining
class (1 byte) and the size of the decomposition (1 byte).
-- A set of decomposition tables, classified by size.
So when decomposing a character, we check first the size of the
decomposition, then get the set from the correct table.

Now regarding the shape of the implementation for SCRAM, we need one
thing: a set of routines in src/common/ to build decompositions for a
given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
decomposition and the reordering. The extension attached roughly
implements that. What we can actually do as well is have in contrib/ a
module that does NFK[C|D] using the base APIs in src/common/. Using
arrays of pg_wchar (integers) to manipulate the characters, we can
validate and have a set of regression tests that do *not* have to
print non-ASCII characters. Let me know if this plan looks good, now I
think that I have enough material to get SASLprep done cleanly, with a
minimum memory footprint.

Heikki, others, how does that sound for you?
-- 
Michael


pg_sasl_prepare.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Mon, Feb 6, 2017 at 9:55 PM, Heikki Linnakangas  wrote:
> I rebased the SCRAM authentication patches over current master. Here you
> are.

Thanks! Nice to see you around.

> So, if you haven't paid attention on this for a while, now would be a good
> time to have another look at the patch. I believe all the basic
> functionality, documentation, and tests are there, and there are no known
> bugs. Please review! I'll start reading through these myself again tomorrow.

To all: this wiki page is up to date with all the items that remain:
https://wiki.postgresql.org/wiki/SCRAM_authentication
I am keeping the list there up to date with issues noticed on the way.

> One thing that's missing, that we need to address before the release, is the
> use of SASLPrep to "normalize" the password. We discussed that in the
> previous thread, and I think we have a good path forward on it. I'd be happy
> to leave that for a follow-up commit, after these other patches have been
> committed, so we can discuss that work separately.

Yes, I am actively working on this one now. I am trying to come up
first with something in the shape of an extension to begin with, and
get a patch out of it. That will be more simple for testing. For now
the work that really remains in the patches attached on this thread is
to get the internal work done, all the UTF8-related routines being
already present in scram-common.c to work on the strings.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Tue, Feb 7, 2017 at 3:12 AM, Aleksander Alekseev
 wrote:
> No, I'm afraid `make distclean` doesn't help. I've re-checked twice.

Hm. I can see the failure on macos and python2 builds as well with the
set of patches applied. And the master branch is working properly.
This needs some investigation.
-- 
Michael


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

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

No, I'm afraid `make distclean` doesn't help. I've re-checked twice.

On Mon, Feb 06, 2017 at 12:52:11PM -0300, Alvaro Herrera wrote:
> Aleksander Alekseev wrote:
> > Hello.
> > 
> > Good to know that the work on this great feature continues!
> > 
> > However this set of patches does not pass `make check-world` on my
> > laptop.
> > 
> > Here is how I build and test PostgreSQL:
> > 
> > https://github.com/afiskon/pgscripts/blob/master/full-build.sh
> 
> I think you need "make distclean" instead of "make clean", unless you
> also add --enable-depend to configure.
> 
> -- 
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] SCRAM authentication, take three

[Alvaro Herrera]

Aleksander Alekseev wrote:
> Hello.
> 
> Good to know that the work on this great feature continues!
> 
> However this set of patches does not pass `make check-world` on my
> laptop.
> 
> Here is how I build and test PostgreSQL:
> 
> https://github.com/afiskon/pgscripts/blob/master/full-build.sh

I think you need "make distclean" instead of "make clean", unless you
also add --enable-depend to configure.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] SCRAM authentication, take three

[Aleksander Alekseev]

Hello.

Good to know that the work on this great feature continues!

However this set of patches does not pass `make check-world` on my
laptop.

Here is how I build and test PostgreSQL:

https://github.com/afiskon/pgscripts/blob/master/full-build.sh

Error message:

http://afiskon.ru/s/0b/258c6e4f14_123.txt

regression.diffs:

http://afiskon.ru/s/a0/4993102c32_regression.diffs.txt

regression.out:

http://afiskon.ru/s/4b/acd5a58374_regression.out.txt

Backtrace:

http://afiskon.ru/s/00/c0b32b45a6_bt.txt

I'm using Arch Linux with latest updates, Python version is 3.6.0. I
have no idea what SCRAM has to do with Python, but this issue reproduced
two times of two I did `make check-world`. There is no such issue in
current master branch.

On Mon, Feb 06, 2017 at 02:55:08PM +0200, Heikki Linnakangas wrote:
> I rebased the SCRAM authentication patches over current master. Here you
> are.
> 
> I'm trying to whack this into the final shape that it could actually be
> committed. The previous thread on SCRAM authentication has grown
> ridiculously long and meandered into all kinds of details, so I thought it's
> best to start afresh with a new thread.
> 
> So, if you haven't paid attention on this for a while, now would be a good
> time to have another look at the patch. I believe all the basic
> functionality, documentation, and tests are there, and there are no known
> bugs. Please review! I'll start reading through these myself again tomorrow.
> 
> One thing that's missing, that we need to address before the release, is the
> use of SASLPrep to "normalize" the password. We discussed that in the
> previous thread, and I think we have a good path forward on it. I'd be happy
> to leave that for a follow-up commit, after these other patches have been
> committed, so we can discuss that work separately.
> 
> These are also available on Michael's github repository, at
> https://github.com/michaelpq/postgres/tree/scram.
> 
> - Heikki
> 

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

[HACKERS] SCRAM authentication, take three

[Heikki Linnakangas]

I rebased the SCRAM authentication patches over current master. Here you 
are.


I'm trying to whack this into the final shape that it could actually be 
committed. The previous thread on SCRAM authentication has grown 
ridiculously long and meandered into all kinds of details, so I thought 
it's best to start afresh with a new thread.


So, if you haven't paid attention on this for a while, now would be a 
good time to have another look at the patch. I believe all the basic 
functionality, documentation, and tests are there, and there are no 
known bugs. Please review! I'll start reading through these myself again 
tomorrow.


One thing that's missing, that we need to address before the release, is 
the use of SASLPrep to "normalize" the password. We discussed that in 
the previous thread, and I think we have a good path forward on it. I'd 
be happy to leave that for a follow-up commit, after these other patches 
have been committed, so we can discuss that work separately.


These are also available on Michael's github repository, at 
https://github.com/michaelpq/postgres/tree/scram.


- Heikki



0001-Refactor-SHA2-functions-and-move-them-to-src-common.patch.gz
Description: application/gzip


0002-Add-encoding-routines-for-base64-without-whitespace-.patch.gz
Description: application/gzip


0003-Add-clause-PASSWORD-val-USING-protocol-to-CREATE-ALT.patch.gz
Description: application/gzip


0004-Support-for-SCRAM-SHA-256-authentication-RFC-5802-an.patch.gz
Description: application/gzip


0005-Add-regression-tests-for-passwords.patch.gz
Description: application/gzip


0006-Add-TAP-tests-for-authentication-methods.patch.gz
Description: application/gzip

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