From d129ae6aec6af2182217ee8a235f4df8cd2bbfde Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Tue, 30 Apr 2024 16:15:54 -0600 Subject: [PATCH] caddytls: Evict internal certs from cache based on issuer (#6266) * caddytls: Evict internal certs from cache based on issuer During a config reload, we would keep certs in the cache fi they were used by the next config. If one config uses InternalIssuer and the other uses a public CA, this behavior is problematic / unintuitive, because there is a big difference between private/public CAs. This change should ensure that internal issuers are considered when deciding whether to keep or evict from the cache during a reload, by making them distinct from each other and certs from public CAs. * Make sure new TLS app manages configured certs * Actually make it work --- go.mod | 2 +- go.sum | 4 +-- modules/caddytls/tls.go | 56 +++++++++++++++++++++++++++++++++++------ 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index 76a97b1f..36e2dccd 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/Masterminds/sprig/v3 v3.2.3 github.com/alecthomas/chroma/v2 v2.13.0 github.com/aryann/difflib v0.0.0-20210328193216-ff5ff6dc229b - github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d + github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e github.com/caddyserver/zerossl v0.1.2 github.com/dustin/go-humanize v1.0.1 github.com/go-chi/chi/v5 v5.0.12 diff --git a/go.sum b/go.sum index 8c2fb568..aad167a2 100644 --- a/go.sum +++ b/go.sum @@ -68,8 +68,8 @@ github.com/aws/smithy-go v1.19.0 h1:KWFKQV80DpP3vJrrA9sVAHQ5gc2z8i4EzrLhLlWXcBM= github.com/aws/smithy-go v1.19.0/go.mod h1:NukqUGpCZIILqqiV0NIjeFh24kd/FAa4beRb6nbIUPE= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d h1:fi1dMdHOoyWHXpxpCbaB+H4xdAgQcBP2AXSqpXVpIcg= -github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d/go.mod h1:e1NhB1rF5KZnAuAX6oSyhE7sg1Ru5bWgggw5RtauhEY= +github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e h1:+D6CR2rMrHZe79HSwjgefZWP9y78FMQ2NygXvhF0XVA= +github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e/go.mod h1:e1NhB1rF5KZnAuAX6oSyhE7sg1Ru5bWgggw5RtauhEY= github.com/caddyserver/zerossl v0.1.2 h1:tlEu1VzWGoqcCpivs9liKAKhfpJWYJkHEMmlxRbVAxE= github.com/caddyserver/zerossl v0.1.2/go.mod h1:wtiJEHbdvunr40ZzhXlnIkOB8Xj4eKtBKizCcZitJiQ= github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM= diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go index 2a05d523..14965533 100644 --- a/modules/caddytls/tls.go +++ b/modules/caddytls/tls.go @@ -91,7 +91,8 @@ type TLS struct { // set of subjects with managed certificates, // and hashes of manually-loaded certificates - managing, loaded map[string]struct{} + // (managing's value is an optional issuer key, for distinction) + managing, loaded map[string]string } // CaddyModule returns the Caddy module information. @@ -112,7 +113,7 @@ func (t *TLS) Provision(ctx caddy.Context) error { t.ctx = ctx t.logger = ctx.Logger() repl := caddy.NewReplacer() - t.managing, t.loaded = make(map[string]struct{}), make(map[string]struct{}) + t.managing, t.loaded = make(map[string]string), make(map[string]string) // set up a new certificate cache; this (re)loads all certificates cacheOpts := certmagic.CacheOptions{ @@ -266,7 +267,7 @@ func (t *TLS) Provision(ctx caddy.Context) error { if err != nil { return fmt.Errorf("caching unmanaged certificate: %v", err) } - t.loaded[hash] = struct{}{} + t.loaded[hash] = "" } } @@ -358,10 +359,27 @@ func (t *TLS) Cleanup() error { // compute which certificates were managed or loaded into the cert cache by this // app instance (which is being stopped) that are not managed or loaded by the // new app instance (which just started), and remove them from the cache - var noLongerManaged, noLongerLoaded []string - for subj := range t.managing { - if _, ok := nextTLSApp.managing[subj]; !ok { - noLongerManaged = append(noLongerManaged, subj) + var noLongerManaged []certmagic.SubjectIssuer + var reManage, noLongerLoaded []string + for subj, currentIssuerKey := range t.managing { + // It's a bit nuanced: managed certs can sometimes be different enough that we have to + // swap them out for a different one, even if they are for the same subject/domain. + // We consider "private" certs (internal CA/locally-trusted/etc) to be significantly + // distinct from "public" certs (production CAs/globally-trusted/etc) because of the + // implications when it comes to actual deployments: switching between an internal CA + // and a production CA, for example, is quite significant. Switching from one public CA + // to another, however, is not, and for our purposes we consider those to be the same. + // Anyway, if the next TLS app does not manage a cert for this name at all, definitely + // remove it from the cache. But if it does, and it's not the same kind of issuer/CA + // as we have, also remove it, so that it can swap it out for the right one. + if nextIssuerKey, ok := nextTLSApp.managing[subj]; !ok || nextIssuerKey != currentIssuerKey { + // next app is not managing a cert for this domain at all or is using a different issuer, so remove it + noLongerManaged = append(noLongerManaged, certmagic.SubjectIssuer{Subject: subj, IssuerKey: currentIssuerKey}) + + // then, if the next app is managing a cert for this name, but with a different issuer, re-manage it + if ok && nextIssuerKey != currentIssuerKey { + reManage = append(reManage, subj) + } } } for hash := range t.loaded { @@ -370,10 +388,19 @@ func (t *TLS) Cleanup() error { } } + // remove the certs certCacheMu.RLock() certCache.RemoveManaged(noLongerManaged) certCache.Remove(noLongerLoaded) certCacheMu.RUnlock() + + // give the new TLS app a "kick" to manage certs that it is configured for + // with its own configuration instead of the one we just evicted + if err := nextTLSApp.Manage(reManage); err != nil { + t.logger.Error("re-managing unloaded certificates with new config", + zap.Strings("subjects", reManage), + zap.Error(err)) + } } else { // no more TLS app running, so delete in-memory cert cache certCache.Stop() @@ -407,7 +434,20 @@ func (t *TLS) Manage(names []string) error { return fmt.Errorf("automate: manage %v: %v", names, err) } for _, name := range names { - t.managing[name] = struct{}{} + // certs that are issued solely by our internal issuer are a little bit of + // a special case: if you have an initial config that manages example.com + // using internal CA, then after testing it you switch to a production CA, + // you wouldn't want to keep using the same self-signed cert, obviously; + // so we differentiate these by associating the subject with its issuer key; + // we do this because CertMagic has no notion of "InternalIssuer" like we + // do, so we have to do this logic ourselves + var issuerKey string + if len(ap.Issuers) == 1 { + if intIss, ok := ap.Issuers[0].(*InternalIssuer); ok && intIss != nil { + issuerKey = intIss.IssuerKey() + } + } + t.managing[name] = issuerKey } }