fix: Auto-renew certificates by default, with honest renew reporting and proxy tuning#167
Merged
Merged
Conversation
The per-certificate Renew action reported success but did nothing when the certificate was not yet inside certbot's auto-renew window: it asked certbot to renew only if due, so a not-yet-expiring cert was skipped and the action was a silent no-op. A single renew is an explicit "renew this now" request, so it now forces the reissue. Renew-all is unchanged and still renews only what is due.
Deployed apps are served to end users through the generated reverse proxy, and its defaults left several easy wins unused. Upstream requests now use HTTP/1.1 on every proxied route rather than only the primary one, access logging is buffered instead of writing per request, gzip covers modern asset types (JavaScript, wasm, web fonts) with a minimum-size floor, and proxy read buffers are sized up. The larger win, reusing connections to app containers, is deferred: it requires reworking the runtime DNS-resolution the proxy relies on to keep serving other sites when one backend is briefly down. Tracked in the Albacore serving item.
Code Review SummaryThis PR improves SSL reliability and proxy performance. It fixes a significant issue where certificates would expire because auto-renewal was off by default, introduces 'honest' reporting for renewals, and tunes Nginx for higher throughput. 🚀 Key Improvements
💡 Minor Suggestions
|
Forcing a reissue on every single-certificate Renew was wrong for a not-yet-due certificate and risks hitting duplicate-certificate limits. Force is now an explicit request option the UI can offer deliberately. The default renew asks certbot to renew only if due and reports honestly whether a certificate was actually reissued, rather than always claiming success. Automatic renewal uses the same non-forced path.
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
|
|
||
| // certbotDidRenew reports whether a `certbot renew` run actually reissued a cert, | ||
| // by looking for the phrases certbot prints when it skips a not-yet-due lineage. | ||
| func certbotDidRenew(output string) bool { |
There was a problem hiding this comment.
Relying on string matching for certbot output is fragile across different versions or locales. While effective for a quick fix, a more robust check would involve inspecting the certificate's expiration timestamp on disk using crypto/x509 after the certbot command completes to verify if the file changed.
Suggested change
| func certbotDidRenew(output string) bool { | |
| func certbotDidRenew(output string) bool { | |
| lower := strings.ToLower(output) | |
| return !strings.Contains(lower, "not yet due") && | |
| !strings.Contains(lower, "no renewals were attempted") | |
| } |
Certificates could expire even with the renewal checker running, because automatic renewal only started when explicitly enabled in config and the shipped example set it off. Auto-renewal now defaults on, so an install with certbot enabled renews its certificates before they lapse without extra configuration. An explicit false is still honored for anyone who wants to opt out.
nfebe
changed the title
The global auto-renewal flag gated the entire renewer, so a certificate the user had individually marked for auto-renew would still lapse whenever the global setting was off. The renewer now runs whenever certbot is enabled, and each certificate's own auto-renew setting decides whether it renews. The global flag only supplies the default for certificates that were never toggled; an explicit per-certificate choice, on or off, always wins over it.
|
|
||
| func (s *Server) renewCertificate(c *gin.Context) { | ||
| domain := c.Param("domain") | ||
| force := c.Query("force") == "true" |
There was a problem hiding this comment.
While query parameters are typically strings, explicitly checking for 'true' is fine here. However, consider if this should also support '1' or 'yes' if the UI or external callers might provide them. For now, strict 'true' is acceptable but worth noting.
Suggested change
| force := c.Query("force") == "true" | |
| force := c.DefaultQuery("force", "false") == "true" |
|
|
||
| // certbotDidRenew reports whether a `certbot renew` run actually reissued a cert, | ||
| // by looking for the phrases certbot prints when it skips a not-yet-due lineage. | ||
| func certbotDidRenew(output string) bool { |
There was a problem hiding this comment.
The string matching logic is slightly redundant. Since multiple phrases result in false, we can simplify this with a loop or a more concise switch. Also, ensure the function handles whitespace-only output correctly.
Suggested change
| func certbotDidRenew(output string) bool { | |
| func certbotDidRenew(output string) bool { | |
| lower := strings.ToLower(output) | |
| keywords := []string{ | |
| "no renewals were attempted", | |
| "not yet due for renewal", | |
| "no certificates are due for renewal", | |
| } | |
| for _, k := range keywords { | |
| if strings.Contains(lower, k) { | |
| return false | |
| } | |
| } | |
| return strings.TrimSpace(output) != "" | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three changes on one branch, from the Albacore serving and reliability pass.
Certificates no longer silently expire. Two compounding problems in the renewal path: automatic renewal only started when
auto_renewal_enabledwas explicitly set (and the shipped example set it off), so the renewer never ran; and even when it did, the global flag gated the whole renewer, so a certificate the user had individually marked for auto-renew could still be vetoed by the global setting. Now the renewer runs whenever certbot is enabled, auto-renewal defaults on, and each certificate's own auto-renew setting is authoritative. The global flag only supplies the default for certificates that were never toggled; an explicit per-certificate choice always wins.Single-certificate renewal is honest, with force as an explicit option. The per-certificate Renew action reported success even when it renewed nothing (certbot skips a not-yet-due certificate and exits 0). It now reports whether a certificate was actually reissued, and force-renewal is an explicit request option (
?force=true) the UI can offer deliberately, rather than being applied to every renew.Proxy defaults tuned for faster deployment serving. Upstream requests now use HTTP/1.1 on every proxied route rather than only the primary one, access logging is buffered, gzip covers modern asset types (JavaScript, wasm, web fonts) with a minimum-size floor, and proxy read buffers are sized up. Validated with
openresty -tagainst the realopenresty:alpineimage.Deferred and tracked separately: reusing connections to app containers (Albacore #158) and routing externally-fronted proxy domains (#166).