Skip to content

Commit d129ae6

Browse files
authored
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
1 parent 87c7127 commit d129ae6

File tree

3 files changed

+51
-11
lines changed

3 files changed

+51
-11
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ require (
77
github.com/Masterminds/sprig/v3 v3.2.3
88
github.com/alecthomas/chroma/v2 v2.13.0
99
github.com/aryann/difflib v0.0.0-20210328193216-ff5ff6dc229b
10-
github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d
10+
github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e
1111
github.com/caddyserver/zerossl v0.1.2
1212
github.com/dustin/go-humanize v1.0.1
1313
github.com/go-chi/chi/v5 v5.0.12

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ github.com/aws/smithy-go v1.19.0 h1:KWFKQV80DpP3vJrrA9sVAHQ5gc2z8i4EzrLhLlWXcBM=
6868
github.com/aws/smithy-go v1.19.0/go.mod h1:NukqUGpCZIILqqiV0NIjeFh24kd/FAa4beRb6nbIUPE=
6969
github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
7070
github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
71-
github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d h1:fi1dMdHOoyWHXpxpCbaB+H4xdAgQcBP2AXSqpXVpIcg=
72-
github.com/caddyserver/certmagic v0.20.1-0.20240419174353-855d4670a49d/go.mod h1:e1NhB1rF5KZnAuAX6oSyhE7sg1Ru5bWgggw5RtauhEY=
71+
github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e h1:+D6CR2rMrHZe79HSwjgefZWP9y78FMQ2NygXvhF0XVA=
72+
github.com/caddyserver/certmagic v0.20.1-0.20240423172519-140a6fa9202e/go.mod h1:e1NhB1rF5KZnAuAX6oSyhE7sg1Ru5bWgggw5RtauhEY=
7373
github.com/caddyserver/zerossl v0.1.2 h1:tlEu1VzWGoqcCpivs9liKAKhfpJWYJkHEMmlxRbVAxE=
7474
github.com/caddyserver/zerossl v0.1.2/go.mod h1:wtiJEHbdvunr40ZzhXlnIkOB8Xj4eKtBKizCcZitJiQ=
7575
github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM=

modules/caddytls/tls.go

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ type TLS struct {
9191

9292
// set of subjects with managed certificates,
9393
// and hashes of manually-loaded certificates
94-
managing, loaded map[string]struct{}
94+
// (managing's value is an optional issuer key, for distinction)
95+
managing, loaded map[string]string
9596
}
9697

9798
// CaddyModule returns the Caddy module information.
@@ -112,7 +113,7 @@ func (t *TLS) Provision(ctx caddy.Context) error {
112113
t.ctx = ctx
113114
t.logger = ctx.Logger()
114115
repl := caddy.NewReplacer()
115-
t.managing, t.loaded = make(map[string]struct{}), make(map[string]struct{})
116+
t.managing, t.loaded = make(map[string]string), make(map[string]string)
116117

117118
// set up a new certificate cache; this (re)loads all certificates
118119
cacheOpts := certmagic.CacheOptions{
@@ -266,7 +267,7 @@ func (t *TLS) Provision(ctx caddy.Context) error {
266267
if err != nil {
267268
return fmt.Errorf("caching unmanaged certificate: %v", err)
268269
}
269-
t.loaded[hash] = struct{}{}
270+
t.loaded[hash] = ""
270271
}
271272
}
272273

@@ -358,10 +359,27 @@ func (t *TLS) Cleanup() error {
358359
// compute which certificates were managed or loaded into the cert cache by this
359360
// app instance (which is being stopped) that are not managed or loaded by the
360361
// new app instance (which just started), and remove them from the cache
361-
var noLongerManaged, noLongerLoaded []string
362-
for subj := range t.managing {
363-
if _, ok := nextTLSApp.managing[subj]; !ok {
364-
noLongerManaged = append(noLongerManaged, subj)
362+
var noLongerManaged []certmagic.SubjectIssuer
363+
var reManage, noLongerLoaded []string
364+
for subj, currentIssuerKey := range t.managing {
365+
// It's a bit nuanced: managed certs can sometimes be different enough that we have to
366+
// swap them out for a different one, even if they are for the same subject/domain.
367+
// We consider "private" certs (internal CA/locally-trusted/etc) to be significantly
368+
// distinct from "public" certs (production CAs/globally-trusted/etc) because of the
369+
// implications when it comes to actual deployments: switching between an internal CA
370+
// and a production CA, for example, is quite significant. Switching from one public CA
371+
// to another, however, is not, and for our purposes we consider those to be the same.
372+
// Anyway, if the next TLS app does not manage a cert for this name at all, definitely
373+
// remove it from the cache. But if it does, and it's not the same kind of issuer/CA
374+
// as we have, also remove it, so that it can swap it out for the right one.
375+
if nextIssuerKey, ok := nextTLSApp.managing[subj]; !ok || nextIssuerKey != currentIssuerKey {
376+
// next app is not managing a cert for this domain at all or is using a different issuer, so remove it
377+
noLongerManaged = append(noLongerManaged, certmagic.SubjectIssuer{Subject: subj, IssuerKey: currentIssuerKey})
378+
379+
// then, if the next app is managing a cert for this name, but with a different issuer, re-manage it
380+
if ok && nextIssuerKey != currentIssuerKey {
381+
reManage = append(reManage, subj)
382+
}
365383
}
366384
}
367385
for hash := range t.loaded {
@@ -370,10 +388,19 @@ func (t *TLS) Cleanup() error {
370388
}
371389
}
372390

391+
// remove the certs
373392
certCacheMu.RLock()
374393
certCache.RemoveManaged(noLongerManaged)
375394
certCache.Remove(noLongerLoaded)
376395
certCacheMu.RUnlock()
396+
397+
// give the new TLS app a "kick" to manage certs that it is configured for
398+
// with its own configuration instead of the one we just evicted
399+
if err := nextTLSApp.Manage(reManage); err != nil {
400+
t.logger.Error("re-managing unloaded certificates with new config",
401+
zap.Strings("subjects", reManage),
402+
zap.Error(err))
403+
}
377404
} else {
378405
// no more TLS app running, so delete in-memory cert cache
379406
certCache.Stop()
@@ -407,7 +434,20 @@ func (t *TLS) Manage(names []string) error {
407434
return fmt.Errorf("automate: manage %v: %v", names, err)
408435
}
409436
for _, name := range names {
410-
t.managing[name] = struct{}{}
437+
// certs that are issued solely by our internal issuer are a little bit of
438+
// a special case: if you have an initial config that manages example.com
439+
// using internal CA, then after testing it you switch to a production CA,
440+
// you wouldn't want to keep using the same self-signed cert, obviously;
441+
// so we differentiate these by associating the subject with its issuer key;
442+
// we do this because CertMagic has no notion of "InternalIssuer" like we
443+
// do, so we have to do this logic ourselves
444+
var issuerKey string
445+
if len(ap.Issuers) == 1 {
446+
if intIss, ok := ap.Issuers[0].(*InternalIssuer); ok && intIss != nil {
447+
issuerKey = intIss.IssuerKey()
448+
}
449+
}
450+
t.managing[name] = issuerKey
411451
}
412452
}
413453

0 commit comments

Comments
 (0)