ACME tls-alpn-01 handling with multiple key specs

ChangeLog

@@ -1,3 +1,5 @@
+ * Fixed TLS-ALPN-01 challenges when multiple `MDPrivateKeys` are specified
+   with EC keys before RSA ones. Fixes #377.
  * Fixed missing newlines in the status page output. [andreasgroth]
 
 v2.5.1

src/md_crypt.c

@@ -570,7 +570,7 @@ static md_pkey_spec_t PkeySpecDef = { MD_PKEY_TYPE_DEFAULT, {{ 0 }} };
 md_pkey_spec_t *md_pkeys_spec_get(const md_pkeys_spec_t *pks, int index)
 {
     if (md_pkeys_spec_is_empty(pks)) {
-        return index == 1? &PkeySpecDef : NULL;
+        return index == 0? &PkeySpecDef : NULL;
     }
     else if (pks && index >= 0 && index < pks->specs->nelts) {
         return APR_ARRAY_IDX(pks->specs, index, md_pkey_spec_t*);

src/mod_md.c

@@ -1292,55 +1292,95 @@ static int md_add_fallback_cert_files(server_rec *s, apr_pool_t *p,
     return DECLINED;
 }
 
-static int md_answer_challenge(conn_rec *c, const char *servername,
-                               const char **pcert_pem, const char **pkey_pem)
+static int md_get_challenge_cert(conn_rec *c, const char *servername,
+                                 md_srv_conf_t *sc,
+                                 md_pkey_type_t key_type,
+                                 const char **pcert_pem,
+                                 const char **pkey_pem)
 {
-    const char *protocol;
-    int hook_rv = DECLINED;
     apr_status_t rv = APR_ENOENT;
-    md_srv_conf_t *sc;
-    md_store_t *store;
+    int i;
     char *cert_name, *pkey_name;
     const char *cert_pem, *key_pem;
-    int i;
+    md_store_t *store = md_reg_store_get(sc->mc->reg);
+    md_pkey_spec_t *key_spec;
 
-    if (!servername
-        || !(protocol = md_protocol_get(c))
-        || strcmp(PROTO_ACME_TLS_1, protocol)) {
-        goto cleanup;
-    }
-    sc = md_config_get(c->base_server);
-    if (!sc || !sc->mc->reg) goto cleanup;
-
-    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
-                  "Answer challenge[tls-alpn-01] for %s", servername);
-    store = md_reg_store_get(sc->mc->reg);
+    for (i = 0; i < md_pkeys_spec_count(sc->pks); i++) {
+        key_spec = md_pkeys_spec_get(sc->pks, i);
+        if (key_spec->type != key_type)
+          continue;
 
-    for (i = 0; i < md_pkeys_spec_count( sc->pks ); i++) {
-        tls_alpn01_fnames(c->pool, md_pkeys_spec_get(sc->pks,i),
-                          &pkey_name, &cert_name);
+        tls_alpn01_fnames(c->pool, key_spec, &pkey_name, &cert_name);
 
         rv = md_store_load(store, MD_SG_CHALLENGES, servername, cert_name, MD_SV_TEXT,
                            (void**)&cert_pem, c->pool);
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, rv, c,
+                      "Load challenge: cert %s", cert_name);
         if (APR_STATUS_IS_ENOENT(rv)) continue;
         if (APR_SUCCESS != rv) goto cleanup;
 
         rv = md_store_load(store, MD_SG_CHALLENGES, servername, pkey_name, MD_SV_TEXT,
                            (void**)&key_pem, c->pool);
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, rv, c,
+                      "Load challenge: key %s", pkey_name);
         if (APR_STATUS_IS_ENOENT(rv)) continue;
         if (APR_SUCCESS != rv) goto cleanup;
 
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
-                      "Found challenge cert %s, key %s for %s",
+                      "Found challenge: cert %s, key %s for %s",
                       cert_name, pkey_name, servername);
         *pcert_pem = cert_pem;
         *pkey_pem = key_pem;
-        hook_rv = OK;
-        break;
+        return OK;
+    }
+cleanup:
+    return DECLINED;
+}
+
+static int md_answer_challenge(conn_rec *c, const char *servername,
+                               const char **pcert_pem, const char **pkey_pem)
+{
+    const char *protocol;
+    int hook_rv = DECLINED;
+    md_srv_conf_t *sc;
+
+    *pcert_pem = *pkey_pem = NULL;
+
+    if (!servername
+        || !(protocol = md_protocol_get(c))
+        || strcmp(PROTO_ACME_TLS_1, protocol)) {
+        goto cleanup;
     }
+    sc = md_config_get(c->base_server);
+    if (!sc || !sc->mc->reg) goto cleanup;
+
+    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
+                  "Answer challenge[tls-alpn-01] for %s", servername);
+
+    /* A challenge for TLS-ALPN-01 used to have a single certificate,
+     * overriding the single fallback certificate already installed in
+     * the connections SSL* instance.
+     * Since the addition of `MDPrivateKeys`, there can be more than one,
+     * but the server API for a challenge cert can return only one. This
+     * is a short coming of the API.
+     * This means we cannot override all fallback certificates present, just
+     * a single one. If there is an RSA cert in fallback, we need to override
+     * that, because the ACME server has a preference for that (at least LE
+     * has). So we look for an RSA challenge cert first.
+     * The fallback is an EC cert and that works since without RSA challenges,
+     * there should also be no RSA fallbacks.
+     * Bit of a mess. */
+    hook_rv = md_get_challenge_cert(c, servername, sc, MD_PKEY_TYPE_DEFAULT,
+                                    pcert_pem, pkey_pem);
+    if (hook_rv == DECLINED)
+      hook_rv = md_get_challenge_cert(c, servername, sc, MD_PKEY_TYPE_RSA,
+                                      pcert_pem, pkey_pem);
+    if (hook_rv == DECLINED)
+      hook_rv = md_get_challenge_cert(c, servername, sc, MD_PKEY_TYPE_EC,
+                                      pcert_pem, pkey_pem);
 
     if (DECLINED == hook_rv) {
-        ap_log_cerror(APLOG_MARK, APLOG_INFO, rv, c, APLOGNO(10080)
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10080)
                       "%s: unknown tls-alpn-01 challenge host", servername);
     }
 

