Re: [JDBC] Channel binding support for SCRAM-SHA-256

On Tue, Sep 26, 2017 at 11:09 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>> I think the channel-binding negotiation on the client side is wrong.
>> The logic in the patch is
>>
>> +#ifdef USE_SSL
>> + if (state->ssl_in_use)
>> + appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
>> + else
>> + appendPQExpBuffer(&buf, "y");
>> +#else
>> + appendPQExpBuffer(&buf, "n");
>> +#endif
>>
>> But if SSL is compiled in but not used, the client does not in fact
>> support channel binding (for that connection), so it should send "n".
>
> For others, details about this flag are here:
> gs2-cbind-flag = ("p=" cb-name) / "n" / "y"
> ;; "n" -> client doesn't support channel binding.
> ;; "y" -> client does support channel binding
> ;; but thinks the server does not.
> ;; "p" -> client requires channel binding.
> ;; The selected channel binding follows "p=".
>
> And channel binding description is here:
> https://tools.ietf.org/html/rfc5802#section-6

Changed the patch to do that. Note that if the client enforces the
SASL mechanism to SCRAM-SHA-256-PLUS in a non-SSL context then libpq
complains. This can be done in libpq using pgsaslname.

>> The "y" flag should be sent if ssl_in_use but the client did not see the
>> server advertise SCRAM-SHA256-PLUS. That logic is missing entirely in
>> this patch.
>
> Okay. I think I get your point here. I agree that the client is
> deficient here. This needs some more work.

Hm. I take back the argument that we can use the backend version here.
conn->sversion is only filled at the end of authentication. So the
best thing I think can be done here is to check if channel binding has
been advertised or not, and send "y" if the client thinks that the
server should do support it. If the client has chosen not to use
channel binding, by for example enforcing pgsaslname in libpq to
SCRAM-SHA-256, then "n" should be sent. I have changed the patch
accordingly, doing at the same time some refactoring in pg_SASL_init.
pg_fe_scram_init() gets initialized only when the mechanism is
selected.

>> You have the server reject a client that does not support channel
>> binding ("n") on all SSL connections. I don't think this is correct.
>> It is up to the client to use channel binding or not, even on SSL
>> connections.
>
> It seems that I got confused with the meaning of "y" mixed with
> ssl_in_use. The server should reject "y" instead only if SSL is in
> use.

Okay. In order to address that, what just needs to be done is to
remove the error message that I have added in the backend when "n" is
sent by the client. So this thing is ripped off. This way, when a v10
libpq connects to a v11 backend, the connection is able to work even
with SSL connections.

>> We should update pg_hba.conf to allow a method specification of
>> "scram-sha256-plus", i.e., only advertise the channel binding variant to
>> the client. Then you could make policy decisions like rejecting clients
>> that do not use channel binding on SSL connections. This could be a
>> separate patch later.
>
> OK, I agree that there could be some value in that. This complicates a
> bit hba rule checks, but nothing really complicated either.

This would be useful, but I have let it for now to not complicate the
series more.

>> The error message in the "p" case if SSL is not in use is a bit
>> confusing: "but the server does not need it". I think this could be
>> left at the old message "but it is not supported". This ties into my
>> interpretation from above that whether channel binding is "supported"
>> depends on whether SSL is in use for a particular connection.
>
> Check.

Okay, I switched the error message to that (diff with previous patch series):
@@ -850,7 +850,7 @@ read_client_first_message(scram_state *state, char *input)
if (!state->ssl_in_use)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg("client supports SCRAM channel
binding, but server does not need it for non-SSL connections")));
+ errmsg("client requires SCRAM channel
binding, but it is not supported")));

>> Some small code things:
>> - prefer to use size_t over int for length (tls_finish_len etc.)
>> - tls_finish should be tls_finished
>> - typos: certificate_bash -> certificate_hash
>
> Yes, thanks for spotting those.

Fixed.

>> In the patch for tls-server-end-point, I think the selection of the hash
>> function deviates slightly from the RFC. The RFC only says to
>> substitute MD5 and SHA-1. It doesn't say to turn SHA-224 into SHA-256,
>> for example. There is also the problem that the code as written will
>> turn any unrecognized hash method into SHA-256. If think the code
>> should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
>> for the rest. (I don't know anything about the details of OpenSSL APIs,
>> but that function sounded right to me.)
>
> Relevant bits from the RFC: https://tools.ietf.org/html/rfc5929#section-4.1
> o if the certificate's signatureAlgorithm uses a single hash
> function, and that hash function is either MD5 [RFC1321] or SHA-1
> [RFC3174], then use SHA-256 [FIPS-180-3];
> o if the certificate's signatureAlgorithm uses no hash functions or
> uses multiple hash functions, then this channel binding type's
> channel bindings are undefined at this time (updates to is channel
> binding type may occur to address this issue if it ever arises).
>
> OK. Hm. I think that we need as well to check for the case where
> EVP_get_digestbynid() returns NULL then and issue an ERROR on it. That
> seems to be the second point outlined by the RFC I am quoting here.

This has been fixed. i have noticed on the way that the error messages
in the frontend when a certificate hash cannot be generated just
complain about an OOM. This is not always correct, so I have made
things more verbose with better error messages for each error code
path.

> This patch got the feedback I was looking for, and this requires some
> rework anyway. So I am marking the patch as returned with feedback for
> now. This won't make it in time for this CF.

Attached is a new patch set with the comments from above. On top of
that, I have changed a couple of things:
- 0001 is unchanged, still the same refactoring for the SSL tests.
- 0002 implements tls-unique, now including tests using the default
channel binding tls-unique with something in the SSL test suite. This
patch also now introduces all the infrastructure to plug in correctly
new libpq parameters and more channel binding types.
- 0003 is shorter, and introduces a set of libpq parameters useful for
tests, taking advantage of 0002. Another case where the connection
parameter saslname is useful is to enforce not using channel binding
when connecting to a v10 server using a SSL context with a v11 libpq.
- 0004 introduces tls-server-end-point.
This has required some work to get it shaped as wanted, I am adding it
to the next CF, as version 2.
--
Michael