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
