Resolve public TLS PR review comments
- commit
- 8070205df737e4905394f725ac1aeaea342451fd
- author
- brice · · 2026-04-29 17:34 UTC
- parent
- 3d6e47e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
12 files · +224 -38
M docs/cli/tls.md +1 -1
| @@ -70,7 +70,7 @@ Errors | |||
| 70 | 70 | { | |
| 71 | 71 | "id": "cert-abc123", | |
| 72 | 72 | "names": ["example.com", "www.example.com"], | |
| 73 | - | "challenge": "", | |
| 73 | + | "challenge": "http-01", | |
| 74 | 74 | "status": "valid", | |
| 75 | 75 | "not_after": "2026-05-29T12:00:00Z", | |
| 76 | 76 | "renewal_pending": false | |
| @@ -70,7 +70,7 @@ Errors | |||||
| 70 | { | 70 | { | ||
| 71 | "id": "cert-abc123", | 71 | "id": "cert-abc123", | ||
| 72 | "names": ["example.com", "www.example.com"], | 72 | "names": ["example.com", "www.example.com"], | ||
| 73 | - | "challenge": "", | 73 | + | "challenge": "http-01", |
| 74 | "status": "valid", | 74 | "status": "valid", | ||
| 75 | "not_after": "2026-05-29T12:00:00Z", | 75 | "not_after": "2026-05-29T12:00:00Z", | ||
| 76 | "renewal_pending": false | 76 | "renewal_pending": false | ||
M internal/adapters/out/acmelego/cloudflare.go +4 -1
| @@ -76,7 +76,9 @@ func WithCloudflareBaseURL(baseURL string) CloudflareZoneResolverOption { | |||
| 76 | 76 | // WithCloudflareHTTPClient sets the HTTP client for the resolver. | |
| 77 | 77 | func WithCloudflareHTTPClient(client *http.Client) CloudflareZoneResolverOption { | |
| 78 | 78 | return func(r *CloudflareZoneResolver) { | |
| 79 | - | r.client = client | |
| 79 | + | if client != nil { | |
| 80 | + | r.client = client | |
| 81 | + | } | |
| 80 | 82 | } | |
| 81 | 83 | } | |
| 82 | 84 | ||
| @@ -103,6 +105,7 @@ var _ out.CloudflareZoneResolver = (*CloudflareZoneResolver)(nil) | |||
| 103 | 105 | // example.com, com) and returns the first active zone found. | |
| 104 | 106 | func (r *CloudflareZoneResolver) FindZone(ctx context.Context, domainName string) (out.CloudflareZone, error) { | |
| 105 | 107 | domainName = strings.TrimSuffix(strings.TrimSpace(domainName), ".") | |
| 108 | + | domainName = strings.ToLower(domainName) | |
| 106 | 109 | if domainName == "" { | |
| 107 | 110 | return out.CloudflareZone{}, fmt.Errorf("cloudflare zone resolver: empty domain") | |
| 108 | 111 | } | |
| @@ -76,7 +76,9 @@ func WithCloudflareBaseURL(baseURL string) CloudflareZoneResolverOption { | |||||
| 76 | // WithCloudflareHTTPClient sets the HTTP client for the resolver. | 76 | // WithCloudflareHTTPClient sets the HTTP client for the resolver. | ||
| 77 | func WithCloudflareHTTPClient(client *http.Client) CloudflareZoneResolverOption { | 77 | func WithCloudflareHTTPClient(client *http.Client) CloudflareZoneResolverOption { | ||
| 78 | return func(r *CloudflareZoneResolver) { | 78 | return func(r *CloudflareZoneResolver) { | ||
| 79 | - | r.client = client | 79 | + | if client != nil { |
| 80 | + | r.client = client | |||
| 81 | + | } | |||
| 80 | } | 82 | } | ||
| 81 | } | 83 | } | ||
| 82 | 84 | ||||
| @@ -103,6 +105,7 @@ var _ out.CloudflareZoneResolver = (*CloudflareZoneResolver)(nil) | |||||
| 103 | // example.com, com) and returns the first active zone found. | 105 | // example.com, com) and returns the first active zone found. | ||
| 104 | func (r *CloudflareZoneResolver) FindZone(ctx context.Context, domainName string) (out.CloudflareZone, error) { | 106 | func (r *CloudflareZoneResolver) FindZone(ctx context.Context, domainName string) (out.CloudflareZone, error) { | ||
| 105 | domainName = strings.TrimSuffix(strings.TrimSpace(domainName), ".") | 107 | domainName = strings.TrimSuffix(strings.TrimSpace(domainName), ".") | ||
| 108 | + | domainName = strings.ToLower(domainName) | |||
| 106 | if domainName == "" { | 109 | if domainName == "" { | ||
| 107 | return out.CloudflareZone{}, fmt.Errorf("cloudflare zone resolver: empty domain") | 110 | return out.CloudflareZone{}, fmt.Errorf("cloudflare zone resolver: empty domain") | ||
| 108 | } | 111 | } | ||
M internal/adapters/out/acmelego/cloudflare_test.go +77 -0
| @@ -181,4 +181,81 @@ func TestCloudflareZoneResolverImplementsInterface(t *testing.T) { | |||
| 181 | 181 | ||
| 182 | 182 | // Check defaults | |
| 183 | 183 | assert.Equal(t, "https://api.cloudflare.com/client/v4", r.baseURL) | |
| 184 | + | assert.NotNil(t, r.client) | |
| 185 | + | assert.Equal(t, defaultCloudflareTimeout, r.client.Timeout) | |
| 186 | + | } | |
| 187 | + | ||
| 188 | + | func TestWithCloudflareHTTPClientNilLeavesDefault(t *testing.T) { | |
| 189 | + | // Passing nil must not replace the default HTTP client. | |
| 190 | + | r := NewCloudflareZoneResolver("token", WithCloudflareHTTPClient(nil)) | |
| 191 | + | require.NotNil(t, r.client) | |
| 192 | + | assert.Equal(t, defaultCloudflareTimeout, r.client.Timeout, | |
| 193 | + | "default timeout should be preserved when nil is passed") | |
| 194 | + | } | |
| 195 | + | ||
| 196 | + | func TestCloudflareZoneResolverFindZoneLowercasesInput(t *testing.T) { | |
| 197 | + | // The server expects lowercase names; uppercase input must be normalized. | |
| 198 | + | callCount := 0 | |
| 199 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 200 | + | callCount++ | |
| 201 | + | name := r.URL.Query().Get("name") | |
| 202 | + | assert.Equal(t, "example.com", name, "should receive lowercased domain, got %q", name) | |
| 203 | + | ||
| 204 | + | w.Header().Set("Content-Type", "application/json") | |
| 205 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |
| 206 | + | Success: true, | |
| 207 | + | Result: []zoneAPIResult{ | |
| 208 | + | {ID: "zone-1", Name: "example.com", Status: "active"}, | |
| 209 | + | }, | |
| 210 | + | }) | |
| 211 | + | })) | |
| 212 | + | defer server.Close() | |
| 213 | + | ||
| 214 | + | resolver := NewCloudflareZoneResolver("test-token", | |
| 215 | + | WithCloudflareBaseURL(server.URL)) | |
| 216 | + | ||
| 217 | + | // Uppercase input — should lowercase to EXAMPLE.COM -> example.com | |
| 218 | + | zone, err := resolver.FindZone(context.Background(), "EXAMPLE.COM") | |
| 219 | + | require.NoError(t, err) | |
| 220 | + | assert.Equal(t, "zone-1", zone.ID) | |
| 221 | + | assert.Equal(t, 1, callCount) | |
| 222 | + | } | |
| 223 | + | ||
| 224 | + | func TestCloudflareZoneResolverFindZoneMixedCaseWithTrailingDot(t *testing.T) { | |
| 225 | + | // Mixed case with trailing dot should be lowercased and dot-stripped. | |
| 226 | + | // The search chain tries "mixed.example.com" (empty), then "example.com" (found). | |
| 227 | + | callCount := 0 | |
| 228 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 229 | + | callCount++ | |
| 230 | + | name := r.URL.Query().Get("name") | |
| 231 | + | switch name { | |
| 232 | + | case "mixed.example.com": | |
| 233 | + | // No zone found for subdomain | |
| 234 | + | w.Header().Set("Content-Type", "application/json") | |
| 235 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |
| 236 | + | Success: true, | |
| 237 | + | Result: []zoneAPIResult{}, | |
| 238 | + | }) | |
| 239 | + | case "example.com": | |
| 240 | + | w.Header().Set("Content-Type", "application/json") | |
| 241 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |
| 242 | + | Success: true, | |
| 243 | + | Result: []zoneAPIResult{ | |
| 244 | + | {ID: "zone-1", Name: "example.com", Status: "active"}, | |
| 245 | + | }, | |
| 246 | + | }) | |
| 247 | + | default: | |
| 248 | + | t.Errorf("unexpected zone name query: %s", name) | |
| 249 | + | } | |
| 250 | + | })) | |
| 251 | + | defer server.Close() | |
| 252 | + | ||
| 253 | + | resolver := NewCloudflareZoneResolver("test-token", | |
| 254 | + | WithCloudflareBaseURL(server.URL)) | |
| 255 | + | ||
| 256 | + | zone, err := resolver.FindZone(context.Background(), "Mixed.EXAMPLE.Com.") | |
| 257 | + | require.NoError(t, err) | |
| 258 | + | assert.Equal(t, "zone-1", zone.ID) | |
| 259 | + | // First call: mixed.example.com (empty), second: example.com (found) | |
| 260 | + | assert.Equal(t, 2, callCount, "should try mixed.example.com then find example.com") | |
| 184 | 261 | } | |
| @@ -181,4 +181,81 @@ func TestCloudflareZoneResolverImplementsInterface(t *testing.T) { | |||||
| 181 | 181 | ||||
| 182 | // Check defaults | 182 | // Check defaults | ||
| 183 | assert.Equal(t, "https://api.cloudflare.com/client/v4", r.baseURL) | 183 | assert.Equal(t, "https://api.cloudflare.com/client/v4", r.baseURL) | ||
| 184 | + | assert.NotNil(t, r.client) | |||
| 185 | + | assert.Equal(t, defaultCloudflareTimeout, r.client.Timeout) | |||
| 186 | + | } | |||
| 187 | + | ||||
| 188 | + | func TestWithCloudflareHTTPClientNilLeavesDefault(t *testing.T) { | |||
| 189 | + | // Passing nil must not replace the default HTTP client. | |||
| 190 | + | r := NewCloudflareZoneResolver("token", WithCloudflareHTTPClient(nil)) | |||
| 191 | + | require.NotNil(t, r.client) | |||
| 192 | + | assert.Equal(t, defaultCloudflareTimeout, r.client.Timeout, | |||
| 193 | + | "default timeout should be preserved when nil is passed") | |||
| 194 | + | } | |||
| 195 | + | ||||
| 196 | + | func TestCloudflareZoneResolverFindZoneLowercasesInput(t *testing.T) { | |||
| 197 | + | // The server expects lowercase names; uppercase input must be normalized. | |||
| 198 | + | callCount := 0 | |||
| 199 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 200 | + | callCount++ | |||
| 201 | + | name := r.URL.Query().Get("name") | |||
| 202 | + | assert.Equal(t, "example.com", name, "should receive lowercased domain, got %q", name) | |||
| 203 | + | ||||
| 204 | + | w.Header().Set("Content-Type", "application/json") | |||
| 205 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |||
| 206 | + | Success: true, | |||
| 207 | + | Result: []zoneAPIResult{ | |||
| 208 | + | {ID: "zone-1", Name: "example.com", Status: "active"}, | |||
| 209 | + | }, | |||
| 210 | + | }) | |||
| 211 | + | })) | |||
| 212 | + | defer server.Close() | |||
| 213 | + | ||||
| 214 | + | resolver := NewCloudflareZoneResolver("test-token", | |||
| 215 | + | WithCloudflareBaseURL(server.URL)) | |||
| 216 | + | ||||
| 217 | + | // Uppercase input — should lowercase to EXAMPLE.COM -> example.com | |||
| 218 | + | zone, err := resolver.FindZone(context.Background(), "EXAMPLE.COM") | |||
| 219 | + | require.NoError(t, err) | |||
| 220 | + | assert.Equal(t, "zone-1", zone.ID) | |||
| 221 | + | assert.Equal(t, 1, callCount) | |||
| 222 | + | } | |||
| 223 | + | ||||
| 224 | + | func TestCloudflareZoneResolverFindZoneMixedCaseWithTrailingDot(t *testing.T) { | |||
| 225 | + | // Mixed case with trailing dot should be lowercased and dot-stripped. | |||
| 226 | + | // The search chain tries "mixed.example.com" (empty), then "example.com" (found). | |||
| 227 | + | callCount := 0 | |||
| 228 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 229 | + | callCount++ | |||
| 230 | + | name := r.URL.Query().Get("name") | |||
| 231 | + | switch name { | |||
| 232 | + | case "mixed.example.com": | |||
| 233 | + | // No zone found for subdomain | |||
| 234 | + | w.Header().Set("Content-Type", "application/json") | |||
| 235 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |||
| 236 | + | Success: true, | |||
| 237 | + | Result: []zoneAPIResult{}, | |||
| 238 | + | }) | |||
| 239 | + | case "example.com": | |||
| 240 | + | w.Header().Set("Content-Type", "application/json") | |||
| 241 | + | json.NewEncoder(w).Encode(zoneAPIResponse{ | |||
| 242 | + | Success: true, | |||
| 243 | + | Result: []zoneAPIResult{ | |||
| 244 | + | {ID: "zone-1", Name: "example.com", Status: "active"}, | |||
| 245 | + | }, | |||
| 246 | + | }) | |||
| 247 | + | default: | |||
| 248 | + | t.Errorf("unexpected zone name query: %s", name) | |||
| 249 | + | } | |||
| 250 | + | })) | |||
| 251 | + | defer server.Close() | |||
| 252 | + | ||||
| 253 | + | resolver := NewCloudflareZoneResolver("test-token", | |||
| 254 | + | WithCloudflareBaseURL(server.URL)) | |||
| 255 | + | ||||
| 256 | + | zone, err := resolver.FindZone(context.Background(), "Mixed.EXAMPLE.Com.") | |||
| 257 | + | require.NoError(t, err) | |||
| 258 | + | assert.Equal(t, "zone-1", zone.ID) | |||
| 259 | + | // First call: mixed.example.com (empty), second: example.com (found) | |||
| 260 | + | assert.Equal(t, 2, callCount, "should try mixed.example.com then find example.com") | |||
| 184 | } | 261 | } | ||
M internal/adapters/out/acmestore/store.go +4 -0
| @@ -113,6 +113,10 @@ func (s *Store) LoadAll(_ context.Context) ([]out.StoredCertificate, error) { | |||
| 113 | 113 | if !safeID(id) { | |
| 114 | 114 | continue | |
| 115 | 115 | } | |
| 116 | + | // Skip temporary and backup directories created by Save. | |
| 117 | + | if strings.Contains(id, ".tmp-") || strings.HasSuffix(id, ".old") { | |
| 118 | + | continue | |
| 119 | + | } | |
| 116 | 120 | ||
| 117 | 121 | cert, err := s.loadOne(id) | |
| 118 | 122 | if err != nil { | |
| @@ -113,6 +113,10 @@ func (s *Store) LoadAll(_ context.Context) ([]out.StoredCertificate, error) { | |||||
| 113 | if !safeID(id) { | 113 | if !safeID(id) { | ||
| 114 | continue | 114 | continue | ||
| 115 | } | 115 | } | ||
| 116 | + | // Skip temporary and backup directories created by Save. | |||
| 117 | + | if strings.Contains(id, ".tmp-") || strings.HasSuffix(id, ".old") { | |||
| 118 | + | continue | |||
| 119 | + | } | |||
| 116 | 120 | ||||
| 117 | cert, err := s.loadOne(id) | 121 | cert, err := s.loadOne(id) | ||
| 118 | if err != nil { | 122 | if err != nil { | ||
M internal/adapters/out/acmestore/store_test.go +40 -2
| @@ -215,10 +215,10 @@ func TestStoreLoadAllInvalidChallenge(t *testing.T) { | |||
| 215 | 215 | err = os.WriteFile(filepath.Join(certDirPath, metaFile), metaData, metaMode) | |
| 216 | 216 | require.NoError(t, err) | |
| 217 | 217 | ||
| 218 | - | // LoadAll should return an error | |
| 218 | + | // LoadAll should return an error wrapping domain.ErrACMEChallengeInvalid | |
| 219 | 219 | _, err = store.LoadAll(ctx) | |
| 220 | 220 | require.Error(t, err) | |
| 221 | - | assert.Contains(t, err.Error(), "acme challenge invalid") | |
| 221 | + | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | |
| 222 | 222 | } | |
| 223 | 223 | ||
| 224 | 224 | func TestStoreLoadAllCorruptPrivkey(t *testing.T) { | |
| @@ -284,6 +284,44 @@ func TestStoreLoadAllCorruptFullchain(t *testing.T) { | |||
| 284 | 284 | assert.Contains(t, err.Error(), "fullchain.pem") | |
| 285 | 285 | } | |
| 286 | 286 | ||
| 287 | + | func TestStoreLoadAllSkipsTempAndBackupDirs(t *testing.T) { | |
| 288 | + | root := t.TempDir() | |
| 289 | + | ctx := context.Background() | |
| 290 | + | ||
| 291 | + | store, err := New(root) | |
| 292 | + | require.NoError(t, err) | |
| 293 | + | ||
| 294 | + | fullchainPEM, privKeyPEM := generateTestCertPEM(t) | |
| 295 | + | ||
| 296 | + | // Save a valid certificate first. | |
| 297 | + | cert := out.StoredCertificate{ | |
| 298 | + | ID: "valid.example.com", | |
| 299 | + | Names: []string{"valid.example.com"}, | |
| 300 | + | Challenge: domain.ACMEChallengeHTTP01, | |
| 301 | + | CertPEM: fullchainPEM, | |
| 302 | + | FullchainPEM: fullchainPEM, | |
| 303 | + | PrivateKeyPEM: privKeyPEM, | |
| 304 | + | } | |
| 305 | + | err = store.Save(ctx, cert) | |
| 306 | + | require.NoError(t, err) | |
| 307 | + | ||
| 308 | + | // Create a tmp-* directory that Save would have left behind. | |
| 309 | + | tmpDir := filepath.Join(root, certDir, "orphan.tmp-abc123") | |
| 310 | + | err = os.MkdirAll(tmpDir, dirMode) | |
| 311 | + | require.NoError(t, err) | |
| 312 | + | ||
| 313 | + | // Create a .old directory that Save would have left behind. | |
| 314 | + | oldDir := filepath.Join(root, certDir, "old-cert.example.com.old") | |
| 315 | + | err = os.MkdirAll(oldDir, dirMode) | |
| 316 | + | require.NoError(t, err) | |
| 317 | + | ||
| 318 | + | // LoadAll should return only the valid cert, skipping both temp and backup dirs. | |
| 319 | + | certs, err := store.LoadAll(ctx) | |
| 320 | + | require.NoError(t, err) | |
| 321 | + | require.Len(t, certs, 1, "should skip tmp-* and *.old directories") | |
| 322 | + | assert.Equal(t, "valid.example.com", certs[0].ID) | |
| 323 | + | } | |
| 324 | + | ||
| 287 | 325 | func TestStoreLoadAllInvalidPEM(t *testing.T) { | |
| 288 | 326 | root := t.TempDir() | |
| 289 | 327 | ctx := context.Background() | |
| @@ -215,10 +215,10 @@ func TestStoreLoadAllInvalidChallenge(t *testing.T) { | |||||
| 215 | err = os.WriteFile(filepath.Join(certDirPath, metaFile), metaData, metaMode) | 215 | err = os.WriteFile(filepath.Join(certDirPath, metaFile), metaData, metaMode) | ||
| 216 | require.NoError(t, err) | 216 | require.NoError(t, err) | ||
| 217 | 217 | ||||
| 218 | - | // LoadAll should return an error | 218 | + | // LoadAll should return an error wrapping domain.ErrACMEChallengeInvalid |
| 219 | _, err = store.LoadAll(ctx) | 219 | _, err = store.LoadAll(ctx) | ||
| 220 | require.Error(t, err) | 220 | require.Error(t, err) | ||
| 221 | - | assert.Contains(t, err.Error(), "acme challenge invalid") | 221 | + | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) |
| 222 | } | 222 | } | ||
| 223 | 223 | ||||
| 224 | func TestStoreLoadAllCorruptPrivkey(t *testing.T) { | 224 | func TestStoreLoadAllCorruptPrivkey(t *testing.T) { | ||
| @@ -284,6 +284,44 @@ func TestStoreLoadAllCorruptFullchain(t *testing.T) { | |||||
| 284 | assert.Contains(t, err.Error(), "fullchain.pem") | 284 | assert.Contains(t, err.Error(), "fullchain.pem") | ||
| 285 | } | 285 | } | ||
| 286 | 286 | ||||
| 287 | + | func TestStoreLoadAllSkipsTempAndBackupDirs(t *testing.T) { | |||
| 288 | + | root := t.TempDir() | |||
| 289 | + | ctx := context.Background() | |||
| 290 | + | ||||
| 291 | + | store, err := New(root) | |||
| 292 | + | require.NoError(t, err) | |||
| 293 | + | ||||
| 294 | + | fullchainPEM, privKeyPEM := generateTestCertPEM(t) | |||
| 295 | + | ||||
| 296 | + | // Save a valid certificate first. | |||
| 297 | + | cert := out.StoredCertificate{ | |||
| 298 | + | ID: "valid.example.com", | |||
| 299 | + | Names: []string{"valid.example.com"}, | |||
| 300 | + | Challenge: domain.ACMEChallengeHTTP01, | |||
| 301 | + | CertPEM: fullchainPEM, | |||
| 302 | + | FullchainPEM: fullchainPEM, | |||
| 303 | + | PrivateKeyPEM: privKeyPEM, | |||
| 304 | + | } | |||
| 305 | + | err = store.Save(ctx, cert) | |||
| 306 | + | require.NoError(t, err) | |||
| 307 | + | ||||
| 308 | + | // Create a tmp-* directory that Save would have left behind. | |||
| 309 | + | tmpDir := filepath.Join(root, certDir, "orphan.tmp-abc123") | |||
| 310 | + | err = os.MkdirAll(tmpDir, dirMode) | |||
| 311 | + | require.NoError(t, err) | |||
| 312 | + | ||||
| 313 | + | // Create a .old directory that Save would have left behind. | |||
| 314 | + | oldDir := filepath.Join(root, certDir, "old-cert.example.com.old") | |||
| 315 | + | err = os.MkdirAll(oldDir, dirMode) | |||
| 316 | + | require.NoError(t, err) | |||
| 317 | + | ||||
| 318 | + | // LoadAll should return only the valid cert, skipping both temp and backup dirs. | |||
| 319 | + | certs, err := store.LoadAll(ctx) | |||
| 320 | + | require.NoError(t, err) | |||
| 321 | + | require.Len(t, certs, 1, "should skip tmp-* and *.old directories") | |||
| 322 | + | assert.Equal(t, "valid.example.com", certs[0].ID) | |||
| 323 | + | } | |||
| 324 | + | ||||
| 287 | func TestStoreLoadAllInvalidPEM(t *testing.T) { | 325 | func TestStoreLoadAllInvalidPEM(t *testing.T) { | ||
| 288 | root := t.TempDir() | 326 | root := t.TempDir() | ||
| 289 | ctx := context.Background() | 327 | ctx := context.Background() | ||
M internal/domain/errors.go +2 -0
| @@ -98,6 +98,8 @@ var ( | |||
| 98 | 98 | ErrACMEChallengeInvalid = errors.New("acme challenge invalid") | |
| 99 | 99 | ErrCloudflareTokenMissing = errors.New("cloudflare api token missing") | |
| 100 | 100 | ErrCertificateStoreRequired = errors.New("certificate store required") | |
| 101 | + | ErrCertificateIssuerRequired = errors.New("certificate issuer required") | |
| 102 | + | ErrRouteSourceRequired = errors.New("route source required") | |
| 101 | 103 | ErrHTTPChallengeSinkRequired = errors.New("http challenge sink required") | |
| 102 | 104 | ErrTLSRouteNotCovered = errors.New("tls route not covered by public certificate") | |
| 103 | 105 | ) | |
| @@ -98,6 +98,8 @@ var ( | |||||
| 98 | ErrACMEChallengeInvalid = errors.New("acme challenge invalid") | 98 | ErrACMEChallengeInvalid = errors.New("acme challenge invalid") | ||
| 99 | ErrCloudflareTokenMissing = errors.New("cloudflare api token missing") | 99 | ErrCloudflareTokenMissing = errors.New("cloudflare api token missing") | ||
| 100 | ErrCertificateStoreRequired = errors.New("certificate store required") | 100 | ErrCertificateStoreRequired = errors.New("certificate store required") | ||
| 101 | + | ErrCertificateIssuerRequired = errors.New("certificate issuer required") | |||
| 102 | + | ErrRouteSourceRequired = errors.New("route source required") | |||
| 101 | ErrHTTPChallengeSinkRequired = errors.New("http challenge sink required") | 103 | ErrHTTPChallengeSinkRequired = errors.New("http challenge sink required") | ||
| 102 | ErrTLSRouteNotCovered = errors.New("tls route not covered by public certificate") | 104 | ErrTLSRouteNotCovered = errors.New("tls route not covered by public certificate") | ||
| 103 | ) | 105 | ) | ||
M internal/usecase/publictls/config.go +16 -7
| @@ -27,12 +27,21 @@ type EffectiveChallenge struct { | |||
| 27 | 27 | Reason string | |
| 28 | 28 | } | |
| 29 | 29 | ||
| 30 | + | // validPort checks that port is in the valid TCP port range 1..65535. | |
| 31 | + | func validPort(p int) bool { | |
| 32 | + | return p >= 1 && p <= 65535 | |
| 33 | + | } | |
| 34 | + | ||
| 30 | 35 | // ResolveEffectiveChallenge resolves the effective ACME challenge configuration | |
| 31 | 36 | // from the given Config and optional token resolver. | |
| 32 | 37 | func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.SecretResolver) (EffectiveChallenge, error) { | |
| 33 | 38 | if !cfg.Enabled { | |
| 39 | + | configuredMode := domain.ACMEChallengeAuto | |
| 40 | + | if parsed, err := domain.ParseACMEChallengeMode(cfg.Challenge); err == nil { | |
| 41 | + | configuredMode = parsed | |
| 42 | + | } | |
| 34 | 43 | return EffectiveChallenge{ | |
| 35 | - | ConfiguredMode: domain.ACMEChallengeAuto, | |
| 44 | + | ConfiguredMode: configuredMode, | |
| 36 | 45 | Mode: domain.ACMEChallengeAuto, | |
| 37 | 46 | TokenSource: domain.ACMETokenSourceNone, | |
| 38 | 47 | Reason: "ACME disabled", | |
| @@ -43,8 +52,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 43 | 52 | return EffectiveChallenge{}, domain.ErrACMEEmailRequired | |
| 44 | 53 | } | |
| 45 | 54 | ||
| 46 | - | if cfg.TLSPort == 0 { | |
| 47 | - | return EffectiveChallenge{}, fmt.Errorf("%w: tls_port must be set", domain.ErrACMEChallengeInvalid) | |
| 55 | + | if !validPort(cfg.TLSPort) { | |
| 56 | + | return EffectiveChallenge{}, fmt.Errorf("%w: tls_port must be in range 1..65535", domain.ErrACMEChallengeInvalid) | |
| 48 | 57 | } | |
| 49 | 58 | ||
| 50 | 59 | parsedMode, err := domain.ParseACMEChallengeMode(cfg.Challenge) | |
| @@ -54,8 +63,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 54 | 63 | ||
| 55 | 64 | switch parsedMode { | |
| 56 | 65 | case domain.ACMEChallengeHTTP01: | |
| 57 | - | if cfg.HTTPPort == 0 { | |
| 58 | - | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be set for http-01 challenge", domain.ErrACMEChallengeInvalid) | |
| 66 | + | if !validPort(cfg.HTTPPort) { | |
| 67 | + | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be in range 1..65535 for http-01 challenge", domain.ErrACMEChallengeInvalid) | |
| 59 | 68 | } | |
| 60 | 69 | return EffectiveChallenge{ | |
| 61 | 70 | ConfiguredMode: domain.ACMEChallengeHTTP01, | |
| @@ -91,8 +100,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 91 | 100 | Reason: "auto selected cloudflare dns-01 (token available)", | |
| 92 | 101 | }, nil | |
| 93 | 102 | } | |
| 94 | - | if cfg.HTTPPort == 0 { | |
| 95 | - | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be set for http-01 fallback in auto mode", domain.ErrACMEChallengeInvalid) | |
| 103 | + | if !validPort(cfg.HTTPPort) { | |
| 104 | + | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be in range 1..65535 for http-01 fallback in auto mode", domain.ErrACMEChallengeInvalid) | |
| 96 | 105 | } | |
| 97 | 106 | return EffectiveChallenge{ | |
| 98 | 107 | ConfiguredMode: domain.ACMEChallengeAuto, | |
| @@ -27,12 +27,21 @@ type EffectiveChallenge struct { | |||||
| 27 | Reason string | 27 | Reason string | ||
| 28 | } | 28 | } | ||
| 29 | 29 | ||||
| 30 | + | // validPort checks that port is in the valid TCP port range 1..65535. | |||
| 31 | + | func validPort(p int) bool { | |||
| 32 | + | return p >= 1 && p <= 65535 | |||
| 33 | + | } | |||
| 34 | + | ||||
| 30 | // ResolveEffectiveChallenge resolves the effective ACME challenge configuration | 35 | // ResolveEffectiveChallenge resolves the effective ACME challenge configuration | ||
| 31 | // from the given Config and optional token resolver. | 36 | // from the given Config and optional token resolver. | ||
| 32 | func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.SecretResolver) (EffectiveChallenge, error) { | 37 | func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.SecretResolver) (EffectiveChallenge, error) { | ||
| 33 | if !cfg.Enabled { | 38 | if !cfg.Enabled { | ||
| 39 | + | configuredMode := domain.ACMEChallengeAuto | |||
| 40 | + | if parsed, err := domain.ParseACMEChallengeMode(cfg.Challenge); err == nil { | |||
| 41 | + | configuredMode = parsed | |||
| 42 | + | } | |||
| 34 | return EffectiveChallenge{ | 43 | return EffectiveChallenge{ | ||
| 35 | - | ConfiguredMode: domain.ACMEChallengeAuto, | 44 | + | ConfiguredMode: configuredMode, |
| 36 | Mode: domain.ACMEChallengeAuto, | 45 | Mode: domain.ACMEChallengeAuto, | ||
| 37 | TokenSource: domain.ACMETokenSourceNone, | 46 | TokenSource: domain.ACMETokenSourceNone, | ||
| 38 | Reason: "ACME disabled", | 47 | Reason: "ACME disabled", | ||
| @@ -43,8 +52,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 43 | return EffectiveChallenge{}, domain.ErrACMEEmailRequired | 52 | return EffectiveChallenge{}, domain.ErrACMEEmailRequired | ||
| 44 | } | 53 | } | ||
| 45 | 54 | ||||
| 46 | - | if cfg.TLSPort == 0 { | 55 | + | if !validPort(cfg.TLSPort) { |
| 47 | - | return EffectiveChallenge{}, fmt.Errorf("%w: tls_port must be set", domain.ErrACMEChallengeInvalid) | 56 | + | return EffectiveChallenge{}, fmt.Errorf("%w: tls_port must be in range 1..65535", domain.ErrACMEChallengeInvalid) |
| 48 | } | 57 | } | ||
| 49 | 58 | ||||
| 50 | parsedMode, err := domain.ParseACMEChallengeMode(cfg.Challenge) | 59 | parsedMode, err := domain.ParseACMEChallengeMode(cfg.Challenge) | ||
| @@ -54,8 +63,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 54 | 63 | ||||
| 55 | switch parsedMode { | 64 | switch parsedMode { | ||
| 56 | case domain.ACMEChallengeHTTP01: | 65 | case domain.ACMEChallengeHTTP01: | ||
| 57 | - | if cfg.HTTPPort == 0 { | 66 | + | if !validPort(cfg.HTTPPort) { |
| 58 | - | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be set for http-01 challenge", domain.ErrACMEChallengeInvalid) | 67 | + | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be in range 1..65535 for http-01 challenge", domain.ErrACMEChallengeInvalid) |
| 59 | } | 68 | } | ||
| 60 | return EffectiveChallenge{ | 69 | return EffectiveChallenge{ | ||
| 61 | ConfiguredMode: domain.ACMEChallengeHTTP01, | 70 | ConfiguredMode: domain.ACMEChallengeHTTP01, | ||
| @@ -91,8 +100,8 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 91 | Reason: "auto selected cloudflare dns-01 (token available)", | 100 | Reason: "auto selected cloudflare dns-01 (token available)", | ||
| 92 | }, nil | 101 | }, nil | ||
| 93 | } | 102 | } | ||
| 94 | - | if cfg.HTTPPort == 0 { | 103 | + | if !validPort(cfg.HTTPPort) { |
| 95 | - | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be set for http-01 fallback in auto mode", domain.ErrACMEChallengeInvalid) | 104 | + | return EffectiveChallenge{}, fmt.Errorf("%w: http_port must be in range 1..65535 for http-01 fallback in auto mode", domain.ErrACMEChallengeInvalid) |
| 96 | } | 105 | } | ||
| 97 | return EffectiveChallenge{ | 106 | return EffectiveChallenge{ | ||
| 98 | ConfiguredMode: domain.ACMEChallengeAuto, | 107 | ConfiguredMode: domain.ACMEChallengeAuto, | ||
M internal/usecase/publictls/config_test.go +44 -2
| @@ -32,6 +32,35 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||
| 32 | 32 | assert.Equal(t, "ACME disabled", ec.Reason) | |
| 33 | 33 | }) | |
| 34 | 34 | ||
| 35 | + | t.Run("disabled preserves configured challenge in ConfiguredMode", func(t *testing.T) { | |
| 36 | + | cfg := Config{Enabled: false, Challenge: "http-01"} | |
| 37 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| 38 | + | require.NoError(t, err) | |
| 39 | + | assert.Equal(t, domain.ACMEChallengeHTTP01, ec.ConfiguredMode, "should preserve http-01") | |
| 40 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode, "Mode should remain auto") | |
| 41 | + | assert.Equal(t, domain.ACMETokenSourceNone, ec.TokenSource) | |
| 42 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |
| 43 | + | }) | |
| 44 | + | ||
| 45 | + | t.Run("disabled preserves cloudflare-dns-01 in ConfiguredMode", func(t *testing.T) { | |
| 46 | + | cfg := Config{Enabled: false, Challenge: "cloudflare-dns-01"} | |
| 47 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| 48 | + | require.NoError(t, err) | |
| 49 | + | assert.Equal(t, domain.ACMEChallengeCloudflareDNS01, ec.ConfiguredMode, "should preserve cloudflare-dns-01") | |
| 50 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode, "Mode should remain auto") | |
| 51 | + | assert.Equal(t, domain.ACMETokenSourceNone, ec.TokenSource) | |
| 52 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |
| 53 | + | }) | |
| 54 | + | ||
| 55 | + | t.Run("disabled with unparseable challenge defaults ConfiguredMode to auto", func(t *testing.T) { | |
| 56 | + | cfg := Config{Enabled: false, Challenge: "unknown-challenge"} | |
| 57 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| 58 | + | require.NoError(t, err) | |
| 59 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.ConfiguredMode, "unparseable defaults to auto") | |
| 60 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode) | |
| 61 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |
| 62 | + | }) | |
| 63 | + | ||
| 35 | 64 | t.Run("enabled requires non-empty email", func(t *testing.T) { | |
| 36 | 65 | cfg := Config{Enabled: true, Email: "", TLSPort: 8443} | |
| 37 | 66 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| @@ -39,12 +68,18 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||
| 39 | 68 | assert.ErrorIs(t, err, domain.ErrACMEEmailRequired) | |
| 40 | 69 | }) | |
| 41 | 70 | ||
| 42 | - | t.Run("enabled requires non-zero tls port", func(t *testing.T) { | |
| 71 | + | t.Run("enabled requires valid tls port in 1..65535", func(t *testing.T) { | |
| 43 | 72 | cfg := Config{Enabled: true, Email: "admin@example.com", TLSPort: 0} | |
| 44 | 73 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| 45 | 74 | require.Error(t, err) | |
| 46 | 75 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | |
| 47 | 76 | assert.Contains(t, err.Error(), "tls_port") | |
| 77 | + | ||
| 78 | + | cfg2 := Config{Enabled: true, Email: "admin@example.com", TLSPort: 65536} | |
| 79 | + | _, err2 := ResolveEffectiveChallenge(context.Background(), cfg2, nil) | |
| 80 | + | require.Error(t, err2) | |
| 81 | + | assert.ErrorIs(t, err2, domain.ErrACMEChallengeInvalid) | |
| 82 | + | assert.Contains(t, err2.Error(), "tls_port") | |
| 48 | 83 | }) | |
| 49 | 84 | ||
| 50 | 85 | t.Run("auto with cloudflare token returns dns-01", func(t *testing.T) { | |
| @@ -76,11 +111,18 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||
| 76 | 111 | assert.ErrorIs(t, err, domain.ErrCloudflareTokenMissing) | |
| 77 | 112 | }) | |
| 78 | 113 | ||
| 79 | - | t.Run("http-01 requires http port", func(t *testing.T) { | |
| 114 | + | t.Run("http-01 requires valid http port in 1..65535", func(t *testing.T) { | |
| 80 | 115 | cfg := Config{Enabled: true, Email: "admin@example.com", Challenge: "http-01", TLSPort: 8443, HTTPPort: 0} | |
| 81 | 116 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |
| 82 | 117 | require.Error(t, err) | |
| 83 | 118 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | |
| 119 | + | assert.Contains(t, err.Error(), "http_port") | |
| 120 | + | ||
| 121 | + | cfg2 := Config{Enabled: true, Email: "admin@example.com", Challenge: "http-01", TLSPort: 8443, HTTPPort: 65536} | |
| 122 | + | _, err2 := ResolveEffectiveChallenge(context.Background(), cfg2, nil) | |
| 123 | + | require.Error(t, err2) | |
| 124 | + | assert.ErrorIs(t, err2, domain.ErrACMEChallengeInvalid) | |
| 125 | + | assert.Contains(t, err2.Error(), "http_port") | |
| 84 | 126 | }) | |
| 85 | 127 | ||
| 86 | 128 | t.Run("cloudflare-dns-01 preserves resolver errors", func(t *testing.T) { | |
| @@ -32,6 +32,35 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||||
| 32 | assert.Equal(t, "ACME disabled", ec.Reason) | 32 | assert.Equal(t, "ACME disabled", ec.Reason) | ||
| 33 | }) | 33 | }) | ||
| 34 | 34 | ||||
| 35 | + | t.Run("disabled preserves configured challenge in ConfiguredMode", func(t *testing.T) { | |||
| 36 | + | cfg := Config{Enabled: false, Challenge: "http-01"} | |||
| 37 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |||
| 38 | + | require.NoError(t, err) | |||
| 39 | + | assert.Equal(t, domain.ACMEChallengeHTTP01, ec.ConfiguredMode, "should preserve http-01") | |||
| 40 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode, "Mode should remain auto") | |||
| 41 | + | assert.Equal(t, domain.ACMETokenSourceNone, ec.TokenSource) | |||
| 42 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |||
| 43 | + | }) | |||
| 44 | + | ||||
| 45 | + | t.Run("disabled preserves cloudflare-dns-01 in ConfiguredMode", func(t *testing.T) { | |||
| 46 | + | cfg := Config{Enabled: false, Challenge: "cloudflare-dns-01"} | |||
| 47 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |||
| 48 | + | require.NoError(t, err) | |||
| 49 | + | assert.Equal(t, domain.ACMEChallengeCloudflareDNS01, ec.ConfiguredMode, "should preserve cloudflare-dns-01") | |||
| 50 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode, "Mode should remain auto") | |||
| 51 | + | assert.Equal(t, domain.ACMETokenSourceNone, ec.TokenSource) | |||
| 52 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |||
| 53 | + | }) | |||
| 54 | + | ||||
| 55 | + | t.Run("disabled with unparseable challenge defaults ConfiguredMode to auto", func(t *testing.T) { | |||
| 56 | + | cfg := Config{Enabled: false, Challenge: "unknown-challenge"} | |||
| 57 | + | ec, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | |||
| 58 | + | require.NoError(t, err) | |||
| 59 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.ConfiguredMode, "unparseable defaults to auto") | |||
| 60 | + | assert.Equal(t, domain.ACMEChallengeAuto, ec.Mode) | |||
| 61 | + | assert.Equal(t, "ACME disabled", ec.Reason) | |||
| 62 | + | }) | |||
| 63 | + | ||||
| 35 | t.Run("enabled requires non-empty email", func(t *testing.T) { | 64 | t.Run("enabled requires non-empty email", func(t *testing.T) { | ||
| 36 | cfg := Config{Enabled: true, Email: "", TLSPort: 8443} | 65 | cfg := Config{Enabled: true, Email: "", TLSPort: 8443} | ||
| 37 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | 66 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | ||
| @@ -39,12 +68,18 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||||
| 39 | assert.ErrorIs(t, err, domain.ErrACMEEmailRequired) | 68 | assert.ErrorIs(t, err, domain.ErrACMEEmailRequired) | ||
| 40 | }) | 69 | }) | ||
| 41 | 70 | ||||
| 42 | - | t.Run("enabled requires non-zero tls port", func(t *testing.T) { | 71 | + | t.Run("enabled requires valid tls port in 1..65535", func(t *testing.T) { |
| 43 | cfg := Config{Enabled: true, Email: "admin@example.com", TLSPort: 0} | 72 | cfg := Config{Enabled: true, Email: "admin@example.com", TLSPort: 0} | ||
| 44 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | 73 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | ||
| 45 | require.Error(t, err) | 74 | require.Error(t, err) | ||
| 46 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | 75 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | ||
| 47 | assert.Contains(t, err.Error(), "tls_port") | 76 | assert.Contains(t, err.Error(), "tls_port") | ||
| 77 | + | ||||
| 78 | + | cfg2 := Config{Enabled: true, Email: "admin@example.com", TLSPort: 65536} | |||
| 79 | + | _, err2 := ResolveEffectiveChallenge(context.Background(), cfg2, nil) | |||
| 80 | + | require.Error(t, err2) | |||
| 81 | + | assert.ErrorIs(t, err2, domain.ErrACMEChallengeInvalid) | |||
| 82 | + | assert.Contains(t, err2.Error(), "tls_port") | |||
| 48 | }) | 83 | }) | ||
| 49 | 84 | ||||
| 50 | t.Run("auto with cloudflare token returns dns-01", func(t *testing.T) { | 85 | t.Run("auto with cloudflare token returns dns-01", func(t *testing.T) { | ||
| @@ -76,11 +111,18 @@ func TestResolveEffectiveChallenge(t *testing.T) { | |||||
| 76 | assert.ErrorIs(t, err, domain.ErrCloudflareTokenMissing) | 111 | assert.ErrorIs(t, err, domain.ErrCloudflareTokenMissing) | ||
| 77 | }) | 112 | }) | ||
| 78 | 113 | ||||
| 79 | - | t.Run("http-01 requires http port", func(t *testing.T) { | 114 | + | t.Run("http-01 requires valid http port in 1..65535", func(t *testing.T) { |
| 80 | cfg := Config{Enabled: true, Email: "admin@example.com", Challenge: "http-01", TLSPort: 8443, HTTPPort: 0} | 115 | cfg := Config{Enabled: true, Email: "admin@example.com", Challenge: "http-01", TLSPort: 8443, HTTPPort: 0} | ||
| 81 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | 116 | _, err := ResolveEffectiveChallenge(context.Background(), cfg, nil) | ||
| 82 | require.Error(t, err) | 117 | require.Error(t, err) | ||
| 83 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | 118 | assert.ErrorIs(t, err, domain.ErrACMEChallengeInvalid) | ||
| 119 | + | assert.Contains(t, err.Error(), "http_port") | |||
| 120 | + | ||||
| 121 | + | cfg2 := Config{Enabled: true, Email: "admin@example.com", Challenge: "http-01", TLSPort: 8443, HTTPPort: 65536} | |||
| 122 | + | _, err2 := ResolveEffectiveChallenge(context.Background(), cfg2, nil) | |||
| 123 | + | require.Error(t, err2) | |||
| 124 | + | assert.ErrorIs(t, err2, domain.ErrACMEChallengeInvalid) | |||
| 125 | + | assert.Contains(t, err2.Error(), "http_port") | |||
| 84 | }) | 126 | }) | ||
| 85 | 127 | ||||
| 86 | t.Run("cloudflare-dns-01 preserves resolver errors", func(t *testing.T) { | 128 | t.Run("cloudflare-dns-01 preserves resolver errors", func(t *testing.T) { | ||
M internal/usecase/publictls/integration_test.go +5 -2
| @@ -5,9 +5,11 @@ import ( | |||
| 5 | 5 | "testing" | |
| 6 | 6 | ||
| 7 | 7 | "github.com/stretchr/testify/assert" | |
| 8 | + | "github.com/stretchr/testify/mock" | |
| 8 | 9 | "github.com/stretchr/testify/require" | |
| 9 | 10 | ||
| 10 | 11 | "github.com/bnema/gordon/internal/boundaries/out" | |
| 12 | + | outmocks "github.com/bnema/gordon/internal/boundaries/out/mocks" | |
| 11 | 13 | "github.com/bnema/gordon/internal/domain" | |
| 12 | 14 | ) | |
| 13 | 15 | ||
| @@ -27,8 +29,9 @@ func TestPublicTLSIntegration_DNS01WildcardStatusAndCertificateLookup(t *testing | |||
| 27 | 29 | }, | |
| 28 | 30 | } | |
| 29 | 31 | ||
| 30 | - | // Mock zone resolver returns "example.com" zone. | |
| 31 | - | zoneResolver := newZoneResolverMock(t, "example.com") | |
| 32 | + | // Mock zone resolver returns "example.com" zone for 3 routes. | |
| 33 | + | zoneResolver := outmocks.NewMockCloudflareZoneResolver(t) | |
| 34 | + | zoneResolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "example.com"}, nil).Times(3) | |
| 32 | 35 | ||
| 33 | 36 | // Mock issuer: returns valid stored cert for any order. | |
| 34 | 37 | issuer, recorder := newMockPublicCertificateIssuer(t, func(_ context.Context, order out.CertificateOrder) (*out.StoredCertificate, error) { | |
| @@ -5,9 +5,11 @@ import ( | |||||
| 5 | "testing" | 5 | "testing" | ||
| 6 | 6 | ||||
| 7 | "github.com/stretchr/testify/assert" | 7 | "github.com/stretchr/testify/assert" | ||
| 8 | + | "github.com/stretchr/testify/mock" | |||
| 8 | "github.com/stretchr/testify/require" | 9 | "github.com/stretchr/testify/require" | ||
| 9 | 10 | ||||
| 10 | "github.com/bnema/gordon/internal/boundaries/out" | 11 | "github.com/bnema/gordon/internal/boundaries/out" | ||
| 12 | + | outmocks "github.com/bnema/gordon/internal/boundaries/out/mocks" | |||
| 11 | "github.com/bnema/gordon/internal/domain" | 13 | "github.com/bnema/gordon/internal/domain" | ||
| 12 | ) | 14 | ) | ||
| 13 | 15 | ||||
| @@ -27,8 +29,9 @@ func TestPublicTLSIntegration_DNS01WildcardStatusAndCertificateLookup(t *testing | |||||
| 27 | }, | 29 | }, | ||
| 28 | } | 30 | } | ||
| 29 | 31 | ||||
| 30 | - | // Mock zone resolver returns "example.com" zone. | 32 | + | // Mock zone resolver returns "example.com" zone for 3 routes. |
| 31 | - | zoneResolver := newZoneResolverMock(t, "example.com") | 33 | + | zoneResolver := outmocks.NewMockCloudflareZoneResolver(t) |
| 34 | + | zoneResolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "example.com"}, nil).Times(3) | |||
| 32 | 35 | ||||
| 33 | // Mock issuer: returns valid stored cert for any order. | 36 | // Mock issuer: returns valid stored cert for any order. | ||
| 34 | issuer, recorder := newMockPublicCertificateIssuer(t, func(_ context.Context, order out.CertificateOrder) (*out.StoredCertificate, error) { | 37 | issuer, recorder := newMockPublicCertificateIssuer(t, func(_ context.Context, order out.CertificateOrder) (*out.StoredCertificate, error) { | ||
M internal/usecase/publictls/renewal.go +19 -10
| @@ -94,16 +94,10 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||
| 94 | 94 | if s.deps.Issuer == nil { | |
| 95 | 95 | return fmt.Errorf("certificate issuer is nil") | |
| 96 | 96 | } | |
| 97 | - | unlock, err := s.deps.Store.Lock(ctx) | |
| 98 | - | if err != nil { | |
| 99 | - | return fmt.Errorf("acquire store lock: %w", err) | |
| 100 | - | } | |
| 101 | - | defer func() { _ = unlock() }() | |
| 102 | 97 | ||
| 103 | - | // Collect due certs under lock, copying values before releasing the lock so | |
| 104 | - | // renewal uses a stable snapshot even if the cache changes concurrently. | |
| 98 | + | // Collect due certs from in-memory cache under s.mu (no store lock needed). | |
| 105 | 99 | s.mu.Lock() | |
| 106 | - | due := make([]out.StoredCertificate, 0) | |
| 100 | + | due := make([]out.StoredCertificate, 0, len(s.certs)) | |
| 107 | 101 | for _, cert := range s.certs { | |
| 108 | 102 | if ShouldRenew(*cert, now) { | |
| 109 | 103 | due = append(due, *cert) | |
| @@ -127,9 +121,24 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||
| 127 | 121 | continue | |
| 128 | 122 | } | |
| 129 | 123 | ||
| 130 | - | if err := s.deps.Store.Save(ctx, *renewed); err != nil { | |
| 124 | + | // Acquire store lock only around Save to avoid holding it across | |
| 125 | + | // the potentially long-running Issuer.Renew call. | |
| 126 | + | unlock, err := s.deps.Store.Lock(ctx) | |
| 127 | + | if err != nil { | |
| 128 | + | s.mu.Lock() | |
| 129 | + | s.lastErr[cert.ID] = fmt.Sprintf("acquire store lock: %v", err) | |
| 130 | + | s.mu.Unlock() | |
| 131 | + | continue | |
| 132 | + | } | |
| 133 | + | ||
| 134 | + | saveErr := s.deps.Store.Save(ctx, *renewed) | |
| 135 | + | if unlockErr := unlock(); unlockErr != nil { | |
| 136 | + | s.log.Warn().Err(unlockErr).Msg("failed to release store lock") | |
| 137 | + | } | |
| 138 | + | ||
| 139 | + | if saveErr != nil { | |
| 131 | 140 | s.mu.Lock() | |
| 132 | - | s.lastErr[cert.ID] = fmt.Sprintf("save renewed certificate: %v", err) | |
| 141 | + | s.lastErr[cert.ID] = fmt.Sprintf("save renewed certificate: %v", saveErr) | |
| 133 | 142 | s.mu.Unlock() | |
| 134 | 143 | continue | |
| 135 | 144 | } | |
| @@ -94,16 +94,10 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||||
| 94 | if s.deps.Issuer == nil { | 94 | if s.deps.Issuer == nil { | ||
| 95 | return fmt.Errorf("certificate issuer is nil") | 95 | return fmt.Errorf("certificate issuer is nil") | ||
| 96 | } | 96 | } | ||
| 97 | - | unlock, err := s.deps.Store.Lock(ctx) | |||
| 98 | - | if err != nil { | |||
| 99 | - | return fmt.Errorf("acquire store lock: %w", err) | |||
| 100 | - | } | |||
| 101 | - | defer func() { _ = unlock() }() | |||
| 102 | 97 | ||||
| 103 | - | // Collect due certs under lock, copying values before releasing the lock so | 98 | + | // Collect due certs from in-memory cache under s.mu (no store lock needed). |
| 104 | - | // renewal uses a stable snapshot even if the cache changes concurrently. | |||
| 105 | s.mu.Lock() | 99 | s.mu.Lock() | ||
| 106 | - | due := make([]out.StoredCertificate, 0) | 100 | + | due := make([]out.StoredCertificate, 0, len(s.certs)) |
| 107 | for _, cert := range s.certs { | 101 | for _, cert := range s.certs { | ||
| 108 | if ShouldRenew(*cert, now) { | 102 | if ShouldRenew(*cert, now) { | ||
| 109 | due = append(due, *cert) | 103 | due = append(due, *cert) | ||
| @@ -127,9 +121,24 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||||
| 127 | continue | 121 | continue | ||
| 128 | } | 122 | } | ||
| 129 | 123 | ||||
| 130 | - | if err := s.deps.Store.Save(ctx, *renewed); err != nil { | 124 | + | // Acquire store lock only around Save to avoid holding it across |
| 125 | + | // the potentially long-running Issuer.Renew call. | |||
| 126 | + | unlock, err := s.deps.Store.Lock(ctx) | |||
| 127 | + | if err != nil { | |||
| 128 | + | s.mu.Lock() | |||
| 129 | + | s.lastErr[cert.ID] = fmt.Sprintf("acquire store lock: %v", err) | |||
| 130 | + | s.mu.Unlock() | |||
| 131 | + | continue | |||
| 132 | + | } | |||
| 133 | + | ||||
| 134 | + | saveErr := s.deps.Store.Save(ctx, *renewed) | |||
| 135 | + | if unlockErr := unlock(); unlockErr != nil { | |||
| 136 | + | s.log.Warn().Err(unlockErr).Msg("failed to release store lock") | |||
| 137 | + | } | |||
| 138 | + | ||||
| 139 | + | if saveErr != nil { | |||
| 131 | s.mu.Lock() | 140 | s.mu.Lock() | ||
| 132 | - | s.lastErr[cert.ID] = fmt.Sprintf("save renewed certificate: %v", err) | 141 | + | s.lastErr[cert.ID] = fmt.Sprintf("save renewed certificate: %v", saveErr) |
| 133 | s.mu.Unlock() | 142 | s.mu.Unlock() | ||
| 134 | continue | 143 | continue | ||
| 135 | } | 144 | } | ||
M internal/usecase/publictls/service.go +8 -4
| @@ -121,20 +121,24 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||
| 121 | 121 | return nil | |
| 122 | 122 | } | |
| 123 | 123 | ||
| 124 | - | log := s.log.With().Str("component", "Reconcile").Logger() | |
| 124 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |
| 125 | + | zerowrap.FieldLayer: "usecase", | |
| 126 | + | zerowrap.FieldUseCase: "Reconcile", | |
| 127 | + | }) | |
| 128 | + | log := zerowrap.FromCtx(ctx) | |
| 125 | 129 | log.Debug().Msg("starting certificate reconciliation") | |
| 126 | 130 | ||
| 127 | 131 | if s.deps.Store == nil { | |
| 128 | 132 | log.Warn().Msg("certificate store is nil, cannot reconcile") | |
| 129 | - | return fmt.Errorf("certificate store is nil") | |
| 133 | + | return fmt.Errorf("%w: certificate store is nil", domain.ErrCertificateStoreRequired) | |
| 130 | 134 | } | |
| 131 | 135 | if s.deps.Issuer == nil { | |
| 132 | 136 | log.Warn().Msg("certificate issuer is nil, cannot reconcile") | |
| 133 | - | return fmt.Errorf("certificate issuer is nil") | |
| 137 | + | return fmt.Errorf("%w: certificate issuer is nil", domain.ErrCertificateIssuerRequired) | |
| 134 | 138 | } | |
| 135 | 139 | if s.deps.Routes == nil { | |
| 136 | 140 | log.Warn().Msg("route source is nil, cannot reconcile") | |
| 137 | - | return fmt.Errorf("route source is nil") | |
| 141 | + | return fmt.Errorf("%w: route source is nil", domain.ErrRouteSourceRequired) | |
| 138 | 142 | } | |
| 139 | 143 | ||
| 140 | 144 | // Determine effective challenge mode. | |
| @@ -121,20 +121,24 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||||
| 121 | return nil | 121 | return nil | ||
| 122 | } | 122 | } | ||
| 123 | 123 | ||||
| 124 | - | log := s.log.With().Str("component", "Reconcile").Logger() | 124 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ |
| 125 | + | zerowrap.FieldLayer: "usecase", | |||
| 126 | + | zerowrap.FieldUseCase: "Reconcile", | |||
| 127 | + | }) | |||
| 128 | + | log := zerowrap.FromCtx(ctx) | |||
| 125 | log.Debug().Msg("starting certificate reconciliation") | 129 | log.Debug().Msg("starting certificate reconciliation") | ||
| 126 | 130 | ||||
| 127 | if s.deps.Store == nil { | 131 | if s.deps.Store == nil { | ||
| 128 | log.Warn().Msg("certificate store is nil, cannot reconcile") | 132 | log.Warn().Msg("certificate store is nil, cannot reconcile") | ||
| 129 | - | return fmt.Errorf("certificate store is nil") | 133 | + | return fmt.Errorf("%w: certificate store is nil", domain.ErrCertificateStoreRequired) |
| 130 | } | 134 | } | ||
| 131 | if s.deps.Issuer == nil { | 135 | if s.deps.Issuer == nil { | ||
| 132 | log.Warn().Msg("certificate issuer is nil, cannot reconcile") | 136 | log.Warn().Msg("certificate issuer is nil, cannot reconcile") | ||
| 133 | - | return fmt.Errorf("certificate issuer is nil") | 137 | + | return fmt.Errorf("%w: certificate issuer is nil", domain.ErrCertificateIssuerRequired) |
| 134 | } | 138 | } | ||
| 135 | if s.deps.Routes == nil { | 139 | if s.deps.Routes == nil { | ||
| 136 | log.Warn().Msg("route source is nil, cannot reconcile") | 140 | log.Warn().Msg("route source is nil, cannot reconcile") | ||
| 137 | - | return fmt.Errorf("route source is nil") | 141 | + | return fmt.Errorf("%w: route source is nil", domain.ErrRouteSourceRequired) |
| 138 | } | 142 | } | ||
| 139 | 143 | ||||
| 140 | // Determine effective challenge mode. | 144 | // Determine effective challenge mode. | ||
M internal/usecase/publictls/targets_test.go +4 -9
| @@ -13,13 +13,6 @@ import ( | |||
| 13 | 13 | "github.com/bnema/gordon/internal/domain" | |
| 14 | 14 | ) | |
| 15 | 15 | ||
| 16 | - | func newZoneResolverMock(t *testing.T, zone string) *outmocks.MockCloudflareZoneResolver { | |
| 17 | - | t.Helper() | |
| 18 | - | resolver := outmocks.NewMockCloudflareZoneResolver(t) | |
| 19 | - | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: zone}, nil).Maybe() | |
| 20 | - | return resolver | |
| 21 | - | } | |
| 22 | - | ||
| 23 | 16 | func TestDeriveTargets_HTTP01PerRoute(t *testing.T) { | |
| 24 | 17 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "registry.example.com"}} | |
| 25 | 18 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeHTTP01, routes, nil, nil) | |
| @@ -36,7 +29,8 @@ func TestDeriveTargets_HTTP01PerRoute(t *testing.T) { | |||
| 36 | 29 | ||
| 37 | 30 | func TestDeriveTargets_DNS01WildcardBases(t *testing.T) { | |
| 38 | 31 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "api.prod.example.com"}, {Domain: "example.com"}} | |
| 39 | - | resolver := newZoneResolverMock(t, "example.com") | |
| 32 | + | resolver := outmocks.NewMockCloudflareZoneResolver(t) | |
| 33 | + | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "example.com"}, nil).Times(3) | |
| 40 | 34 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | |
| 41 | 35 | require.NoError(t, err) | |
| 42 | 36 | require.Len(t, targets, 2) | |
| @@ -70,7 +64,8 @@ func TestDeriveTargets_DNS01NilResolver_ReturnsError(t *testing.T) { | |||
| 70 | 64 | ||
| 71 | 65 | func TestDeriveTargets_DNS01MismatchedZone_ReturnsError(t *testing.T) { | |
| 72 | 66 | routes := []domain.Route{{Domain: "app.example.com"}} | |
| 73 | - | resolver := newZoneResolverMock(t, "other.test") | |
| 67 | + | resolver := outmocks.NewMockCloudflareZoneResolver(t) | |
| 68 | + | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "other.test"}, nil).Once() | |
| 74 | 69 | _, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | |
| 75 | 70 | require.Error(t, err) | |
| 76 | 71 | assert.Contains(t, err.Error(), "does not match host") | |
| @@ -13,13 +13,6 @@ import ( | |||||
| 13 | "github.com/bnema/gordon/internal/domain" | 13 | "github.com/bnema/gordon/internal/domain" | ||
| 14 | ) | 14 | ) | ||
| 15 | 15 | ||||
| 16 | - | func newZoneResolverMock(t *testing.T, zone string) *outmocks.MockCloudflareZoneResolver { | |||
| 17 | - | t.Helper() | |||
| 18 | - | resolver := outmocks.NewMockCloudflareZoneResolver(t) | |||
| 19 | - | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: zone}, nil).Maybe() | |||
| 20 | - | return resolver | |||
| 21 | - | } | |||
| 22 | - | ||||
| 23 | func TestDeriveTargets_HTTP01PerRoute(t *testing.T) { | 16 | func TestDeriveTargets_HTTP01PerRoute(t *testing.T) { | ||
| 24 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "registry.example.com"}} | 17 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "registry.example.com"}} | ||
| 25 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeHTTP01, routes, nil, nil) | 18 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeHTTP01, routes, nil, nil) | ||
| @@ -36,7 +29,8 @@ func TestDeriveTargets_HTTP01PerRoute(t *testing.T) { | |||||
| 36 | 29 | ||||
| 37 | func TestDeriveTargets_DNS01WildcardBases(t *testing.T) { | 30 | func TestDeriveTargets_DNS01WildcardBases(t *testing.T) { | ||
| 38 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "api.prod.example.com"}, {Domain: "example.com"}} | 31 | routes := []domain.Route{{Domain: "app.example.com"}, {Domain: "api.prod.example.com"}, {Domain: "example.com"}} | ||
| 39 | - | resolver := newZoneResolverMock(t, "example.com") | 32 | + | resolver := outmocks.NewMockCloudflareZoneResolver(t) |
| 33 | + | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "example.com"}, nil).Times(3) | |||
| 40 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | 34 | targets, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | ||
| 41 | require.NoError(t, err) | 35 | require.NoError(t, err) | ||
| 42 | require.Len(t, targets, 2) | 36 | require.Len(t, targets, 2) | ||
| @@ -70,7 +64,8 @@ func TestDeriveTargets_DNS01NilResolver_ReturnsError(t *testing.T) { | |||||
| 70 | 64 | ||||
| 71 | func TestDeriveTargets_DNS01MismatchedZone_ReturnsError(t *testing.T) { | 65 | func TestDeriveTargets_DNS01MismatchedZone_ReturnsError(t *testing.T) { | ||
| 72 | routes := []domain.Route{{Domain: "app.example.com"}} | 66 | routes := []domain.Route{{Domain: "app.example.com"}} | ||
| 73 | - | resolver := newZoneResolverMock(t, "other.test") | 67 | + | resolver := outmocks.NewMockCloudflareZoneResolver(t) |
| 68 | + | resolver.EXPECT().FindZone(mock.Anything, mock.Anything).Return(out.CloudflareZone{Name: "other.test"}, nil).Once() | |||
| 74 | _, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | 69 | _, err := DeriveCertificateTargets(context.Background(), domain.ACMEChallengeCloudflareDNS01, routes, nil, resolver) | ||
| 75 | require.Error(t, err) | 70 | require.Error(t, err) | ||
| 76 | assert.Contains(t, err.Error(), "does not match host") | 71 | assert.Contains(t, err.Error(), "does not match host") | ||