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

On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> Here is a review of the meat of the code, leaving aside the discussion
> of the libpq connection parameters.
>
> Overall, the structure of the code makes sense and it fits in well with
> the existing SCRAM code.

Thanks for the review, Peter.

> 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

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

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

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

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

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

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

Thanks Peter for looking at it and pointing out some deficiencies.
--
Michael