Negotiating the SCRAM channel binding type

On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:

Currently, there is no negotiation of the channel binding type between
client and server. The server advertises that it supports channel binding,
or not, and the client decides what channel binding to use. If the server
doesn't support the binding type that the client chose, authentication will
fail.

Based on recent discussions, it looks like there's going to be differences
in this area [1]. OpenSSL can support both tls-unique and
tls-server-end-point. Java only supports tls-server-end-point, while GnuTLS
only supports tls-unique. And Mac OS Secure Transports supports neither one.
Furthermore, it's not clear how TLS v1.3 affects this. tls-unique might no
longer be available in TLS v1.3, but we might get new channel binding types
to replace it. So this is about to get really messy, if there is no way to
negotiate. (Yes, it's going to be messy even with negotiation.)

I think we must fix that before we release v11, because this affects the
protocol. There needs to be a way for the server to advertise what channel
binding types it supports.

Yes, this does make sense. From a security perspective, it doesn't
matter which channel binding method we use, assuming there are no
unfixed exploits. The difference between the channel binding methods is
explained in our PG 11 docs:

https://www.postgresql.org/docs/11/static/sasl-authentication.html#SASL-SCRAM-SHA-256

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:

as the supported mechanisms, we change that into:

SCRAM-SHA-256-PLUS tls-unique
SCRAM-SHA-256-PLUS tls-server-end-point
SCRAM-SHA-256

Can we say for sure that there won't be other options associated to a
given SASL mechanism? It seems to me that something like the following
is better long-term, with channel binding available as a comma-separated
list of items:

SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point
SCRAM-SHA-256

Thoughts? Unfortunately this breaks compatibility with current v11beta
clients, but IMHO we must fix this. If we want to alleviate that, and save a
few bytes of network traffic, we could decide that plain
"SCRAM-SHA-256-PLUS" implies tls-unique, so the list would be:

SCRAM-SHA-256-PLUS
SCRAM-SHA-256-PLUS tls-server-end-point
SCRAM-SHA-256

That would be mostly compatible with current libpq clients.

I am not sure it is worth caring about the compatibility at a beta2
stage, as v11 is not released yet. Still I agree that not specifying
anything should mean the default, which is per the RFCs currently
existing tls-unique.

Another piece of the puzzle is here as well:
/messages/by-id/20180122072936.GB1772@paquier.xyz
We need to allow a given SSL implementation to specify what the list of
channel bindings it supports is.
--
Michael

On 11/07/18 14:37, Michael Paquier wrote:

On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:

as the supported mechanisms, we change that into:

SCRAM-SHA-256-PLUS tls-unique
SCRAM-SHA-256-PLUS tls-server-end-point
SCRAM-SHA-256

Can we say for sure that there won't be other options associated to a
given SASL mechanism? It seems to me that something like the following
is better long-term, with channel binding available as a comma-separated
list of items:

SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point
SCRAM-SHA-256

That would be more complicated to parse. Yeah, we might need further
options for some SASL mechanisms in the future, but we can cross that
bridge when we get there. I don't see any need to complicate this case
for that.

I started digging into this more closely, and ran into a little problem.
If channel binding is not used, the client sends a flag to the server to
indicate if it's because the client doesn't support channel binding, or
because it thought that the server doesn't support it. This is the
gs2-cbind-flag. How should the flag be set, if the server supports
channel binding type A, while client supports only type B? The purpose
of the flag is to detect downgrade attacks, where a man-in-the-middle
removes the PLUS variants from the list of supported mechanisms. If we
treat incompatible channel binding types as "client doesn't support
channel binding", then the downgrade attack becomes possible (the
attacker can replace the legit PLUS variants with bogus channel binding
types that the client surely doesn't support). If we treat it as "server
doesn't support channel binding", then we won't downgrade to the
non-channel-binding variant, in the legitimate case that the client and
server both support channel binding, but with incompatible types.

What we'd really want, is to include the full list of server's supported
mechanisms as part of the exchange, not just a boolean "y/n" flag. But
that's not what the spec says :-(.

I guess we should err on the side of caution, and fail the
authentication in that case. That's unfortunate, but it's better than
not negotiating at all. At least with the negotiation, the
authentication will work if there is any mutually supported channel
binding type.

- Heikki

On 11/07/18 12:27, Heikki Linnakangas wrote:

Based on recent discussions, it looks like there's going to be
differences in this area [1]. OpenSSL can support both tls-unique and
tls-server-end-point. Java only supports tls-server-end-point, while
GnuTLS only supports tls-unique. And Mac OS Secure Transports supports
neither one. Furthermore, it's not clear how TLS v1.3 affects this.
tls-unique might no longer be available in TLS v1.3, but we might get
new channel binding types to replace it. So this is about to get really
messy, if there is no way to negotiate. (Yes, it's going to be messy
even with negotiation.)

I've been reading up on the discussions on GnuTLS and Secure Transport,
as well as the specs for tls-server-end-point.

In a nutshell, to get the token for tls-server-end-point, you need to
get the peer's certificate from the TLS library, in raw DER format, and
calculate a hash over it. The hash algorithm depends on the
signatureAlgorithm in the certificate, so you need to parse the
certificate to extract that. We don't want to re-implement X509 parsing,
so realistically we need the TLS library to have support functions for that.

Looking at the GnuTLS docs, I believe it has everything we need.
gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be
used to get the certificate, and
gnutls_x509_crt_get_signature_algorithm() gets the signatureAlgorithm.

The macOS Secure Transport documentation is a bit harder to understand,
but I think it has everything we need as well.
SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData()
functions get you the certificate in DER format. You can get the
signature algorithm with SecCertificateCopyValues(), with the right
constants.

Am I missing something? I think we can support tls-server-end-point with
all TLS implementations we might care about.

- Heikki

On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote:

In a nutshell, to get the token for tls-server-end-point, you need to get
the peer's certificate from the TLS library, in raw DER format, and
calculate a hash over it. The hash algorithm depends on the
signatureAlgorithm in the certificate, so you need to parse the certificate
to extract that. We don't want to re-implement X509 parsing, so
realistically we need the TLS library to have support functions for that.

Looking at the GnuTLS docs, I believe it has everything we need.
gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used
to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets
the signatureAlgorithm.

The macOS Secure Transport documentation is a bit harder to understand, but
I think it has everything we need as well.
SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData()
functions get you the certificate in DER format. You can get the signature
algorithm with SecCertificateCopyValues(), with the right constants.

Am I missing something? I think we can support tls-server-end-point with all
TLS implementations we might care about.

That seems right to me.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:

That would be more complicated to parse. Yeah, we might need further options
for some SASL mechanisms in the future, but we can cross that bridge when we
get there. I don't see any need to complicate this case for that.

Okay, fine for me.

I started digging into this more closely, and ran into a little problem. If
channel binding is not used, the client sends a flag to the server to
indicate if it's because the client doesn't support channel binding, or
because it thought that the server doesn't support it. This is the
gs2-cbind-flag. How should the flag be set, if the server supports channel
binding type A, while client supports only type B? The purpose of the flag
is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
variants from the list of supported mechanisms. If we treat incompatible
channel binding types as "client doesn't support channel binding", then the
downgrade attack becomes possible (the attacker can replace the legit PLUS
variants with bogus channel binding types that the client surely doesn't
support). If we treat it as "server doesn't support channel binding", then
we won't downgrade to the non-channel-binding variant, in the legitimate
case that the client and server both support channel binding, but with
incompatible types.

What we'd really want, is to include the full list of server's supported
mechanisms as part of the exchange, not just a boolean "y/n" flag. But
that's not what the spec says :-(.

Let's not break the spec :) I understand from it that the client is in
charge of the choice, so we are rather free to choose the reaction the
client should have. In the second phase of the exchange, the client
communicates back to the server the channel binding it has decided to
choose, it is not up to the server to pick up one if the client thinks
that it can use multiple ones.

I guess we should err on the side of caution, and fail the authentication in
that case. That's unfortunate, but it's better than not negotiating at all.
At least with the negotiation, the authentication will work if there is any
mutually supported channel binding type.

I think that in this case the client should throw an error
unconditionally if it wants to use channel A but server supports only B.
It is always possible for the client to adjust the channel binding type
it wants to use by using the connection parameter scram_channel_binding,
or to recompile. If there is incompatibility between channel binding
types, it would actually mean that the client and the server are not
compiled with the same SSL implementation, would that actually work? Say
a server has only tls-unique with gnu's library and the client uses JDBC
which only has tls-server-end-point..

So, to keep things simple, it seems to me that we should just make the
server send the list, and then check at client-level if the list sent by
server includes what the client wants to use, complaining if the option
is not present.
--
Michael

On 12/07/18 07:14, Michael Paquier wrote:

On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:

I started digging into this more closely, and ran into a little problem. If
channel binding is not used, the client sends a flag to the server to
indicate if it's because the client doesn't support channel binding, or
because it thought that the server doesn't support it. This is the
gs2-cbind-flag. How should the flag be set, if the server supports channel
binding type A, while client supports only type B? The purpose of the flag
is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
variants from the list of supported mechanisms. If we treat incompatible
channel binding types as "client doesn't support channel binding", then the
downgrade attack becomes possible (the attacker can replace the legit PLUS
variants with bogus channel binding types that the client surely doesn't
support). If we treat it as "server doesn't support channel binding", then
we won't downgrade to the non-channel-binding variant, in the legitimate
case that the client and server both support channel binding, but with
incompatible types.

What we'd really want, is to include the full list of server's supported
mechanisms as part of the exchange, not just a boolean "y/n" flag. But
that's not what the spec says :-(.

Let's not break the spec :) I understand from it that the client is in
charge of the choice, so we are rather free to choose the reaction the
client should have. In the second phase of the exchange, the client
communicates back to the server the channel binding it has decided to
choose, it is not up to the server to pick up one if the client thinks
that it can use multiple ones.

The case where the client and the server both support the same channel
binding type, we're OK. The problematic case is when e.g. the server
only supports tls-unique and the client only supports
tls-server-end-point. What we would (usually) like to happen, is to fall
back to not using channel binding. But it's not clear how to make that
work, and still protect from downgrade attacks. If the client responds
"y", meaning "the client supports channel binding, but it looks like the
server doesn't", then the server will reject the authentication, because
it did actually support channel binding. Just not the same one that the
client did. If the client could reply "y", and also echo back the
server's list of supported channel bindings in the same message, that
would solve the problem. But the spec doesn't do that.

I guess we should err on the side of caution, and fail the authentication in
that case. That's unfortunate, but it's better than not negotiating at all.
At least with the negotiation, the authentication will work if there is any
mutually supported channel binding type.

I think that in this case the client should throw an error
unconditionally if it wants to use channel A but server supports only B.
It is always possible for the client to adjust the channel binding type
it wants to use by using the connection parameter scram_channel_binding,
or to recompile. If there is incompatibility between channel binding
types, it would actually mean that the client and the server are not
compiled with the same SSL implementation, would that actually work? Say
a server has only tls-unique with gnu's library and the client uses JDBC
which only has tls-server-end-point..

We would like to fall back to not using channel binding at all in that
scenario, assuming the configuration doesn't require channel binding.
But yeah, rejecting the connection seems to be the best we can do, given
what the protocol is.

So, to keep things simple, it seems to me that we should just make the
server send the list, and then check at client-level if the list sent by
server includes what the client wants to use, complaining if the option
is not present.

Yep.

It seems that all implementations can support tls-server-end-point,
after all, so I'm not too worried about this anymore. The spec says that
it's the default, but I don't actually see any advantage to using it
over tls-server-end-point. I think the main reason for tls-unique to
exist is that it doesn't require the server to have a TLS certificate,
but PostgreSQL requires that anyway.

Actually, I wonder if we should just rip out the tls-unique support,
after all? We can add it back at a later version, if there is a need for
it, but I don't think we will. With security-related code, simpler is
better.

- Heikki

On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:

It seems that all implementations can support tls-server-end-point, after
all, so I'm not too worried about this anymore. The spec says that it's the
default, but I don't actually see any advantage to using it over
tls-server-end-point. I think the main reason for tls-unique to exist is
that it doesn't require the server to have a TLS certificate, but PostgreSQL
requires that anyway.

Er. My memories about the spec are a bit different: tls-unique must be
implemented and is the default.

[ ... digging ... ]

Here you go:
https://tools.ietf.org/html/rfc5802#section-6.1
--
Michael

On 12/07/18 12:06, Michael Paquier wrote:

On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:

It seems that all implementations can support tls-server-end-point, after
all, so I'm not too worried about this anymore. The spec says that it's the
default, but I don't actually see any advantage to using it over
tls-server-end-point. I think the main reason for tls-unique to exist is
that it doesn't require the server to have a TLS certificate, but PostgreSQL
requires that anyway.

Er. My memories about the spec are a bit different: tls-unique must be
implemented and is the default.

[ ... digging ... ]

Here you go:
https://tools.ietf.org/html/rfc5802#section-6.1

Meh. We're not going implement tls-unique, anyway, in some of the
upcoming non-OpenSSL TLS implementations that don't support it.

Hmm. That is actually in a section called "Default Channel Binding". And
the first paragraph says "A default channel binding type agreement
process for all SASL application protocols that do not provide their own
channel binding type agreement is provided as follows". Given that, it's
not entirely clear to me if the requirement to support tls-unique is for
all implementations of SCRAM, or only those applications that don't
provide their own channel binding type agreement.

- Heikki

On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote:

Meh. We're not going implement tls-unique, anyway, in some of the upcoming
non-OpenSSL TLS implementations that don't support it.

True enough. Only GnuTLS supports it:
https://www.gnutls.org/manual/html_node/Channel-Bindings.html

Hmm. That is actually in a section called "Default Channel Binding". And the
first paragraph says "A default channel binding type agreement process for
all SASL application protocols that do not provide their own channel binding
type agreement is provided as follows". Given that, it's not entirely clear
to me if the requirement to support tls-unique is for all implementations of
SCRAM, or only those applications that don't provide their own channel
binding type agreement.

I am not sure, but I get that as tls-unique must be the default if
available, so if it is technically possible to have it we should have it
in priority. If not, then a channel binding type which is supported by
both the server and the client can be chosen.
--
Michael

On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote:

Looking at the GnuTLS docs, I believe it has everything we need.
gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used
to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets
the signatureAlgorithm.

Looking at the docs, there is gnutls_x509_crt_get_fingerprint() which
can provide the certificate hash. So if the signature algorithm is MD5
or SHA-1, it would be simple enough to upgrade it to SHA-256 and
calculate the hash. They have way better docs than OpenSSL, which is
nice.
--
Michael

On 12/07/18 16:08, Michael Paquier wrote:

On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote:

Hmm. That is actually in a section called "Default Channel Binding". And the
first paragraph says "A default channel binding type agreement process for
all SASL application protocols that do not provide their own channel binding
type agreement is provided as follows". Given that, it's not entirely clear
to me if the requirement to support tls-unique is for all implementations of
SCRAM, or only those applications that don't provide their own channel
binding type agreement.

I am not sure, but I get that as tls-unique must be the default if
available, so if it is technically possible to have it we should have it
in priority. If not, then a channel binding type which is supported by
both the server and the client can be chosen.

Another interesting piece of legalese is in RFC 5929 Channel Bindings
for TLS:

For many applications, there may be two or more potentially
applicable TLS channel binding types. Existing security frameworks
(such as the GSS-API [RFC2743] or the SASL [RFC4422] GS2 framework
[RFC5801]) and security mechanisms generally do not support
negotiation of channel binding types. Therefore, application peers
need to agree a priori as to what channel binding type to use (or
agree to rules for deciding what channel binding type to use).

The specifics of whether and how to negotiate channel binding types
are beyond the scope of this document. However, it is RECOMMENDED
that application protocols making use of TLS channel bindings, use
'tls-unique' exclusively, except, perhaps, where server-side proxies
are common in deployments of an application protocol. In the latter
case an application protocol MAY specify that 'tls-server-end-point'
channel bindings must be used when available, with 'tls-unique' being
used when 'tls-server-end-point' channel bindings are not available.
Alternatively, the application may negotiate which channel binding
type to use, or may make the choice of channel binding type
configurable.

In any case, I'm quite convinced now that we should just remove
tls-unique, and stick to tls-server-end-point. Patch attached, please
take a look.

- Heikki

On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote:

In any case, I'm quite convinced now that we should just remove tls-unique,
and stick to tls-server-end-point. Patch attached, please take a look.

You are making good points here.

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.

-   "SCRAM authentication with invalid channel binding");
+   "Basic SCRAM authentication");
Or "SCRAM authentication with SSL and channel binding?"

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

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

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

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

