Some thoughts about SCRAM implementation

Hi!

There's some ongoing discussion about SCRAM (like this thread
https://www.postgresql.org/message-id/243d8c11-6149-a4bb-0909-136992f74b23%40iki.fi)
but I wanted to open a new thread that covers these topics and other,
more general ones. Here are some thoughts based on my perspective on
SCRAM, which I gained thanks to studying it as part of the effort to
implement SCRAM in the JDBC driver. FYI, some very early code can be
found here: https://github.com/ahachete/scram. Although there's still a
lot to do, I'm doing great progress and I expect to have a working
version within 2 weeks.

So here's what I think:

- Work so far is great. Thanks Heikki, Michael and others for the effort
involved. I love to have SCRAM support in Postgres!

- I think channel binding support should be added. SCRAM brings security
improvements over md5 and other simpler digest algorithms. But where it
really shines is together with channel binding. This is the only method
to prevent MITM attacks. Implementing it should not very difficult.
There are several possible channel binding mechanisms, but the mandatory
and probably more typical one is 'tls-unique' which basically means
getting the byte array from the TLSfinish() message and comparing it
with the same data sent by the client. That's more or less all it takes
to implement it. So I would go for it.

- One SCRAM parameter, i, the iteration count, is important, and in my
opinion should be configurable. AFAIK it is currently hard coded at
4096, which is the minimum value accepted by the RFC. But it should be
at least 15K (RFC says), and given that it affects computing time of the
authentication, it should be configurable. It's also now specified per
user, which I think its too much granularity (it should suffice to say
this group of users all require this iteration count).

- The SCRAM RFC states that server should advertise supported
mechanisms. I see discussion going into not advertising them. I think it
should be done, I don't see reasons not to do it, and it will become
compliant with the RFC.

- I don't see why proposed scram mechanism names for pg_hba.conf are not
following the IANA Registry standard (
https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram),
which is uppercase (like in SCRAM-SHA-256).

- SCRAM also supports the concept of authzid, which is kind of what
pg_ident does: authenticate the user as the user sent, but login as the
user specified here. It could also be supported.

Based on these comments, I'd like to propose the following changes:

* Implement channel binding.

* Add a GUC for the channel binding technique to use. It could be named
as "scram_channel_binding_name" or "sasl_channel_binding_name", for
example. And probably supported value as of today, 'tls-unique'.

* AuthenticationSASL backend message:
- The String field make it a CSV that would be a list of one or
more supported IANA SCRAM authentication methods. This info comes from
pg_scram.conf (see below).
- Add another String to specify the variety of channel binding
supported (if any), like 'tls-unique'. This info comes from the GUC.

* Treat the configuration of scram in pg_hba.conf, which seems
complicated, similarly to how pg_ident.conf is treated. Basically in
pg_hba.conf specify a "scram=scram_entry_name" value and then look for a
pg_scram.conf file for an entry with that name. The pg_scram.conf might
have the following structure:

name | scram_mechanisms | i
scramlight | SCRAM-SHA-1 | 4096
scramstrong | SCRAM-SHA-256,SCRAM-SHA-256-PLUS | 16384

* The nonce length is not specified by the RFC. I see typical
implementations use 24 chars for the client and 18 for the server.
Current code uses 10. I think it should not hurt making it at least 16
or 18.

* It seems to me that the errors defined by the RFC, sent on the
server-final-message if there were errors, are never sent with the
current implementation. I think they should be sent as per the standard,
and also proceed until the last stage even if errors were detected
earlier (to conform with the RFC).

Thoughts?

Álvaro

--

Álvaro Hernández Tortosa

-----------
<8K>data