From: YASUOKA Masahiko Subject: Re: iked: support for leaf cert files containing chains To: alex@cooperi.net, tobhe@openbsd.org, dlg@openbsd.org Cc: tech@openbsd.org Date: Fri, 12 Jul 2024 18:42:15 +0200 On Wed, 10 Jul 2024 12:07:46 +0200 (CEST) YASUOKA Masahiko wrote: > On Wed, 10 Jul 2024 12:09:10 +1000 > Alex Wilson 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. I merged the "error depth" logging part to the diff. Also I got one positive feedback on offline for the changing behaviour. If there is no objection, i'd like to commit this. comments? ok? Index: sbin/iked/ca.c =================================================================== RCS file: /cvs/src/sbin/iked/ca.c,v diff -u -p -r1.102 ca.c --- sbin/iked/ca.c 18 Jun 2024 05:08:41 -0000 1.102 +++ sbin/iked/ca.c 12 Jul 2024 16:31:57 -0000 @@ -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 *); @@ -1036,6 +1039,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 */ ibuf_free(env->sc_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) { @@ -1843,6 +1881,7 @@ ca_validate_cert(struct iked *env, struc const char *errstr = "failed"; X509_NAME *subj; char *subj_name; + int depth; if (issuerp) *issuerp = NULL; @@ -1906,6 +1945,7 @@ ca_validate_cert(struct iked *env, struc result = X509_verify_cert(csc); error = X509_STORE_CTX_get_error(csc); + depth = X509_STORE_CTX_get_error_depth(csc); if (error == 0 && issuerp) { if (X509_STORE_CTX_get1_issuer(issuerp, csc, cert) != 1) { log_debug("%s: cannot get issuer", __func__); @@ -1936,7 +1976,8 @@ ca_validate_cert(struct iked *env, struc subj_name = X509_NAME_oneline(subj, NULL, 0); if (subj_name == NULL) goto err; - log_debug("%s: %s %.100s", __func__, subj_name, errstr); + log_debug("%s: %s (depth = %d) %.100s", __func__, subj_name, + depth, errstr); OPENSSL_free(subj_name); } err: