SCRAM authentication, take three

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

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

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

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

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

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

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

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

--
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/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

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

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

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

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

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

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

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

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

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

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

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