I thought we had fixed this :( OK florian
On 2021-09-17 14:57 +01, Stuart Henderson <stu.li...@spacehopper.org> wrote: > Moved to tech@. Full original mail at > https://marc.info/?l=openbsd-misc&m=163187837530385&w=2 > > On 2021-09-17, Wolf <w...@wolfsden.cz> 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. */ > > -- I'm not entirely sure you are real.
