tls: Fix OCSP stapling bug when certificate names overlap other certs

[mholt]

https://caddy.community/t/random-ocsp-response-errors-for-random-clients/2473?u=matt

Certificates are keyed by name in the cache, optimized for fast lookups
during TLS handshakes using SNI. A more "correct" way that is truly a
1:1 would be to cache certificates by a hash of the leaf's DER bytes,
but this involves an extra index to maintain. So instead of that, we
simply choose to prevent overlap when keying certificates by server
name. This avoids the ambiguity when updating OCSP staples, for instance.

(The fillCertFromLeaf changes are just cleanup, not actually related to the bug.)

[slightfoot]

I don't see anything wrong with the diff. I wonder though about overlap in regards to wildcard certificates. Also, about the use of certCacheMu without defer. In the odd situation where we attempt to recover but then the lock is still held.

[mholt]

@slightfoot Thanks for the review!

I wonder though about overlap in regards to wildcard certificates.

I don't think this will be a problem; only one certificate per name is mapped in the cache. Per my testing the wildcards were OK. Certificates are chosen based on most specific to least specific match, so wildcards will only be used if there's not another more specific matching certificate.

Also, about the use of certCacheMu without defer. In the odd situation where we attempt to recover but then the lock is still held.

Good thing to notice, for sure. Defer doesn't run until the end of the function, I'd have to refactor to make that happen. But the loop is correct; that code shouldn't panic (famous last words, I know) and it doesn't return. Good opportunity for cleanup in the future perhaps.

With that, merging this change now!