Support tls-exporter as channel binding for TLSv1.3
Hi all,
RFC9266, that has been released not so long ago, has added
tls-exporter as a new channel binding type:
https://www.rfc-editor.org/rfc/rfc5929.html
An advantage over tls-server-end-point, AFAIK, is that this prevents
man-in-the-middle attacks even if the attacker holds the server's
private key, which was the kind of job that tls-unique does for
TLSv1.2, though we've decided at the end to drop it during the PG11
dev cycle because it does things poorly.
This patch provides an implementation, tests and documentation for the
so-said feature. An environment variable called PGCHANNELBINDINGTYPE
is added, as well as new connection parameter called
channel_binding_type. The key point of the implementation is
SSL_export_keying_material(), that is available down to 1.0.1 (oldest
version supported on HEAD), so this should not require a ./configure
check.
Perhaps the part about the new libpq parameter could be refactored as
of its own patch, with the addition of channel_binding_type in the
SCRAM status structures. Note also that tls-exporter is aimed for
TLSv1.3 and newer protocols, but OpenSSL allows the thing to work with
older protocols (testable with ssl_max_protocol_version, for example),
and I don't see a need to prevent this scenario. An extra thing is
that attempting to use tls-exporter with a backend <= 15 and a client
= 16 causes a failure during the SASL exchange, where the backend
complains about tls-exporter being unsupported.
Jacob Champion should be considered as the primary author of the
patch, even if I have spent some time on this patch before sending it
here. I am adding that to the next commit fest.
Thanks,
--
Michael
Attachments:
0001-tls-exporter-as-channel-binding-for-SCRAM-SSL.patchtext/x-diff; charset=us-asciiDownload
From bd086e2d8b34444151ca52d2e2cd43b8f4a9f522 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 29 Aug 2022 14:52:41 +0900
Subject: [PATCH] tls-exporter as channel binding for SCRAM/SSL
---
src/include/libpq/libpq-be.h | 6 +++
src/backend/libpq/auth-scram.c | 50 ++++++++++++++++++------
src/backend/libpq/be-secure-openssl.c | 18 +++++++++
src/interfaces/libpq/fe-auth-scram.c | 45 +++++++++++++++++----
src/interfaces/libpq/fe-connect.c | 27 +++++++++++++
src/interfaces/libpq/fe-secure-openssl.c | 30 ++++++++++++++
src/interfaces/libpq/libpq-int.h | 14 +++++++
src/test/ssl/t/002_scram.pl | 15 +++++++
doc/src/sgml/libpq.sgml | 28 +++++++++++++
doc/src/sgml/protocol.sgml | 3 +-
10 files changed, 216 insertions(+), 20 deletions(-)
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 6d452ec6d9..78ff51d053 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -298,6 +298,12 @@ extern void be_tls_get_peer_subject_name(Port *port, char *ptr, size_t len);
extern void be_tls_get_peer_issuer_name(Port *port, char *ptr, size_t len);
extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
+extern unsigned char *be_tls_export_keying_material(Port *port,
+ const char *label,
+ const unsigned char *ctx,
+ size_t ctxlen,
+ size_t outlen);
+
/*
* Get the server certificate hash for SCRAM channel binding type
* tls-server-end-point.
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index ee7f52218a..6029e33596 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -148,6 +148,7 @@ typedef struct
/* Fields of the first message from client */
char cbind_flag;
+ char *channel_binding_type;
char *client_first_message_bare;
char *client_username;
char *client_nonce;
@@ -876,7 +877,6 @@ static void
read_client_first_message(scram_state *state, const char *input)
{
char *p = pstrdup(input);
- char *channel_binding_type;
/*------
@@ -1009,17 +1009,17 @@ read_client_first_message(scram_state *state, const char *input)
errmsg("malformed SCRAM message"),
errdetail("The client selected SCRAM-SHA-256 without channel binding, but the SCRAM message includes channel binding data.")));
- channel_binding_type = read_attr_value(&p, 'p');
+ state->channel_binding_type = read_attr_value(&p, 'p');
/*
- * The only channel binding type we support is
- * tls-server-end-point.
+ * We support tls-server-end-point and tls-exporter.
*/
- if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
+ if (strcmp(state->channel_binding_type, "tls-server-end-point") != 0
+ && strcmp(state->channel_binding_type, "tls-exporter") != 0)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("unsupported SCRAM channel-binding type \"%s\"",
- sanitize_str(channel_binding_type))));
+ sanitize_str(state->channel_binding_type))));
break;
default:
ereport(ERROR,
@@ -1286,18 +1286,46 @@ read_client_final_message(scram_state *state, const char *input)
Assert(state->cbind_flag == 'p');
- /* Fetch hash data of server's SSL certificate */
- cbind_data = be_tls_get_certificate_hash(state->port,
- &cbind_data_len);
+ if (strcmp(state->channel_binding_type, "tls-exporter") == 0)
+ {
+ /*------
+ * From the specification (RFC 9266):
+ *
+ * The [tls-exporter] EKM is obtained using the keying material
+ * exporters for TLS as defined in [RFC5705] and [RFC8446]
+ * section 7.5 by supplying the following inputs:
+ *
+ * Label: The ASCII string "EXPORTER-Channel-Binding" with no
+ * terminating NUL.
+ *
+ * Context value: Zero-length string.
+ *
+ * Length: 32 bytes.
+ *------
+ */
+ cbind_data_len = 32;
+ cbind_data = (char *)
+ be_tls_export_keying_material(state->port,
+ "EXPORTER-Channel-Binding",
+ (unsigned char *) "", 0,
+ cbind_data_len);
+ }
+ else /* tls-server-end-point */
+ {
+ /* Fetch hash data of server's SSL certificate */
+ cbind_data = be_tls_get_certificate_hash(state->port,
+ &cbind_data_len);
+ }
/* should not happen */
if (cbind_data == NULL || cbind_data_len == 0)
elog(ERROR, "could not get server certificate hash");
- cbind_header_len = strlen("p=tls-server-end-point,,"); /* p=type,, */
+ cbind_header_len = strlen(state->channel_binding_type) + 4; /* p=type,, */
cbind_input_len = cbind_header_len + cbind_data_len;
cbind_input = palloc(cbind_input_len);
- snprintf(cbind_input, cbind_input_len, "p=tls-server-end-point,,");
+ snprintf(cbind_input, cbind_input_len, "p=%s,,",
+ state->channel_binding_type);
memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
b64_message_len = pg_b64_enc_len(cbind_input_len);
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 55d4b29f7e..163567bf51 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1424,6 +1424,24 @@ be_tls_get_peer_serial(Port *port, char *ptr, size_t len)
ptr[0] = '\0';
}
+unsigned char *
+be_tls_export_keying_material(Port *port, const char *label,
+ const unsigned char *ctx, size_t ctxlen,
+ size_t outlen)
+{
+ int rc;
+ unsigned char *out = palloc(outlen);
+
+ rc = SSL_export_keying_material(port->ssl, out, outlen,
+ label, strlen(label),
+ ctx, ctxlen,
+ 1 /* use the context */);
+ if (rc < 1)
+ elog(ERROR, "could not export keying material");
+
+ return out;
+}
+
#ifdef HAVE_X509_GET_SIGNATURE_NID
char *
be_tls_get_certificate_hash(Port *port, size_t *len)
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 35cfd9987d..ca1a583783 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -400,7 +400,7 @@ build_client_first_message(fe_scram_state *state)
if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0)
{
Assert(conn->ssl_in_use);
- appendPQExpBufferStr(&buf, "p=tls-server-end-point");
+ appendPQExpBuffer(&buf, "p=%s", conn->channel_binding_type);
}
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
else if (conn->channel_binding[0] != 'd' && /* disable */
@@ -484,10 +484,38 @@ build_client_final_message(fe_scram_state *state)
size_t cbind_input_len;
int encoded_cbind_len;
- /* Fetch hash data of server's SSL certificate */
- cbind_data =
- pgtls_get_peer_certificate_hash(state->conn,
- &cbind_data_len);
+ if (strcmp(state->conn->channel_binding_type, "tls-exporter") == 0)
+ {
+ /*------
+ * From the spec:
+ *
+ * The [tls-exporter] EKM is obtained using the keying material
+ * exporters for TLS as defined in [RFC5705] and [RFC8446] section
+ * 7.5 by supplying the following inputs:
+ *
+ * Label: The ASCII string "EXPORTER-Channel-Binding" with no
+ * terminating NUL.
+ *
+ * Context value: Zero-length string.
+ *
+ * Length: 32 bytes.
+ *------
+ */
+ cbind_data_len = 32;
+ cbind_data = (char *)
+ pgtls_export_keying_material(state->conn,
+ "EXPORTER-Channel-Binding",
+ (unsigned char *) "", 0,
+ cbind_data_len);
+ }
+ else /* tls-server-end-point */
+ {
+ /* Fetch hash data of server's SSL certificate */
+ cbind_data =
+ pgtls_get_peer_certificate_hash(state->conn,
+ &cbind_data_len);
+ }
+
if (cbind_data == NULL)
{
/* error message is already set on error */
@@ -498,7 +526,7 @@ build_client_final_message(fe_scram_state *state)
appendPQExpBufferStr(&buf, "c=");
/* p=type,, */
- cbind_header_len = strlen("p=tls-server-end-point,,");
+ cbind_header_len = strlen(state->conn->channel_binding_type) + 4;
cbind_input_len = cbind_header_len + cbind_data_len;
cbind_input = malloc(cbind_input_len);
if (!cbind_input)
@@ -506,8 +534,9 @@ build_client_final_message(fe_scram_state *state)
free(cbind_data);
goto oom_error;
}
- memcpy(cbind_input, "p=tls-server-end-point,,", cbind_header_len);
- memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
+ snprintf(cbind_input, cbind_input_len, "p=%s,,",
+ state->conn->channel_binding_type);
+ memcpy(cbind_input + cbind_header_len , cbind_data, cbind_data_len);
encoded_cbind_len = pg_b64_enc_len(cbind_input_len);
if (!enlargePQExpBuffer(&buf, encoded_cbind_len))
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 917b19e0e9..53ff024f1d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -122,6 +122,7 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
#else
#define DefaultChannelBinding "disable"
#endif
+#define DefaultChannelBindingType "tls-server-end-point"
#define DefaultTargetSessionAttrs "any"
#ifdef USE_SSL
#define DefaultSSLMode "prefer"
@@ -205,6 +206,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Channel-Binding", "", 8, /* sizeof("require") == 8 */
offsetof(struct pg_conn, channel_binding)},
+ {"channel_binding_type", "PGCHANNELBINDINGTYPE", NULL, NULL,
+ "Channel-Binding-Type", "", 20, /* sizeof("tls-server-end-point") == 20 */
+ offsetof(struct pg_conn, channel_binding_type)},
+
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
"Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
offsetof(struct pg_conn, connect_timeout)},
@@ -1263,6 +1268,28 @@ connectOptions2(PGconn *conn)
goto oom_error;
}
+ /*
+ * validate channel_binding_type option
+ */
+ if (conn->channel_binding_type)
+ {
+ if (strcmp(conn->channel_binding_type, "tls-server-end-point") != 0
+ && strcmp(conn->channel_binding_type, "tls-exporter") != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid %s value: \"%s\"\n"),
+ "channel_binding_type", conn->channel_binding_type);
+ return false;
+ }
+ }
+ else
+ {
+ conn->channel_binding_type = strdup(DefaultChannelBindingType);
+ if (!conn->channel_binding_type)
+ goto oom_error;
+ }
+
/*
* validate sslmode option
*/
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 3798bb3f11..89fa4513c4 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -378,6 +378,36 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
return n;
}
+unsigned char *
+pgtls_export_keying_material(PGconn *conn, const char *label,
+ const unsigned char *ctx, size_t ctxlen,
+ size_t outlen)
+{
+ int rc;
+ unsigned char *out = malloc(outlen);
+
+ if (out == NULL)
+ {
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ return NULL;
+ }
+
+ rc = SSL_export_keying_material(conn->ssl, out, outlen,
+ label, strlen(label),
+ ctx, ctxlen,
+ 1 /* use the context */);
+ if (rc < 1)
+ {
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("could not export keying material\n"));
+ free(out);
+ return NULL;
+ }
+
+ return out;
+}
+
#ifdef HAVE_X509_GET_SIGNATURE_NID
char *
pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index c75ed63a2c..f412e0c2ce 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -373,6 +373,8 @@ struct pg_conn
char *pgpassfile; /* path to a file containing password(s) */
char *channel_binding; /* channel binding mode
* (require,prefer,disable) */
+ char *channel_binding_type; /* from the IANA Channel-Binding Types
+ * registry */
char *keepalives; /* use TCP keepalives? */
char *keepalives_idle; /* time between TCP keepalives */
char *keepalives_interval; /* time between TCP keepalive
@@ -795,6 +797,18 @@ extern bool pgtls_read_pending(PGconn *conn);
*/
extern ssize_t pgtls_write(PGconn *conn, const void *ptr, size_t len);
+/*
+ * Export keying material, for SCRAM channel binding type tls-exporter.
+ *
+ * NULL is sent back to the caller in the evenf of an error, with an
+ * error message for the caller to consume.
+ */
+extern unsigned char *pgtls_export_keying_material(PGconn *conn,
+ const char *label,
+ const unsigned char *ctx,
+ size_t ctxlen,
+ size_t outlen);
+
/*
* Get the hash of the server certificate, for SCRAM channel binding type
* tls-server-end-point.
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 588f47a39b..f4c175424f 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -105,6 +105,21 @@ $node->connect_fails(
qr/channel binding required but not supported by server's authentication request/
);
+# Tests for channel_binding_type
+$node->connect_fails(
+ "$common_connstr user=ssltestuser channel_binding=require channel_binding_type=invalid_value",
+ "SCRAM with SSL and channel_binding_type=invalid_value",
+ expected_stderr => qr/invalid channel_binding_type value: "invalid_value"/);
+if ($supports_tls_server_end_point)
+{
+ $node->connect_ok(
+ "$common_connstr user=ssltestuser channel_binding=require channel_binding_type=tls-server-end-point",
+ "SCRAM with SSL and channel_binding_type=tls-server-end-point");
+}
+$node->connect_ok(
+ "$common_connstr user=ssltestuser channel_binding=require channel_binding_type=tls-exporter",
+ "SCRAM with SSL and channel_binding_type=tls-exporter");
+
# Now test with auth method 'cert' by connecting to 'certdb'. Should fail,
# because channel binding is not performed. Note that ssl/client.key may
# be used in a different test, so the name of this temporary client key
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 8a1a9e9932..b29dcd69d0 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1242,6 +1242,24 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+
+ <varlistentry id="libpq-connect-channel-binding-type" xreflabel="channel_binding_type">
+ <term><literal>channel_binding_type</literal></term>
+ <listitem>
+ <para>
+ This option controls the type of channel binding used by the client
+ when <literal>channel_binding</literal> is enabled. Supported
+ values are <literal>tls-server-end-point</literal>
+ (<ulink url="https://tools.ietf.org/html/rfc5929">RFC 5929</ulink>)
+ and <literal>tls-exporter</literal>
+ (<ulink url="https://tools.ietf.org/html/rfc9266">RFC 9266</ulink>
+ channel binding for <literal>TLSv1.3</literal> that has the advantage
+ to prevent man-in-the-middle attacks when the attacker has the
+ server's private key).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-connect-timeout" xreflabel="connect_timeout">
<term><literal>connect_timeout</literal></term>
<listitem>
@@ -7766,6 +7784,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGCHANNELBINDINGTYPE</envar></primary>
+ </indexterm>
+ <envar>PGCHANNELBINDINGTYPE</envar> behaves the same as the <xref
+ linkend="libpq-connect-channel-binding-type"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 87870c5b10..a66abb1f29 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1752,7 +1752,8 @@ SELCT 1/0;<!-- this typo is intentional -->
<firstterm>Channel binding</firstterm> is supported in PostgreSQL builds with
SSL support. The SASL mechanism name for SCRAM with channel binding is
<literal>SCRAM-SHA-256-PLUS</literal>. The channel binding type used by
- PostgreSQL is <literal>tls-server-end-point</literal>.
+ PostgreSQL are <literal>tls-server-end-point</literal> and
+ <literal>tls-exporter</literal>.
</para>
<para>
--
2.37.2
On Sun, Aug 28, 2022 at 11:02 PM Michael Paquier <michael@paquier.xyz> wrote:
RFC9266, that has been released not so long ago, has added
tls-exporter as a new channel binding type:
https://www.rfc-editor.org/rfc/rfc5929.html
Hi Michael, thank you for sending this!
Note also that tls-exporter is aimed for
TLSv1.3 and newer protocols, but OpenSSL allows the thing to work with
older protocols (testable with ssl_max_protocol_version, for example),
and I don't see a need to prevent this scenario.
For protocols less than 1.3 we'll need to ensure that the extended
master secret is in use:
This channel binding mechanism is defined only when the TLS handshake
results in unique master secrets. This is true of TLS versions prior
to 1.3 when the extended master secret extension of [RFC7627] is in
use, and it is always true for TLS 1.3 (see Appendix D of [RFC8446]).
OpenSSL should have an API for that (SSL_get_extms_support); I don't
know when it was introduced.
If we want to cross all our T's, we should also disallow tls-exporter
if the server was unable to set SSL_OP_NO_RENEGOTIATION.
An extra thing is
that attempting to use tls-exporter with a backend <= 15 and a client= 16 causes a failure during the SASL exchange, where the backend
complains about tls-exporter being unsupported.
Yep.
--
Did you have any thoughts about contributing the Python tests (or
porting them to Perl, bleh) so that we could test failure modes as
well? Unfortunately those Python tests were also OpenSSL-based, so
they're less powerful than an independent implementation...
Thanks,
--Jacob
On Wed, Aug 31, 2022 at 04:16:29PM -0700, Jacob Champion wrote:
For protocols less than 1.3 we'll need to ensure that the extended
master secret is in use:This channel binding mechanism is defined only when the TLS handshake
results in unique master secrets. This is true of TLS versions prior
to 1.3 when the extended master secret extension of [RFC7627] is in
use, and it is always true for TLS 1.3 (see Appendix D of [RFC8446]).OpenSSL should have an API for that (SSL_get_extms_support); I don't
know when it was introduced.
This is only available from 1.1.0, meaning that we'd better disable
tls-exporter when building with something older than that :( With
1.0.2 already not supported by upstream even if a bunch of vendors
keep it around for compatibility, I guess that's fine as long as
the default setting is tls-server-end-point. It would not be complex
to switch to tls-exporter by default when using TLSv1.3 and
tls-server-end-point for TLS <= v1.2 though, but that makes the code
more complicated and OpenSSL does not complain with tls-exporter when
using < 1.3. If we switch the default on the fly, we could drop
channel_binding_type and control which one gets used depending on
ssl_max/min_server_protocol. I don't like that much, TBH, as this
creates more dependencies across our the internal code with the
initial choice of the connection parameters, making it more complex to
maintain in the long-term. At least that's my impression.
If we want to cross all our T's, we should also disallow tls-exporter
if the server was unable to set SSL_OP_NO_RENEGOTIATION.
Hmm. Okay. I have not considered that. But TLSv1.3 has no support
for renegotiation, isn't it? And you mean to fail hard when using
TLS <= v1.2 with tls-exporter on the backend's SSL_CTX_set_options()
call? We cannot do that as the backend's SSL context is initialized
before authentication, but we could re-check the state of the SSL
options afterwards, during authentication, and force a failure.
Did you have any thoughts about contributing the Python tests (or
porting them to Perl, bleh) so that we could test failure modes as
well? Unfortunately those Python tests were also OpenSSL-based, so
they're less powerful than an independent implementation...
No. I have not been able to look at that with the time I had,
unfortunately.
--
Michael
On Wed, Aug 31, 2022 at 5:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Aug 31, 2022 at 04:16:29PM -0700, Jacob Champion wrote:
OpenSSL should have an API for that (SSL_get_extms_support); I don't
know when it was introduced.This is only available from 1.1.0, meaning that we'd better disable
tls-exporter when building with something older than that :( With
1.0.2 already not supported by upstream even if a bunch of vendors
keep it around for compatibility, I guess that's fine as long as
the default setting is tls-server-end-point.
Yeah, that should be fine. Requiring newer OpenSSLs for stronger
crypto will probably be uncontroversial.
It would not be complex
to switch to tls-exporter by default when using TLSv1.3 and
tls-server-end-point for TLS <= v1.2 though, but that makes the code
more complicated and OpenSSL does not complain with tls-exporter when
using < 1.3. If we switch the default on the fly, we could drop
channel_binding_type and control which one gets used depending on
ssl_max/min_server_protocol. I don't like that much, TBH, as this
creates more dependencies across our the internal code with the
initial choice of the connection parameters, making it more complex to
maintain in the long-term. At least that's my impression.
I think there are two separate concerns there -- whether to remove the
configuration option, and whether to change the default.
I definitely wouldn't want to remove the option. Users of TLS 1.2
should be able to choose tls-exporter if they want the extra power,
and users of TLS 1.3 should be able to remain on tls-server-end-point
if they need it in the future.
Changing the default is trickier. tls-server-end-point is our default
in the wild. We're not RFC-compliant already, because we don't
implement tls-unique at all. And there's no negotiation, so it seems
like switching the default for TLS 1.3 would impact interoperability
between newer clients and older servers. The advantage would be that
users of newer clients would have to opt in before servers could
forward their credentials around on their behalf. Maybe that's
something we could switch safely in the future, once tls-exporter is
more widely deployed?
If we want to cross all our T's, we should also disallow tls-exporter
if the server was unable to set SSL_OP_NO_RENEGOTIATION.Hmm. Okay. I have not considered that. But TLSv1.3 has no support
for renegotiation, isn't it?
Right. We only need to worry about that when we're using an older TLS.
And you mean to fail hard when using
TLS <= v1.2 with tls-exporter on the backend's SSL_CTX_set_options()
call? We cannot do that as the backend's SSL context is initialized
before authentication, but we could re-check the state of the SSL
options afterwards, during authentication, and force a failure.
Sounds reasonable.
Did you have any thoughts about contributing the Python tests (or
porting them to Perl, bleh) so that we could test failure modes as
well? Unfortunately those Python tests were also OpenSSL-based, so
they're less powerful than an independent implementation...No. I have not been able to look at that with the time I had,
unfortunately.
All good. Thanks!
--Jacob
On Wed, Sep 7, 2022 at 10:03 AM Jacob Champion <jchampion@timescale.com> wrote:
Yeah, that should be fine. Requiring newer OpenSSLs for stronger
crypto will probably be uncontroversial.
While looking into this I noticed that I left the following code in place:
#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
In other words, we're still deciding whether to advertise -PLUS based
only on whether we support tls-server-end-point. Maybe all the
necessary features landed in OpenSSL in the same version, but I
haven't double-checked that, and in any case I think I need to make
this code more correct in the next version of this patch.
--Jacob
On Mon, Sep 19, 2022 at 09:27:41AM -0700, Jacob Champion wrote:
While looking into this I noticed that I left the following code in place:
#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)In other words, we're still deciding whether to advertise -PLUS based
only on whether we support tls-server-end-point.
X509_get_signature_nid() has been introduced in 1.0.2.
SSL_export_keying_material() is older than that, being present since
1.0.1. Considering the fact that we want to always have
tls-server-end-point as default, it seems to me that we should always
publish SCRAM-SHA-256-PLUS and support channel binding when building
with OpenSSL >= 1.0.2. The same can be said about the areas where we
have HAVE_BE_TLS_GET_[PEER_]CERTIFICATE_HASH.
There could be a point in supporting tls-exporter as default in 1.0.1,
or just allow it if the caller gives it in the connection string, but
as that's the next version we are going to drop support for (sooner
than later would be better IMO), I don't really want to add more
maintenance burden in this area as 1.0.1 is not that used anyway as
far as I recall.
Maybe all the necessary features landed in OpenSSL in the same
version, but I haven't double-checked that, and in any case I think
I need to make this code more correct in the next version of this
patch.
I was planning to continue working on this patch based on your latest
review. Anyway, as that's originally your code, I am fine to let you
take the lead here. Just let me know which way you prefer, and I'll
stick to it :)
--
Michael
On Mon, Sep 19, 2022 at 5:54 PM Michael Paquier <michael@paquier.xyz> wrote:
X509_get_signature_nid() has been introduced in 1.0.2.
SSL_export_keying_material() is older than that, being present since
1.0.1. Considering the fact that we want to always have
tls-server-end-point as default, it seems to me that we should always
publish SCRAM-SHA-256-PLUS and support channel binding when building
with OpenSSL >= 1.0.2. The same can be said about the areas where we
have HAVE_BE_TLS_GET_[PEER_]CERTIFICATE_HASH.
Should we advertise support even if the client is too old to provide
an extended master secret?
I was planning to continue working on this patch based on your latest
review. Anyway, as that's originally your code, I am fine to let you
take the lead here. Just let me know which way you prefer, and I'll
stick to it :)
Well, I'm working on a next version, but it's ballooning in complexity
as I try to navigate the fix for OpenSSL 1.0.1 (which is currently
failing the tests, unsurprisingly). You mentioned not wanting to add
maintenance burden for 1.0.1, and I'm curious to see whether the
approach you have in mind would be easier than what mine is turning
out to be. Maybe we can compare notes?
--Jacob
On Tue, Sep 20, 2022 at 11:01 AM Jacob Champion <jchampion@timescale.com> wrote:
Well, I'm working on a next version, but it's ballooning in complexity
as I try to navigate the fix for OpenSSL 1.0.1 (which is currently
failing the tests, unsurprisingly).
To be more specific: I think I'm hitting the case that Heikki pointed
out several years ago [1]/messages/by-id/ec787074-2305-c6f4-86aa-6902f98485a4@iki.fi:
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.
The problem was deferred when tls-unique was removed. We might have to
actually solve it now.
bcc: Heikki, in case he would like to weigh in.
--Jacob
[1]: /messages/by-id/ec787074-2305-c6f4-86aa-6902f98485a4@iki.fi
On Tue, Sep 20, 2022 at 11:51:44AM -0700, Jacob Champion wrote:
On Tue, Sep 20, 2022 at 11:01 AM Jacob Champion <jchampion@timescale.com> wrote:
Well, I'm working on a next version, but it's ballooning in complexity
as I try to navigate the fix for OpenSSL 1.0.1 (which is currently
failing the tests, unsurprisingly).To be more specific: I think I'm hitting the case that Heikki pointed
out several years ago [1]: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.The problem was deferred when tls-unique was removed. We might have to
actually solve it now.
Right. One thing that would reduce the complexity of the equation is
to drop support for tls-server-end-point in the backend in PG >= 16 as
the versions of OpenSSL we still support on HEAD would cover
completely tls-exporter.
We should have in libpq the code to support both tls-server-end-point
and tls-exporter as channel bindings, for backward-compatibility. If
we were to drop support for OpenSSL 1.0.1, things get a bit easier
here, as we would be sure that channel binding is supported by all the
code paths of libpq. Having both channel_binding_type with
channel_binding=require would offer some protection against downgrade
attacks. That does not feel completely water-proof, still default
settings like sslmode=prefer are not really secure either..
--
Michael
On Wed, Oct 12, 2022 at 11:01 PM Michael Paquier <michael@paquier.xyz> wrote:
One thing that would reduce the complexity of the equation is
to drop support for tls-server-end-point in the backend in PG >= 16 as
the versions of OpenSSL we still support on HEAD would cover
completely tls-exporter.
Is the intent to backport tls-exporter client support? Or is the
compatibility break otherwise acceptable?
It seemed like there was also some general interest in proxy TLS
termination (see also the PROXY effort, though it has stalled a bit).
For that, I would think tls-server-end-point is an important feature.
--Jacob
On Thu, Oct 13, 2022 at 10:30:37AM -0700, Jacob Champion wrote:
Is the intent to backport tls-exporter client support? Or is the
compatibility break otherwise acceptable?
Well, I'd rather say yes thanks to the complexity avoided in the
backend as that's the most sensible piece security-wise, even if we
always require a certificate to exist in PG. A server attempting to
trick a client in downgrading would still be a problem :/
tls-exporter would be a new feature, so backporting is out of the
picture.
It seemed like there was also some general interest in proxy TLS
termination (see also the PROXY effort, though it has stalled a bit).
For that, I would think tls-server-end-point is an important feature.
Oh, okay. That's an argument in favor of not doing that, then.
Perhaps we'd better revisit the introduction of tls-exporter once we
know more about all that, and it looks like we would need a way to be
able to negotiate which channel binding to use (I recall that the
surrounding RFCs allowed some extra negotiation, vaguely, but my
impression may be wrong).
--
Michael
On Fri, Oct 14, 2022 at 11:00:10AM +0900, Michael Paquier wrote:
Oh, okay. That's an argument in favor of not doing that, then.
Perhaps we'd better revisit the introduction of tls-exporter once we
know more about all that, and it looks like we would need a way to be
able to negotiate which channel binding to use (I recall that the
surrounding RFCs allowed some extra negotiation, vaguely, but my
impression may be wrong).
I am not sure what can be done for that now, so I have marked the
patch as returned with feedback.
--
Michael