-
Notifications
You must be signed in to change notification settings - Fork 3
fix: Auto-renew certificates by default, with honest renew reporting and proxy tuning #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
944eb1a
efa72e7
4ccd3cf
2b595ff
386fd23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -176,28 +176,49 @@ func (m *Manager) RenewCertificates() (*RenewalResult, error) { | |||||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| func (m *Manager) RenewCertificate(domain string) (*RenewalResult, error) { | ||||||||||||||||||||||||||||||||||||||||||||
| func (m *Manager) RenewCertificate(domain string, force bool) (*RenewalResult, error) { | ||||||||||||||||||||||||||||||||||||||||||||
| m.mu.Lock() | ||||||||||||||||||||||||||||||||||||||||||||
| defer m.mu.Unlock() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if !m.certificateExistsLocked(domain) { | ||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("certificate for domain %q not found", domain) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| output, err := m.executeCertbot([]string{ | ||||||||||||||||||||||||||||||||||||||||||||
| "renew", | ||||||||||||||||||||||||||||||||||||||||||||
| "--non-interactive", | ||||||||||||||||||||||||||||||||||||||||||||
| "--cert-name", domain, | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| args := []string{"renew", "--non-interactive", "--cert-name", domain} | ||||||||||||||||||||||||||||||||||||||||||||
| if force { | ||||||||||||||||||||||||||||||||||||||||||||
| args = append(args, "--force-renewal") | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| output, err := m.executeCertbot(args) | ||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("renewal failed for %s: %s - %w", domain, string(output), err) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return &RenewalResult{ | ||||||||||||||||||||||||||||||||||||||||||||
| Success: true, | ||||||||||||||||||||||||||||||||||||||||||||
| Message: string(output), | ||||||||||||||||||||||||||||||||||||||||||||
| RenewedDomains: []string{domain}, | ||||||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||||||
| // Without --force-renewal certbot skips a not-yet-due cert and still exits 0, | ||||||||||||||||||||||||||||||||||||||||||||
| // so distinguish an actual reissue from a no-op instead of always claiming success. | ||||||||||||||||||||||||||||||||||||||||||||
| renewed := force || certbotDidRenew(string(output)) | ||||||||||||||||||||||||||||||||||||||||||||
| result := &RenewalResult{ | ||||||||||||||||||||||||||||||||||||||||||||
| Success: true, | ||||||||||||||||||||||||||||||||||||||||||||
| Renewed: renewed, | ||||||||||||||||||||||||||||||||||||||||||||
| Message: string(output), | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if renewed { | ||||||||||||||||||||||||||||||||||||||||||||
| result.RenewedDomains = []string{domain} | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| return result, nil | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The string matching logic is slightly redundant. Since multiple phrases result in
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| lower := strings.ToLower(output) | ||||||||||||||||||||||||||||||||||||||||||||
| switch { | ||||||||||||||||||||||||||||||||||||||||||||
| case strings.Contains(lower, "no renewals were attempted"), | ||||||||||||||||||||||||||||||||||||||||||||
| strings.Contains(lower, "not yet due for renewal"), | ||||||||||||||||||||||||||||||||||||||||||||
| strings.Contains(lower, "no certificates are due for renewal"): | ||||||||||||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| func (m *Manager) certificateExistsLocked(domain string) bool { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -423,6 +444,7 @@ type CertificateResult struct { | |||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| type RenewalResult struct { | ||||||||||||||||||||||||||||||||||||||||||||
| Success bool `json:"success"` | ||||||||||||||||||||||||||||||||||||||||||||
| Renewed bool `json:"renewed"` | ||||||||||||||||||||||||||||||||||||||||||||
| Message string `json:"message"` | ||||||||||||||||||||||||||||||||||||||||||||
| RenewedDomains []string `json:"renewed_domains,omitempty"` | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.