Re: [PATCH 0/2] Re: Logging SSL pre-master-key

[Willy Tarreau]

On Sat, Apr 28, 2018 at 07:15:44PM -0400, Patrick Hemmer wrote:
> After much delay, I've addressed the requested changes as a new patch.

Both patches merged now (with SMP_F_CONST removed as noticed by Emeric).

Thanks!
Willy

[PATCH 0/2] Re: Logging SSL pre-master-key

[Patrick Hemmer]

On 2017/6/30 10:32, Willy Tarreau wrote:
> Hi Patrick,
>
> On Fri, Jun 30, 2017 at 10:28:11AM -0400, Patrick Hemmer wrote:
>>> The issue I'm having is that there's no notification
>>> that this will not work. Using #ifdef ensures that what is not
supported will
>>> report an error. Then the user looks at the keyword in the doc and reads
>>> "requires openssl 1.1 or above" and understands why there's this
problem.
>> I can include an additional patch to change the existing SSL fetches to
>> the desired behavior. Would that be acceptable?
>
> Yes I think it would be a good idea. It may break a config or two while
> upgrading to 1.8 but for good since these ones silently fail right now.
>
> Thanks,
> Willy


After much delay, I've addressed the requested changes as a new patch.

-Patrick

Re: Logging SSL pre-master-key

[Willy Tarreau]

Hi Patrick,

On Fri, Jun 30, 2017 at 10:28:11AM -0400, Patrick Hemmer wrote:
> > The issue I'm having is that there's no notification
> > that this will not work. Using #ifdef ensures that what is not supported 
> > will
> > report an error. Then the user looks at the keyword in the doc and reads
> > "requires openssl 1.1 or above" and understands why there's this problem.
> I can include an additional patch to change the existing SSL fetches to
> the desired behavior. Would that be acceptable?

Yes I think it would be a good idea. It may break a config or two while
upgrading to 1.8 but for good since these ones silently fail right now. 

Thanks,
Willy

Re: Logging SSL pre-master-key

[Patrick Hemmer]

On 2017/6/30 01:00, Willy Tarreau wrote:
> Hi Patrick, sorry for the delay :-/
>
> On Mon, Jun 19, 2017 at 01:54:36PM -0400, Patrick Hemmer wrote:
>> 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).
> OK, that totally makes sense then.
>
  static int
 +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp, 
 const char *kw, void *private)
 +{
 +#if OPENSSL_VERSION_NUMBER >= 0x1010L || 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.
> But this has already been causing problems, because people build with their
> lib, then configure their logs, and suddenly realize that the field is empty
> and report the bug here. The issue I'm having is that there's no notification
> that this will not work. Using #ifdef ensures that what is not supported will
> report an error. Then the user looks at the keyword in the doc and reads
> "requires openssl 1.1 or above" and understands why there's this problem.
I can include an additional patch to change the existing SSL fetches to
the desired behavior. Would that be acceptable?
>
>> The ALPN/NPN fetches are the only ones
>> that make using the fetch a config error if the underlying support is
>> missing.
> For SSL indeed, but if you look at other places like TCP, bind keywords
> or server keywords, it's exactly the opposite. There are other places
> which are cleaner, it's only the arg resolver which has the ifdef so
> that instead of not parsing, it's parsed and reported as "not supported"
> or "requires version blah". I just can't find such an example but I'm
> pretty sure we do have some.
>
> Cheers,
> Willy

Re: Logging SSL pre-master-key

[Emmanuel Hocdet]

Hi Willy, Patrick

> Le 30 juin 2017 à 07:00, Willy Tarreau  a écrit :
> 
> Hi Patrick, sorry for the delay :-/
> 
> On Mon, Jun 19, 2017 at 01:54:36PM -0400, Patrick Hemmer wrote:
>> 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).
> 
> OK, that totally makes sense then.
> 
 static int
 +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp, 
 const char *kw, void *private)
 +{
 +#if OPENSSL_VERSION_NUMBER >= 0x1010L || 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.
> 
> But this has already been causing problems, because people build with their
> lib, then configure their logs, and suddenly realize that the field is empty
> and report the bug here. The issue I'm having is that there's no notification
> that this will not work. Using #ifdef ensures that what is not supported will
> report an error. Then the user looks at the keyword in the doc and reads
> "requires openssl 1.1 or above" and understands why there's this problem.
> 
>> The ALPN/NPN fetches are the only ones
>> that make using the fetch a config error if the underlying support is
>> missing.
> 
> For SSL indeed, but if you look at other places like TCP, bind keywords
> or server keywords, it's exactly the opposite. There are other places
> which are cleaner, it's only the arg resolver which has the ifdef so
> that instead of not parsing, it's parsed and reported as "not supported"
> or "requires version blah". I just can't find such an example but I'm
> pretty sure we do have some.
> 

ALPN/NPN fetches depend inderectly on ALPN//NPN settings.
. used ALPN/NPN fetches could be always valid because without ALPN/NPN
  setting support  ALPN/NPN fetches return 0 is valid.
. ALPN//NPN settings return error if lib doesn’t support it

++
Manu

Re: Logging SSL pre-master-key

[Willy Tarreau]

Hi Patrick, sorry for the delay :-/

On Mon, Jun 19, 2017 at 01:54:36PM -0400, Patrick Hemmer wrote:
> 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).

