kore

commit 599617e7b40713c68b2ae4f9a1730e21172ef457
parent 4cace25330f74057fe2e1b644970a6934108ca4d
Author: Joris Vink <joris@coders.se>
Date:   Tue,  5 Jan 2021 23:25:29 +0100

More ACME protocol improvements.

- Make sure tls-alpn01 works even if the underlying SSL library ends up
  calling the ALPN callback *before* the SNI extension was parsed and
  the correct domain was selected.

LibreSSL still does this, and older OpenSSL did too I believe, however
OpenSSL grew a clue and always makes sure SNI is called first.

Yes, TLS extensions have no fixed order but it still makes sense to
notify applications using your library of the SNI extension first
before anything else almost.

Oh well.

Diffstat:
include/kore/acme.h
 | 
1
+
include/kore/kore.h
 | 
14
++++++++------
src/acme.c
 | 
47
++++++++++++++++++++++++++++++++---------------
src/connection.c
 | 
2
+-
src/kore.c
 | 
10
++++++++++

5 files changed, 52 insertions(+), 22 deletions(-)
diff --git a/include/kore/acme.h b/include/kore/acme.h
@@ -49,6 +49,7 @@ void	kore_acme_run(void);
 void	kore_acme_setup(void);
 void	kore_acme_get_paths(const char *, char **, char **);
 
+void	kore_acme_tls_challenge_use_cert(SSL *, struct kore_domain *);
 int	kore_acme_tls_alpn(SSL *, const unsigned char **, unsigned char *,
 	    const unsigned char *, unsigned int, void *);
 
diff --git a/include/kore/kore.h b/include/kore/kore.h
@@ -190,17 +190,19 @@ TAILQ_HEAD(netbuf_head, netbuf);
 #define CONN_PROTO_HTTP		1
 #define CONN_PROTO_WEBSOCKET	2
 #define CONN_PROTO_MSG		3
+#define CONN_PROTO_ACME_ALPN	200
 
 #define KORE_EVENT_READ		0x01
 #define KORE_EVENT_WRITE	0x02
 #define KORE_EVENT_ERROR	0x04
 
-#define CONN_IDLE_TIMER_ACT	0x01
-#define CONN_CLOSE_EMPTY	0x02
-#define CONN_WS_CLOSE_SENT	0x04
-#define CONN_IS_BUSY		0x08
-#define CONN_ACME_CHALLENGE	0x10
-#define CONN_LOG_TLS_FAILURE	0x20
+#define CONN_IDLE_TIMER_ACT	0x0001
+#define CONN_CLOSE_EMPTY	0x0002
+#define CONN_WS_CLOSE_SENT	0x0004
+#define CONN_IS_BUSY		0x0008
+#define CONN_LOG_TLS_FAILURE	0x0020
+#define CONN_TLS_ALPN_ACME_SEEN	0x0040
+#define CONN_TLS_SNI_SEEN	0x0080
 
 #define KORE_IDLE_TIMER_MAX	5000
 
diff --git a/src/acme.c b/src/acme.c
@@ -151,7 +151,7 @@ struct acme_auth {
 #define ACME_ORDER_STATE_FETCH_CERT		7
 #define ACME_ORDER_STATE_COMPLETE		8
 #define ACME_ORDER_TICK				1000
-#define ACME_ORDER_TIMEOUT			60000
+#define ACME_ORDER_TIMEOUT			120000
 
 #define ACME_ORDER_CSR_REQUESTED		0x1000
 
@@ -201,7 +201,6 @@ static char	*acme_nonce_fetch(void);
 static char	*acme_thumbprint_component(void);
 static char	*acme_base64url(const void *, size_t);
 static char	*acme_protected_component(const char *, const char *);
-static void	acme_tls_challenge_use_cert(SSL *, struct kore_domain *);
 static void	acme_keymgr_key_req(const char *, const void *, size_t, int);
 
 static void	acme_parse_directory(void);
@@ -367,13 +366,10 @@ int
 kore_acme_tls_alpn(SSL *ssl, const unsigned char **out, unsigned char *outlen,
     const unsigned char *in, unsigned int inlen, void *udata)
 {
-	struct kore_domain	*dom = udata;
-
-	if (dom->acme == 0)
-		return (SSL_TLSEXT_ERR_NOACK);
+	struct connection	*c;
 
-	if (dom->acme_challenge == 0)
-		return (SSL_TLSEXT_ERR_NOACK);
+	if ((c = SSL_get_ex_data(ssl, 0)) == NULL)
+		fatal("%s: no connection data present", __func__);
 
 	if (inlen != sizeof(acme_alpn_name))
 		return (SSL_TLSEXT_ERR_NOACK);
@@ -381,13 +377,20 @@ kore_acme_tls_alpn(SSL *ssl, const unsigned char **out, unsigned char *outlen,
 	if (memcmp(acme_alpn_name, in, sizeof(acme_alpn_name)))
 		return (SSL_TLSEXT_ERR_NOACK);
 
-	kore_log(LOG_NOTICE, "[%s] acme-tls/1 challenge requested",
-	    dom->domain);
-
 	*out = in + 1;
 	*outlen = inlen - 1;
 
-	acme_tls_challenge_use_cert(ssl, dom);
+	c->flags |= CONN_TLS_ALPN_ACME_SEEN;
+
+	/*
+	 * If SNI was already done, we can continue, otherwise we mark
+	 * that we saw the right ALPN negotiation on this connection
+	 * and wait for the SNI extension to be parsed.
+	 */
+	if (c->flags & CONN_TLS_SNI_SEEN) {
+		/* SNI was seen, we are on the right domain. */
+		kore_acme_tls_challenge_use_cert(ssl, udata);
+	}
 
 	return (SSL_TLSEXT_ERR_OK);
 }
@@ -413,13 +416,27 @@ kore_acme_get_paths(const char *domain, char **key, char **cert)
 	*key = kore_strdup(path);
 }
 
-static void
-acme_tls_challenge_use_cert(SSL *ssl, struct kore_domain *dom)
+void
+kore_acme_tls_challenge_use_cert(SSL *ssl, struct kore_domain *dom)
 {
 	struct connection	*c;
 	const unsigned char	*ptr;
 	X509			*x509;
 
+	if (dom->acme == 0) {
+		kore_log(LOG_NOTICE, "[%s] ACME not active", dom->domain);
+		return;
+	}
+
+	if (dom->acme_challenge == 0) {
+		kore_log(LOG_NOTICE,
+		    "[%s] ACME auth challenge not active", dom->domain);
+		return;
+	}
+
+	kore_log(LOG_NOTICE, "[%s] acme-tls/1 challenge requested",
+	    dom->domain);
+
 	if ((c = SSL_get_ex_data(ssl, 0)) == NULL)
 		fatal("%s: no connection data present", __func__);
 
@@ -433,7 +450,7 @@ acme_tls_challenge_use_cert(SSL *ssl, struct kore_domain *dom)
 	SSL_clear_chain_certs(ssl);
 	SSL_set_verify(ssl, SSL_VERIFY_NONE, NULL);
 
-	c->flags |= CONN_ACME_CHALLENGE;
+	c->proto = CONN_PROTO_ACME_ALPN;
 }
 
 static void
diff --git a/src/connection.c b/src/connection.c
@@ -305,7 +305,7 @@ kore_connection_handle(struct connection *c)
 		}
 
 #if defined(KORE_USE_ACME)
-		if (c->flags & CONN_ACME_CHALLENGE) {
+		if (c->proto == CONN_PROTO_ACME_ALPN) {
 			kore_log(LOG_INFO, "disconnecting acme client");
 			kore_connection_disconnect(c);
 			return (KORE_RESULT_OK);
diff --git a/src/kore.c b/src/kore.c
@@ -385,6 +385,16 @@ kore_tls_sni_cb(SSL *ssl, int *ad, void *arg)
 			SSL_set_verify(ssl, SSL_VERIFY_NONE, NULL);
 		}
 
+#if defined(KORE_USE_ACME)
+		/*
+		 * If ALPN callback was called before SNI was parsed we
+		 * must make sure we swap to the correct certificate now.
+		 */
+		if (c->flags & CONN_TLS_ALPN_ACME_SEEN)
+			kore_acme_tls_challenge_use_cert(ssl, dom);
+
+		c->flags |= CONN_TLS_SNI_SEEN;
+#endif
 		return (SSL_TLSEXT_ERR_OK);
 	}