Re: Negotiating the SCRAM channel binding type

Thanks for the review!

On 22/07/18 16:54, Michael Paquier wrote:
> On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote:
>> This removes the scram_channel_binding libpq option altogether, since there
>> is now only one supported channel binding type.
>
> This can also be used to switch off channel binding on the client-side,
> which is the behavior you could get only by using a version of libpq
> compiled with v10 in the context of an SSL connection. Do we really
> want to lose this switch as well? I like feature switches.

Well, it'd be useless for users, there is no reason to switch off
channel binding if both the client and server support it. It might not
add any security you care about, but it won't do any harm either. The
non-channel-binding codepath is still exercised with non-SSL connections.

> +PostgreSQL is <literal>tls-server-end-point</literal>. If other channel
> +binding types are supported in the future, the server will advertise
> +them as separate SASL mechanisms.
> I don't think that this is actually true, per my remark of upthread we
> could also use an option-based approach with each SASL mechanism sent by
> the server. I would recommend to remove this last sentence.

Ok. That's how I'm envisioning we'd add future binding types, but since
we're not sure, I'll leave it out.

> + man-in-the-middle attacks by mixing the signature of the server's
> + certificate into the transmitted password hash. While a fake server can
> + retransmit the real server's certificate, it doesn't have access to the
> + private key matching that certificate, and therefore cannot prove it is
> + the owner, causing SSL connection failure
> Nit: insisting on the fact that tls-server-end-point is used in this
> context.

Yeah, that's the assumption. Now that we only do tls-server-end-point, I
think we can assume that without further explanation.

> +void
> +pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
> +{
> + /*
> + * Advertise the mechanisms in decreasing order of importance. So the
> + * channel-binding variants go first, if they are supported. Channel
> + * binding is only supported with SSL, and only if the SSL implementation
> + * has a function to get the certificate's hash
> [...]
> +#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
> + if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
> + state->channel_binding_in_use = true;
> + else
> +#endif
> Hm. I think that this should be part of the set of APIs that each SSL
> implementation has to provide. It is not clear to me yet if using the
> flavor of SSL in Windows or macos universe will allow end-point to work,
> and this could make this proposal more complicated. And
> HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would
> recommend to remove all SSL-implementation-specific code from auth*.c
> and fe-auth*.c, keeping those in their own file. One simple way to
> address this problem would be to make each SSL implementation return a
> boolean to tell if it supports SCRAM channel binding or not, with Port*
> of PGconn* in input to be able to filter connections using SSL or not.

The idea here is that if the SSL implementation implements the required
functions, it #defines HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH (in the
client) and/or HAVE_BE_TLS_GET_CERTIFICATE_HASH (in the server). So the
code above is not implementation-specific; it doesn't know the details
of OpenSSL, it only refers to the compile-time flag which the SSL
implementation-specific code defines. The flag is part of the API that
the SSL implementation provides, it's just a compile-time flag rather
than run-time.

I'll try to clarify the comments on this.

> + if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + (errmsg("unsupported SCRAM channel-binding type
> \"%s\"",
> + sanitize_str(channel_binding_type)))));
> Let's give up on sanitize_str. I am not sure that it is a good idea to
> print in error messages server-side something that the client has sent.

Well, the point of sanitize_str is to make it safe. You're right that
it's not strictly necessary, but I think it would be helpful to print
out the channel binding type that the client attempted to use.

> And a couple of lines down the call to be_tls_get_certificate_hash in
> auth-scram.c is only protected by USE_SSL... So compilation would
> likely break once a new SSL implementation is added, and libpq-be.h gets
> uglier.

Fixed by changing "#ifdef USE_SSL" to "#ifdef
HAVE_BE_TLS_GET_CERTIFICATE_HASH".

It's true that there is some risk for libpq-be.h (and libpq-int.h) to
become ugly, if we add more SSL implementations, and if those
implementations have complicated conditions on whether they can get the
certificate hashes. In practice, I think it's going to be OK. All the
SSL implementations we've talked about - GnuTLS, macOS, Windows - do
support the functionality, so we don't need complicated #ifdefs in the
header. But we can revisit this if it gets messy.

I did some further testing with this, compiling with and without
HAVE_BE_TLS_GET_CERTIFICATE_HASH and
HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH, and fixed a few combinations that
did not work. And I fixed the other comment typos etc. that you pointed out.

I have committed this now, because I think it's important to get this
into the next beta version, and I'd like to get a full cycle on the
buildfarm before that. But if you have the chance, please have one more
look at the committed version, to make sure I didn't mess something up.

- Heikki