Re: [HACKERS] Some thoughts about SCRAM implementation - Mailing list pgsql-hackers

On 04/10/2017 12:39 PM, Álvaro Hernández Tortosa wrote:
> - 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.

We missed the boat for PostgreSQL 10. You're right that it probably 
wouldn't be difficult to implement, but until there's a concrete patch 
to discuss, that's a moot point.

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

Yeah, a GUC might be a good idea.

The iteration count is a part of the stored SCRAM verifier, so it's 
determined when you set the user's password. That's why it's per-user.

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

The SASL spec [RFC4422] says that mechanism negotiation is protocol 
specific. The SCRAM spec [RFC5802] prescribes how negotiation of 
channel-binding is done, and the current implementation is compliant 
with that. (Which is very straightforward when neither the client nor 
the server supports channel binding).

The negotiation of the mechanism is being discussed one the "Letting the 
client choose the protocol to use during a SASL exchange" thread. I'm 
just writing a more concrete proposal based on your suggestion of 
sending a list of SASL mechanisms in the AuthenticationSASL message. 
Stay tuned!

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

All the existing authentication mechanisms are spelled in lowercase. And 
uppercase is ugly. It's a matter of taste, for sure.

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

Yeah, it might be handy for something like pgbouncer, which could then 
have one userid+password to authenticate, but then switch to another 
user. But the SCRAM part of that seems to be just a small part of the 
overall feature. In any case, this is clearly Postgres 11 material.

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

Wouldn't hurt, I guess. IIRC I checked some other implementations, when 
I picked 10, but I don't remember which ones anymore. Got a reference 
for 24/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).

The server does send "e=invalid-proof", if the password is incorrect (or 
the user doesn't exist). All other errors are reported with 
ereport(ERROR). That is allowed by the spec:

>    o  e: This attribute specifies an error that occurred during
>       authentication exchange.  It is sent by the server in its final
>       message and can help diagnose the reason for the authentication
>       exchange failure.  On failed authentication, the entire server-
>       final-message is OPTIONAL; specifically, a server implementation
>       MAY conclude the SASL exchange with a failure without sending the
>       server-final-message.  This results in an application-level error
>       response without an extra round-trip.  If the server-final-message
>       is sent on authentication failure, then the "e" attribute MUST be
>       included.

It's a bit awkward to use the spec's "e=xxx" mechanism for other errors, 
because the "e=xxx" error code can only be sent in the 
server-final-message. If an error is detected before that stage, the 
server would need to continue the authentication to the end, until it 
could report it. That's why.

As Jeff Janes pointed out at [1], reporting even the invalid password 
case with "e=invalid-proof" actually leads to a different and less 
user-friendly message with psql. So we may still change that too.

[1] 
https://www.postgresql.org/message-id/CAMkU%3D1w3jQ53M1OeNfN8Cxd9O%2BA_9VONJivTbYoYRRdRsLT6vA%40mail.gmail.com

- Heikki