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