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.

    Really? That's a real shame.... I know we're very late in the CF cycle but, again, this would be a real shame.

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

    Cool.

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.

    Sure. But I think per-user is too much granularity. And if it is not, it should be a parameter exposed to the CREATE USER command, such that it can be (effectively) set per-user. I maintain the best approach could be the suggested pg_scram.conf with the format (or a similar one) that I proposed in the OP.

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

    Yeah. But what I'm saying is that we might want to add an extra String to AuthenticationSASL message for channel binding, so that the message format needs not to be changed when channel binding is added. But the channel binding method name needs to be negotiated, and so there needs to be room for it.

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!

    Yepp, I will reply there, thanks :)

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

    And I agree with you. It's ugly. But it's standard. I'd say let's favor standardization vs our own taste.

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

    Again, it should be a tiny change, just an extra attribute sent over the message. But I guess time was over... just saying... ;)

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

    First reference is the RFC example itself (non-mandatory, of course). But then I saw many followed this. As a quick example, GNU SASL defines:

#define SNONCE_ENTROPY_BYTES 18
https://www.gnu.org/software/gsasl/coverage/lib/scram/server.c.gcov.frameset.html

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

    OK, agreed. Now... what happens if the client still sends a final message if the server returned error with the server-first-message? I don't really care, but at least we should think of how to react to that.

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.

    Makes sense.

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

    Thanks,

    Álvaro

-- 

Álvaro Hernández Tortosa


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