OK, that totally makes sense then.

> >>  static int
> >> +smp_fetch_ssl_fc_session_key(const struct arg *args, struct sample *smp, 
> >> const char *kw, void *private)
> >> +{
> >> +#if OPENSSL_VERSION_NUMBER >= 0x1010L || 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.

But this has already been causing problems, because people build with their
lib, then configure their logs, and suddenly realize that the field is empty
and report the bug here. The issue I'm having is that there's no notification
that this will not work. Using #ifdef ensures that what is not supported will
report an error. Then the user looks at the keyword in the doc and reads
"requires openssl 1.1 or above" and understands why there's this problem.

> The ALPN/NPN fetches are the only ones
> that make using the fetch a config error if the underlying support is
> missing.

For SSL indeed, but if you look at other places like TCP, bind keywords
or server keywords, it's exactly the opposite. There are other places
which are cleaner, it's only the arg resolver which has the ifdef so
that instead of not parsing, it's parsed and reported as "not supported"
or "requires version blah". I just can't find such an example but I'm
pretty sure we do have some.

Cheers,
Willy

Re: Logging SSL pre-master-key

[Willy Tarreau]

Hi Patrick,

On Thu, Jun 22, 2017 at 03:57:18PM -0400, Patrick Hemmer wrote:
> 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.

Sorry, I've been quite busy these last days and didn't have time to
look at your updated patch. I'm keeping your mail marked unread to
get back to you as stuff cools down.

Willy

Re: Logging SSL pre-master-key

[Patrick Hemmer]

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 >= 0x1010L || 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 != _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,

Re: Logging SSL pre-master-key

[Patrick Hemmer]

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 >= 0x1010L || 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 != _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,  

Re: Logging SSL pre-master-key

[Willy Tarreau]

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.

> +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 >= 0x1010L || 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.

> + 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 != _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

Re: Logging SSL pre-master-key

[Patrick Hemmer]

On 2017/6/16 09:34, Willy Tarreau wrote:
> Hi Patrick,
>
> On Mon, Jun 12, 2017 at 07:31:36PM -0400, Patrick Hemmer wrote:
>> I patched my haproxy to add a ssl_fc_session_key fetch, and with the
>> value I was able to decrypt my test sessions encrypted with
>> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256.
>>
>> Since the implementation was fairly easy, I've included a patch for it.
>> But I've never submitted anything before, so there's a good chance of
>> something being wrong.
> No problem, that's what public review is made for. BTW at first glance
> your patch looks clean ;-)
>
>> The only thing is that the function to do the extraction was added in
>> 1.1.0
>> (https://github.com/openssl/openssl/commit/858618e7e037559b75b0bfca4d30440f9515b888)
>> The underlying vars are still there, and when I looked have been there
>> since as early as I could find (going back to 1998). But I'm not sure
>> how you feel about extracting the values without the helper function.
> I'd then suggest to proceed differently (if that's OK for you), which
> is to only expose this sample fetch function in 1.1.0 and above. If
> you're fine with running on 1.1 you won't feel any difference. Others
> who don't need this sample fetch right now will not feel any risk of
> build problem.
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.

