BUG/MINOR: ssl: Fix update of default certificate

The default SSL_CTX used by a specific frontend is the one of the first
ckch instance created for this frontend. If this instance has SNIs, then
the SSL context is linked to the instance through the list of SNIs
contained in it. If the instance does not have any SNIs though, then the
SSL_CTX is only referenced by the bind_conf structure and the instance
itself has no link to it.
When trying to update a certificate used by the default instance through
a cli command, a new version of the default instance was rebuilt but the
default SSL context referenced in the bind_conf structure would not be
changed, resulting in a buggy behavior in which depending on the SNI
used by the client, he could either use the new version of the updated
certificate or the original one.

This patch adds a reference to the default SSL context in the default
ckch instances so that it can be hot swapped during a certificate
update.

This should fix GitHub issue #1143.

It can be backported as far as 2.2.

diff --git a/include/haproxy/ssl_ckch-t.h b/include/haproxy/ssl_ckch-t.h
index 2604a523a27a179bdc5987707347ebc652089b5a..f5fd48f024e48835c6f39a78bb97608efeddfb29 100644 (file)
--- a/include/haproxy/ssl_ckch-t.h
+++ b/include/haproxy/ssl_ckch-t.h
@@ -86,7 +86,7 @@ struct ckch_inst {
        struct ckch_store *ckch_store; /* pointer to the store used to generate this inst */
        struct crtlist_entry *crtlist_entry; /* pointer to the crtlist_entry used, or NULL */
        struct server *server; /* pointer to the server if is_server_instance is set, NULL otherwise */
-       SSL_CTX *ctx; /* pointer to the SSL context used by this instance if it is a server one (is_server_instance set) */
+       SSL_CTX *ctx; /* pointer to the SSL context used by this instance */
        unsigned int is_default:1;      /* This instance is used as the default ctx for this bind_conf */
        unsigned int is_server_instance:1; /* This instance is used by a backend server */
        /* space for more flag there */

diff --git a/reg-tests/ssl/set_default_cert.crt-list b/reg-tests/ssl/set_default_cert.crt-list
new file mode 100644 (file)
index 0000000..a0d2caa
--- /dev/null
+++ b/reg-tests/ssl/set_default_cert.crt-list
@@ -0,0 +1,2 @@
+set_default_cert.pem !*
+set_default_cert.pem www.test1.com

diff --git a/reg-tests/ssl/set_default_cert.pem b/reg-tests/ssl/set_default_cert.pem
new file mode 100644 (file)
index 0000000..550208a
--- /dev/null
+++ b/reg-tests/ssl/set_default_cert.pem
@@ -0,0 +1,52 @@
+-----BEGIN CERTIFICATE-----
+MIIENjCCAh4CAQEwDQYJKoZIhvcNAQELBQAwWzELMAkGA1UEBhMCRlIxDjAMBgNV
+BAgMBVBhcmlzMQ4wDAYDVQQHDAVQYXJpczEVMBMGA1UECgwMSEFQcm94eSBUZWNo
+MRUwEwYDVQQDDAxIQVByb3h5IFRlY2gwHhcNMjEwMzAyMTcxODUwWhcNMjIwMzAy
+MTcxODUwWjBnMQswCQYDVQQGEwJGUjETMBEGA1UECAwKU29tZS1TdGF0ZTEOMAwG
+A1UEBwwFUGFyaXMxHTAbBgNVBAoMFEhBUHJveHkgVGVjaG5vbG9naWVzMRQwEgYD
+VQQDDAsqLnRlc3QxLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
+APjmyWLJ1olKg/EOarln7oQB7pdUrF6kS1YG+Nz0sgFzxnU0PHn/IeARCprHyEZ4
+eBOrQ0CHhM5hdEFDX8iq32rektcQqwfH83iwD9zXxFBJ7ItoWG6YAN6WLXjEDYEI
+hxLJMlW3kfYODKhNMvoqXyZi2wTyAJI+aLJI7pbeD+YNb0AwOnSH5ag5ohZIr3QU
+99UD/VUhndv4OP8JZwBiV6Qy79jVDVbPFGaOc70VkMQSCHytyudQicUZrYQdIw1E
+981JF/UpbnECLWyB3V+4t1KtWOW90vkUoBEj8Nxe6kYnMaNSjQhfKSF6zGmUOXYp
+oHPCgms8v4JaovQygo02Qi8CAwEAATANBgkqhkiG9w0BAQsFAAOCAgEAAz8IntYc
+zrbIqweHfD9CZTNIQiobhQmgykT0KQ23Gm2y/e3o63XOqxDv0bEctg4zE83w3g7d
+mJlEyCB0N0qC8UGGsbRm5Cny7H//g3u06NqSMBYbdU+BgZBBj16I5Kcw/kSBb9dA
+wslLlrUjBj6dK83EB1cpyqpyZHIXkR/E424ggfc45rmD60AtU0SvzVNZfIK0PmB0
+3YKiUlO7cl5CzTeTg2BooRvqwblya62SRkhfFL7NCRX1/S9tO/XiaYzgP7J6J09x
+yYs2XhQqJFgtS+1vDp8rHKhcANFVXBJ6rDSbp1qBv7qZkQhlFf8hQtd5iBXvCb0a
+KtN9L4o6t1wvyo0BbERroGU7rkPPUNiMc3gWEf/mgwGLsNNOYqY5eYoeAF7arX5f
+c4LCHiAYMWa/bEY29zmm51GH5ddxFSu1j95Hfd+HlNcX8Oyfed2oCoSamochmbzA
+Kktk0QfCYIv4LlaG5pUliLa6DCLK7yMfT5RC5GGb350p3uDobVj/taY2cVwXOBQb
+MjXK32K9CFrnqKQptPV1ohlWgNiqhvxiGp3Yx17Cn54WL9ksO+8TlwWAttazKVlT
+40tHqGOu6ld90xGZitxL2oA9kBg9Nkxas/f9+9p6sJe5wj09dj/cqRjyiKv7nek1
+TIPtsNbJghDRDQ3uPEYHdX0h490qGMyGARw=
+-----END CERTIFICATE-----
+-----BEGIN RSA PRIVATE KEY-----
+MIIEpgIBAAKCAQEA+ObJYsnWiUqD8Q5quWfuhAHul1SsXqRLVgb43PSyAXPGdTQ8
+ef8h4BEKmsfIRnh4E6tDQIeEzmF0QUNfyKrfat6S1xCrB8fzeLAP3NfEUEnsi2hY
+bpgA3pYteMQNgQiHEskyVbeR9g4MqE0y+ipfJmLbBPIAkj5oskjult4P5g1vQDA6
+dIflqDmiFkivdBT31QP9VSGd2/g4/wlnAGJXpDLv2NUNVs8UZo5zvRWQxBIIfK3K
+51CJxRmthB0jDUT3zUkX9SlucQItbIHdX7i3Uq1Y5b3S+RSgESPw3F7qRicxo1KN
+CF8pIXrMaZQ5dimgc8KCazy/glqi9DKCjTZCLwIDAQABAoIBAQC/arWb7L+56/2W
+iFDZb62GBfpYlXzOeCmb6la/jsvKxB/vCRItfGGv8Usnh9dlIsat0bsxyEcBdP80
+Jb1nFMonZS6miSIPJN4Ahd5dJ+7JFGD/QWso+mtIw1QLGTONdWJztxmnxDpTcbCY
+Sm6W57kvSz1HC1oXHjnkSqR6kCLH9y6/i7ox6IPYyDA1t/TKJMnKFOPkxKJ8A96v
+1avPrCWfXWYdn6Og5ERd8FJF2L5BYImmmkPpoUeWPyMBfAYqdK5FRijO6JMn/h5k
+XkJm+2bru+cRwcNYUNPuDIa+ZBWhjFfZfSOhOoECeKLe+lhfcFPC7cCSeDJAjGtR
+dakm15ohAoGBAP4+rVBeSCBhPH27T3HWp74qMWkYJzkdqTV0wUUJ1wtuWZFDg/RP
+OYKC+6cM0nW3K+j/9pTWMS1eM61x/VNyFQGUq/rMJGEWFH08NXnV8AxCtwKUV/rP
+Uq3MB4QWfSYGMo9QL+9lu23fMWYpBLo+KIcqPjLb+8FEJGmaC9JCIYQfAoGBAPqe
+qp7NzMmX2b1VR2XXm1CZzwTEFXb4NeDIxSfNbsqXCmws3jgBX3Lx7dQ9k8ymvaA5
+ucYLU3ppozeB//8Ir9lSA1A4w3VN9a+l1ZdQpKQ4SuHtqDwkmKAT85vmGHCPhwlq
+Er9ests3wQ4T/8HPG92QWs+Gg34F+x9U6h2FMv/xAoGBAOM6h1HWAeaWoSbKWvWm
+YKNQOHryMFQW011IbVfTtJOt23U9/1hB2mdvw5SInCzDOgZzhiF90dP3Zn5063FB
++84+3vo2q6jtwAAx6KVsdK+wjLpMdNlfpEhamrkOFGoAjf2SMFVo+fv3x8HDlUsT
+NMuhEJgKDlasHVMYb8pKeoQHAoGBAMAF7ij6+lvD03tz6d6oUkJxduLp8qBTEcUH
+T7hteOQU0lGMFz/GHYIOx/EEtUfqwgQP9r09VFrIsdwH6UNZPpM+eXdv5qLsdsB8
+SalEisGguA9fbrWWPLL6Vn8uz67+6bJW6cJjJps8ntjQjffLXkhnII09PWbD4mNh
+RngT5L2hAoGBANqa+yYSvEGNAxvdfxE0u3U/4OtjCl168nNwHXmyaCKZ1e4XYflz
+wGI4J1ngcCKN37RkCgfu/XRKrc82XhAhV+YYjAUqQYrTyh26b4v9Dp9tBUWiv7bk
+6L+ZlCms+HpsuYmsCAu/od41OWSSpdg+R3VOE0t3rp0r1QdAGYd1nwQC
+-----END RSA PRIVATE KEY-----

diff --git a/reg-tests/ssl/set_ssl_cert.vtc b/reg-tests/ssl/set_ssl_cert.vtc
index d6d45261f9d1bf113ee9b532e5b586e1b0bb9c17..a606b477d9761d28953f655a00eb022f57c5d1d2 100644 (file)
--- a/reg-tests/ssl/set_ssl_cert.vtc
+++ b/reg-tests/ssl/set_ssl_cert.vtc
@@ -3,11 +3,19 @@
 # This reg-test uses the "set ssl cert" command to update a certificate over the CLI.
 # It requires socat to upload the certificate
 #
-# this check does 3 requests, the first one will use "www.test1.com" as SNI,
+# This check has two separate parts.
+# In the first part, there are 3 requests, the first one will use "www.test1.com" as SNI,
 # the second one with the same but that must fail and the third one will use
 # "localhost". Since vtest can't do SSL, we use haproxy as an SSL client with 2
 # chained listen section.
 #
+# In the second part, we check the update of a default certificate in a crt-list.
+# This corresponds to a bug raised in https://github.com/haproxy/haproxy/issues/1143.
+# A certificate is used as default certificate as well as regular one, and during the update
+# the default certificate would not be properly updated if the default instance did not have
+# any SNI. The test consists in checking that the used certificate is the right one after
+# updating it via a "set ssl cert" call.
+#
 # If this test does not work anymore:
 # - Check that you have socat
 
@@ -17,7 +25,7 @@ varnishtest "Test the 'set ssl cert' feature of the CLI"
 #REQUIRE_BINARIES=socat
 feature ignore_unknown_macro
 
-server s1 -repeat 3 {
+server s1 -repeat 9 {
   rxreq
   txresp
 } -start
@@ -27,6 +35,7 @@ haproxy h1 -conf {
         tune.ssl.default-dh-param 2048
         tune.ssl.capture-cipherlist-size 1
         stats socket "${tmpdir}/h1/stats" level admin
+        crt-base ${testdir}
 
     defaults
         mode http
@@ -41,15 +50,30 @@ haproxy h1 -conf {
     listen clear-lst
         bind "fd@${clearlst}"
         balance roundrobin
+
+        http-response set-header X-SSL-Server-SHA1 %[ssl_s_sha1,hex]
+
         retries 0 # 2nd SSL connection must fail so skip the retry
         server s1 "${tmpdir}/ssl.sock" ssl verify none sni str(www.test1.com)
         server s2 "${tmpdir}/ssl.sock" ssl verify none sni str(www.test1.com)
         server s3 "${tmpdir}/ssl.sock" ssl verify none sni str(localhost)
 
+        server s4 "${tmpdir}/other-ssl.sock" ssl verify none sni str(www.test1.com)
+        server s5 "${tmpdir}/other-ssl.sock" ssl verify none sni str(other.test1.com) # uses the default certificate
+        server s6 "${tmpdir}/other-ssl.sock" ssl verify none sni str(www.test1.com)
+        server s7 "${tmpdir}/other-ssl.sock" ssl verify none sni str(other.test1.com) # uses the default certificate
+
+        server s8 "${tmpdir}/other-ssl.sock" ssl verify none sni str(www.test1.com)
+        server s9 "${tmpdir}/other-ssl.sock" ssl verify none sni str(other.test1.com) # uses the default certificate
+
     listen ssl-lst
         bind "${tmpdir}/ssl.sock" ssl crt ${testdir}/common.pem strict-sni
+        server s1 ${s1_addr}:${s1_port}
 
+    listen other-ssl-lst
+        bind "${tmpdir}/other-ssl.sock" ssl crt-list ${testdir}/set_default_cert.crt-list
         server s1 ${s1_addr}:${s1_port}
+
 } -start
 
 
@@ -97,3 +121,84 @@ haproxy h1 -cli {
     expect ~ ".*SHA1 FingerPrint: A490D069DBAFBEE66DE434BEC34030ADE8BCCBF1"
 }
 
+
+
+# The following requests are aimed at a backend that uses the set_default_cert.crt-list file
+
+# Uses the www.test1.com sni
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "9DC18799428875976DDE706E9956035EE88A4CB3"
+    expect resp.status == 200
+} -run
+
+# Uses the other.test1.com sni and the default line of the crt-list
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "9DC18799428875976DDE706E9956035EE88A4CB3"
+    expect resp.status == 200
+} -run
+
+shell {
+    printf "set ssl cert ${testdir}/set_default_cert.pem <<\n$(cat ${testdir}/common.pem)\n\n" | socat "${tmpdir}/h1/stats" -
+}
+
+# Certificate should not have changed yet
+haproxy h1 -cli {
+    send "show ssl cert ${testdir}/set_default_cert.pem"
+    expect ~ ".*SHA1 FingerPrint: 9DC18799428875976DDE706E9956035EE88A4CB3"
+}
+
+shell {
+    echo "commit ssl cert ${testdir}/set_default_cert.pem" | socat "${tmpdir}/h1/stats" -
+}
+
+haproxy h1 -cli {
+    send "show ssl cert ${testdir}/set_default_cert.pem"
+    expect ~ ".*SHA1 FingerPrint: 2195C9F0FD58470313013FC27C1B9CF9864BD1C6"
+}
+
+# Uses the www.test1.com sni
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "2195C9F0FD58470313013FC27C1B9CF9864BD1C6"
+    expect resp.status == 200
+} -run
+
+# Uses the other.test1.com sni and the default line of the crt-list
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "2195C9F0FD58470313013FC27C1B9CF9864BD1C6"
+    expect resp.status == 200
+} -run
+
+# Restore original certificate
+shell {
+    printf "set ssl cert ${testdir}/set_default_cert.pem <<\n$(cat ${testdir}/set_default_cert.pem)\n\n" | socat "${tmpdir}/h1/stats" -
+    echo "commit ssl cert ${testdir}/set_default_cert.pem" | socat "${tmpdir}/h1/stats" -
+}
+
+haproxy h1 -cli {
+    send "show ssl cert ${testdir}/set_default_cert.pem"
+    expect ~ ".*SHA1 FingerPrint: 9DC18799428875976DDE706E9956035EE88A4CB"
+}
+
+# Uses the www.test1.com sni
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "9DC18799428875976DDE706E9956035EE88A4CB3"
+    expect resp.status == 200
+} -run
+
+# Uses the other.test1.com sni and the default line of the crt-list
+client c1 -connect ${h1_clearlst_sock} {
+    txreq
+    rxresp
+    expect resp.http.X-SSL-Server-SHA1 == "9DC18799428875976DDE706E9956035EE88A4CB3"
+    expect resp.status == 200
+} -run

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index a86ed26c7c141b5fb113f0e1864400ba7b73efc8..b06c2ae9682b9bba0f10f9e17f8401ccc4283da0 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -2942,7 +2942,6 @@ void ssl_sock_load_cert_sni(struct ckch_inst *ckch_inst, struct bind_conf *bind_
 
        struct sni_ctx *sc0, *sc0b, *sc1;
        struct ebmb_node *node;
-       int def = 0;
 
        list_for_each_entry_safe(sc0, sc0b, &ckch_inst->sni_ctx, by_ckch_inst) {
 
@@ -2976,14 +2975,13 @@ void ssl_sock_load_cert_sni(struct ckch_inst *ckch_inst, struct bind_conf *bind_
                        ebst_insert(&bind_conf->sni_w_ctx, &sc0->name);
                else
                        ebst_insert(&bind_conf->sni_ctx, &sc0->name);
+       }
 
-               /* replace the default_ctx if required with the first ctx */
-               if (ckch_inst->is_default && !def) {
-                       SSL_CTX_free(bind_conf->default_ctx);
-                       SSL_CTX_up_ref(sc0->ctx);
-                       bind_conf->default_ctx = sc0->ctx;
-                       def = 1;
-               }
+       /* replace the default_ctx if required with the instance's ctx. */
+       if (ckch_inst->is_default) {
+               SSL_CTX_free(bind_conf->default_ctx);
+               SSL_CTX_up_ref(ckch_inst->ctx);
+               bind_conf->default_ctx = ckch_inst->ctx;
        }
 }
 
@@ -3421,6 +3419,12 @@ int ckch_inst_new_load_store(const char *path, struct ckch_store *ckchs, struct
                SSL_CTX_up_ref(ctx);
        }
 
+       /* Always keep a reference to the newly constructed SSL_CTX in the
+        * instance. This way if the instance has no SNIs, the SSL_CTX will
+        * still be linked. */
+       SSL_CTX_up_ref(ctx);
+       ckch_inst->ctx = ctx;
+
        /* everything succeed, the ckch instance can be used */
        ckch_inst->bind_conf = bind_conf;
        ckch_inst->ssl_conf = ssl_conf;