Moved to tech@. Full original mail at https://marc.info/?l=openbsd-misc&m=163187837530385&w=2
On 2021-09-17, Wolf <[email protected]> wrote on misc@: > Use of only CN is not allowed according to Baseline Requirements 1.8.0 > from CA Browser Forum. Using CN is not prohibited, but if it is present, > value in it must also be present in SAN. And SAN is always required. Baseline requirements relate to the CA-signed certificate; a CA will make changes to the certificate requested in a CSR for baseline requirements compliance. > While currently Let's Encrypt does accept requests without CN, in the > interest of future-proofing, this commit modifies acme-client to always > generate SAN and include CN in it. There are two issues: - if you only list a plain domain (no "alternative names") in acme-client.conf then subjectAlternativeName isn't generated in the CSR at all. a CA is definitely expected to fix this up. If this was the only problem then I would recommend deferring to post-7.0. - if you list alternative domains, only the alternatives and not the main domain are listed in subjectAlternativeName in the CSR. This is definitely wrong. If SAN is present, the domain name in the subject must be ignored. Current CAs are clearly fixing this up, but we are generating the SANs incorrectly here and I don't think we should rely on them being fixed, So I think for this reason it's worth getting this in pre 7.0. > I recommend viewing the diff with whitespace differences ignored (-w), > helps to show how minimal it is. Indeed; "if (altsz > 1)" is removed shifting the indentation up a level, and the loop initialiser is changed from "i = 1" to "i = 0". > I've tested this and it issues valid certificate that includes both CN > and SAN (check current cert of wolfsden.cz if you want to make sure). > > Baseline Requirements can be found here [0], CN is described in > 7.1.4.2.2. > > Why this is even a problem: My version of acme-client has some tests and > pebble (let's encrypt's test server) already does not accept > certificates without correct SAN [1]. pebble is not exactly "letsencrypt's test server" (which I'd think of as their staging server) it's test server *software* for integration testing etc and is expected to be more strict than a real server. (i.e. if someone wants to test this against a fussy server, they will need to run the software themselves, running against letsencrypt-staging is *not* sufficient). > usr.sbin/acme-client/keyproc.c | 90 +++++++++++++++++----------------- > 1 file changed, 44 insertions(+), 46 deletions(-) [quoting removed so they diff will apply directly; it is unchanged from the original] This is OK sthen@. diff --git a/usr.sbin/acme-client/keyproc.c b/usr.sbin/acme-client/keyproc.c index b4cc6184e26..3a1d68526d9 100644 --- a/usr.sbin/acme-client/keyproc.c +++ b/usr.sbin/acme-client/keyproc.c @@ -175,53 +175,51 @@ keyproc(int netsock, const char *keyfile, const char **alts, size_t altsz, * TODO: is this the best way of doing this? */ - if (altsz > 1) { - nid = NID_subject_alt_name; - if ((exts = sk_X509_EXTENSION_new_null()) == NULL) { - warnx("sk_X509_EXTENSION_new_null"); - goto out; - } - /* Initialise to empty string. */ - if ((sans = strdup("")) == NULL) { - warn("strdup"); - goto out; - } - sansz = strlen(sans) + 1; - - /* - * For each SAN entry, append it to the string. - * We need a single SAN entry for all of the SAN - * domains: NOT an entry per domain! - */ - - for (i = 1; i < altsz; i++) { - cc = asprintf(&san, "%sDNS:%s", - i > 1 ? "," : "", alts[i]); - if (cc == -1) { - warn("asprintf"); - goto out; - } - pp = recallocarray(sans, sansz, sansz + strlen(san), 1); - if (pp == NULL) { - warn("recallocarray"); - goto out; - } - sans = pp; - sansz += strlen(san); - strlcat(sans, san, sansz); - free(san); - san = NULL; - } - - if (!add_ext(exts, nid, sans)) { - warnx("add_ext"); - goto out; - } else if (!X509_REQ_add_extensions(x, exts)) { - warnx("X509_REQ_add_extensions"); - goto out; - } - sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free); + nid = NID_subject_alt_name; + if ((exts = sk_X509_EXTENSION_new_null()) == NULL) { + warnx("sk_X509_EXTENSION_new_null"); + goto out; } + /* Initialise to empty string. */ + if ((sans = strdup("")) == NULL) { + warn("strdup"); + goto out; + } + sansz = strlen(sans) + 1; + + /* + * For each SAN entry, append it to the string. + * We need a single SAN entry for all of the SAN + * domains: NOT an entry per domain! + */ + + for (i = 0; i < altsz; i++) { + cc = asprintf(&san, "%sDNS:%s", + i ? "," : "", alts[i]); + if (cc == -1) { + warn("asprintf"); + goto out; + } + pp = recallocarray(sans, sansz, sansz + strlen(san), 1); + if (pp == NULL) { + warn("recallocarray"); + goto out; + } + sans = pp; + sansz += strlen(san); + strlcat(sans, san, sansz); + free(san); + san = NULL; + } + + if (!add_ext(exts, nid, sans)) { + warnx("add_ext"); + goto out; + } else if (!X509_REQ_add_extensions(x, exts)) { + warnx("X509_REQ_add_extensions"); + goto out; + } + sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free); /* Sign the X509 request using SHA256. */
