On Wed, 10 Jul 2024 12:09:10 +1000
Alex Wilson <alex@cooperi.net> wrote:
> Hi,
> 
> At the moment, if you give iked(8) a leaf cert file that contains a
> "chain" (i.e. it's a PEM leaf cert concatenated with an intermediate
> CA cert), the code in ca_reload uses the leaf cert but just discards
> the chain.
> 
> acme-client(1) can output either a "full chain" file (leaf cert with
> chain as above) or a separate leaf cert + chain file, but it refuses
> to put a separated CA chain file in a different directory to the leaf
> cert unless it's a child of the leaf cert dir (since it only unveils
> the leaf cert dir). iked(8) on the other hand wants the leaf certs and
> CA certs in two different sibling directories, so it's kind of awkward
> using the two of them together at the moment.
> 
> I would like to use these two together with a private ACME server for
> issuing shortish-term IKE certs (lifetime in double-digit
> hours). Since the intermediate on the ACME server also has a
> relatively short lifetime (months), I would rather not have to
> separate it off by hand to put it in the IKE ca dir and keep that up
> to date: it would be much simpler if acme-client could just update the
> intermediate for iked directly.
> 
> I think the easiest way to do this would be to make iked able to grab
> intermediates out of leaf cert files and add them to its list of CA
> certs, so users who want to use these two together can just use a full
> chain file.
> 
> Patch attached for that solution, it also adds the "error depth" to
> the debug output of ca_validate_cert to make it a bit easier to figure
> out what's going wrong when things aren't validating.
> 
> We (dlg@ and I) have been running this at work for a while and it
> hasn't been a complete disaster there yet, if that counts for
> anything.
> 
> Thoughts? Opinions?

Great.  I think this functionality is needed.

I also planed to suggest a diff which does almost same thing.
Attached at last.

The main difference is my diff assumes the first cert of a file is a
server's cert and the second and after certificates are intermediate
ca's cert.  Previously if a file has multiple certificates, they are
treated as server certificates.  But I suppose that was not
intentional.  So the diff proposes to change the behavior.


diff --git a/sbin/iked/ca.c b/sbin/iked/ca.c
index 042bce995f2..5832c862c68 100644
--- a/sbin/iked/ca.c
+++ b/sbin/iked/ca.c
@@ -44,6 +44,8 @@
 #include "iked.h"
 #include "ikev2.h"
 
+struct ca_store;
+
 void	 ca_run(struct privsep *, struct privsep_proc *, void *);
 void	 ca_shutdown(void);
 void	 ca_reset(struct iked *);
@@ -53,6 +55,7 @@ int	 ca_cert_local(struct iked *, X509 *);
 int	 ca_getreq(struct iked *, struct imsg *);
 int	 ca_getcert(struct iked *, struct imsg *);
 int	 ca_getauth(struct iked *, struct imsg *);
+int	 ca_load_cert_file(struct ca_store *, char *);
 X509	*ca_by_subjectpubkey(X509_STORE *, uint8_t *, size_t);
 X509	*ca_by_issuer(X509_STORE *, X509_NAME *, struct iked_static_id *);
 X509	*ca_by_subjectaltname(X509_STORE *, struct iked_static_id *);
@@ -1035,6 +1038,31 @@ ca_reload(struct iked *env)
 	}
 	closedir(dir);
 
+	/*
+	 * Load certificates
+	 */
+	if ((dir = opendir(IKED_CERT_DIR)) == NULL)
+		return (-1);
+
+	while ((entry = readdir(dir)) != NULL) {
+		if ((entry->d_type != DT_REG) &&
+		    (entry->d_type != DT_LNK))
+			continue;
+
+		if (snprintf(file, sizeof(file), "%s%s",
+		    IKED_CERT_DIR, entry->d_name) < 0)
+			continue;
+
+		if (!ca_load_cert_file(store, file)) {
+			log_warn("%s: failed to load cert file %s", __func__,
+			    entry->d_name);
+			ca_sslerror(__func__);
+			continue;
+		}
+		log_debug("%s: loaded cert file %s", __func__, entry->d_name);
+	}
+	closedir(dir);
+
 	/*
 	 * Save CAs signatures for the IKEv2 CERTREQ
 	 */
@@ -1085,32 +1113,6 @@ ca_reload(struct iked *env)
 		    iov, iovcnt);
 	}
 
-	/*
-	 * Load certificates
-	 */
-	if ((dir = opendir(IKED_CERT_DIR)) == NULL)
-		return (-1);
-
-	while ((entry = readdir(dir)) != NULL) {
-		if ((entry->d_type != DT_REG) &&
-		    (entry->d_type != DT_LNK))
-			continue;
-
-		if (snprintf(file, sizeof(file), "%s%s",
-		    IKED_CERT_DIR, entry->d_name) < 0)
-			continue;
-
-		if (!X509_load_cert_file(store->ca_certlookup, file,
-		    X509_FILETYPE_PEM)) {
-			log_warn("%s: failed to load cert file %s", __func__,
-			    entry->d_name);
-			ca_sslerror(__func__);
-			continue;
-		}
-		log_debug("%s: loaded cert file %s", __func__, entry->d_name);
-	}
-	closedir(dir);
-
 	h = X509_STORE_get0_objects(store->ca_certs);
 	for (i = 0; i < sk_X509_OBJECT_num(h); i++) {
 		xo = sk_X509_OBJECT_value(h, i);
@@ -1137,6 +1139,42 @@ ca_reload(struct iked *env)
 	return (0);
 }
 
+int
+ca_load_cert_file(struct ca_store *store, char *file)
+{
+	int	 ret = 0, i, count = 0;
+	BIO	*in = NULL;
+	X509	*x = NULL;
+
+	in = BIO_new(BIO_s_file());
+
+	if ((in == NULL) || (BIO_read_filename(in, file) <= 0))
+		goto done;
+
+	if ((x = PEM_read_bio_X509_AUX(in, NULL, NULL, "")) == NULL)
+		goto done;
+	if ((i = X509_STORE_add_cert(store->ca_certs, x)) == 0)
+		goto done;
+	count++;
+	X509_free(x);
+	x = NULL;
+
+	while ((x = PEM_read_bio_X509_AUX(in, NULL, NULL, "")) != NULL) {
+		i = X509_STORE_add_cert(store->ca_cas, x);
+		if (i == 0)
+			goto done;
+		count++;
+		X509_free(x);
+		x = NULL;
+	}
+	ret = count;
+done:
+	X509_free(x);
+	BIO_free(in);
+
+	return (ret);
+}
+
 X509 *
 ca_by_subjectpubkey(X509_STORE *ctx, uint8_t *sig, size_t siglen)
 {