MEDIUM: ssl: Keep a reference to the client's certificate for use in logs

Most of the SSL sample fetches related to the client certificate were
based on the SSL_get_peer_certificate function which returns NULL when
the verification process failed. This made it impossible to use those
fetches in a log format since they would always be empty.

The patch adds a reference to the X509 object representing the client
certificate in the SSL structure and makes use of this reference in the
fetches.

The reference can only be obtained in ssl_sock_bind_verifycbk which
means that in case of an SSL error occurring before the verification
process ("no shared cipher" for instance, which happens while processing
the Client Hello), we won't ever start the verification process and it
will be impossible to get information about the client certificate.

This patch also allows most of the ssl_c_XXX fetches to return a usable
value in case of connection failure (because of a verification error for
instance) by making the "conn->flags & CO_FL_WAIT_XPRT" test (which
requires a connection to be established) less strict.

Thanks to this patch, a log-format such as the following should return
usable information in case of an error occurring during the verification
process :
    log-format "DN=%{+Q}[ssl_c_s_dn] serial=%[ssl_c_serial,hex] \
                hash=%[ssl_c_sha1,hex]"

It should answer to GitHub issue #693.

diff --git a/include/haproxy/ssl_utils.h b/include/haproxy/ssl_utils.h
index 2ebe034a34fa3d558134863ebd60d1fa3274561c..28c5a8472616b0f7d01476081c4280e023a4ba0f 100644 (file)
--- a/include/haproxy/ssl_utils.h
+++ b/include/haproxy/ssl_utils.h
@@ -39,6 +39,8 @@ int ssl_sock_get_dn_entry(X509_NAME *a, const struct buffer *entry, int pos,
 int ssl_sock_get_dn_formatted(X509_NAME *a, const struct buffer *format, struct buffer *out);
 int ssl_sock_get_dn_oneline(X509_NAME *a, struct buffer *out);
 
+X509* ssl_sock_get_peer_certificate(SSL *ssl);
+
 #endif /* _HAPROXY_SSL_UTILS_H */
 #endif /* USE_OPENSSL */
 

diff --git a/reg-tests/ssl/ssl_errors.vtc b/reg-tests/ssl/ssl_errors.vtc
index f138249158efa516d0cb239a3227db51dcb2354a..0d652d4a08014e5bdb57115e0e04e1a6dea186bc 100644 (file)
--- a/reg-tests/ssl/ssl_errors.vtc
+++ b/reg-tests/ssl/ssl_errors.vtc
@@ -39,22 +39,25 @@ barrier b1 cond 4 -cyclic
 
 syslog Slg_cust_fmt -level info {
     recv
-    expect ~ ".*conn_status:\"0:Success\" hsk_err:\"0:-\""
+    expect ~ ".*conn_status:\"0:Success\" hsk_err:\"0:-\" CN=\"/C=FR/O=HAProxy Technologies/CN=Client\",serial=1007,hash=063DCC2E6A9159E66994B325D6D2EF3D17A75B6F"
 
     barrier b1 sync
 
     recv
-    expect ~ ".*conn_status:\"30:SSL client CA chain cannot be verified\" hsk_err:\"337100934:error:1417C086:SSL routines:tls_process_client_certificate:certificate verify failed\""
+    expect ~ ".*conn_status:\"30:SSL client CA chain cannot be verified\" hsk_err:\"337100934:error:1417C086:SSL routines:tls_process_client_certificate:certificate verify failed\" CN=\"/C=FR/O=HAProxy Technologies/CN=Client\",serial=1007,hash=063DCC2E6A9159E66994B325D6D2EF3D17A75B6F"
 
     barrier b1 sync
 
     recv
-    expect ~ ".*conn_status:\"31:SSL client certificate not trusted\" hsk_err:\"337100934:error:1417C086:SSL routines:tls_process_client_certificate:certificate verify failed\""
+    expect ~ ".*conn_status:\"31:SSL client certificate not trusted\" hsk_err:\"337100934:error:1417C086:SSL routines:tls_process_client_certificate:certificate verify failed\" CN=\"/C=FR/O=HAProxy Technologies/CN=Client\",serial=1007,hash=063DCC2E6A9159E66994B325D6D2EF3D17A75B6F"
 
     barrier b1 sync
 
+    # In case of an error occuring before the certificate verification process,
+    # the client certificate chain is never parsed and verified so we can't
+    # have information about the client's certificate.
     recv
-    expect ~ ".*conn_status:\"34:SSL handshake failure\" hsk_err:\"337678529:error:142090C1:SSL routines:tls_early_post_process_client_hello:no shared cipher\""
+    expect ~ ".*conn_status:\"34:SSL handshake failure\" hsk_err:\"337678529:error:142090C1:SSL routines:tls_early_post_process_client_hello:no shared cipher\" CN=\"\",serial=-,hash=-"
 } -start
 
 syslog Slg_https_fmt -level info {
@@ -136,7 +139,7 @@ haproxy h1 -conf {
         log ${Slg_cust_fmt_addr}:${Slg_cust_fmt_port} local0
        option log-error-via-logformat
         mode http
-        log-format "conn_status:\"%[fc_conn_err]:%[fc_conn_err_str]\" hsk_err:\"%[ssl_fc_hsk_err]:%[ssl_fc_hsk_err_str]\""
+        log-format "conn_status:\"%[fc_conn_err]:%[fc_conn_err_str]\" hsk_err:\"%[ssl_fc_hsk_err]:%[ssl_fc_hsk_err_str]\" CN=%{+Q}[ssl_c_s_dn],serial=%[ssl_c_serial,hex],hash=%[ssl_c_sha1,hex]"
         bind "${tmpdir}/cust_logfmt_ssl.sock" ssl crt ${testdir}/set_cafile_server.pem ca-verify-file ${testdir}/set_cafile_rootCA.crt ca-file ${testdir}/set_cafile_interCA1.crt verify required ciphersuites "TLS_AES_256_GCM_SHA384"
         server s1 ${s1_addr}:${s1_port}
 

diff --git a/src/ssl_sample.c b/src/ssl_sample.c
index 8bfea07f0b11fe02be933905917d047c56dbf8f5..f4e0c93a814bcb2137db57e21ca91261f9b6bae8 100644 (file)
--- a/src/ssl_sample.c
+++ b/src/ssl_sample.c
@@ -109,13 +109,13 @@ smp_fetch_ssl_x_der(const struct arg *args, struct sample *smp, const char *kw,
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
 
@@ -226,13 +226,13 @@ smp_fetch_ssl_x_serial(const struct arg *args, struct sample *smp, const char *k
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
 
@@ -280,13 +280,13 @@ smp_fetch_ssl_x_sha1(const struct arg *args, struct sample *smp, const char *kw,
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -331,13 +331,13 @@ smp_fetch_ssl_x_notafter(const struct arg *args, struct sample *smp, const char
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -383,13 +383,13 @@ smp_fetch_ssl_x_i_dn(const struct arg *args, struct sample *smp, const char *kw,
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -451,13 +451,13 @@ smp_fetch_ssl_x_notbefore(const struct arg *args, struct sample *smp, const char
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -503,13 +503,13 @@ smp_fetch_ssl_x_s_dn(const struct arg *args, struct sample *smp, const char *kw,
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -560,13 +560,13 @@ smp_fetch_ssl_c_used(const struct arg *args, struct sample *smp, const char *kw,
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        /* SSL_get_peer_certificate returns a ptr on allocated X509 struct */
-       crt = SSL_get_peer_certificate(ssl);
+       crt = ssl_sock_get_peer_certificate(ssl);
        if (crt) {
                X509_free(crt);
        }
@@ -599,13 +599,13 @@ smp_fetch_ssl_x_version(const struct arg *args, struct sample *smp, const char *
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -645,13 +645,13 @@ smp_fetch_ssl_x_sig_alg(const struct arg *args, struct sample *smp, const char *
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)
@@ -701,13 +701,13 @@ smp_fetch_ssl_x_key_alg(const struct arg *args, struct sample *smp, const char *
        if (!ssl)
                return 0;
 
-       if (conn->flags & CO_FL_WAIT_XPRT) {
+       if (conn->flags & CO_FL_WAIT_XPRT && !conn->err_code) {
                smp->flags |= SMP_F_MAY_CHANGE;
                return 0;
        }
 
        if (cert_peer)
-               crt = SSL_get_peer_certificate(ssl);
+               crt = ssl_sock_get_peer_certificate(ssl);
        else
                crt = SSL_get_certificate(ssl);
        if (!crt)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index ba61243aca5730d777b08d6fe21e6a70e7fccbb9..bc827833da3d23b73cd584ed2a79e74865835882 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -443,6 +443,8 @@ struct pool_head *pool_head_ssl_keylog __read_mostly = NULL;
 struct pool_head *pool_head_ssl_keylog_str __read_mostly = NULL;
 #endif
 
+int ssl_client_crt_ref_index = -1;
+
 #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0)
 struct list tlskeys_reference = LIST_HEAD_INIT(tlskeys_reference);
 #endif
@@ -1557,19 +1559,51 @@ int ssl_sock_bind_verifycbk(int ok, X509_STORE_CTX *x_store)
        struct connection *conn;
        struct ssl_sock_ctx *ctx;
        int err, depth;
+       X509 *client_crt;
+       STACK_OF(X509) *certs;
 
        ssl = X509_STORE_CTX_get_ex_data(x_store, SSL_get_ex_data_X509_STORE_CTX_idx());
        conn = SSL_get_ex_data(ssl, ssl_app_data_index);
+       client_crt = SSL_get_ex_data(ssl, ssl_client_crt_ref_index);
 
        ctx = conn->xprt_ctx;
 
        ctx->xprt_st |= SSL_SOCK_ST_FL_VERIFY_DONE;
 
+       depth = X509_STORE_CTX_get_error_depth(x_store);
+       err = X509_STORE_CTX_get_error(x_store);
+
        if (ok) /* no errors */
                return ok;
 
-       depth = X509_STORE_CTX_get_error_depth(x_store);
-       err = X509_STORE_CTX_get_error(x_store);
+       /* Keep a reference to the client's certificate in order to be able to
+        * dump some fetches values in a log even when the verification process
+        * fails. */
+       if (depth == 0) {
+               X509_free(client_crt);
+               client_crt = X509_STORE_CTX_get0_cert(x_store);
+               if (client_crt) {
+                       X509_up_ref(client_crt);
+                       SSL_set_ex_data(ssl, ssl_client_crt_ref_index, client_crt);
+               }
+       }
+       else {
+               /* An error occurred on a CA certificate of the certificate
+                * chain, we might never call this verify callback on the client
+                * certificate's depth (which is 0) so we try to store the
+                * reference right now. */
+               if (X509_STORE_CTX_get0_chain(x_store) != NULL) {
+                       certs = X509_STORE_CTX_get1_chain(x_store);
+                       if (certs) {
+                               client_crt = sk_X509_value(certs, 0);
+                               if (client_crt) {
+                                       X509_up_ref(client_crt);
+                                       SSL_set_ex_data(ssl, ssl_client_crt_ref_index, client_crt);
+                               }
+                       }
+                       sk_X509_pop_free(certs, X509_free);
+               }
+       }
 
        /* check if CA error needs to be ignored */
        if (depth > 0) {
@@ -7330,6 +7364,14 @@ static void ssl_sock_keylog_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *a
 }
 #endif
 
+static void ssl_sock_clt_crt_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp)
+{
+       if (!ptr)
+               return;
+
+       X509_free((X509*)ptr);
+}
+
 __attribute__((constructor))
 static void __ssl_sock_init(void)
 {
@@ -7377,6 +7419,7 @@ static void __ssl_sock_init(void)
 #ifdef HAVE_SSL_KEYLOG
        ssl_keylog_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_keylog_free_func);
 #endif
+       ssl_client_crt_ref_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_clt_crt_free_func);
 #ifndef OPENSSL_NO_ENGINE
        ENGINE_load_builtin_engines();
        hap_register_post_check(ssl_check_async_engine_count);

diff --git a/src/ssl_utils.c b/src/ssl_utils.c
index cb941409c9615bce02ac5ea05a61e38db76030ad..ce17e955342dc21b496ece3022f8c6291839a199 100644 (file)
--- a/src/ssl_utils.c
+++ b/src/ssl_utils.c
@@ -287,3 +287,31 @@ int ssl_sock_get_dn_oneline(X509_NAME *a, struct buffer *out)
        return 1;
 }
 
+
+extern int ssl_client_crt_ref_index;
+
+/*
+ * This function fetches the SSL certificate for a specific connection (either
+ * client certificate or server certificate depending on the cert_peer
+ * parameter).
+ * When trying to get the peer certificate from the server side, we first try to
+ * use the dedicated SSL_get_peer_certificate function, but we fall back to
+ * trying to get the client certificate reference that might have been stored in
+ * the SSL structure's ex_data during the verification process.
+ * Returns NULL in case of failure.
+ */
+X509* ssl_sock_get_peer_certificate(SSL *ssl)
+{
+       X509* cert;
+
+       cert = SSL_get_peer_certificate(ssl);
+       /* Get the client certificate reference stored in the SSL
+        * structure's ex_data during the verification process. */
+       if (!cert) {
+               cert = SSL_get_ex_data(ssl, ssl_client_crt_ref_index);
+               if (cert)
+                       X509_up_ref(cert);
+       }
+
+       return cert;
+}