On 2017/6/19 13:54, Patrick Hemmer wrote:
>
>
> On 2017/6/17 00:00, Willy Tarreau wrote:
>> Hi Patrick,
>>
>> On Fri, Jun 16, 2017 at 09:36:30PM -0400, Patrick Hemmer wrote:
>>> The main reason I had for supporting the older code is that it seems
>>> many (most?) linux distros, such as the one we use (CentOS/7), still
>>> ship with 1.0.1 or 1.0.2. However since this is a minor change and a
>>> feature enhancement, I doubt this will get backported to 1.7, meaning
>>> we'll have to manually patch it into the version we use. And since we're
>>> doing that, we can just use the patch that supports older OpenSSL.
>> Yes in this case I think it makes sense. And given the proliferation
>> of *ssl implementations, I really prefer to avoid adding code touching
>> directly internal openssl stuff.
>>
>>> Anyway, here is the updated patch with the support for <1.1.0 dropped,
>>> as well as the BoringSSL support Emmanuel requested.
>> OK cool.
>>
>>> One other minor thing I was unsure about was the fetch name. Currently I
>>> have it as "ssl_fc_session_key", but "ssl_fc_session_master_key" might
>>> be more accurate. However this is rather verbose and would make it far
>>> longer than any other sample fetch name, and I was unsure if there were
>>> rules around the naming.
>> There are no particular rules but it's true that long names are painful,
>> especially for those with "fat fingers". I'd suggest that given that it
>> starts with "ssl_fc_" and "ssl_bc_", the context is already quite
>> restricted and you could probably call them "smk" or "smkey". One would
>> argue that looking at the doc will be necessary, but with long names you
>> also have to look at the doc to find how to spell them anyway, the
>> difference being that short names don't mangle your configs too much,
>> especially in logformat strings.
> Well my argument for keeping the name starting with `ssl_fc_session_`
> is that there is also `ssl_fc_session_id`. These 2 fetches pull their
> attribute from the same "session" structure. They are also closely
> related as using `ssl_fc_session_key` almost requires the
> `ssl_fc_session_id` value (you could technically get by without it,
> but it'll make using the key rather difficult unless you have some
> other way of correlating a key with a specific SSL session).
>>> +ssl_bc_session_key : binary
>>> + Returns the SSL session master key of the back connection when the
>>> outgoing
>>> + connection was made over an SSL/TLS transport layer. It is useful to
>>> decrypt
>>> + traffic sent using ephemeral ciphers.
>> Here it would be nice to add a short sentence like "Not available before
>> openssl 1.1.0" so that users don't waste too much time on something not
>> working.
>>
>>> static int
>>> +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp,
>>> const char *kw, void *private)
>>> +{
>>> +#if OPENSSL_VERSION_NUMBER >= 0x10100000L || defined(OPENSSL_IS_BORINGSSL)
>> Here I'd put the ifdef around the whole function, like we have for
>> ALPN for example, so that there's no risk it could be used in a non-working
>> config.
> My objection to this is that most of the other sample fetches don't do
> this, and so it makes the user experience inconsistent. For example
> the `ssl_fc_session_id` fetch, which `ssl_fc_session_key` is strongly
> related to, behaves the same way. The ALPN/NPN fetches are the only
> ones that make using the fetch a config error if the underlying
> support is missing.
>>> + struct connection *conn = objt_conn((kw[4] != 'b') ? smp->sess->origin :
>>> + smp->strm ? smp->strm->si[1].end :
>>> NULL);
>>> +
>>> + SSL_SESSION *ssl_sess;
>>> + int data_len;
>>> + struct chunk *data;
>>> +
>>> + if (!conn || !conn->xprt_ctx || conn->xprt != &ssl_sock)
>>> + return 0;
>>> +
>>> + ssl_sess = SSL_get_session(conn->xprt_ctx);
>>> + if (!ssl_sess)
>>> + return 0;
>>> +
>>> + data = get_trash_chunk();
>>> + data_len = SSL_SESSION_get_master_key(ssl_sess, (unsigned char
>>> *)data->str, data->size);
>>> + if (!data_len)
>>> + return 0;
>>> +
>>> + smp->flags = SMP_F_CONST;
>>> + smp->data.type = SMP_T_BIN;
>>> + data->len = data_len;
>> If you want, you can even get rid of your data_len variable by directly
>> assigning SSL_SESSION_get_master_key() to data->len.
>>
>>> +static int
>>> smp_fetch_ssl_fc_sni(const struct arg *args, struct sample *smp, const
>>> char *kw, void *private)
>>> {
>>> #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
>>> @@ -7841,6 +7875,7 @@ static struct sample_fetch_kw_list
>>> sample_fetch_keywords = {ILH, {
>>> { "ssl_bc_unique_id", smp_fetch_ssl_fc_unique_id, 0,
>>> NULL, SMP_T_BIN, SMP_USE_L5SRV },
>>> { "ssl_bc_use_keysize", smp_fetch_ssl_fc_use_keysize, 0,
>>> NULL, SMP_T_SINT, SMP_USE_L5SRV },
>>> { "ssl_bc_session_id", smp_fetch_ssl_fc_session_id, 0,
>>> NULL, SMP_T_BIN, SMP_USE_L5SRV },
>>> + { "ssl_bc_session_key", smp_fetch_ssl_fc_session_key, 0,
>>> NULL, SMP_T_BIN, SMP_USE_L5SRV },
>> And then this line an be surrounded by #ifdef. The real benefit is that
>> it refuses to parse where it will not work.
>>
>> Thanks,
>> Willy
>
Haven't heard anything back about the consistency aspect, so here's an
updated patch with the other changes not affected by user experience
consistency.
-Patrick
From 62ad353cccebe58af7f39d596f232d7b3bcd8afb Mon Sep 17 00:00:00 2001
From: Patrick Hemmer <[email protected]>
Date: Mon, 12 Jun 2017 18:03:48 -0400
Subject: [PATCH] MINOR: ssl: add fetch 'ssl_fc_session_key' and
'ssl_bc_session_key'
These fetches return the SSL master key of the front/back connection.
This is useful to decrypt traffic encrypted with ephemeral ciphers.
---
doc/configuration.txt | 13 +++++++++++++
src/ssl_sock.c | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 082b857..17c1b1d 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -13930,6 +13930,12 @@ ssl_bc_session_id : binary
made over an SSL/TLS transport layer. It is useful to log if we want to know
if session was reused or not.
+ssl_bc_session_key : binary
+ Returns the SSL session master key of the back connection when the outgoing
+ connection was made over an SSL/TLS transport layer. It is useful to decrypt
+ traffic sent using ephemeral ciphers. This requires OpenSSL >= 1.1.0, or
+ BoringSSL.
+
ssl_bc_use_keysize : integer
Returns the symmetric cipher key size used in bits when the outgoing
connection was made over an SSL/TLS transport layer.
@@ -14185,6 +14191,13 @@ ssl_fc_session_id : binary
a server. It is important to note that some browsers refresh their session ID
every few minutes.
+ssl_fc_session_key : binary
+ Returns the SSL session master key of the front connection when the incoming
+ connection was made over an SSL/TLS transport layer. It is useful to decrypt
+ traffic sent using ephemeral ciphers. This requires OpenSSL >= 1.1.0, or
+ BoringSSL.
+
+
ssl_fc_sni : string
This extracts the Server Name Indication TLS extension (SNI) field from an
incoming connection made via an SSL/TLS transport layer and locally
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 3680515..d6c28b5 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6170,6 +6170,38 @@ smp_fetch_ssl_fc_session_id(const struct arg *args,
struct sample *smp, const ch
}
static int
+smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp, const
char *kw, void *private)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L || defined(OPENSSL_IS_BORINGSSL)
+ struct connection *conn = objt_conn((kw[4] != 'b') ? smp->sess->origin :
+ smp->strm ? smp->strm->si[1].end :
NULL);
+
+ SSL_SESSION *ssl_sess;
+ struct chunk *data;
+
+ if (!conn || !conn->xprt_ctx || conn->xprt != &ssl_sock)
+ return 0;
+
+ ssl_sess = SSL_get_session(conn->xprt_ctx);
+ if (!ssl_sess)
+ return 0;
+
+ data = get_trash_chunk();
+ data->len = SSL_SESSION_get_master_key(ssl_sess, (unsigned char
*)data->str, data->size);
+ if (!data->len)
+ return 0;
+
+ smp->flags = SMP_F_CONST;
+ smp->data.type = SMP_T_BIN;
+ smp->data.u.str = *data;
+
+ return 1;
+#else
+ return 0;
+#endif
+}
+
+static int
smp_fetch_ssl_fc_sni(const struct arg *args, struct sample *smp, const char
*kw, void *private)
{
#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
@@ -7841,6 +7873,7 @@ static struct sample_fetch_kw_list sample_fetch_keywords
= {ILH, {
{ "ssl_bc_unique_id", smp_fetch_ssl_fc_unique_id, 0,
NULL, SMP_T_BIN, SMP_USE_L5SRV },
{ "ssl_bc_use_keysize", smp_fetch_ssl_fc_use_keysize, 0,
NULL, SMP_T_SINT, SMP_USE_L5SRV },
{ "ssl_bc_session_id", smp_fetch_ssl_fc_session_id, 0,
NULL, SMP_T_BIN, SMP_USE_L5SRV },
+ { "ssl_bc_session_key", smp_fetch_ssl_fc_session_key, 0,
NULL, SMP_T_BIN, SMP_USE_L5SRV },
{ "ssl_c_ca_err", smp_fetch_ssl_c_ca_err, 0,
NULL, SMP_T_SINT, SMP_USE_L5CLI },
{ "ssl_c_ca_err_depth", smp_fetch_ssl_c_ca_err_depth, 0,
NULL, SMP_T_SINT, SMP_USE_L5CLI },
{ "ssl_c_der", smp_fetch_ssl_x_der, 0,
NULL, SMP_T_BIN, SMP_USE_L5CLI },
@@ -7882,6 +7915,7 @@ static struct sample_fetch_kw_list sample_fetch_keywords
= {ILH, {
{ "ssl_fc_unique_id", smp_fetch_ssl_fc_unique_id, 0,
NULL, SMP_T_BIN, SMP_USE_L5CLI },
{ "ssl_fc_use_keysize", smp_fetch_ssl_fc_use_keysize, 0,
NULL, SMP_T_SINT, SMP_USE_L5CLI },
{ "ssl_fc_session_id", smp_fetch_ssl_fc_session_id, 0,
NULL, SMP_T_BIN, SMP_USE_L5CLI },
+ { "ssl_fc_session_key", smp_fetch_ssl_fc_session_key, 0,
NULL, SMP_T_BIN, SMP_USE_L5CLI },
{ "ssl_fc_sni", smp_fetch_ssl_fc_sni, 0,
NULL, SMP_T_STR, SMP_USE_L5CLI },
{ "ssl_fc_cipherlist_bin", smp_fetch_ssl_fc_cl_bin, 0,
NULL, SMP_T_STR, SMP_USE_L5CLI },
{ "ssl_fc_cipherlist_hex", smp_fetch_ssl_fc_cl_hex, 0,
NULL, SMP_T_BIN, SMP_USE_L5CLI },
--
2.7.4 (Apple Git-66)