test/modules/md/test_702_auto.py

@@ -1,4 +1,5 @@
 import os
+import re
 import time
 from datetime import timedelta
 
@@ -576,7 +577,10 @@ def test_md_702_033(self, env):
 
 
     # test case: test "tls-alpn-01" challenge handling
-    def test_md_702_040(self, env):
+    @pytest.mark.parametrize("pkey_spec", [
+            "", "rsa 4096", "secp384r1", "secp384r1 rsa 4096",
+        ])
+    def test_md_702_040(self, env, pkey_spec):
         domain = self.test_domain
         domains = [domain, "www." + domain]
         #
@@ -585,6 +589,8 @@ def test_md_702_040(self, env):
         conf.add("LogLevel core:debug")
         conf.add("Protocols http/1.1 acme-tls/1")
         conf.add_drive_mode("auto")
+        if len(pkey_spec):
+            conf.add(f"MDPrivateKeys {pkey_spec}")
         conf.add("MDCAChallenges tls-alpn-01")
         conf.add_md(domains)
         conf.add_vhost(domains=domains)
@@ -597,7 +603,10 @@ def test_md_702_040(self, env):
         stat = env.get_md_status(domain)
         assert stat["proto"]["acme-tls/1"] == domains
         assert env.await_completion([domain])
-        env.check_md_complete(domain)
+        if re.match(r'secp384r1', pkey_spec):
+            env.check_md_complete(domain, pkey='secp384r1')
+        else:
+            env.check_md_complete(domain)
         #        
         # check SSL running OK
         cert = env.get_cert(domain)