Certs are saved using `NamesKey`, but are loaded using the CommonName

[Rjevski]

What version of the package are you using?

aba1313fdf2bf611c82904fa8d69b19a57f48cf9 - but this is not a recent bug and more of a design decision?

What are you trying to do?

Trying to use CertMagic (from within Caddy) with a CA that sets the CommonName to a static value (SANs are correct though). Issuance works fine, but upon renewal the certificate fails to reload, which just causes it to try renewing again and again.

Steps to reproduce:

• have a CA that issues certs with the requested SANs but sets the CN to a static value such as "Some Certificate"
• make/configure an issuer module for said CA (I use a custom one)
• wait for the cert to near expiry
• observe the following in the logs:

tls     certificate is in emergency renewal window; expiration imminent {"subjects": [], "expiration": "2025/10/08 16:01:01.000", "ari_cert_id": "", "next_ari_update": null, "renew_check_interval": 600, "window_start": "0001/01/01 00:00:00.000", "window_end": "0001/01/01 00:00:00.000", "remaining": 252.010517}
tls.renew       acquiring lock  {"identifier": "example.com"}
tls.renew       lock acquired   {"identifier": "example.com"}
tls.renew       renewing certificate    {"identifier": "example.com", "remaining": 251.999574}
events  event   {"name": "cert_obtaining", "id": "48ad74ca-2b0c-4f93-af52-d26ad7d778c6", "origin": "tls", "data": {"forced":false,"identifier":"example.com","issuer":"some_issuer_module","remaining":251999574000,"renewal":true}}
tls     created CSR     {"identifiers": ["example.com"], "san_dns_names": ["example.com"], "san_emails": [], "common_name": "", "extra_extensions": 0}
tls.issuance.some_issuer_module       issuing certificate     {"names": ["example.com"]}
tls.issuance.some_issuer_module       certificate issued successfully {"id": "195190520411994972438916923622062358641371501636"}
tls.renew       certificate renewed successfully        {"identifier": "example.com", "issuer": "some_issuer_module"}
events  event   {"name": "cert_obtained", "id": "f27fc6c8-1a6f-40a7-beb0-b63199b0fcc4", "origin": "tls", "data": {"certificate_path":"certificates/some_issuer_module/example.com/example.com.crt","csr_pem":"...","identifier":"example.com","issuer":"some_issuer_module","metadata_path":"certificates/some_issuer_module/example.com/example.com.json","private_key_path":"certificates/some_issuer_module/example.com/example.com.key","remaining":251999574000,"renewal":true,"storage_path":"certificates/some_issuer_module/example.com"}}
tls.renew       releasing lock  {"identifier": "example.com"}
tls     reloading managed certificate   {"identifiers": ["some certificate"]}
tls     job failed      {"error": "example.com: reloading renewed certificate into memory: loading managed certificate for [some certificate] from storage: open data/certificates/some_issuer_module/cloudflare_origin_certificate/cloudflare_origin_certificate.key: no such file or directory"}

So what's happening is that the cert is correctly renewed and stored at certificates/some_issuer_module/example.com, but it's trying to reload it from certificates/some_issuer_module/some_certificate/some_certificate.key.

Actually, an easier way to reproduce could be to use a normal ACME/etc issuer, then stop Caddy, and manually replace the issued certificate by one signed by a local self-signed CA where the CommonName has been overridden to some random value and the expiry set to an hour or something (as to trigger the proactive renewal). This should trigger a (successful) ACME renewal, but the renewed certificate will not be loaded due to this issue.

What steps did you take?

When a certificate is obtained and then saved, saveCertResource is called, which then uses the cert.NamesKey() (sorted, comma-separated list of SANs) as a storage key to store the cert. This is fine.

During the renewal process however, after the certificate has been renewed via the Issuer (which does the above process), reloadManagedCertificate is called with the old certificate, to pick up the new certificate and update the cert cache.

In there, we do:

newCert, err := cfg.loadManagedCertificate(ctx, oldCert.Names[0])

Names is a list containing the CommonName as the first item, so "Some Certificate" to match the above log excerpt.

It goes to:

cfg.loadCertResourceAnyIssuer(ctx, domain)

