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

On Sat, Nov 18, 2017 at 4:31 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> I made some significant changes to the logic.

Thanks!

> The selection of the channel binding flag (n/y/p) in the client seemed
> wrong. Your code treated 'y' as an error, but I think that is a
> legitimate case, for example a PG11 client connecting to a PG10 server
> over SSL. The client supports channel binding in that case and
> (correctly) thinks the server does not, so we use the 'y' flag and
> proceed normally without channel binding.
>
> The selection of the mechanism in the client was similarly incorrect, I
> think. The code would not tolerate a situation were an SSL connection
> is in use but the server does not advertise the -PLUS mechanism, which
> again could be from a PG10 server.

Hm, OK. I have been likely confused by the fact that eSws is a valid
b64-encoded cbind-input on v10 servers. And the spec has no direct
mention of the matter, only of biws.

> The creation of the channel binding data didn't match the spec, because
> the gs2-header (p=type,,) was not included in the data put through
> base64. This was done incorrectly on both server and client, so the
> protocol still worked. (However, in the non-channel-binding case we
> hardcode "biws", which is exactly the base64-encoding of the gs2-header.
> So that was inconsistent.)

Meh-to-self, you are right. Still it seems to me that your version is
forgetting something.. Please see below.

> I think we also need to backpatch a bug fix into PG10 so that the server
> can accept base64("y,,") as channel binding data. Otherwise, the above
> scenario of a PG11 client connecting to a PG10 server over SSL will
> currently fail because the server will not accept the channel binding data.

Yes, without the additional comparison, the failure is weird for the
user. Here is what I have with an unpatched v10 server:
psql: FATAL: unexpected SCRAM channel-binding attribute in client-final-message
This is going to need a one-liner in read_client_final_message()'s auth-scram.c.

> Please check my patch and think through these changes. I'm happy to
> commit the patch as is if there are no additional insights.

Here are some comments.

+ * The client requires channel biding. Channel binding type
s/biding/binding/.

if (!state->ssl_in_use)
+ /*
+ * Without SSL, we don't support channel binding.
+ *
Better to add brackets if adding a comment.

+ * Read value provided by client; only tls-unique is supported
+ * for now. XXX Not sure whether it would be safe to print
+ * the name of an unsupported binding type in the error
+ * message. Pranksters could print arbitrary strings into the
+ * log that way.
That's why I didn't show those strings in the error messages of the
previous versions. Those are useless as well, except for debugging the
feature and the protocol.

+ cbind_header_len = 4 + strlen(state->channel_binding_type); /*
p=type,, */
+ cbind_input_len = cbind_header_len + cbind_data_len;
+ cbind_input = malloc(cbind_input_len);
+ if (!cbind_input)
+ goto oom_error;
+ snprintf(cbind_input, cbind_input_len, "p=%s",
state->channel_binding_type);
+ memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
By looking at RFC5802, a base64 encoding of cbind-input is used:
cbind-input = gs2-header [ cbind-data ]
gs2-cbind-flag "," [ authzid ] ","
However you are missing two commands after p=%s, no?
--
Michael