Handling better supported channel binding types for SSL implementations
Hi all,
Per the recent discussions around support of new SSL implementations for
Postgres, it is becoming rather clear to me that the backend needs to be
a bit smarter with the way it needs to decide if it should publish or
not SCRAM-SHA-256-PLUS in the list that the clients can use to choose a
SASL mechanism for authentication.
This has been discussed here in the MacOS SSL implementation:
/messages/by-id/20180122014637.GE22690@paquier.xyz
As well as here for GnuTLS:
/messages/by-id/CAB7nPqRJteyoyyj8E__v1D1SMoj8jpv6ZPyHuc7Md45+ED-uMA@mail.gmail.com
Attached is a patch which is an attempt to make this choice cleaner for
new SSL implementations. As we are (rightly!) calling the APIs to fetch
the channel binding data only when necessary, I think that we need an
extra API for SSL implementations to let the server decide if channel
binding mechanisms should be published or not. There could be multiple
possibilities, like making this API return only a boolean flag. However
I have made a more extensible choice by having each SSL implementation
build a list of supported channel bindings. This matters for each
implementation as:
- GnuTLS only supports tls-unique.
- OpenSSL supports both tls-unique and tls-server-end-point.
- MacOS would support none.
However there is as well the argument that this list's contents are not
directly used now, and based on what I saw from the MacOS SSL and GnuTLS
patches that would not be the case after either.
I am adding that to the next CF for consideration.
Thoughts?
--
Michael
Attachments:
betls-channel-binding-list-v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 746d7cbb8a..ecaab21e13 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -873,6 +873,7 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
int inputlen;
int result;
bool initial;
+ List *channel_bindings = NIL;
/*
* SASL auth is not supported for protocol versions before 3, because it
@@ -898,7 +899,17 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
strlen(SCRAM_SHA256_NAME) + 3);
p = sasl_mechs;
- if (port->ssl_in_use)
+#ifdef USE_SSL
+ /*
+ * Get the list of channel binding types supported by this SSL
+ * implementation to determine if server should publish -PLUS
+ * mechanisms or not.
+ */
+ channel_bindings = be_tls_list_channel_bindings();
+#endif
+
+ if (port->ssl_in_use &&
+ list_length(channel_bindings) > 0)
{
strcpy(p, SCRAM_SHA256_PLUS_NAME);
p += strlen(SCRAM_SHA256_PLUS_NAME) + 1;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index fc6e8a0a88..95511a61b3 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -58,6 +58,7 @@
#include <openssl/ec.h>
#endif
+#include "common/scram-common.h"
#include "libpq/libpq.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -1215,6 +1216,18 @@ be_tls_get_peerdn_name(Port *port, char *ptr, size_t len)
ptr[0] = '\0';
}
+/*
+ * Routine to get the list of channel binding types available in this SSL
+ * implementation. For OpenSSL, both tls-unique and tls-server-end-point
+ * are supported.
+ */
+List *
+be_tls_list_channel_bindings(void)
+{
+ return list_make2(pstrdup(SCRAM_CHANNEL_BINDING_TLS_UNIQUE),
+ pstrdup(SCRAM_CHANNEL_BINDING_TLS_END_POINT));
+}
+
/*
* Routine to get the expected TLS Finished message information from the
* client, useful for authorization when doing channel binding.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 49cb263110..3c37e800c1 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -209,6 +209,7 @@ extern bool be_tls_get_compression(Port *port);
extern void be_tls_get_version(Port *port, char *ptr, size_t len);
extern void be_tls_get_cipher(Port *port, char *ptr, size_t len);
extern void be_tls_get_peerdn_name(Port *port, char *ptr, size_t len);
+extern List *be_tls_list_channel_bindings(void);
extern char *be_tls_get_peer_finished(Port *port, size_t *len);
extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
#endif
On 22 Jan 2018, at 08:29, Michael Paquier <michael.paquier@gmail.com> wrote:
Attached is a patch which is an attempt to make this choice cleaner for
new SSL implementations. As we are (rightly!) calling the APIs to fetch
the channel binding data only when necessary, I think that we need an
extra API for SSL implementations to let the server decide if channel
binding mechanisms should be published or not. There could be multiple
possibilities, like making this API return only a boolean flag. However
I have made a more extensible choice by having each SSL implementation
build a list of supported channel bindings.
An extensible API makes more sense than on/off (or one on/off call per
binding). Perhaps a way to validate the contents of the list is required
though? Or an assertion on the contents to catch errors during testing.
Nitpicking: In src/backend/libpq/auth.c:CheckSCRAMAuth(), this comment reads a
bit strange:
+ * Get the list of channel binding types supported by this SSL
+ * implementation to determine if server should publish -PLUS
+ * mechanisms or not.Since auth.c isn’t tied to any SSL implementation, shouldn’t it be “supported
by the configured SSL implementation” or something along those lines?
cheers ./daniel
On Mon, Jan 22, 2018 at 11:07:55AM +0100, Daniel Gustafsson wrote:
An extensible API makes more sense than on/off (or one on/off call per
binding). Perhaps a way to validate the contents of the list is
required though? Or an assertion on the contents to catch errors
during testing.
Do you have something specific in mind?
Nitpicking: In src/backend/libpq/auth.c:CheckSCRAMAuth(), this comment
reads a bit strange:+ * Get the list of channel binding types supported by this SSL + * implementation to determine if server should publish -PLUS + * mechanisms or not.Since auth.c isn’t tied to any SSL implementation, shouldn’t it be
“supported by the configured SSL implementation” or something along
those lines?
Yes, your words sound better.
--
Michael
On 22 Jan 2018, at 14:05, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Jan 22, 2018 at 11:07:55AM +0100, Daniel Gustafsson wrote:
An extensible API makes more sense than on/off (or one on/off call per
binding). Perhaps a way to validate the contents of the list is
required though? Or an assertion on the contents to catch errors
during testing.Do you have something specific in mind?
Not really, but IIUC a bug in this code could lead to channel binding not being
used for a connection even if the user wanted it (and may miss that ixt didn’t
happen due to not looking at logs etc)? Considering the limited set of values
(currently) it should be quite simple to check for offending entries, if there
is indeed a risk of “silently” not using channel binding.
cheers ./daniel
On Mon, Jan 22, 2018 at 04:52:11PM +0100, Daniel Gustafsson wrote:
Not really, but IIUC a bug in this code could lead to channel binding
not being used for a connection even if the user wanted it (and may
miss that ixt didn’t happen due to not looking at logs etc)?
Possible, if an implementation decides to send a NIL list as return
result of this API when it should not.
Considering the limited set of values (currently) it should be quite
simple to check for offending entries, if there is indeed a risk of
“silently” not using channel binding.
I think I understand the point you are coming at, but a problem is that
the channel binding type the client wants to use is known by the server
in SCRAM authentication only after reading the client-first message,
which happens of course after the client decided to choose if channel
binding is used or not. Now the server needs to emit first a list of
supported SASL mechanisms which are consistent with what the SSL
implementation can do when the mechanism is negotiated. Another, third
approach that I can think of is to have this additional API in betls
emit a list of mechanisms, but I am not sure that this is worth the
complication as at the end what you want to know is if at least one
channel binding type is supported or not.
We could as well live with the existing set of betls APIs, just that the
implementation of secure transport for MacOS will need a hack in auth.c
to stop the -PLUS mechanisms to be sent. Putting this load into each
be-secure-*.c file is cleaner in my opinion.
--
Michael
On 23 Jan 2018, at 03:24, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Jan 22, 2018 at 04:52:11PM +0100, Daniel Gustafsson wrote:
Not really, but IIUC a bug in this code could lead to channel binding
not being used for a connection even if the user wanted it (and may
miss that ixt didn’t happen due to not looking at logs etc)?Possible, if an implementation decides to send a NIL list as return
result of this API when it should not.Considering the limited set of values (currently) it should be quite
simple to check for offending entries, if there is indeed a risk of
“silently” not using channel binding.I think I understand the point you are coming at, but a problem is that
the channel binding type the client wants to use is known by the server
in SCRAM authentication only after reading the client-first message,
which happens of course after the client decided to choose if channel
binding is used or not. Now the server needs to emit first a list of
supported SASL mechanisms which are consistent with what the SSL
implementation can do when the mechanism is negotiated. Another, third
approach that I can think of is to have this additional API in betls
emit a list of mechanisms, but I am not sure that this is worth the
complication as at the end what you want to know is if at least one
channel binding type is supported or not.
Thanks for the explanation, I agree that the proposed approach makes the most
sense.
We could as well live with the existing set of betls APIs, just that the
implementation of secure transport for MacOS will need a hack in auth.c
to stop the -PLUS mechanisms to be sent. Putting this load into each
be-secure-*.c file is cleaner in my opinion.
I completely agree, let’s avoid such hacks.
cheers ./daniel
On 1/22/18 02:29, Michael Paquier wrote:
However there is as well the argument that this list's contents are not
directly used now, and based on what I saw from the MacOS SSL and GnuTLS
patches that would not be the case after either.
Right, there is no facility for negotiating the channel binding type, so
a boolean result should be enough.
In which case we wouldn't actually need this for GnuTLS yet.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jan 23, 2018 at 12:08:37PM -0500, Peter Eisentraut wrote:
On 1/22/18 02:29, Michael Paquier wrote:
However there is as well the argument that this list's contents are not
directly used now, and based on what I saw from the MacOS SSL and GnuTLS
patches that would not be the case after either.Right, there is no facility for negotiating the channel binding type, so
a boolean result should be enough.
I am not completely convinced either that we need to complicate the code
to handle channel binding type negotiation.
In which case we wouldn't actually need this for GnuTLS yet.
Sure. This depends mainly on how the patch for Mac's Secure Transport
moves forward.
--
Michael
On 1/23/18 21:27, Michael Paquier wrote:
On Tue, Jan 23, 2018 at 12:08:37PM -0500, Peter Eisentraut wrote:
On 1/22/18 02:29, Michael Paquier wrote:
However there is as well the argument that this list's contents are not
directly used now, and based on what I saw from the MacOS SSL and GnuTLS
patches that would not be the case after either.Right, there is no facility for negotiating the channel binding type, so
a boolean result should be enough.I am not completely convinced either that we need to complicate the code
to handle channel binding type negotiation.In which case we wouldn't actually need this for GnuTLS yet.
Sure. This depends mainly on how the patch for Mac's Secure Transport
moves forward.
Moved to next CF along with those other two patches.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 08, 2018 at 02:19:55PM -0500, Peter Eisentraut wrote:
Moved to next CF along with those other two patches.
Attached is a refreshed patch for this thread, which failed to apply
after recent changes. This is tied with patches for other SSL
implementations, so let's see about it later if necessary.
--
Michael
Attachments:
betls-channel-binding-list-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 63f37902e6..6ad5e2eedf 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -873,6 +873,7 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
int inputlen;
int result;
bool initial;
+ List *channel_bindings = NIL;
/*
* SASL auth is not supported for protocol versions before 3, because it
@@ -898,7 +899,17 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
strlen(SCRAM_SHA_256_NAME) + 3);
p = sasl_mechs;
- if (port->ssl_in_use)
+#ifdef USE_SSL
+ /*
+ * Get the list of channel binding types supported by the SSL
+ * implementation used to determine if server should publish any
+ * SASL mechanism supporting channel binding or not.
+ */
+ channel_bindings = be_tls_list_channel_bindings();
+#endif
+
+ if (port->ssl_in_use &&
+ list_length(channel_bindings) > 0)
{
strcpy(p, SCRAM_SHA_256_PLUS_NAME);
p += strlen(SCRAM_SHA_256_PLUS_NAME) + 1;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 48b468f62f..cc55ce04ef 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -36,6 +36,7 @@
#include <openssl/ec.h>
#endif
+#include "common/scram-common.h"
#include "libpq/libpq.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -1184,6 +1185,18 @@ be_tls_get_certificate_hash(Port *port, size_t *len)
#endif
}
+/*
+ * Routine to get the list of channel binding types available in this SSL
+ * implementation. For OpenSSL, both tls-unique and tls-server-end-point
+ * are supported.
+ */
+List *
+be_tls_list_channel_bindings(void)
+{
+ return list_make2(pstrdup(SCRAM_CHANNEL_BINDING_TLS_UNIQUE),
+ pstrdup(SCRAM_CHANNEL_BINDING_TLS_END_POINT));
+}
+
/*
* Convert an X509 subject name to a cstring.
*
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 7698cd1f88..1a314346b8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -259,6 +259,7 @@ extern bool be_tls_get_compression(Port *port);
extern const char *be_tls_get_version(Port *port);
extern const char *be_tls_get_cipher(Port *port);
extern void be_tls_get_peerdn_name(Port *port, char *ptr, size_t len);
+extern List *be_tls_list_channel_bindings(void);
/*
* Get the expected TLS Finished message information from the client, useful
On 28/05/18 06:46, Michael Paquier wrote:
On Thu, Mar 08, 2018 at 02:19:55PM -0500, Peter Eisentraut wrote:
Moved to next CF along with those other two patches.
Attached is a refreshed patch for this thread, which failed to apply
after recent changes. This is tied with patches for other SSL
implementations, so let's see about it later if necessary.
This was obsoleted by commit 77291139, I'll close this in the commitfest.
- Heikki