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.
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.
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 (
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
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
#define SNONCE_ENTROPY_BYTES 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
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.
As Jeff Janes pointed out at , 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.
Álvaro Hernández Tortosa