caddytls: Regularly reload static certificates #6941
Conversation
modules/caddytls/tls.go
Outdated
| storageCleanTicker *time.Ticker | ||
| storageCleanStop chan struct{} | ||
| logger *zap.Logger | ||
| events *caddyevents.App |
There was a problem hiding this comment.
Is it fine to add magic as a new field here? 🤔
There was a problem hiding this comment.
Hmm, likely not, since certmagic configs are mostly ephemeral and not tied to an entire TLS app config.
There was a problem hiding this comment.
Got it! There isn't any state (other than certCache) stored in it, so in the reloadUnmanagedCertificates function, I can simply create a new object, right?
func reloadUnmanagedCertificates() {
certCacheMu.RLock()
magic := certmagic.New(certCache, certmagic.Config{
Storage: ctx.Storage(),
Logger: t.logger,
OnEvent: t.onEvent,
OCSP: certmagic.OCSPConfig{
DisableStapling: t.DisableOCSPStapling,
},
DisableStorageCheck: t.DisableStorageCheck,
})
certCacheMu.RUnlock()
// add/remove certs from magic...
}
modules/caddytls/tls.go
Outdated
| if unmanaged > 0 { | ||
| t.regularlyReloadUnmanagedCertificates() | ||
| } |
There was a problem hiding this comment.
I think this reloading should always run (if explicitly enabled via config), independent of loaded certs
modules/caddytls/tls.go
Outdated
| } | ||
|
|
||
| func (t *TLS) regularlyReloadUnmanagedCertificates() { | ||
| t.unmanagedCertsTicker = time.NewTicker(2 * time.Hour) |
There was a problem hiding this comment.
This will be configurable, same as StorageCleanInterval, but it needs to be explicitly enabled in the config. It should probably be disabled by default, right?
There was a problem hiding this comment.
Yep, I'd have this behavior be off by default.
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We could also try to remove expired certs from the cache in this function
There was a problem hiding this comment.
Yeah, we'd definitely want to remove the prior certificate.
There was a problem hiding this comment.
Thanks for taking the initiative on this!
I haven't had much time to give this some thought, but, I think there's several ways to do this. Note that cert loader modules don't necessarily get their certificates from disk. I think if we are going to load certs from disk, we should be careful to initiate this only from modules which load from disk.
I was kind of thinking of something that watched the file system (fsnotify, I guess?) for changes, and then reloads only in that case. But yes, as you mentioned in a comment, we'd also need to remove the old cert from the cache. It's not my favorite, but it'd be opt-in I guess.
The ticker approach isn't my favorite either: it does a lot of unnecessary work. It's also not as effective... if a cert is updated, it might not be reflected in Caddy for up to the ticker interval. i.e. it's kind of slow. And then it has to load the cert every time, since you're not sure if it changed or not until you load it and check. (Well, I guess you could stat it, but still... almost all stats will end up being unnecessary.)
So I almost wonder if an approach that uses fsnotify in the relevant cert loader modules' Provision() method (and probably also cleaning things up in Cleanup()) would be a more conventional approach? What do you think?
modules/caddytls/tls.go
Outdated
| storageCleanTicker *time.Ticker | ||
| storageCleanStop chan struct{} | ||
| logger *zap.Logger | ||
| events *caddyevents.App |
There was a problem hiding this comment.
Hmm, likely not, since certmagic configs are mostly ephemeral and not tied to an entire TLS app config.
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Yeah, we'd definitely want to remove the prior certificate.
modules/caddytls/tls.go
Outdated
| } | ||
|
|
||
| func (t *TLS) regularlyReloadUnmanagedCertificates() { | ||
| t.unmanagedCertsTicker = time.NewTicker(2 * time.Hour) |
There was a problem hiding this comment.
Yep, I'd have this behavior be off by default.
pascalgn
force-pushed
the
update-unmanaged-certificates
branch
from
e1aa980 to
a874bb3
Compare
|
@mholt I pushed some changes. I changed CertificateLoader like this: type CertificateLoader interface {
Initialize(updateCertificates func(add []Certificate, remove []string) error) error
}That way, existing loaders (e.g. StorageLoader) will simply call the updateCertificates callback during the Initialize method, but other loaders (i.e. FolderLoader) will call it during Initialize, but also afterwards, during fsnotify events. Would it make sense like that? |
|
I was thinking more of using each loader modules' As for removing... let me think more on that. |
|
When moving the code into the Provision method, how would it access the certCache? |
|
Via the We might need to add a method or two to the |
Fixes #6933