Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256 - Mailing list pgsql-hackers

On Sat, Nov 18, 2017 at 4:31 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.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