Re: Some thoughts about SCRAM implementation

On 10/04/17 13:02, Heikki Linnakangas wrote:
> 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