(domain is thus the CN from the previous step)

Which then ultimately calls via a shortcut if there's only one issuer (it's the case here):

cfg.loadCertResource(ctx, cfg.Issuers[0], certNamesKey)

(certNamesKey is the CN)

Which then:

normalizedName, err := idna.ToASCII(certNamesKey)
certBytes, err := cfg.Storage.Load(ctx, StorageKeys.SiteCert(certRes.issuerKey, normalizedName))

So at this point we just use the CommonName as part of the storage key, which doesn't match what we do during certificate issuance, and breaks in the above scenario. I think this will also break in a more common scenario where you use a normal CA that does set CN for the first SAN, but if you have multiple certs with the same CN but different additional SANs, it will reload the wrong one during renewal and thus effectively never load the renewed cert.

I think fundamentally the problem is that during the post-renewal reload, certs are considered only based on a single domain from the old certificate - which happens to be the CommonName in this case which breaks. But even if we assume we don't use the common name and instead use a sorted list of SANs, an issuer module potentially choose to omit some SANs for some reason, which would break this method too.

What did you expect to happen, and what actually happened instead?

The same logic in generating the cert's storage key should be used during issuance and reloading.

How do you think this should be fixed?

Not sure yet, I am opening this more as a discussion as I don't believe this problem really affects most users of publicly-trusted CAs (although running many domains in Caddy with multiple certs with the same CommonName but different combinations of SANs might trigger it).

I think during the renewal process, the actual storage key used for saving the certificate should be returned, and then used for reloading the certificate. This means we are sure to reload the same certificate we've just renewed, even if the CA amended the list of SANs or tampered with the CN.

[mholt]

Ah wow, weird edge case!

CertMagic only automates single-name certificates (for very good reasons). So you're right, it's not expecting an automated cert to have both a CommonName and a SAN.

This does make me wonder if we should drop support for adding the CommonName to the Names slice. It's been deprecated for 30 years. Wonder if we should start strong-arming that.

[Rjevski]

CertMagic only automates single-name certificates (for very good reasons)

Out of curiosity, what is this good reason and should this be reconsidered since CertMagic is also meant to be used in Caddy? I could see certs with multiple different and potentially overlapping SANs being useful in Caddy.

drop support for adding the CommonName to the Names slice

From a browser perspective, probably? But keep in mind people might use CertMagic/Caddy to front servers for machine-to-machine communications, including those for legacy devices which may not be updated and still expect valid CNs. Also I think either way, the multiple SAN issue should be considered regardless, which will still remain even if CN is ignored.

I think it would be a big change and we need to proceed with caution. As it stands this doesn't seem to be an issue that affects typical users, so probably best to ignore for now. Maybe if I have time at one point I will get a closer look at this and try to fix it.

I think the "proper" fix if we want to preserve CN support and handle multiple/overlapping SANs properly would be to decouple certificate path names from domains/SANs. So maybe identify certs by some ID/hash derived from config, and use that to resolve the path to a JSON file - that file would contain the path to the certificate(s) obtained with said config and maybe a cache of CN+SANs. Then you'd have everything you need to identify the certificate and pick it for TLS handshake purposes, and only load the actual cert file if you need it based on which CN/SAN you are looking up?

[francislavoie]

See https://github.com/https-dev/docs/blob/master/acme-ops.md#use-one-name-per-certificate

[mholt]

Yep. That document doesn't even mention the privacy concerns, where bunching one domain with others automatically associates them. Cloudflare used to put, I think 100 domains on a single cert, and now they just use single-SAN certs these days.

for legacy devices which may not be updated and still expect valid CNs.

I think if the device is so legacy it still requires and depends on something that has been deprecated for 30 years, our library is not the one for it.

So maybe identify certs by some ID/hash derived from config, and use that to resolve the path to a JSON file - that file would contain the path to the certificate(s) obtained with said config and maybe a cache of CN+SANs. Then you'd have everything you need to identify the certificate and pick it for TLS handshake purposes, and only load the actual cert file if you need it based on which CN/SAN you are looking up?

That's essentially a database... and one problem is the configs can change. All you can really be sure of is the actual subject name.