Thread: [HACKERS] SCRAM authentication, take three

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


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

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Best regards,
Aleksander Alekseev

On Tue, Feb 7, 2017 at 3:12 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> 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

On Mon, Feb 6, 2017 at 9:55 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Tue, Feb 7, 2017 at 11:28 AM, Michael Paquier
<michael.paquier@gmail.com> 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

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

On 02/09/2017 09:33 AM, Michael Paquier wrote:
> On Tue, Feb 7, 2017 at 11:28 AM, Michael Paquier
> <michael.paquier@gmail.com> 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

On 02/07/2017 04:20 AM, Michael Paquier wrote:
> On Tue, Feb 7, 2017 at 3:12 AM, Aleksander Alekseev
> <a.alekseev@postgrespro.ru> 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
>
>  28000    E    ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION                    invalid_authorization_specification
>  28P01    E    ERRCODE_INVALID_PASSWORD                                       invalid_password
> +28P01    E    ERRCODE_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


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

On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Wed, Feb 15, 2017 at 8:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Wed, Feb 15, 2017 at 9:27 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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

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

On Fri, Feb 17, 2017 at 7:29 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 9:27 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Feb 15, 2017 at 7:58 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Sun, Feb 19, 2017 at 6:55 PM, Robert Haas <robertmhaas@gmail.com> 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

On Wed, Feb 15, 2017 at 8:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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
>>
>>  28000    E    ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION
>> invalid_authorization_specification
>>  28P01    E    ERRCODE_INVALID_PASSWORD
>> invalid_password
>> +28P01    E    ERRCODE_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

On Sun, Feb 19, 2017 at 10:07 PM, Michael Paquier
<michael.paquier@gmail.com> 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

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

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
> <michael.paquier@gmail.com> 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

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
> > <michael.paquier@gmail.com> 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

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
> > > <michael.paquier@gmail.com> 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

On Mon, Feb 20, 2017 at 9:41 PM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> 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

On Tue, Feb 21, 2017 at 9:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 9:41 PM, Aleksander Alekseev
> <a.alekseev@postgrespro.ru> 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
-- 
Michael

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

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 <michael.paquier@gmail.com> wrote in
<CAB7nPqRbR3GmFYdedCAhzukfKrgBLTLtMvENOmPrVWREsZkF8g@mail.gmail.com>
> On Tue, Feb 21, 2017 at 9:53 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Mon, Feb 20, 2017 at 9:41 PM, Aleksander Alekseev
> > <a.alekseev@postgrespro.ru> 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+0000 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 for the current implementation in charlint and it
seems correct to me. Some description about the difference
between them is needed.

====
In use_sasl_prepare, the recompose part i sthe 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.)

>   if (lastClass < chClass &&
>       recompose_code(starterCh, ch, &composite))
>       recomp_chars[starterPos] = composite;
>       starterCh = composite;
>   }
>   else if (chClass == 0)
....


====
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)

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Mar 2, 2017 at 9:13 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> 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+0000 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

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

On Fri, Mar 3, 2017 at 2:43 PM, Michael Paquier
<michael.paquier@gmail.com> 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

On Mon, Mar 6, 2017 at 11:50 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Mar 3, 2017 at 2:43 PM, Michael Paquier
> <michael.paquier@gmail.com> 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

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

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

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 '<method>') 
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

On Tue, Mar 7, 2017 at 9:36 PM, Heikki Linnakangas <hlinnaka@iki.fi> 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 '<method>')
> 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

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

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

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=EwfMDYwCk1Q@mail.gmail.com
[2] https://www.postgresql.org/message-id/20170118052356.GA5952@gust
[3] https://www.postgresql.org/message-id/CAB7nPqSALxkOOHBK3ugBF+Kfq4pqgTgJK_os68f3NkXGhDOz6w@mail.gmail.com

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:
> sasl    scram_sha256
> sasl    scram_sha256,channel
> sasl    scram_sha512
> sasl    scram_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  DATABASE        USER            ADDRESS                 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 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.


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.

[1] 
https://www.postgresql.org/message-id/CAB7nPqS99Z31f7jhoYYMoBDbuZSQRpn+HQzByA=EwfMDYwCk1Q@mail.gmail.com

[2] https://www.postgresql.org/message-id/20170118052356.GA5952@gust

- Heikki

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:
>>> sasl    scram_sha256
>>> sasl    scram_sha256,channel
>>> sasl    scram_sha512
>>> sasl    scram_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  DATABASE        USER            ADDRESS                 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?
>
> 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

On 7 April 2017 at 15:59, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Fri, Apr 7, 2017 at 9:59 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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:
sasl    scram_sha256
sasl    scram_sha256,channel
sasl    scram_sha512
sasl    scram_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

On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On Fri, Apr 7, 2017 at 6:32 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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

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:
> >sasl    scram_sha256
> >sasl    scram_sha256,channel
> >sasl    scram_sha512
> >sasl    scram_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  DATABASE        USER            ADDRESS                 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
unlesssomeone implements an  ability to name multiple methods per HBA line, you must choose exactly one  SASL method.
Theconcrete work for v10 would be merely renaming "scram" to  "scram-sha-256".
 

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

I had been favoring (2), but I'm starting to like (1) more.  (2) assumes all
SASL methods or all SCRAM methods have something in common with each other and
not with other methods, and that's not so.  For example, one might implement
simultaneous md5 and scram-sha-256.  (1) is harder to implement, but we can
defer that work indefinitely.

Thanks,
nm

On Sat, Apr 8, 2017 at 9:28 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 6:32 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sat, Apr 8, 2017 at 1:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Fri, Apr 7, 2017 at 3:59 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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

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

On 10 April 2017 at 12:34, Michael Paquier <michael.paquier@gmail.com> 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

On 10 April 2017 at 13:57, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 10 April 2017 at 12:34, Michael Paquier <michael.paquier@gmail.com> 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

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:
>>> sasl    scram_sha256
>>> sasl    scram_sha256,channel
>>> sasl    scram_sha512
>>> sasl    scram_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  DATABASE        USER            ADDRESS                 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 exactly one
>    SASL method.  The concrete work for v10 would be merely renaming "scram" to
>    "scram-sha-256".
>
> 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.
>
> I had been favoring (2), but I'm starting to like (1) more.  (2) assumes all
> SASL methods or all SCRAM methods have something in common with each other and
> not with other methods, and that's not so.  For example, one might implement
> simultaneous md5 and scram-sha-256.  (1) is harder to implement, but we can
> defer that work indefinitely.

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.

Given that, we don't necessarily even need to support a list of methods, 
if we always implicitly also allow anything stronger than what's 
specified. At least until someone implements something completely 
different like OAUTHBEARER, that's not clearly stronger or weaker than 
the other methods.

- Heikki

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

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

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

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  <bruce@momjian.us>        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 +

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

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

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

On 16 April 2017 07:14:21 EEST, Noah Misch <noah@leadboat.com> 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

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-a5ba3d03d881@iki.fi

- Heikki

On Tue, Apr 18, 2017 at 7:58 AM, Magnus Hagander <magnus@hagander.net> 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

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