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
