feat(server): reload TLS certificates on SIGHUP#4382
Conversation
|
What's the usecase for this change? I think that users are either running only miniflux, and so it's already taking care of renewing certificates automatically, or are using something else to do so, and can hook new certificate generations to a miniflux restart. Moreover, this is adding a new dependency :< |
|
The use case is: with auto-reloading, you can point miniflux at a acme.sh-generated certificate and when that is renewed, it'll be picked up automatically without a miniflux restart. |
|
What's the advantage compared to |
|
The advantage is that the reload happens without a brief downtime (in my case, it’s „brew restart miniflux“ which is a bit slower than „systemctl reload miniflux“). |
|
fsnotify is unreliable. AFAIK none of the big three will guarantee that event will fire on file change Partly because of this e.g. syncthing on top of fsnotify events has a timer for resync And since this is the case you'd need to poll once in a while anyway. It seems like doing manual reload still better as you won't be polling quicker than delay between update and reload. Also since certificates updated quite rarely and usually at least couple days before they expire I don't really get it why update them ASAP. Will debounce logic continue to try re-read keypair on load failure? I am not sure if it's doing it but ig it's a good idea to try until it successfully load as 1s may not be enough for every case |
|
Afaik, watching individual files is not always reliable, but watching their parent directory should be fine. This is not about updating to a new certificate ASAP (because, as you mentioned, there is usually a decent overlap between old and new certificates), but about making certificate refreshes zero-configuration and maintenance-free. There's no SIGHUP to be sent, no process to be killed - just configure paths once and forget about it. If there's agreement that this is a good idea, I'm happy to iterate on the implementation and e.g. remove fsnotify or improving retries for the debounce logic. |
|
What's wrong with SIGHUP though? |
|
Nothing wrong per se, besides that it's an external trigger that needs to be set up. I'm using macOS and manage |
|
Can't |
|
Afaict, |
|
I've updated the PR to only reload files when SIGHUP is received. Would that be acceptable? |
|
I'm still not convinced that this is worth adding a new dependency for such a niche feature, or even to implement it in the first place to be honest. |
13db826 to
475659a
Compare
Instead of watching certificate files with fsnotify, reload them when the process receives a SIGHUP signal. This is simpler, more reliable, and avoids platform-specific file watching edge cases. - Add certificateLoader that caches the TLS cert and reloads on demand - Use tls.Config.GetCertificate callback so new connections pick up the reloaded cert without a server restart - Share a single loader across all TLS listeners to avoid duplicate work - Handle SIGHUP in the daemon to trigger reload - Keep the existing certificate if the new one fails to load
475659a to
dcb7ac6
Compare
|
@jvoisin apologies, I forgot to push the branch. The SIGHUP approach does not bring in a new dependency. |
Instead of requiring a server restart when TLS certificates are renewed, this allows reloading them by sending
SIGHUPto the miniflux process:When SIGHUP is received, the certificate and key are re-read from disk. If the new pair is invalid (e.g., partial write, mismatched), the old certificate continues to be served and the error is logged.
How it works
certificateLoaderloads and caches the TLS cert at startup, and serves it viatls.Config.GetCertificate— so every TLS handshake gets the current cert without restart.StartWebServercreates one shared loader for all TLS listeners, avoiding duplicate work when multiple addresses are configured.SIGHUPand callsReload()on the loader.