Anyway, here is the updated patch with the support for <1.1.0 dropped,
as well as the BoringSSL support Emmanuel requested.

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.

-Patrick
From 217f02d6cd39cb0f40cae74098acf3b586442194 Mon Sep 17 00:00:00 2001
From: Patrick Hemmer 
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 | 10 ++
 src/ssl_sock.c| 36 
 2 files changed, 46 insertions(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 49bfd85..3b9d96e 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -13930,6 +13930,11 @@ 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.
+
 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 +14190,11 @@ 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.
+
 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..73fbc31 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6170,6 +6170,40 @@ 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 >= 0x1010L || 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;
+   int data_len;
+   struct chunk *data;
+
+   if (!conn || !conn->xprt_ctx || conn->xprt != _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 = 

Re: Logging SSL pre-master-key

[Willy Tarreau]

Hi Patrick,

On Mon, Jun 12, 2017 at 07:31:36PM -0400, Patrick Hemmer wrote:
> I patched my haproxy to add a ssl_fc_session_key fetch, and with the
> value I was able to decrypt my test sessions encrypted with
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256.
> 
> Since the implementation was fairly easy, I've included a patch for it.
> But I've never submitted anything before, so there's a good chance of
> something being wrong.

No problem, that's what public review is made for. BTW at first glance
your patch looks clean ;-)

> The only thing is that the function to do the extraction was added in
> 1.1.0
> (https://github.com/openssl/openssl/commit/858618e7e037559b75b0bfca4d30440f9515b888)
> The underlying vars are still there, and when I looked have been there
> since as early as I could find (going back to 1998). But I'm not sure
> how you feel about extracting the values without the helper function.

I'd then suggest to proceed differently (if that's OK for you), which
is to only expose this sample fetch function in 1.1.0 and above. If
you're fine with running on 1.1 you won't feel any difference. Others
who don't need this sample fetch right now will not feel any risk of
build problem.

Cheers,
Willy

Re: Logging SSL pre-master-key

[Emmanuel Hocdet]

Hi Patrick, Lukas

> Le 13 juin 2017 à 19:26, Lukas Tribus  a écrit :
> 
> Hi Patrick,
> 
> 
> Am 13.06.2017 um 01:31 schrieb Patrick Hemmer:
>> 
>> 
>> On 2017/6/12 15:14, Lukas Tribus wrote:
>>> Hello,
>>> 
>>> 
>>> Am 12.06.2017 um 19:35 schrieb Patrick Hemmer:
 Would we be able to get a new sample which provides the SSL session
 master-key?
 This is so that when performing packet captures with ephemeral ciphers
 (DHE), we can decrypt the traffic in the capture.
>>> There is no master key. What you need is the key for the symmetric
>>> crypto, and you cannot extract it from haproxy currently.
>>> 
>>> More importantly, OpenSSL implements this functionality only the master
>>> branch (see [1] and [2]), none of the release branches actually have
>>> this functionality.
>>> So we need OpenSSL to release a new branch with this functionality
>>> (1.1.1), we have to implement it in haproxy and then still it will only
>>> work for <=TLSv1.2.
>>> 
>>> TLSv1.3 will need additional secrets and a different key logging API [3].
>>> 
>>> 
>>> I suggest you use SSLKEYLOGFILE features in the browsers at this point,
>>> as the functionality is far from being ready for any OpenSSL based
>>> application.
>>> 
>>> 
>>> Regards,
>>> Lukas
>>> 
>>> [1]
>>> https://github.com/openssl/openssl/commit/2faa1b48fd6864f6bb8f992fd638378202fdd416
>>> [2]
>>> https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_keylog_callback.html
>>> [3] https://github.com/openssl/openssl/pull/2287
>>> 
>> 
>> Maybe there's some misunderstanding, because we seem to be talking
>> about different things, as there definitely is a master key.
>> 
>> I patched my haproxy to add a ssl_fc_session_key fetch, and with the
>> value I was able to decrypt my test sessions encrypted with
>> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256.
> 
> We are talking about the same thing, I just find the term "master key"
> confusing, but I understand that is what everyone uses in this context.
> 
> 
> 
>> 
>> Since the implementation was fairly easy, I've included a patch for
>> it. But I've never submitted anything before, so there's a good chance
>> of something being wrong.
>> 
>> The only thing is that the function to do the extraction was added in
>> 1.1.0
>> (https://github.com/openssl/openssl/commit/858618e7e037559b75b0bfca4d30440f9515b888
>>  
>> )
> 
> Very nice, so the openssl features I mentioned are only necessary if we
> need callback functions (which is not the case), so we can already do
> this in 1.1.0.
> 
> 
> Patch needs review, CC +Emeric +Emmanuel.
> 
> 

Sounds good.

SSL_SESSION_get_master_key also exist in boringssl, the #if should be:
#if (OPENSSL_VERSION_NUMBER >= 0x1010fL) || defined(OPENSSL_IS_BORINGSSL)

Manu

Re: Logging SSL pre-master-key

[Lukas Tribus]

Hi Patrick,


Am 13.06.2017 um 01:31 schrieb Patrick Hemmer:
>
>
> On 2017/6/12 15:14, Lukas Tribus wrote:
>> Hello,
>>
>>
>> Am 12.06.2017 um 19:35 schrieb Patrick Hemmer:
>>> Would we be able to get a new sample which provides the SSL session
>>> master-key?
>>> This is so that when performing packet captures with ephemeral ciphers
>>> (DHE), we can decrypt the traffic in the capture.
>> There is no master key. What you need is the key for the symmetric
>> crypto, and you cannot extract it from haproxy currently.
>>
>> More importantly, OpenSSL implements this functionality only the master
>> branch (see [1] and [2]), none of the release branches actually have
>> this functionality.
>> So we need OpenSSL to release a new branch with this functionality
>> (1.1.1), we have to implement it in haproxy and then still it will only
>> work for <=TLSv1.2.
>>
>> TLSv1.3 will need additional secrets and a different key logging API [3].
>>
>>
>> I suggest you use SSLKEYLOGFILE features in the browsers at this point,
>> as the functionality is far from being ready for any OpenSSL based
>> application.
>>
>>
>> Regards,
>> Lukas
>>
>> [1]
>> https://github.com/openssl/openssl/commit/2faa1b48fd6864f6bb8f992fd638378202fdd416
>> [2]
>> https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_keylog_callback.html
>> [3] https://github.com/openssl/openssl/pull/2287
>>
>
> Maybe there's some misunderstanding, because we seem to be talking
> about different things, as there definitely is a master key.
>
> I patched my haproxy to add a ssl_fc_session_key fetch, and with the
> value I was able to decrypt my test sessions encrypted with
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256.

We are talking about the same thing, I just find the term "master key"
confusing, but I understand that is what everyone uses in this context.



>
> Since the implementation was fairly easy, I've included a patch for
> it. But I've never submitted anything before, so there's a good chance
> of something being wrong.
>
> The only thing is that the function to do the extraction was added in
> 1.1.0
> (https://github.com/openssl/openssl/commit/858618e7e037559b75b0bfca4d30440f9515b888)

Very nice, so the openssl features I mentioned are only necessary if we
need callback functions (which is not the case), so we can already do
this in 1.1.0.


Patch needs review, CC +Emeric +Emmanuel.



Regards,
Lukas

Re: Logging SSL pre-master-key

[Patrick Hemmer]

On 2017/6/12 15:14, Lukas Tribus wrote:
> Hello,
>
>
> Am 12.06.2017 um 19:35 schrieb Patrick Hemmer:
>> Would we be able to get a new sample which provides the SSL session
>> master-key?
>> This is so that when performing packet captures with ephemeral ciphers
>> (DHE), we can decrypt the traffic in the capture.
> There is no master key. What you need is the key for the symmetric
> crypto, and you cannot extract it from haproxy currently.
>
> More importantly, OpenSSL implements this functionality only the master
> branch (see [1] and [2]), none of the release branches actually have
> this functionality.
> So we need OpenSSL to release a new branch with this functionality
> (1.1.1), we have to implement it in haproxy and then still it will only
> work for <=TLSv1.2.
>
> TLSv1.3 will need additional secrets and a different key logging API [3].
>
>
> I suggest you use SSLKEYLOGFILE features in the browsers at this point,
> as the functionality is far from being ready for any OpenSSL based
> application.
>
>
> Regards,
> Lukas
>
> [1]
> https://github.com/openssl/openssl/commit/2faa1b48fd6864f6bb8f992fd638378202fdd416
> [2]
> https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_keylog_callback.html
> [3] https://github.com/openssl/openssl/pull/2287
>

Maybe there's some misunderstanding, because we seem to be talking about
different things, as there definitely is a master key.

I patched my haproxy to add a ssl_fc_session_key fetch, and with the
value I was able to decrypt my test sessions encrypted with
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256.

Since the implementation was fairly easy, I've included a patch for it.
But I've never submitted anything before, so there's a good chance of
something being wrong.

The only thing is that the function to do the extraction was added in
1.1.0
(https://github.com/openssl/openssl/commit/858618e7e037559b75b0bfca4d30440f9515b888)
The underlying vars are still there, and when I looked have been there
since as early as I could find (going back to 1998). But I'm not sure
how you feel about extracting the values without the helper function.

-Patrick
From a6fa01c65f615887b08f86bb67ac7ef6231dbc34 Mon Sep 17 00:00:00 2001
From: Patrick Hemmer 
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 | 10 ++
 src/ssl_sock.c| 45 +
 2 files changed, 55 insertions(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 49bfd85..e7cfd85 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -13930,6 +13930,11 @@ 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 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.
+
 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 +14190,11 @@ 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 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.
+
 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 af09cfb..2fe7e2f 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6170,6 +6170,49 @@ 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)
+{
+   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;
+
+   smp->flags = SMP_F_CONST;
+   smp->data.type = SMP_T_BIN;
+
+   if (!conn || !conn->xprt_ctx || conn->xprt != _sock)
+   return 0;
+
+   ssl_sess = SSL_get_session(conn->xprt_ctx);
+   if (!ssl_sess)
+   return 0;
+
+
+#if OPENSSL_VERSION_NUMBER >= 0x1010L
+   data = get_trash_chunk();
+   data_len = SSL_SESSION_get_master_key(ssl_sess, (unsigned char 
*)data->str, data->size);
+   if (!data_len)
+   return 0;
+#else
+ 

Re: Logging SSL pre-master-key

[Lukas Tribus]

Hello,


Am 12.06.2017 um 19:35 schrieb Patrick Hemmer:
> Would we be able to get a new sample which provides the SSL session
> master-key?
> This is so that when performing packet captures with ephemeral ciphers
> (DHE), we can decrypt the traffic in the capture.

There is no master key. What you need is the key for the symmetric
crypto, and you cannot extract it from haproxy currently.

More importantly, OpenSSL implements this functionality only the master
branch (see [1] and [2]), none of the release branches actually have
this functionality.
So we need OpenSSL to release a new branch with this functionality
(1.1.1), we have to implement it in haproxy and then still it will only
work for <=TLSv1.2.

TLSv1.3 will need additional secrets and a different key logging API [3].


I suggest you use SSLKEYLOGFILE features in the browsers at this point,
as the functionality is far from being ready for any OpenSSL based
application.


Regards,
Lukas

[1]
https://github.com/openssl/openssl/commit/2faa1b48fd6864f6bb8f992fd638378202fdd416
[2]
https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_keylog_callback.html
[3] https://github.com/openssl/openssl/pull/2287