Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions internal/api/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -4726,15 +4726,21 @@ func (s *Server) getCertificate(c *gin.Context) {

func (s *Server) renewCertificate(c *gin.Context) {
domain := c.Param("domain")
force := c.Query("force") == "true"

Copy link
Copy Markdown

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.

Suggested change
force := c.Query("force") == "true"
force := c.DefaultQuery("force", "false") == "true"


result, err := s.proxyOrchestrator.RenewCertificate(domain)
result, err := s.proxyOrchestrator.RenewCertificate(domain, force)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}

message := "Certificate renewed"
if !result.Renewed {
message = "Certificate is not yet due for renewal"
}

c.JSON(http.StatusOK, gin.H{
"message": "Certificate renewed",
"message": message,
"domain": domain,
"result": result,
})
Expand Down
6 changes: 3 additions & 3 deletions internal/proxy/orchestrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,12 @@ func (o *Orchestrator) RenewCertificates() (*ssl.RenewalResult, error) {
return result, nil
}

func (o *Orchestrator) RenewCertificate(domain string) (*ssl.RenewalResult, error) {
func (o *Orchestrator) RenewCertificate(domain string, force bool) (*ssl.RenewalResult, error) {
if err := o.ssl.ValidateDomain(domain); err != nil {
return nil, err
}

result, err := o.ssl.RenewCertificate(domain)
result, err := o.ssl.RenewCertificate(domain, force)
if err != nil {
return nil, err
}
Expand All @@ -293,7 +293,7 @@ func (o *Orchestrator) RenewCertificatesForDomains(domains []string) *ssl.MultiC
if !o.ssl.CertificateExists(domain) {
continue
}
if _, err := o.ssl.RenewCertificate(domain); err != nil {
if _, err := o.ssl.RenewCertificate(domain, false); err != nil {
result.Results = append(result.Results, &ssl.CertificateResult{
Domain: domain,
Success: false,
Expand Down
2 changes: 1 addition & 1 deletion internal/ssl/autorenew.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (r *Renewer) Run() {
if !cert.AutoRenew {
continue
}
if _, err := r.manager.RenewCertificate(cert.Domain); err != nil {
if _, err := r.manager.RenewCertificate(cert.Domain, false); err != nil {
log.Printf("auto-renew: failed to renew %s (days_left=%d): %v", cert.Domain, cert.DaysLeft, err)
continue
}
Expand Down
44 changes: 33 additions & 11 deletions internal/ssl/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 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")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) != ""
}

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 {
Expand Down Expand Up @@ -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"`
}
Expand Down
6 changes: 5 additions & 1 deletion internal/ssl/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import (
)

type mockExecutor struct {
mu sync.Mutex
mu sync.Mutex
calls []executorCall
err error
out []byte
}

type executorCall struct {
Expand All @@ -29,6 +30,9 @@ func (e *mockExecutor) Execute(cfg *config.ServiceExecConfig, args []string) ([]
if e.err != nil {
return nil, e.err
}
if e.out != nil {
return e.out, nil
}
return []byte("ok"), nil
}

Expand Down
60 changes: 38 additions & 22 deletions internal/ssl/renewal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,48 +71,64 @@ func newTestManager(t *testing.T) (*Manager, *mockExecutor, string) {
return m, mock, certsDir
}

func TestRenewCertificate_PassesCertName(t *testing.T) {
func hasArg(args []string, want string) bool {
for _, a := range args {
if a == want {
return true
}
}
return false
}

func TestRenewCertificate_ForceAddsForceRenewal(t *testing.T) {
m, mock, certsDir := newTestManager(t)
writeTestCert(t, certsDir, "example.com", time.Now().Add(10*24*time.Hour))

result, err := m.RenewCertificate("example.com")
result, err := m.RenewCertificate("example.com", true)
if err != nil {
t.Fatalf("RenewCertificate: %v", err)
}
if !result.Success {
t.Error("expected Success=true")
if !result.Success || !result.Renewed {
t.Errorf("expected Success and Renewed true, got %+v", result)
}
if len(result.RenewedDomains) != 1 || result.RenewedDomains[0] != "example.com" {
t.Errorf("RenewedDomains = %v, want [example.com]", result.RenewedDomains)
}

if len(mock.calls) != 1 {
t.Fatalf("expected 1 executor call, got %d", len(mock.calls))
args := mock.calls[0].args
if !hasArg(args, "renew") || !hasArg(args, "--cert-name") || !hasArg(args, "--force-renewal") {
t.Errorf("expected force renewal args, got: %v", args)
}
}

args := mock.calls[0].args
var hasRenew, hasNonInteractive, hasCertName bool
for i, arg := range args {
switch arg {
case "renew":
hasRenew = true
case "--non-interactive":
hasNonInteractive = true
case "--cert-name":
if i+1 < len(args) && args[i+1] == "example.com" {
hasCertName = true
}
}
func TestRenewCertificate_NoForceSkipsForceRenewalAndReportsNotDue(t *testing.T) {
m, mock, certsDir := newTestManager(t)
mock.out = []byte("Cert not yet due for renewal\nNo renewals were attempted.")
writeTestCert(t, certsDir, "example.com", time.Now().Add(60*24*time.Hour))

result, err := m.RenewCertificate("example.com", false)
if err != nil {
t.Fatalf("RenewCertificate: %v", err)
}
if !hasRenew || !hasNonInteractive || !hasCertName {
t.Errorf("unexpected certbot args: %v", args)
if !result.Success {
t.Error("expected Success true")
}
if result.Renewed {
t.Error("expected Renewed false when certbot reports the cert is not yet due")
}
if len(result.RenewedDomains) != 0 {
t.Errorf("expected no RenewedDomains, got %v", result.RenewedDomains)
}

if hasArg(mock.calls[0].args, "--force-renewal") {
t.Errorf("did not expect --force-renewal without force, got: %v", mock.calls[0].args)
}
}

func TestRenewCertificate_ErrorsWhenMissing(t *testing.T) {
m, mock, _ := newTestManager(t)

_, err := m.RenewCertificate("missing.example.com")
_, err := m.RenewCertificate("missing.example.com", false)
if err == nil {
t.Fatal("expected error for missing certificate")
}
Expand Down
12 changes: 10 additions & 2 deletions templates/infra/nginx/nginx.conf
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,30 @@ http {
'$status $body_bytes_sent "$http_referer" '
'"$http_user_agent" "$http_x_forwarded_for"';

access_log /dev/stdout main;
access_log /dev/stdout main buffer=32k flush=5s;

sendfile on;
tcp_nopush on;
tcp_nodelay on;
keepalive_timeout 65;
keepalive_requests 1000;
types_hash_max_size 2048;
server_names_hash_bucket_size 128;
server_names_hash_max_size 2048;

proxy_http_version 1.1;
proxy_buffering on;
proxy_buffer_size 8k;
proxy_buffers 16 8k;
proxy_busy_buffers_size 16k;

# Gzip compression
gzip on;
gzip_vary on;
gzip_proxied any;
gzip_comp_level 6;
gzip_types text/plain text/css text/xml application/json application/javascript application/rss+xml application/atom+xml image/svg+xml;
gzip_min_length 1024;
gzip_types text/plain text/css text/xml text/javascript application/json application/javascript application/xml application/rss+xml application/atom+xml application/wasm application/manifest+json font/woff font/woff2 image/svg+xml;

# Security headers
add_header X-Frame-Options "SAMEORIGIN" always;
Expand Down
13 changes: 11 additions & 2 deletions templates/infra/nginx/nginx.lua.conf
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,31 @@ http {
'$status $body_bytes_sent "$http_referer" '
'"$http_user_agent" "$http_x_forwarded_for"';

access_log /dev/stdout main;
access_log /dev/stdout main buffer=32k flush=5s;

sendfile on;
tcp_nopush on;
tcp_nodelay on;
keepalive_timeout 65;
keepalive_requests 1000;
types_hash_max_size 2048;
server_names_hash_bucket_size 128;
server_names_hash_max_size 2048;

# HTTP/1.1 to upstreams on every proxied location, not just the primary route.
proxy_http_version 1.1;
proxy_buffering on;
proxy_buffer_size 8k;
proxy_buffers 16 8k;
proxy_busy_buffers_size 16k;

# Gzip compression
gzip on;
gzip_vary on;
gzip_proxied any;
gzip_comp_level 6;
gzip_types text/plain text/css text/xml application/json application/javascript application/rss+xml application/atom+xml image/svg+xml;
gzip_min_length 1024;
gzip_types text/plain text/css text/xml text/javascript application/json application/javascript application/xml application/rss+xml application/atom+xml application/wasm application/manifest+json font/woff font/woff2 image/svg+xml;

# Security headers
add_header X-Frame-Options "SAMEORIGIN" always;
Expand Down
Loading