-    * is advertised in decreasing order of importance.  So the
-    * channel-binding variants go first, if they are supported. Channel
-    * binding is only supported in SSL builds.
+    * is advertised in decreasing order of importance.
This comment is a duplicate of what is with pg_be_scram_get_mechanisms.

+#ifdef HAVE_X509_GET_SIGNATURE_NID
char *
pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
{
-#ifdef HAVE_X509_GET_SIGNATURE_NID
I would have kept the ifdef at its original place, to keep the
OpenSSL-specific CFLAGS [be|fe]-secure-openssl.c.

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.

+ * The username to was provided by the client in the startup message, and is
Nit: two spaces between "to" and "was".

+ errdetail("The client selected SCRAM-SHA-256 without channel
binding, but the SCRAM message includes channel binding data,")));
Why a comma at the end?

+       /*
+        * Chose channel binding, but the SSL library doesn't support it.
+        * Shouldn't happen.
+        */
+       termPQExpBuffer(&buf);
+       return NULL;
An error message is misisng here?
--
Michael

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

On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote:

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.

Two things that I am really unhappy about is first that you completely
wiped out the test suite for channel binding. We know that channel
binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why
didn't you keep the check on supports_tls_server_end_point to determine
if the connection should be a failure or a success?

Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and
the other flags over-complicated, but I won't fight hard on that point
if you want to go your way.

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.

This I definitely agree with, getting this patch in before beta 3 is the
best thing to do now.
--
Michael

On 05/08/18 15:08, Michael Paquier wrote:

On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote:

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.

Two things that I am really unhappy about is first that you completely
wiped out the test suite for channel binding. We know that channel
binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why
didn't you keep the check on supports_tls_server_end_point to determine
if the connection should be a failure or a success?

That test just tested that the scram_channel_binding libpq option works,
but I removed the option. I know you wanted to keep it as a feature
flag, but as discussed earlier, I don't think that'd be useful.

Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and
the other flags over-complicated, but I won't fight hard on that point
if you want to go your way.

I don't feel too strongly about this either, so if you want to write a
patch to refactor that, I'm all ears. Note that I had to do something,
so that the server code knows whether to advertise SCRAM-SHA-256-PLUS or
not, and likewise that the client knows whether to choose channel
binding or not.

- Heikki

On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote:

That test just tested that the scram_channel_binding libpq option works, but
I removed the option. I know you wanted to keep it as a feature flag, but as
discussed earlier, I don't think that'd be useful.

Sorry for the noise, I missed that there is still the test "Basic SCRAM
authentication with SSL" so that would be fine. I would have preferred
keeping around the negative test so as we don't break SSL connections
when the client enforced cbind_flag to 'n' as that would be useful when
adding new SSL implementations as that would avoid manual tests which
people will most likely forget, but well...

You can remove $supports_tls_server_end_point in 002_scram.pl by the
way. Should I remove it or perhaps you would prefer to do it?
--
Michael

On 05/08/18 15:45, Michael Paquier wrote:

On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote:

That test just tested that the scram_channel_binding libpq option works, but
I removed the option. I know you wanted to keep it as a feature flag, but as
discussed earlier, I don't think that'd be useful.

Sorry for the noise, I missed that there is still the test "Basic SCRAM
authentication with SSL" so that would be fine. I would have preferred
keeping around the negative test so as we don't break SSL connections
when the client enforced cbind_flag to 'n' as that would be useful when
adding new SSL implementations as that would avoid manual tests which
people will most likely forget, but well...

The only negative test there was, was to check for bogus
scram_channel_binding option, "scram_channel_binding=not-exists". Yeah,
it would be nice to have some, but this commit didn't really change that
situation.

I'm hoping that we add a libpq option to actually force channel binding
soon. That'll make channel binding actually useful to users, but it will
also make it easier to write tests to check that channel binding is
actually used or not used, in the right situations.

You can remove $supports_tls_server_end_point in 002_scram.pl by the
way. Should I remove it or perhaps you would prefer to do it?

Ah, good catch. I'll go and remove it, thanks!

- Heikki

On 05/08/18 17:11, I wrote:

The only negative test there was, was to check for bogus
scram_channel_binding option, "scram_channel_binding=not-exists". Yeah,
it would be nice to have some, but this commit didn't really change that
situation.

Sorry, I see now that there was indeed a test for
scram_channel_binding='', which meant "no channel binding". That was
confusing, I assumed '' was the default.

I'm hoping that we add a libpq option to actually force channel binding
soon. That'll make channel binding actually useful to users, but it will
also make it easier to write tests to check that channel binding is
actually used or not used, in the right situations.

Nevertheless, this we should do.

- Heikki