Address public TLS review findings
- commit
- 67091c047fc3ac328c9f2ee5d49701291796dc1e
- author
- brice · · 2026-04-30 03:19 UTC
- parent
- 48b6401
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
23 files · +388 -126
M docs/concepts.md +1 -1
| @@ -79,7 +79,7 @@ When a request comes in for `app.example.com`, Gordon: | |||
| 79 | 79 | ||
| 80 | 80 | ### Route Domains | |
| 81 | 81 | ||
| 82 | - | Routes use plain hostnames. HTTPS is enabled by default for routes; see the [routes example](./config/routes.md#development-setup) with `dev-app.example.com`. Cloudflare is one deployment option, but Gordon can also terminate TLS with its built-in listener and certificates issued by Gordon's internal CA, or with static certificates. Gordon does not provide ACME or Let's Encrypt challenge automation. | |
| 82 | + | Routes use plain hostnames. HTTPS is enabled by default for routes; see the [routes example](./config/routes.md#development-setup) with `dev-app.example.com`. Cloudflare is one deployment option, but Gordon can also terminate TLS with its built-in listener and certificates issued by Gordon's internal CA, with static certificates, or with public ACME certificates when `[tls.acme]` is enabled. ACME supports HTTP-01 and Cloudflare DNS-01 challenge modes. | |
| 83 | 83 | ||
| 84 | 84 | ```toml | |
| 85 | 85 | [routes] | |
| @@ -79,7 +79,7 @@ When a request comes in for `app.example.com`, Gordon: | |||||
| 79 | 79 | ||||
| 80 | ### Route Domains | 80 | ### Route Domains | ||
| 81 | 81 | ||||
| 82 | - | Routes use plain hostnames. HTTPS is enabled by default for routes; see the [routes example](./config/routes.md#development-setup) with `dev-app.example.com`. Cloudflare is one deployment option, but Gordon can also terminate TLS with its built-in listener and certificates issued by Gordon's internal CA, or with static certificates. Gordon does not provide ACME or Let's Encrypt challenge automation. | 82 | + | Routes use plain hostnames. HTTPS is enabled by default for routes; see the [routes example](./config/routes.md#development-setup) with `dev-app.example.com`. Cloudflare is one deployment option, but Gordon can also terminate TLS with its built-in listener and certificates issued by Gordon's internal CA, with static certificates, or with public ACME certificates when `[tls.acme]` is enabled. ACME supports HTTP-01 and Cloudflare DNS-01 challenge modes. |
| 83 | 83 | ||||
| 84 | ```toml | 84 | ```toml | ||
| 85 | [routes] | 85 | [routes] | ||
M docs/config/reference.md +11 -0
| @@ -33,6 +33,14 @@ token_secret = "" # Path in secrets backend to JWT si | |||
| 33 | 33 | token_expiry = "720h" # Token expiry duration (720h = 30 days) | |
| 34 | 34 | access_token_ttl = "15m" # Ephemeral access token lifetime (default: 15m) | |
| 35 | 35 | ||
| 36 | + | # ============================================================================= | |
| 37 | + | # PUBLIC TLS / ACME | |
| 38 | + | # ============================================================================= | |
| 39 | + | [tls.acme] | |
| 40 | + | enabled = false # Enable public ACME certificates (requires server.tls_port > 0) | |
| 41 | + | email = "" # ACME account email when enabled | |
| 42 | + | challenge = "auto" # "auto", "http-01", or "cloudflare-dns-01" | |
| 43 | + | ||
| 36 | 44 | # ============================================================================= | |
| 37 | 45 | # API (applies to both Registry and Admin endpoints) | |
| 38 | 46 | # ============================================================================= | |
| @@ -195,6 +203,9 @@ keep_last = 3 # Keep N newest tags per repository | |||
| 195 | 203 | | `server.registry_allowed_ips` | `[]` | IPs or CIDR ranges allowed to access the registry (empty = allow all) | | |
| 196 | 204 | | `server.proxy_allowed_ips` | `[]` | IPs or CIDR ranges allowed to reach the proxy (empty = allow all) | | |
| 197 | 205 | | `server.registry_listen_address` | `""` | Bind address for registry (empty = all interfaces) | | |
| 206 | + | | `tls.acme.enabled` | `false` | Enable public ACME certificates (requires `server.tls_port > 0`) | | |
| 207 | + | | `tls.acme.email` | `""` | ACME account email when enabled | | |
| 208 | + | | `tls.acme.challenge` | `"auto"` | ACME challenge mode: `auto`, `http-01`, or `cloudflare-dns-01` | | |
| 198 | 209 | | `auth.enabled` | `true` | Enable authentication; when `false`, run local-only mode (loopback-only `/v2/*`, `/admin/*` disabled) | | |
| 199 | 210 | | `auth.secrets_backend` | `"unsafe"` | Secrets storage | | |
| 200 | 211 | | `auth.token_expiry` | `"720h"` | 30 days | | |
| @@ -33,6 +33,14 @@ token_secret = "" # Path in secrets backend to JWT si | |||||
| 33 | token_expiry = "720h" # Token expiry duration (720h = 30 days) | 33 | token_expiry = "720h" # Token expiry duration (720h = 30 days) | ||
| 34 | access_token_ttl = "15m" # Ephemeral access token lifetime (default: 15m) | 34 | access_token_ttl = "15m" # Ephemeral access token lifetime (default: 15m) | ||
| 35 | 35 | ||||
| 36 | + | # ============================================================================= | |||
| 37 | + | # PUBLIC TLS / ACME | |||
| 38 | + | # ============================================================================= | |||
| 39 | + | [tls.acme] | |||
| 40 | + | enabled = false # Enable public ACME certificates (requires server.tls_port > 0) | |||
| 41 | + | email = "" # ACME account email when enabled | |||
| 42 | + | challenge = "auto" # "auto", "http-01", or "cloudflare-dns-01" | |||
| 43 | + | ||||
| 36 | # ============================================================================= | 44 | # ============================================================================= | ||
| 37 | # API (applies to both Registry and Admin endpoints) | 45 | # API (applies to both Registry and Admin endpoints) | ||
| 38 | # ============================================================================= | 46 | # ============================================================================= | ||
| @@ -195,6 +203,9 @@ keep_last = 3 # Keep N newest tags per repository | |||||
| 195 | | `server.registry_allowed_ips` | `[]` | IPs or CIDR ranges allowed to access the registry (empty = allow all) | | 203 | | `server.registry_allowed_ips` | `[]` | IPs or CIDR ranges allowed to access the registry (empty = allow all) | | ||
| 196 | | `server.proxy_allowed_ips` | `[]` | IPs or CIDR ranges allowed to reach the proxy (empty = allow all) | | 204 | | `server.proxy_allowed_ips` | `[]` | IPs or CIDR ranges allowed to reach the proxy (empty = allow all) | | ||
| 197 | | `server.registry_listen_address` | `""` | Bind address for registry (empty = all interfaces) | | 205 | | `server.registry_listen_address` | `""` | Bind address for registry (empty = all interfaces) | | ||
| 206 | + | | `tls.acme.enabled` | `false` | Enable public ACME certificates (requires `server.tls_port > 0`) | | |||
| 207 | + | | `tls.acme.email` | `""` | ACME account email when enabled | | |||
| 208 | + | | `tls.acme.challenge` | `"auto"` | ACME challenge mode: `auto`, `http-01`, or `cloudflare-dns-01` | | |||
| 198 | | `auth.enabled` | `true` | Enable authentication; when `false`, run local-only mode (loopback-only `/v2/*`, `/admin/*` disabled) | | 209 | | `auth.enabled` | `true` | Enable authentication; when `false`, run local-only mode (loopback-only `/v2/*`, `/admin/*` disabled) | | ||
| 199 | | `auth.secrets_backend` | `"unsafe"` | Secrets storage | | 210 | | `auth.secrets_backend` | `"unsafe"` | Secrets storage | | ||
| 200 | | `auth.token_expiry` | `"720h"` | 30 days | | 211 | | `auth.token_expiry` | `"720h"` | 30 days | | ||
M docs/config/server.md +22 -1
| @@ -84,12 +84,33 @@ tls_key_file = "/etc/gordon/tls/key.pem" | |||
| 84 | 84 | ||
| 85 | 85 | The static certificate is served for SNI-matching domains (including wildcards). All other domains get on-demand certs from the internal CA. | |
| 86 | 86 | ||
| 87 | + | #### Public ACME certificates | |
| 88 | + | ||
| 89 | + | Gordon can obtain public certificates through Let's Encrypt-compatible ACME when `[tls.acme]` is enabled. The HTTPS listener must also be enabled (`server.tls_port > 0`). | |
| 90 | + | ||
| 91 | + | ```toml | |
| 92 | + | [server] | |
| 93 | + | tls_port = 8443 | |
| 94 | + | ||
| 95 | + | [tls.acme] | |
| 96 | + | enabled = true | |
| 97 | + | email = "admin@example.com" | |
| 98 | + | challenge = "auto" # auto, http-01, or cloudflare-dns-01 | |
| 99 | + | ``` | |
| 100 | + | ||
| 101 | + | Challenge behavior: | |
| 102 | + | - `http-01` serves `/.well-known/acme-challenge/<token>` on the HTTP proxy port. | |
| 103 | + | - `cloudflare-dns-01` uses a Cloudflare API token from `pass`, `GORDON_CLOUDFLARE_API_TOKEN_FILE`, or `GORDON_CLOUDFLARE_API_TOKEN`. | |
| 104 | + | - `auto` selects Cloudflare DNS-01 when a token is available, otherwise falls back to HTTP-01. | |
| 105 | + | ||
| 106 | + | Static certificates have priority, then public ACME certificates, then Gordon's internal CA fallback. Gordon uses the `go-acme/lego` ACME client, including its DNS-provider support for Cloudflare DNS-01. | |
| 107 | + | ||
| 87 | 108 | #### Direct HTTP Onboarding | |
| 88 | 109 | ||
| 89 | 110 | When `tls_port` is non-zero, direct HTTP clients (those not arriving through a trusted proxy) are restricted to CA onboarding paths only: | |
| 90 | 111 | ||
| 91 | 112 | - `/`, `/ca`, `/ca.crt`, `/ca.mobileconfig` — serve the onboarding page and CA certificate downloads | |
| 92 | - | - `/.well-known/acme-challenge/*` — returns `404 Not Found` (reserved for future ACME support) | |
| 113 | + | - `/.well-known/acme-challenge/*` — serves ACME HTTP-01 challenges when public ACME HTTP-01 is enabled, otherwise returns `404 Not Found` | |
| 93 | 114 | - All other paths return `403 Forbidden` | |
| 94 | 115 | ||
| 95 | 116 | This lets new clients discover and trust the internal CA over plain HTTP without exposing the full application. Trusted proxy traffic (e.g. from Cloudflare) continues through the normal HTTP proxy path unaffected. | |
| @@ -84,12 +84,33 @@ tls_key_file = "/etc/gordon/tls/key.pem" | |||||
| 84 | 84 | ||||
| 85 | The static certificate is served for SNI-matching domains (including wildcards). All other domains get on-demand certs from the internal CA. | 85 | The static certificate is served for SNI-matching domains (including wildcards). All other domains get on-demand certs from the internal CA. | ||
| 86 | 86 | ||||
| 87 | + | #### Public ACME certificates | |||
| 88 | + | ||||
| 89 | + | Gordon can obtain public certificates through Let's Encrypt-compatible ACME when `[tls.acme]` is enabled. The HTTPS listener must also be enabled (`server.tls_port > 0`). | |||
| 90 | + | ||||
| 91 | + | ```toml | |||
| 92 | + | [server] | |||
| 93 | + | tls_port = 8443 | |||
| 94 | + | ||||
| 95 | + | [tls.acme] | |||
| 96 | + | enabled = true | |||
| 97 | + | email = "admin@example.com" | |||
| 98 | + | challenge = "auto" # auto, http-01, or cloudflare-dns-01 | |||
| 99 | + | ``` | |||
| 100 | + | ||||
| 101 | + | Challenge behavior: | |||
| 102 | + | - `http-01` serves `/.well-known/acme-challenge/<token>` on the HTTP proxy port. | |||
| 103 | + | - `cloudflare-dns-01` uses a Cloudflare API token from `pass`, `GORDON_CLOUDFLARE_API_TOKEN_FILE`, or `GORDON_CLOUDFLARE_API_TOKEN`. | |||
| 104 | + | - `auto` selects Cloudflare DNS-01 when a token is available, otherwise falls back to HTTP-01. | |||
| 105 | + | ||||
| 106 | + | Static certificates have priority, then public ACME certificates, then Gordon's internal CA fallback. Gordon uses the `go-acme/lego` ACME client, including its DNS-provider support for Cloudflare DNS-01. | |||
| 107 | + | ||||
| 87 | #### Direct HTTP Onboarding | 108 | #### Direct HTTP Onboarding | ||
| 88 | 109 | ||||
| 89 | When `tls_port` is non-zero, direct HTTP clients (those not arriving through a trusted proxy) are restricted to CA onboarding paths only: | 110 | When `tls_port` is non-zero, direct HTTP clients (those not arriving through a trusted proxy) are restricted to CA onboarding paths only: | ||
| 90 | 111 | ||||
| 91 | - `/`, `/ca`, `/ca.crt`, `/ca.mobileconfig` — serve the onboarding page and CA certificate downloads | 112 | - `/`, `/ca`, `/ca.crt`, `/ca.mobileconfig` — serve the onboarding page and CA certificate downloads | ||
| 92 | - | - `/.well-known/acme-challenge/*` — returns `404 Not Found` (reserved for future ACME support) | 113 | + | - `/.well-known/acme-challenge/*` — serves ACME HTTP-01 challenges when public ACME HTTP-01 is enabled, otherwise returns `404 Not Found` |
| 93 | - All other paths return `403 Forbidden` | 114 | - All other paths return `403 Forbidden` | ||
| 94 | 115 | ||||
| 95 | This lets new clients discover and trust the internal CA over plain HTTP without exposing the full application. Trusted proxy traffic (e.g. from Cloudflare) continues through the normal HTTP proxy path unaffected. | 116 | This lets new clients discover and trust the internal CA over plain HTTP without exposing the full application. Trusted proxy traffic (e.g. from Cloudflare) continues through the normal HTTP proxy path unaffected. | ||
M internal/adapters/in/cli/controlplane_local.go +4 -1
| @@ -335,7 +335,10 @@ func (l *localControlPlane) RemoveAutoRouteAllowedDomain(ctx context.Context, pa | |||
| 335 | 335 | ||
| 336 | 336 | func (l *localControlPlane) GetTLSStatus(ctx context.Context) (*dto.TLSStatusResponse, error) { | |
| 337 | 337 | if l.publicTLSSvc == nil { | |
| 338 | - | return &dto.TLSStatusResponse{}, nil | |
| 338 | + | return &dto.TLSStatusResponse{ | |
| 339 | + | ACMEEnabled: false, | |
| 340 | + | SelectionReason: "public TLS service not configured", | |
| 341 | + | }, nil | |
| 339 | 342 | } | |
| 340 | 343 | ||
| 341 | 344 | status := l.publicTLSSvc.Status(ctx) | |
| @@ -335,7 +335,10 @@ func (l *localControlPlane) RemoveAutoRouteAllowedDomain(ctx context.Context, pa | |||||
| 335 | 335 | ||||
| 336 | func (l *localControlPlane) GetTLSStatus(ctx context.Context) (*dto.TLSStatusResponse, error) { | 336 | func (l *localControlPlane) GetTLSStatus(ctx context.Context) (*dto.TLSStatusResponse, error) { | ||
| 337 | if l.publicTLSSvc == nil { | 337 | if l.publicTLSSvc == nil { | ||
| 338 | - | return &dto.TLSStatusResponse{}, nil | 338 | + | return &dto.TLSStatusResponse{ |
| 339 | + | ACMEEnabled: false, | |||
| 340 | + | SelectionReason: "public TLS service not configured", | |||
| 341 | + | }, nil | |||
| 339 | } | 342 | } | ||
| 340 | 343 | ||||
| 341 | status := l.publicTLSSvc.Status(ctx) | 344 | status := l.publicTLSSvc.Status(ctx) | ||
M internal/adapters/in/cli/controlplane_local_test.go +13 -0
| @@ -5,6 +5,7 @@ import ( | |||
| 5 | 5 | "testing" | |
| 6 | 6 | "time" | |
| 7 | 7 | ||
| 8 | + | "github.com/bnema/gordon/internal/adapters/dto" | |
| 8 | 9 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | |
| 9 | 10 | "github.com/bnema/gordon/internal/domain" | |
| 10 | 11 | "github.com/stretchr/testify/mock" | |
| @@ -95,6 +96,18 @@ func TestLocalControlPlane_RestartWithAttachments(t *testing.T) { | |||
| 95 | 96 | require.Equal(t, "app.local", result.Domain) | |
| 96 | 97 | } | |
| 97 | 98 | ||
| 99 | + | func TestLocalControlPlane_GetTLSStatusWithoutService(t *testing.T) { | |
| 100 | + | t.Parallel() | |
| 101 | + | ||
| 102 | + | cp := &localControlPlane{} | |
| 103 | + | status, err := cp.GetTLSStatus(context.Background()) | |
| 104 | + | require.NoError(t, err) | |
| 105 | + | require.Equal(t, &dto.TLSStatusResponse{ | |
| 106 | + | ACMEEnabled: false, | |
| 107 | + | SelectionReason: "public TLS service not configured", | |
| 108 | + | }, status) | |
| 109 | + | } | |
| 110 | + | ||
| 98 | 111 | func TestLocalControlPlane_GetContainerLogs(t *testing.T) { | |
| 99 | 112 | t.Parallel() | |
| 100 | 113 | ||
| @@ -5,6 +5,7 @@ import ( | |||||
| 5 | "testing" | 5 | "testing" | ||
| 6 | "time" | 6 | "time" | ||
| 7 | 7 | ||||
| 8 | + | "github.com/bnema/gordon/internal/adapters/dto" | |||
| 8 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | 9 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | ||
| 9 | "github.com/bnema/gordon/internal/domain" | 10 | "github.com/bnema/gordon/internal/domain" | ||
| 10 | "github.com/stretchr/testify/mock" | 11 | "github.com/stretchr/testify/mock" | ||
| @@ -95,6 +96,18 @@ func TestLocalControlPlane_RestartWithAttachments(t *testing.T) { | |||||
| 95 | require.Equal(t, "app.local", result.Domain) | 96 | require.Equal(t, "app.local", result.Domain) | ||
| 96 | } | 97 | } | ||
| 97 | 98 | ||||
| 99 | + | func TestLocalControlPlane_GetTLSStatusWithoutService(t *testing.T) { | |||
| 100 | + | t.Parallel() | |||
| 101 | + | ||||
| 102 | + | cp := &localControlPlane{} | |||
| 103 | + | status, err := cp.GetTLSStatus(context.Background()) | |||
| 104 | + | require.NoError(t, err) | |||
| 105 | + | require.Equal(t, &dto.TLSStatusResponse{ | |||
| 106 | + | ACMEEnabled: false, | |||
| 107 | + | SelectionReason: "public TLS service not configured", | |||
| 108 | + | }, status) | |||
| 109 | + | } | |||
| 110 | + | ||||
| 98 | func TestLocalControlPlane_GetContainerLogs(t *testing.T) { | 111 | func TestLocalControlPlane_GetContainerLogs(t *testing.T) { | ||
| 99 | t.Parallel() | 112 | t.Parallel() | ||
| 100 | 113 | ||||
M internal/adapters/out/acmelego/http_provider.go +6 -2
| @@ -23,7 +23,9 @@ func (p *HTTPProvider) Present(_domain, token, keyAuth string) error { | |||
| 23 | 23 | if p.sink == nil { | |
| 24 | 24 | return fmt.Errorf("http-01 sink is nil: cannot Present") | |
| 25 | 25 | } | |
| 26 | - | p.sink.Present(token, keyAuth) | |
| 26 | + | if err := p.sink.Present(token, keyAuth); err != nil { | |
| 27 | + | return fmt.Errorf("present http-01 challenge: %w", err) | |
| 28 | + | } | |
| 27 | 29 | return nil | |
| 28 | 30 | } | |
| 29 | 31 | ||
| @@ -32,6 +34,8 @@ func (p *HTTPProvider) CleanUp(_domain, token, _keyAuth string) error { | |||
| 32 | 34 | if p.sink == nil { | |
| 33 | 35 | return fmt.Errorf("http-01 sink is nil: cannot CleanUp") | |
| 34 | 36 | } | |
| 35 | - | p.sink.CleanUp(token) | |
| 37 | + | if err := p.sink.CleanUp(token); err != nil { | |
| 38 | + | return fmt.Errorf("cleanup http-01 challenge: %w", err) | |
| 39 | + | } | |
| 36 | 40 | return nil | |
| 37 | 41 | } | |
| @@ -23,7 +23,9 @@ func (p *HTTPProvider) Present(_domain, token, keyAuth string) error { | |||||
| 23 | if p.sink == nil { | 23 | if p.sink == nil { | ||
| 24 | return fmt.Errorf("http-01 sink is nil: cannot Present") | 24 | return fmt.Errorf("http-01 sink is nil: cannot Present") | ||
| 25 | } | 25 | } | ||
| 26 | - | p.sink.Present(token, keyAuth) | 26 | + | if err := p.sink.Present(token, keyAuth); err != nil { |
| 27 | + | return fmt.Errorf("present http-01 challenge: %w", err) | |||
| 28 | + | } | |||
| 27 | return nil | 29 | return nil | ||
| 28 | } | 30 | } | ||
| 29 | 31 | ||||
| @@ -32,6 +34,8 @@ func (p *HTTPProvider) CleanUp(_domain, token, _keyAuth string) error { | |||||
| 32 | if p.sink == nil { | 34 | if p.sink == nil { | ||
| 33 | return fmt.Errorf("http-01 sink is nil: cannot CleanUp") | 35 | return fmt.Errorf("http-01 sink is nil: cannot CleanUp") | ||
| 34 | } | 36 | } | ||
| 35 | - | p.sink.CleanUp(token) | 37 | + | if err := p.sink.CleanUp(token); err != nil { |
| 38 | + | return fmt.Errorf("cleanup http-01 challenge: %w", err) | |||
| 39 | + | } | |||
| 36 | return nil | 40 | return nil | ||
| 37 | } | 41 | } | ||
M internal/adapters/out/acmelego/http_provider_test.go +2 -2
| @@ -10,8 +10,8 @@ import ( | |||
| 10 | 10 | ||
| 11 | 11 | func TestHTTPProviderStoresAndCleansChallenge(t *testing.T) { | |
| 12 | 12 | mockSink := outmocks.NewMockHTTPChallengeSink(t) | |
| 13 | - | mockSink.EXPECT().Present("token", "key-auth").Return() | |
| 14 | - | mockSink.EXPECT().CleanUp("token").Return() | |
| 13 | + | mockSink.EXPECT().Present("token", "key-auth").Return(nil) | |
| 14 | + | mockSink.EXPECT().CleanUp("token").Return(nil) | |
| 15 | 15 | ||
| 16 | 16 | provider := NewHTTPProvider(mockSink) | |
| 17 | 17 | ||
| @@ -10,8 +10,8 @@ import ( | |||||
| 10 | 10 | ||||
| 11 | func TestHTTPProviderStoresAndCleansChallenge(t *testing.T) { | 11 | func TestHTTPProviderStoresAndCleansChallenge(t *testing.T) { | ||
| 12 | mockSink := outmocks.NewMockHTTPChallengeSink(t) | 12 | mockSink := outmocks.NewMockHTTPChallengeSink(t) | ||
| 13 | - | mockSink.EXPECT().Present("token", "key-auth").Return() | 13 | + | mockSink.EXPECT().Present("token", "key-auth").Return(nil) |
| 14 | - | mockSink.EXPECT().CleanUp("token").Return() | 14 | + | mockSink.EXPECT().CleanUp("token").Return(nil) |
| 15 | 15 | ||||
| 16 | provider := NewHTTPProvider(mockSink) | 16 | provider := NewHTTPProvider(mockSink) | ||
| 17 | 17 | ||||
M internal/adapters/out/acmelego/issuer.go +47 -16
| @@ -49,10 +49,12 @@ type Config struct { | |||
| 49 | 49 | ||
| 50 | 50 | // Issuer implements out.PublicCertificateIssuer using the lego ACME client. | |
| 51 | 51 | type Issuer struct { | |
| 52 | - | cfg Config | |
| 53 | - | mu sync.Mutex | |
| 54 | - | client *lego.Client | |
| 55 | - | user *AccountUser | |
| 52 | + | cfg Config | |
| 53 | + | mu sync.Mutex | |
| 54 | + | client *lego.Client | |
| 55 | + | user *AccountUser | |
| 56 | + | initializing bool | |
| 57 | + | initDone chan struct{} | |
| 56 | 58 | } | |
| 57 | 59 | ||
| 58 | 60 | // NewIssuer creates a new Issuer. Config is validated; the ACME account is | |
| @@ -150,45 +152,74 @@ func (i *Issuer) Renew(ctx context.Context, cert out.StoredCertificate) (*out.St | |||
| 150 | 152 | ||
| 151 | 153 | // ensureClient initializes the lego client and ACME account if not yet done. | |
| 152 | 154 | func (i *Issuer) ensureClient(ctx context.Context) error { | |
| 153 | - | i.mu.Lock() | |
| 154 | - | defer i.mu.Unlock() | |
| 155 | + | for { | |
| 156 | + | i.mu.Lock() | |
| 157 | + | if i.client != nil { | |
| 158 | + | i.mu.Unlock() | |
| 159 | + | return nil | |
| 160 | + | } | |
| 161 | + | if i.initializing { | |
| 162 | + | done := i.initDone | |
| 163 | + | i.mu.Unlock() | |
| 164 | + | select { | |
| 165 | + | case <-done: | |
| 166 | + | continue | |
| 167 | + | case <-ctx.Done(): | |
| 168 | + | return ctx.Err() | |
| 169 | + | } | |
| 170 | + | } | |
| 171 | + | i.initializing = true | |
| 172 | + | i.initDone = make(chan struct{}) | |
| 173 | + | done := i.initDone | |
| 174 | + | i.mu.Unlock() | |
| 175 | + | ||
| 176 | + | user, client, err := i.buildClient(ctx) | |
| 155 | 177 | ||
| 156 | - | if i.client != nil { | |
| 157 | - | return nil | |
| 178 | + | i.mu.Lock() | |
| 179 | + | if err == nil { | |
| 180 | + | i.user = user | |
| 181 | + | i.client = client | |
| 182 | + | } | |
| 183 | + | i.initializing = false | |
| 184 | + | close(done) | |
| 185 | + | i.initDone = nil | |
| 186 | + | i.mu.Unlock() | |
| 187 | + | ||
| 188 | + | return err | |
| 158 | 189 | } | |
| 190 | + | } | |
| 159 | 191 | ||
| 192 | + | func (i *Issuer) buildClient(ctx context.Context) (*AccountUser, *lego.Client, error) { | |
| 160 | 193 | // Load or create ACME account | |
| 161 | 194 | user, err := i.loadOrCreateAccount(ctx) | |
| 162 | 195 | if err != nil { | |
| 163 | - | return fmt.Errorf("acme account: %w", err) | |
| 196 | + | return nil, nil, fmt.Errorf("acme account: %w", err) | |
| 164 | 197 | } | |
| 165 | - | i.user = user | |
| 166 | 198 | ||
| 167 | 199 | client, err := lego.NewClient(i.newLegoConfig(user)) | |
| 168 | 200 | if err != nil { | |
| 169 | - | return fmt.Errorf("create lego client: %w", err) | |
| 201 | + | return nil, nil, fmt.Errorf("create lego client: %w", err) | |
| 170 | 202 | } | |
| 171 | 203 | ||
| 172 | 204 | // Set up challenge providers | |
| 173 | 205 | switch i.cfg.Challenge { | |
| 174 | 206 | case domain.ACMEChallengeHTTP01: | |
| 175 | 207 | if err := client.Challenge.SetHTTP01Provider(NewHTTPProvider(i.cfg.HTTPChallengeSink)); err != nil { | |
| 176 | - | return fmt.Errorf("set http-01 provider: %w", err) | |
| 208 | + | return nil, nil, fmt.Errorf("set http-01 provider: %w", err) | |
| 177 | 209 | } | |
| 178 | 210 | case domain.ACMEChallengeCloudflareDNS01: | |
| 179 | 211 | cfCfg := cloudflare.NewDefaultConfig() | |
| 180 | 212 | cfCfg.AuthToken = i.cfg.Token | |
| 181 | 213 | cfProvider, err := cloudflare.NewDNSProviderConfig(cfCfg) | |
| 182 | 214 | if err != nil { | |
| 183 | - | return fmt.Errorf("create cloudflare dns provider: %w", err) | |
| 215 | + | return nil, nil, fmt.Errorf("create cloudflare dns provider: %w", err) | |
| 184 | 216 | } | |
| 185 | 217 | if err := client.Challenge.SetDNS01Provider(cfProvider); err != nil { | |
| 186 | - | return fmt.Errorf("set dns-01 provider: %w", err) | |
| 218 | + | return nil, nil, fmt.Errorf("set dns-01 provider: %w", err) | |
| 187 | 219 | } | |
| 188 | 220 | } | |
| 189 | 221 | ||
| 190 | - | i.client = client | |
| 191 | - | return nil | |
| 222 | + | return user, client, nil | |
| 192 | 223 | } | |
| 193 | 224 | ||
| 194 | 225 | // loadOrCreateAccount loads an existing ACME account from the store or creates | |
| @@ -49,10 +49,12 @@ type Config struct { | |||||
| 49 | 49 | ||||
| 50 | // Issuer implements out.PublicCertificateIssuer using the lego ACME client. | 50 | // Issuer implements out.PublicCertificateIssuer using the lego ACME client. | ||
| 51 | type Issuer struct { | 51 | type Issuer struct { | ||
| 52 | - | cfg Config | 52 | + | cfg Config |
| 53 | - | mu sync.Mutex | 53 | + | mu sync.Mutex |
| 54 | - | client *lego.Client | 54 | + | client *lego.Client |
| 55 | - | user *AccountUser | 55 | + | user *AccountUser |
| 56 | + | initializing bool | |||
| 57 | + | initDone chan struct{} | |||
| 56 | } | 58 | } | ||
| 57 | 59 | ||||
| 58 | // NewIssuer creates a new Issuer. Config is validated; the ACME account is | 60 | // NewIssuer creates a new Issuer. Config is validated; the ACME account is | ||
| @@ -150,45 +152,74 @@ func (i *Issuer) Renew(ctx context.Context, cert out.StoredCertificate) (*out.St | |||||
| 150 | 152 | ||||
| 151 | // ensureClient initializes the lego client and ACME account if not yet done. | 153 | // ensureClient initializes the lego client and ACME account if not yet done. | ||
| 152 | func (i *Issuer) ensureClient(ctx context.Context) error { | 154 | func (i *Issuer) ensureClient(ctx context.Context) error { | ||
| 153 | - | i.mu.Lock() | 155 | + | for { |
| 154 | - | defer i.mu.Unlock() | 156 | + | i.mu.Lock() |
| 157 | + | if i.client != nil { | |||
| 158 | + | i.mu.Unlock() | |||
| 159 | + | return nil | |||
| 160 | + | } | |||
| 161 | + | if i.initializing { | |||
| 162 | + | done := i.initDone | |||
| 163 | + | i.mu.Unlock() | |||
| 164 | + | select { | |||
| 165 | + | case <-done: | |||
| 166 | + | continue | |||
| 167 | + | case <-ctx.Done(): | |||
| 168 | + | return ctx.Err() | |||
| 169 | + | } | |||
| 170 | + | } | |||
| 171 | + | i.initializing = true | |||
| 172 | + | i.initDone = make(chan struct{}) | |||
| 173 | + | done := i.initDone | |||
| 174 | + | i.mu.Unlock() | |||
| 175 | + | ||||
| 176 | + | user, client, err := i.buildClient(ctx) | |||
| 155 | 177 | ||||
| 156 | - | if i.client != nil { | 178 | + | i.mu.Lock() |
| 157 | - | return nil | 179 | + | if err == nil { |
| 180 | + | i.user = user | |||
| 181 | + | i.client = client | |||
| 182 | + | } | |||
| 183 | + | i.initializing = false | |||
| 184 | + | close(done) | |||
| 185 | + | i.initDone = nil | |||
| 186 | + | i.mu.Unlock() | |||
| 187 | + | ||||
| 188 | + | return err | |||
| 158 | } | 189 | } | ||
| 190 | + | } | |||
| 159 | 191 | ||||
| 192 | + | func (i *Issuer) buildClient(ctx context.Context) (*AccountUser, *lego.Client, error) { | |||
| 160 | // Load or create ACME account | 193 | // Load or create ACME account | ||
| 161 | user, err := i.loadOrCreateAccount(ctx) | 194 | user, err := i.loadOrCreateAccount(ctx) | ||
| 162 | if err != nil { | 195 | if err != nil { | ||
| 163 | - | return fmt.Errorf("acme account: %w", err) | 196 | + | return nil, nil, fmt.Errorf("acme account: %w", err) |
| 164 | } | 197 | } | ||
| 165 | - | i.user = user | |||
| 166 | 198 | ||||
| 167 | client, err := lego.NewClient(i.newLegoConfig(user)) | 199 | client, err := lego.NewClient(i.newLegoConfig(user)) | ||
| 168 | if err != nil { | 200 | if err != nil { | ||
| 169 | - | return fmt.Errorf("create lego client: %w", err) | 201 | + | return nil, nil, fmt.Errorf("create lego client: %w", err) |
| 170 | } | 202 | } | ||
| 171 | 203 | ||||
| 172 | // Set up challenge providers | 204 | // Set up challenge providers | ||
| 173 | switch i.cfg.Challenge { | 205 | switch i.cfg.Challenge { | ||
| 174 | case domain.ACMEChallengeHTTP01: | 206 | case domain.ACMEChallengeHTTP01: | ||
| 175 | if err := client.Challenge.SetHTTP01Provider(NewHTTPProvider(i.cfg.HTTPChallengeSink)); err != nil { | 207 | if err := client.Challenge.SetHTTP01Provider(NewHTTPProvider(i.cfg.HTTPChallengeSink)); err != nil { | ||
| 176 | - | return fmt.Errorf("set http-01 provider: %w", err) | 208 | + | return nil, nil, fmt.Errorf("set http-01 provider: %w", err) |
| 177 | } | 209 | } | ||
| 178 | case domain.ACMEChallengeCloudflareDNS01: | 210 | case domain.ACMEChallengeCloudflareDNS01: | ||
| 179 | cfCfg := cloudflare.NewDefaultConfig() | 211 | cfCfg := cloudflare.NewDefaultConfig() | ||
| 180 | cfCfg.AuthToken = i.cfg.Token | 212 | cfCfg.AuthToken = i.cfg.Token | ||
| 181 | cfProvider, err := cloudflare.NewDNSProviderConfig(cfCfg) | 213 | cfProvider, err := cloudflare.NewDNSProviderConfig(cfCfg) | ||
| 182 | if err != nil { | 214 | if err != nil { | ||
| 183 | - | return fmt.Errorf("create cloudflare dns provider: %w", err) | 215 | + | return nil, nil, fmt.Errorf("create cloudflare dns provider: %w", err) |
| 184 | } | 216 | } | ||
| 185 | if err := client.Challenge.SetDNS01Provider(cfProvider); err != nil { | 217 | if err := client.Challenge.SetDNS01Provider(cfProvider); err != nil { | ||
| 186 | - | return fmt.Errorf("set dns-01 provider: %w", err) | 218 | + | return nil, nil, fmt.Errorf("set dns-01 provider: %w", err) |
| 187 | } | 219 | } | ||
| 188 | } | 220 | } | ||
| 189 | 221 | ||||
| 190 | - | i.client = client | 222 | + | return user, client, nil |
| 191 | - | return nil | |||
| 192 | } | 223 | } | ||
| 193 | 224 | ||||
| 194 | // loadOrCreateAccount loads an existing ACME account from the store or creates | 225 | // loadOrCreateAccount loads an existing ACME account from the store or creates | ||
M internal/adapters/out/acmestore/store.go +5 -5
| @@ -194,9 +194,9 @@ func (s *Store) Save(_ context.Context, cert out.StoredCertificate) error { | |||
| 194 | 194 | return fmt.Errorf("acmestore: unsafe certificate id %q: %w", cert.ID, domain.ErrPathTraversal) | |
| 195 | 195 | } | |
| 196 | 196 | ||
| 197 | - | parent, err := ensureCertsDir(s.root) | |
| 198 | - | if err != nil { | |
| 199 | - | return err | |
| 197 | + | parent := filepath.Join(s.root, certDir) | |
| 198 | + | if err := os.MkdirAll(parent, dirMode); err != nil { | |
| 199 | + | return fmt.Errorf("acmestore: mkdir certs: %w", err) | |
| 200 | 200 | } | |
| 201 | 201 | if err := recoverCertificateDir(parent, cert.ID); err != nil { | |
| 202 | 202 | return fmt.Errorf("acmestore: recover cert %s: %w", cert.ID, err) | |
| @@ -430,9 +430,9 @@ func recoverCertificateDirs(certsPath string) error { | |||
| 430 | 430 | return nil | |
| 431 | 431 | } | |
| 432 | 432 | ||
| 433 | - | // safeID rejects IDs that could cause path traversal. | |
| 433 | + | // safeID rejects IDs that could cause path traversal or collide with store-internal artifacts. | |
| 434 | 434 | func safeID(id string) bool { | |
| 435 | - | return id != "" && !strings.HasPrefix(id, ".") && !strings.Contains(id, "/") && !strings.Contains(id, "\\") && !strings.Contains(id, "..") && !strings.Contains(id, "\x00") | |
| 435 | + | return id != "" && !strings.HasPrefix(id, ".") && !isStoreInternalCertDir(id) && !strings.Contains(id, "/") && !strings.Contains(id, "\\") && !strings.Contains(id, "..") && !strings.Contains(id, "\x00") | |
| 436 | 436 | } | |
| 437 | 437 | ||
| 438 | 438 | // writeAtomic writes data to path atomically by writing to a temporary file | |
| @@ -194,9 +194,9 @@ func (s *Store) Save(_ context.Context, cert out.StoredCertificate) error { | |||||
| 194 | return fmt.Errorf("acmestore: unsafe certificate id %q: %w", cert.ID, domain.ErrPathTraversal) | 194 | return fmt.Errorf("acmestore: unsafe certificate id %q: %w", cert.ID, domain.ErrPathTraversal) | ||
| 195 | } | 195 | } | ||
| 196 | 196 | ||||
| 197 | - | parent, err := ensureCertsDir(s.root) | 197 | + | parent := filepath.Join(s.root, certDir) |
| 198 | - | if err != nil { | 198 | + | if err := os.MkdirAll(parent, dirMode); err != nil { |
| 199 | - | return err | 199 | + | return fmt.Errorf("acmestore: mkdir certs: %w", err) |
| 200 | } | 200 | } | ||
| 201 | if err := recoverCertificateDir(parent, cert.ID); err != nil { | 201 | if err := recoverCertificateDir(parent, cert.ID); err != nil { | ||
| 202 | return fmt.Errorf("acmestore: recover cert %s: %w", cert.ID, err) | 202 | return fmt.Errorf("acmestore: recover cert %s: %w", cert.ID, err) | ||
| @@ -430,9 +430,9 @@ func recoverCertificateDirs(certsPath string) error { | |||||
| 430 | return nil | 430 | return nil | ||
| 431 | } | 431 | } | ||
| 432 | 432 | ||||
| 433 | - | // safeID rejects IDs that could cause path traversal. | 433 | + | // safeID rejects IDs that could cause path traversal or collide with store-internal artifacts. |
| 434 | func safeID(id string) bool { | 434 | func safeID(id string) bool { | ||
| 435 | - | return id != "" && !strings.HasPrefix(id, ".") && !strings.Contains(id, "/") && !strings.Contains(id, "\\") && !strings.Contains(id, "..") && !strings.Contains(id, "\x00") | 435 | + | return id != "" && !strings.HasPrefix(id, ".") && !isStoreInternalCertDir(id) && !strings.Contains(id, "/") && !strings.Contains(id, "\\") && !strings.Contains(id, "..") && !strings.Contains(id, "\x00") |
| 436 | } | 436 | } | ||
| 437 | 437 | ||||
| 438 | // writeAtomic writes data to path atomically by writing to a temporary file | 438 | // writeAtomic writes data to path atomically by writing to a temporary file | ||
M internal/adapters/out/acmestore/store_test.go +1 -1
| @@ -128,7 +128,7 @@ func TestStoreRejectsUnsafeID(t *testing.T) { | |||
| 128 | 128 | store, err := New(root) | |
| 129 | 129 | require.NoError(t, err) | |
| 130 | 130 | ||
| 131 | - | for _, id := range []string{"../escape", ".", ".hidden"} { | |
| 131 | + | for _, id := range []string{"../escape", ".", ".hidden", "tenant.old", "tenant.tmp-123"} { | |
| 132 | 132 | t.Run(id, func(t *testing.T) { | |
| 133 | 133 | cert := out.StoredCertificate{ | |
| 134 | 134 | ID: id, | |
| @@ -128,7 +128,7 @@ func TestStoreRejectsUnsafeID(t *testing.T) { | |||||
| 128 | store, err := New(root) | 128 | store, err := New(root) | ||
| 129 | require.NoError(t, err) | 129 | require.NoError(t, err) | ||
| 130 | 130 | ||||
| 131 | - | for _, id := range []string{"../escape", ".", ".hidden"} { | 131 | + | for _, id := range []string{"../escape", ".", ".hidden", "tenant.old", "tenant.tmp-123"} { |
| 132 | t.Run(id, func(t *testing.T) { | 132 | t.Run(id, func(t *testing.T) { | ||
| 133 | cert := out.StoredCertificate{ | 133 | cert := out.StoredCertificate{ | ||
| 134 | ID: id, | 134 | ID: id, | ||
M internal/app/public_tls_wiring_test.go +27 -6
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package app | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "context" | |
| 4 | 5 | "crypto/rand" | |
| 5 | 6 | "crypto/rsa" | |
| 6 | 7 | "crypto/tls" | |
| @@ -11,10 +12,15 @@ import ( | |||
| 11 | 12 | "time" | |
| 12 | 13 | ||
| 13 | 14 | "github.com/stretchr/testify/assert" | |
| 15 | + | "github.com/stretchr/testify/mock" | |
| 14 | 16 | "github.com/stretchr/testify/require" | |
| 15 | 17 | ||
| 18 | + | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | |
| 16 | 19 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | |
| 20 | + | outmocks "github.com/bnema/gordon/internal/boundaries/out/mocks" | |
| 17 | 21 | "github.com/bnema/gordon/internal/domain" | |
| 22 | + | pkiusecase "github.com/bnema/gordon/internal/usecase/pki" | |
| 23 | + | "github.com/bnema/zerowrap" | |
| 18 | 24 | ) | |
| 19 | 25 | ||
| 20 | 26 | // generateTestCert creates a self-signed certificate for the given hostnames. | |
| @@ -63,21 +69,36 @@ func TestCertificateSelector_StaticCertWins(t *testing.T) { | |||
| 63 | 69 | "static cert should be returned, not public TLS") | |
| 64 | 70 | } | |
| 65 | 71 | ||
| 66 | - | // TestCertificateSelector_PublicTLSErrorPropagates tests that if public TLS | |
| 67 | - | // returns an error (e.g. ErrTLSRouteNotCovered), the error propagates. | |
| 68 | - | func TestCertificateSelector_PublicTLSErrorPropagates(t *testing.T) { | |
| 72 | + | func TestCertificateSelector_PublicTLSErrorFallsThroughWithoutLocalPKI(t *testing.T) { | |
| 69 | 73 | publicTLS := inmocks.NewMockPublicTLSService(t) | |
| 70 | 74 | publicTLS.EXPECT().GetCertificateForHost("acme-required.example.com").Return(nil, domain.ErrTLSRouteNotCovered) | |
| 71 | 75 | selector := &certificateSelector{publicTLS: publicTLS} | |
| 72 | 76 | ||
| 73 | 77 | got, err := selector.GetCertificate(&tls.ClientHelloInfo{ServerName: "acme-required.example.com"}) | |
| 74 | - | assert.ErrorIs(t, err, domain.ErrTLSRouteNotCovered, | |
| 75 | - | "public TLS error should propagate") | |
| 76 | - | assert.Nil(t, got, "no certificate should be returned on error") | |
| 78 | + | require.NoError(t, err) | |
| 79 | + | assert.Nil(t, got, "public TLS error with no local PKI returns nil,nil") | |
| 77 | 80 | } | |
| 78 | 81 | ||
| 79 | 82 | // TestCertificateSelector_PublicTLSNilNil tests that when public TLS returns | |
| 80 | 83 | // nil,nil and no local PKI, the selector returns nil,nil. | |
| 84 | + | func TestCertificateSelector_PublicTLSErrorFallsThroughToLocalPKI(t *testing.T) { | |
| 85 | + | publicTLS := inmocks.NewMockPublicTLSService(t) | |
| 86 | + | publicTLS.EXPECT().GetCertificateForHost("acme-required.example.com").Return(nil, domain.ErrTLSRouteNotCovered) | |
| 87 | + | ||
| 88 | + | routes := outmocks.NewMockRouteChecker(t) | |
| 89 | + | routes.EXPECT().GetRoutes(mock.Anything).Return([]domain.Route{{Domain: "acme-required.example.com"}}).Maybe() | |
| 90 | + | routes.EXPECT().GetExternalRoutes().Return(map[string]string{}).Maybe() | |
| 91 | + | ca, err := pkiadapter.NewCA(t.TempDir(), zerowrap.Default()) | |
| 92 | + | require.NoError(t, err) | |
| 93 | + | pkiSvc := pkiusecase.NewService(context.Background(), ca, routes, zerowrap.Default()) | |
| 94 | + | defer pkiSvc.Stop() | |
| 95 | + | ||
| 96 | + | selector := &certificateSelector{publicTLS: publicTLS, localPKI: pkiSvc} | |
| 97 | + | got, err := selector.GetCertificate(&tls.ClientHelloInfo{ServerName: "acme-required.example.com"}) | |
| 98 | + | require.NoError(t, err) | |
| 99 | + | require.NotNil(t, got, "public TLS error should fall through to local PKI") | |
| 100 | + | } | |
| 101 | + | ||
| 81 | 102 | func TestCertificateSelector_PublicTLSNilNil(t *testing.T) { | |
| 82 | 103 | publicTLS := inmocks.NewMockPublicTLSService(t) | |
| 83 | 104 | publicTLS.EXPECT().GetCertificateForHost("non-acme.example.com").Return(nil, nil) | |
| @@ -1,6 +1,7 @@ | |||||
| 1 | package app | 1 | package app | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "context" | |||
| 4 | "crypto/rand" | 5 | "crypto/rand" | ||
| 5 | "crypto/rsa" | 6 | "crypto/rsa" | ||
| 6 | "crypto/tls" | 7 | "crypto/tls" | ||
| @@ -11,10 +12,15 @@ import ( | |||||
| 11 | "time" | 12 | "time" | ||
| 12 | 13 | ||||
| 13 | "github.com/stretchr/testify/assert" | 14 | "github.com/stretchr/testify/assert" | ||
| 15 | + | "github.com/stretchr/testify/mock" | |||
| 14 | "github.com/stretchr/testify/require" | 16 | "github.com/stretchr/testify/require" | ||
| 15 | 17 | ||||
| 18 | + | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | |||
| 16 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | 19 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | ||
| 20 | + | outmocks "github.com/bnema/gordon/internal/boundaries/out/mocks" | |||
| 17 | "github.com/bnema/gordon/internal/domain" | 21 | "github.com/bnema/gordon/internal/domain" | ||
| 22 | + | pkiusecase "github.com/bnema/gordon/internal/usecase/pki" | |||
| 23 | + | "github.com/bnema/zerowrap" | |||
| 18 | ) | 24 | ) | ||
| 19 | 25 | ||||
| 20 | // generateTestCert creates a self-signed certificate for the given hostnames. | 26 | // generateTestCert creates a self-signed certificate for the given hostnames. | ||
| @@ -63,21 +69,36 @@ func TestCertificateSelector_StaticCertWins(t *testing.T) { | |||||
| 63 | "static cert should be returned, not public TLS") | 69 | "static cert should be returned, not public TLS") | ||
| 64 | } | 70 | } | ||
| 65 | 71 | ||||
| 66 | - | // TestCertificateSelector_PublicTLSErrorPropagates tests that if public TLS | 72 | + | func TestCertificateSelector_PublicTLSErrorFallsThroughWithoutLocalPKI(t *testing.T) { |
| 67 | - | // returns an error (e.g. ErrTLSRouteNotCovered), the error propagates. | |||
| 68 | - | func TestCertificateSelector_PublicTLSErrorPropagates(t *testing.T) { | |||
| 69 | publicTLS := inmocks.NewMockPublicTLSService(t) | 73 | publicTLS := inmocks.NewMockPublicTLSService(t) | ||
| 70 | publicTLS.EXPECT().GetCertificateForHost("acme-required.example.com").Return(nil, domain.ErrTLSRouteNotCovered) | 74 | publicTLS.EXPECT().GetCertificateForHost("acme-required.example.com").Return(nil, domain.ErrTLSRouteNotCovered) | ||
| 71 | selector := &certificateSelector{publicTLS: publicTLS} | 75 | selector := &certificateSelector{publicTLS: publicTLS} | ||
| 72 | 76 | ||||
| 73 | got, err := selector.GetCertificate(&tls.ClientHelloInfo{ServerName: "acme-required.example.com"}) | 77 | got, err := selector.GetCertificate(&tls.ClientHelloInfo{ServerName: "acme-required.example.com"}) | ||
| 74 | - | assert.ErrorIs(t, err, domain.ErrTLSRouteNotCovered, | 78 | + | require.NoError(t, err) |
| 75 | - | "public TLS error should propagate") | 79 | + | assert.Nil(t, got, "public TLS error with no local PKI returns nil,nil") |
| 76 | - | assert.Nil(t, got, "no certificate should be returned on error") | |||
| 77 | } | 80 | } | ||
| 78 | 81 | ||||
| 79 | // TestCertificateSelector_PublicTLSNilNil tests that when public TLS returns | 82 | // TestCertificateSelector_PublicTLSNilNil tests that when public TLS returns | ||
| 80 | // nil,nil and no local PKI, the selector returns nil,nil. | 83 | // nil,nil and no local PKI, the selector returns nil,nil. | ||
| 84 | + | func TestCertificateSelector_PublicTLSErrorFallsThroughToLocalPKI(t *testing.T) { | |||
| 85 | + | publicTLS := inmocks.NewMockPublicTLSService(t) | |||
| 86 | + | publicTLS.EXPECT().GetCertificateForHost("acme-required.example.com").Return(nil, domain.ErrTLSRouteNotCovered) | |||
| 87 | + | ||||
| 88 | + | routes := outmocks.NewMockRouteChecker(t) | |||
| 89 | + | routes.EXPECT().GetRoutes(mock.Anything).Return([]domain.Route{{Domain: "acme-required.example.com"}}).Maybe() | |||
| 90 | + | routes.EXPECT().GetExternalRoutes().Return(map[string]string{}).Maybe() | |||
| 91 | + | ca, err := pkiadapter.NewCA(t.TempDir(), zerowrap.Default()) | |||
| 92 | + | require.NoError(t, err) | |||
| 93 | + | pkiSvc := pkiusecase.NewService(context.Background(), ca, routes, zerowrap.Default()) | |||
| 94 | + | defer pkiSvc.Stop() | |||
| 95 | + | ||||
| 96 | + | selector := &certificateSelector{publicTLS: publicTLS, localPKI: pkiSvc} | |||
| 97 | + | got, err := selector.GetCertificate(&tls.ClientHelloInfo{ServerName: "acme-required.example.com"}) | |||
| 98 | + | require.NoError(t, err) | |||
| 99 | + | require.NotNil(t, got, "public TLS error should fall through to local PKI") | |||
| 100 | + | } | |||
| 101 | + | ||||
| 81 | func TestCertificateSelector_PublicTLSNilNil(t *testing.T) { | 102 | func TestCertificateSelector_PublicTLSNilNil(t *testing.T) { | ||
| 82 | publicTLS := inmocks.NewMockPublicTLSService(t) | 103 | publicTLS := inmocks.NewMockPublicTLSService(t) | ||
| 83 | publicTLS.EXPECT().GetCertificateForHost("non-acme.example.com").Return(nil, nil) | 104 | publicTLS.EXPECT().GetCertificateForHost("non-acme.example.com").Return(nil, nil) | ||
M internal/app/run.go +4 -6
| @@ -2778,14 +2778,12 @@ func (s *certificateSelector) GetCertificate(hello *tls.ClientHelloInfo) (*tls.C | |||
| 2778 | 2778 | // 2. Public ACME TLS. | |
| 2779 | 2779 | if s.publicTLS != nil { | |
| 2780 | 2780 | cert, err := s.publicTLS.GetCertificateForHost(hello.ServerName) | |
| 2781 | - | if err != nil { | |
| 2782 | - | return nil, err | |
| 2783 | - | } | |
| 2784 | - | if cert != nil { | |
| 2781 | + | if err == nil && cert != nil { | |
| 2785 | 2782 | return cert, nil | |
| 2786 | 2783 | } | |
| 2787 | - | // nil, nil means this host is not an ACME-required route. | |
| 2788 | - | // Fall through to local PKI. | |
| 2784 | + | // nil, nil means this host is not an ACME-required route. Errors mean | |
| 2785 | + | // public ACME cannot currently serve this host. In both cases, fall | |
| 2786 | + | // through to local PKI instead of aborting the TLS handshake. | |
| 2789 | 2787 | } | |
| 2790 | 2788 | ||
| 2791 | 2789 | // 3. Local PKI (internal CA). | |
| @@ -2778,14 +2778,12 @@ func (s *certificateSelector) GetCertificate(hello *tls.ClientHelloInfo) (*tls.C | |||||
| 2778 | // 2. Public ACME TLS. | 2778 | // 2. Public ACME TLS. | ||
| 2779 | if s.publicTLS != nil { | 2779 | if s.publicTLS != nil { | ||
| 2780 | cert, err := s.publicTLS.GetCertificateForHost(hello.ServerName) | 2780 | cert, err := s.publicTLS.GetCertificateForHost(hello.ServerName) | ||
| 2781 | - | if err != nil { | 2781 | + | if err == nil && cert != nil { |
| 2782 | - | return nil, err | |||
| 2783 | - | } | |||
| 2784 | - | if cert != nil { | |||
| 2785 | return cert, nil | 2782 | return cert, nil | ||
| 2786 | } | 2783 | } | ||
| 2787 | - | // nil, nil means this host is not an ACME-required route. | 2784 | + | // nil, nil means this host is not an ACME-required route. Errors mean |
| 2788 | - | // Fall through to local PKI. | 2785 | + | // public ACME cannot currently serve this host. In both cases, fall |
| 2786 | + | // through to local PKI instead of aborting the TLS handshake. | |||
| 2789 | } | 2787 | } | ||
| 2790 | 2788 | ||||
| 2791 | // 3. Local PKI (internal CA). | 2789 | // 3. Local PKI (internal CA). | ||
M internal/boundaries/out/mocks/mock_http_challenge_sink.go +36 -14
| @@ -36,9 +36,20 @@ func (_m *MockHTTPChallengeSink) EXPECT() *MockHTTPChallengeSink_Expecter { | |||
| 36 | 36 | } | |
| 37 | 37 | ||
| 38 | 38 | // CleanUp provides a mock function for the type MockHTTPChallengeSink | |
| 39 | - | func (_mock *MockHTTPChallengeSink) CleanUp(token string) { | |
| 40 | - | _mock.Called(token) | |
| 41 | - | return | |
| 39 | + | func (_mock *MockHTTPChallengeSink) CleanUp(token string) error { | |
| 40 | + | ret := _mock.Called(token) | |
| 41 | + | ||
| 42 | + | if len(ret) == 0 { | |
| 43 | + | panic("no return value specified for CleanUp") | |
| 44 | + | } | |
| 45 | + | ||
| 46 | + | var r0 error | |
| 47 | + | if returnFunc, ok := ret.Get(0).(func(string) error); ok { | |
| 48 | + | r0 = returnFunc(token) | |
| 49 | + | } else { | |
| 50 | + | r0 = ret.Error(0) | |
| 51 | + | } | |
| 52 | + | return r0 | |
| 42 | 53 | } | |
| 43 | 54 | ||
| 44 | 55 | // MockHTTPChallengeSink_CleanUp_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CleanUp' | |
| @@ -65,20 +76,31 @@ func (_c *MockHTTPChallengeSink_CleanUp_Call) Run(run func(token string)) *MockH | |||
| 65 | 76 | return _c | |
| 66 | 77 | } | |
| 67 | 78 | ||
| 68 | - | func (_c *MockHTTPChallengeSink_CleanUp_Call) Return() *MockHTTPChallengeSink_CleanUp_Call { | |
| 69 | - | _c.Call.Return() | |
| 79 | + | func (_c *MockHTTPChallengeSink_CleanUp_Call) Return(err error) *MockHTTPChallengeSink_CleanUp_Call { | |
| 80 | + | _c.Call.Return(err) | |
| 70 | 81 | return _c | |
| 71 | 82 | } | |
| 72 | 83 | ||
| 73 | - | func (_c *MockHTTPChallengeSink_CleanUp_Call) RunAndReturn(run func(token string)) *MockHTTPChallengeSink_CleanUp_Call { | |
| 74 | - | _c.Run(run) | |
| 84 | + | func (_c *MockHTTPChallengeSink_CleanUp_Call) RunAndReturn(run func(token string) error) *MockHTTPChallengeSink_CleanUp_Call { | |
| 85 | + | _c.Call.Return(run) | |
| 75 | 86 | return _c | |
| 76 | 87 | } | |
| 77 | 88 | ||
| 78 | 89 | // Present provides a mock function for the type MockHTTPChallengeSink | |
| 79 | - | func (_mock *MockHTTPChallengeSink) Present(token string, keyAuth string) { | |
| 80 | - | _mock.Called(token, keyAuth) | |
| 81 | - | return | |
| 90 | + | func (_mock *MockHTTPChallengeSink) Present(token string, keyAuth string) error { | |
| 91 | + | ret := _mock.Called(token, keyAuth) | |
| 92 | + | ||
| 93 | + | if len(ret) == 0 { | |
| 94 | + | panic("no return value specified for Present") | |
| 95 | + | } | |
| 96 | + | ||
| 97 | + | var r0 error | |
| 98 | + | if returnFunc, ok := ret.Get(0).(func(string, string) error); ok { | |
| 99 | + | r0 = returnFunc(token, keyAuth) | |
| 100 | + | } else { | |
| 101 | + | r0 = ret.Error(0) | |
| 102 | + | } | |
| 103 | + | return r0 | |
| 82 | 104 | } | |
| 83 | 105 | ||
| 84 | 106 | // MockHTTPChallengeSink_Present_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Present' | |
| @@ -111,12 +133,12 @@ func (_c *MockHTTPChallengeSink_Present_Call) Run(run func(token string, keyAuth | |||
| 111 | 133 | return _c | |
| 112 | 134 | } | |
| 113 | 135 | ||
| 114 | - | func (_c *MockHTTPChallengeSink_Present_Call) Return() *MockHTTPChallengeSink_Present_Call { | |
| 115 | - | _c.Call.Return() | |
| 136 | + | func (_c *MockHTTPChallengeSink_Present_Call) Return(err error) *MockHTTPChallengeSink_Present_Call { | |
| 137 | + | _c.Call.Return(err) | |
| 116 | 138 | return _c | |
| 117 | 139 | } | |
| 118 | 140 | ||
| 119 | - | func (_c *MockHTTPChallengeSink_Present_Call) RunAndReturn(run func(token string, keyAuth string)) *MockHTTPChallengeSink_Present_Call { | |
| 120 | - | _c.Run(run) | |
| 141 | + | func (_c *MockHTTPChallengeSink_Present_Call) RunAndReturn(run func(token string, keyAuth string) error) *MockHTTPChallengeSink_Present_Call { | |
| 142 | + | _c.Call.Return(run) | |
| 121 | 143 | return _c | |
| 122 | 144 | } | |
| @@ -36,9 +36,20 @@ func (_m *MockHTTPChallengeSink) EXPECT() *MockHTTPChallengeSink_Expecter { | |||||
| 36 | } | 36 | } | ||
| 37 | 37 | ||||
| 38 | // CleanUp provides a mock function for the type MockHTTPChallengeSink | 38 | // CleanUp provides a mock function for the type MockHTTPChallengeSink | ||
| 39 | - | func (_mock *MockHTTPChallengeSink) CleanUp(token string) { | 39 | + | func (_mock *MockHTTPChallengeSink) CleanUp(token string) error { |
| 40 | - | _mock.Called(token) | 40 | + | ret := _mock.Called(token) |
| 41 | - | return | 41 | + | |
| 42 | + | if len(ret) == 0 { | |||
| 43 | + | panic("no return value specified for CleanUp") | |||
| 44 | + | } | |||
| 45 | + | ||||
| 46 | + | var r0 error | |||
| 47 | + | if returnFunc, ok := ret.Get(0).(func(string) error); ok { | |||
| 48 | + | r0 = returnFunc(token) | |||
| 49 | + | } else { | |||
| 50 | + | r0 = ret.Error(0) | |||
| 51 | + | } | |||
| 52 | + | return r0 | |||
| 42 | } | 53 | } | ||
| 43 | 54 | ||||
| 44 | // MockHTTPChallengeSink_CleanUp_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CleanUp' | 55 | // MockHTTPChallengeSink_CleanUp_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CleanUp' | ||
| @@ -65,20 +76,31 @@ func (_c *MockHTTPChallengeSink_CleanUp_Call) Run(run func(token string)) *MockH | |||||
| 65 | return _c | 76 | return _c | ||
| 66 | } | 77 | } | ||
| 67 | 78 | ||||
| 68 | - | func (_c *MockHTTPChallengeSink_CleanUp_Call) Return() *MockHTTPChallengeSink_CleanUp_Call { | 79 | + | func (_c *MockHTTPChallengeSink_CleanUp_Call) Return(err error) *MockHTTPChallengeSink_CleanUp_Call { |
| 69 | - | _c.Call.Return() | 80 | + | _c.Call.Return(err) |
| 70 | return _c | 81 | return _c | ||
| 71 | } | 82 | } | ||
| 72 | 83 | ||||
| 73 | - | func (_c *MockHTTPChallengeSink_CleanUp_Call) RunAndReturn(run func(token string)) *MockHTTPChallengeSink_CleanUp_Call { | 84 | + | func (_c *MockHTTPChallengeSink_CleanUp_Call) RunAndReturn(run func(token string) error) *MockHTTPChallengeSink_CleanUp_Call { |
| 74 | - | _c.Run(run) | 85 | + | _c.Call.Return(run) |
| 75 | return _c | 86 | return _c | ||
| 76 | } | 87 | } | ||
| 77 | 88 | ||||
| 78 | // Present provides a mock function for the type MockHTTPChallengeSink | 89 | // Present provides a mock function for the type MockHTTPChallengeSink | ||
| 79 | - | func (_mock *MockHTTPChallengeSink) Present(token string, keyAuth string) { | 90 | + | func (_mock *MockHTTPChallengeSink) Present(token string, keyAuth string) error { |
| 80 | - | _mock.Called(token, keyAuth) | 91 | + | ret := _mock.Called(token, keyAuth) |
| 81 | - | return | 92 | + | |
| 93 | + | if len(ret) == 0 { | |||
| 94 | + | panic("no return value specified for Present") | |||
| 95 | + | } | |||
| 96 | + | ||||
| 97 | + | var r0 error | |||
| 98 | + | if returnFunc, ok := ret.Get(0).(func(string, string) error); ok { | |||
| 99 | + | r0 = returnFunc(token, keyAuth) | |||
| 100 | + | } else { | |||
| 101 | + | r0 = ret.Error(0) | |||
| 102 | + | } | |||
| 103 | + | return r0 | |||
| 82 | } | 104 | } | ||
| 83 | 105 | ||||
| 84 | // MockHTTPChallengeSink_Present_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Present' | 106 | // MockHTTPChallengeSink_Present_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Present' | ||
| @@ -111,12 +133,12 @@ func (_c *MockHTTPChallengeSink_Present_Call) Run(run func(token string, keyAuth | |||||
| 111 | return _c | 133 | return _c | ||
| 112 | } | 134 | } | ||
| 113 | 135 | ||||
| 114 | - | func (_c *MockHTTPChallengeSink_Present_Call) Return() *MockHTTPChallengeSink_Present_Call { | 136 | + | func (_c *MockHTTPChallengeSink_Present_Call) Return(err error) *MockHTTPChallengeSink_Present_Call { |
| 115 | - | _c.Call.Return() | 137 | + | _c.Call.Return(err) |
| 116 | return _c | 138 | return _c | ||
| 117 | } | 139 | } | ||
| 118 | 140 | ||||
| 119 | - | func (_c *MockHTTPChallengeSink_Present_Call) RunAndReturn(run func(token string, keyAuth string)) *MockHTTPChallengeSink_Present_Call { | 141 | + | func (_c *MockHTTPChallengeSink_Present_Call) RunAndReturn(run func(token string, keyAuth string) error) *MockHTTPChallengeSink_Present_Call { |
| 120 | - | _c.Run(run) | 142 | + | _c.Call.Return(run) |
| 121 | return _c | 143 | return _c | ||
| 122 | } | 144 | } | ||
M internal/boundaries/out/public_tls.go +2 -2
| @@ -55,10 +55,10 @@ type CloudflareZone struct { | |||
| 55 | 55 | // while an ACME issuer completes domain validation. | |
| 56 | 56 | type HTTPChallengeSink interface { | |
| 57 | 57 | // Present stores the key authorization for a challenge token. | |
| 58 | - | Present(token, keyAuth string) | |
| 58 | + | Present(token, keyAuth string) error | |
| 59 | 59 | ||
| 60 | 60 | // CleanUp removes the key authorization for a challenge token. | |
| 61 | - | CleanUp(token string) | |
| 61 | + | CleanUp(token string) error | |
| 62 | 62 | } | |
| 63 | 63 | ||
| 64 | 64 | // PublicCertificateIssuer defines the contract for obtaining and renewing | |
| @@ -55,10 +55,10 @@ type CloudflareZone struct { | |||||
| 55 | // while an ACME issuer completes domain validation. | 55 | // while an ACME issuer completes domain validation. | ||
| 56 | type HTTPChallengeSink interface { | 56 | type HTTPChallengeSink interface { | ||
| 57 | // Present stores the key authorization for a challenge token. | 57 | // Present stores the key authorization for a challenge token. | ||
| 58 | - | Present(token, keyAuth string) | 58 | + | Present(token, keyAuth string) error |
| 59 | 59 | ||||
| 60 | // CleanUp removes the key authorization for a challenge token. | 60 | // CleanUp removes the key authorization for a challenge token. | ||
| 61 | - | CleanUp(token string) | 61 | + | CleanUp(token string) error |
| 62 | } | 62 | } | ||
| 63 | 63 | ||||
| 64 | // PublicCertificateIssuer defines the contract for obtaining and renewing | 64 | // PublicCertificateIssuer defines the contract for obtaining and renewing | ||
M internal/domain/public_tls.go +5 -2
| @@ -12,10 +12,13 @@ const ( | |||
| 12 | 12 | ACMEChallengeAuto ACMEChallengeMode = "auto" | |
| 13 | 13 | ACMEChallengeHTTP01 ACMEChallengeMode = "http-01" | |
| 14 | 14 | ACMEChallengeCloudflareDNS01 ACMEChallengeMode = "cloudflare-dns-01" | |
| 15 | + | ||
| 16 | + | MaxHTTP01TokenLength = 256 | |
| 17 | + | TLSRenewalWindow = 30 * 24 * time.Hour | |
| 15 | 18 | ) | |
| 16 | 19 | ||
| 17 | 20 | func IsValidHTTP01Token(token string) bool { | |
| 18 | - | return token != "" && !strings.Contains(token, "/") && !strings.Contains(token, "\\") && !strings.Contains(token, "..") && !strings.Contains(token, "\x00") | |
| 21 | + | return token != "" && len(token) <= MaxHTTP01TokenLength && !strings.Contains(token, "/") && !strings.Contains(token, "\\") && !strings.Contains(token, "..") && !strings.Contains(token, "\x00") | |
| 19 | 22 | } | |
| 20 | 23 | ||
| 21 | 24 | func ParseACMEChallengeMode(value string) (ACMEChallengeMode, error) { | |
| @@ -95,7 +98,7 @@ func (c ManagedCertificate) Health(now time.Time) TLSCertificateStatus { | |||
| 95 | 98 | if !now.Before(c.NotAfter) { | |
| 96 | 99 | return TLSCertificateStatusExpired | |
| 97 | 100 | } | |
| 98 | - | if !now.Add(30 * 24 * time.Hour).Before(c.NotAfter) { | |
| 101 | + | if !now.Add(TLSRenewalWindow).Before(c.NotAfter) { | |
| 99 | 102 | return TLSCertificateStatusWarning | |
| 100 | 103 | } | |
| 101 | 104 | return TLSCertificateStatusValid | |
| @@ -12,10 +12,13 @@ const ( | |||||
| 12 | ACMEChallengeAuto ACMEChallengeMode = "auto" | 12 | ACMEChallengeAuto ACMEChallengeMode = "auto" | ||
| 13 | ACMEChallengeHTTP01 ACMEChallengeMode = "http-01" | 13 | ACMEChallengeHTTP01 ACMEChallengeMode = "http-01" | ||
| 14 | ACMEChallengeCloudflareDNS01 ACMEChallengeMode = "cloudflare-dns-01" | 14 | ACMEChallengeCloudflareDNS01 ACMEChallengeMode = "cloudflare-dns-01" | ||
| 15 | + | ||||
| 16 | + | MaxHTTP01TokenLength = 256 | |||
| 17 | + | TLSRenewalWindow = 30 * 24 * time.Hour | |||
| 15 | ) | 18 | ) | ||
| 16 | 19 | ||||
| 17 | func IsValidHTTP01Token(token string) bool { | 20 | func IsValidHTTP01Token(token string) bool { | ||
| 18 | - | return token != "" && !strings.Contains(token, "/") && !strings.Contains(token, "\\") && !strings.Contains(token, "..") && !strings.Contains(token, "\x00") | 21 | + | return token != "" && len(token) <= MaxHTTP01TokenLength && !strings.Contains(token, "/") && !strings.Contains(token, "\\") && !strings.Contains(token, "..") && !strings.Contains(token, "\x00") |
| 19 | } | 22 | } | ||
| 20 | 23 | ||||
| 21 | func ParseACMEChallengeMode(value string) (ACMEChallengeMode, error) { | 24 | func ParseACMEChallengeMode(value string) (ACMEChallengeMode, error) { | ||
| @@ -95,7 +98,7 @@ func (c ManagedCertificate) Health(now time.Time) TLSCertificateStatus { | |||||
| 95 | if !now.Before(c.NotAfter) { | 98 | if !now.Before(c.NotAfter) { | ||
| 96 | return TLSCertificateStatusExpired | 99 | return TLSCertificateStatusExpired | ||
| 97 | } | 100 | } | ||
| 98 | - | if !now.Add(30 * 24 * time.Hour).Before(c.NotAfter) { | 101 | + | if !now.Add(TLSRenewalWindow).Before(c.NotAfter) { |
| 99 | return TLSCertificateStatusWarning | 102 | return TLSCertificateStatusWarning | ||
| 100 | } | 103 | } | ||
| 101 | return TLSCertificateStatusValid | 104 | return TLSCertificateStatusValid | ||
M internal/domain/public_tls_test.go +2 -0
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package domain | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "strings" | |
| 4 | 5 | "testing" | |
| 5 | 6 | "time" | |
| 6 | 7 | ||
| @@ -20,6 +21,7 @@ func TestIsValidHTTP01Token(t *testing.T) { | |||
| 20 | 21 | {name: "backslash", token: `abc\def`, want: false}, | |
| 21 | 22 | {name: "dot dot", token: "abc..def", want: false}, | |
| 22 | 23 | {name: "nul", token: "abc\x00def", want: false}, | |
| 24 | + | {name: "too long", token: strings.Repeat("a", MaxHTTP01TokenLength+1), want: false}, | |
| 23 | 25 | } | |
| 24 | 26 | ||
| 25 | 27 | for _, tt := range tests { | |
| @@ -1,6 +1,7 @@ | |||||
| 1 | package domain | 1 | package domain | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "strings" | |||
| 4 | "testing" | 5 | "testing" | ||
| 5 | "time" | 6 | "time" | ||
| 6 | 7 | ||||
| @@ -20,6 +21,7 @@ func TestIsValidHTTP01Token(t *testing.T) { | |||||
| 20 | {name: "backslash", token: `abc\def`, want: false}, | 21 | {name: "backslash", token: `abc\def`, want: false}, | ||
| 21 | {name: "dot dot", token: "abc..def", want: false}, | 22 | {name: "dot dot", token: "abc..def", want: false}, | ||
| 22 | {name: "nul", token: "abc\x00def", want: false}, | 23 | {name: "nul", token: "abc\x00def", want: false}, | ||
| 24 | + | {name: "too long", token: strings.Repeat("a", MaxHTTP01TokenLength+1), want: false}, | |||
| 23 | } | 25 | } | ||
| 24 | 26 | ||||
| 25 | for _, tt := range tests { | 27 | for _, tt := range tests { | ||
M internal/usecase/publictls/config.go +9 -22
| @@ -74,7 +74,7 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 74 | 74 | }, nil | |
| 75 | 75 | ||
| 76 | 76 | case domain.ACMEChallengeCloudflareDNS01: | |
| 77 | - | token, source, err := resolveToken(ctx, resolver) | |
| 77 | + | token, source, err := resolveToken(ctx, resolver, true) | |
| 78 | 78 | if err != nil { | |
| 79 | 79 | return EffectiveChallenge{}, fmt.Errorf("resolve Cloudflare token: %w", err) | |
| 80 | 80 | } | |
| @@ -90,7 +90,7 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 90 | 90 | }, nil | |
| 91 | 91 | ||
| 92 | 92 | case domain.ACMEChallengeAuto: | |
| 93 | - | token, source := resolveOptionalToken(ctx, resolver) | |
| 93 | + | token, source, _ := resolveToken(ctx, resolver, false) | |
| 94 | 94 | if token != "" { | |
| 95 | 95 | return EffectiveChallenge{ | |
| 96 | 96 | ConfiguredMode: domain.ACMEChallengeAuto, | |
| @@ -115,31 +115,18 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||
| 115 | 115 | } | |
| 116 | 116 | } | |
| 117 | 117 | ||
| 118 | - | // resolveOptionalToken resolves the Cloudflare token, ignoring resolver errors. | |
| 119 | - | // If the resolver is nil or returns an empty token, the result is empty with source none. | |
| 120 | - | func resolveOptionalToken(ctx context.Context, resolver out.SecretResolver) (string, domain.ACMETokenSource) { | |
| 121 | - | if resolver == nil { | |
| 122 | - | return "", domain.ACMETokenSourceNone | |
| 123 | - | } | |
| 124 | - | sv, err := resolver.ResolveCloudflareToken(ctx) | |
| 125 | - | if err != nil { | |
| 126 | - | return "", domain.ACMETokenSourceNone | |
| 127 | - | } | |
| 128 | - | if sv.Value == "" { | |
| 129 | - | return "", domain.ACMETokenSourceNone | |
| 130 | - | } | |
| 131 | - | return sv.Value, sv.Source | |
| 132 | - | } | |
| 133 | - | ||
| 134 | - | // resolveToken resolves the Cloudflare token via the resolver, propagating errors. | |
| 135 | - | // Returns empty token and none source if resolver is nil. | |
| 136 | - | func resolveToken(ctx context.Context, resolver out.SecretResolver) (string, domain.ACMETokenSource, error) { | |
| 118 | + | // resolveToken resolves the Cloudflare token via the resolver. When strict is | |
| 119 | + | // false, resolver errors are ignored so auto mode can fall back to HTTP-01. | |
| 120 | + | func resolveToken(ctx context.Context, resolver out.SecretResolver, strict bool) (string, domain.ACMETokenSource, error) { | |
| 137 | 121 | if resolver == nil { | |
| 138 | 122 | return "", domain.ACMETokenSourceNone, nil | |
| 139 | 123 | } | |
| 140 | 124 | sv, err := resolver.ResolveCloudflareToken(ctx) | |
| 141 | 125 | if err != nil { | |
| 142 | - | return "", domain.ACMETokenSourceNone, err | |
| 126 | + | if strict { | |
| 127 | + | return "", domain.ACMETokenSourceNone, err | |
| 128 | + | } | |
| 129 | + | return "", domain.ACMETokenSourceNone, nil | |
| 143 | 130 | } | |
| 144 | 131 | if sv.Value == "" { | |
| 145 | 132 | return "", domain.ACMETokenSourceNone, nil | |
| @@ -74,7 +74,7 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 74 | }, nil | 74 | }, nil | ||
| 75 | 75 | ||||
| 76 | case domain.ACMEChallengeCloudflareDNS01: | 76 | case domain.ACMEChallengeCloudflareDNS01: | ||
| 77 | - | token, source, err := resolveToken(ctx, resolver) | 77 | + | token, source, err := resolveToken(ctx, resolver, true) |
| 78 | if err != nil { | 78 | if err != nil { | ||
| 79 | return EffectiveChallenge{}, fmt.Errorf("resolve Cloudflare token: %w", err) | 79 | return EffectiveChallenge{}, fmt.Errorf("resolve Cloudflare token: %w", err) | ||
| 80 | } | 80 | } | ||
| @@ -90,7 +90,7 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 90 | }, nil | 90 | }, nil | ||
| 91 | 91 | ||||
| 92 | case domain.ACMEChallengeAuto: | 92 | case domain.ACMEChallengeAuto: | ||
| 93 | - | token, source := resolveOptionalToken(ctx, resolver) | 93 | + | token, source, _ := resolveToken(ctx, resolver, false) |
| 94 | if token != "" { | 94 | if token != "" { | ||
| 95 | return EffectiveChallenge{ | 95 | return EffectiveChallenge{ | ||
| 96 | ConfiguredMode: domain.ACMEChallengeAuto, | 96 | ConfiguredMode: domain.ACMEChallengeAuto, | ||
| @@ -115,31 +115,18 @@ func ResolveEffectiveChallenge(ctx context.Context, cfg Config, resolver out.Sec | |||||
| 115 | } | 115 | } | ||
| 116 | } | 116 | } | ||
| 117 | 117 | ||||
| 118 | - | // resolveOptionalToken resolves the Cloudflare token, ignoring resolver errors. | 118 | + | // resolveToken resolves the Cloudflare token via the resolver. When strict is |
| 119 | - | // If the resolver is nil or returns an empty token, the result is empty with source none. | 119 | + | // false, resolver errors are ignored so auto mode can fall back to HTTP-01. |
| 120 | - | func resolveOptionalToken(ctx context.Context, resolver out.SecretResolver) (string, domain.ACMETokenSource) { | 120 | + | func resolveToken(ctx context.Context, resolver out.SecretResolver, strict bool) (string, domain.ACMETokenSource, error) { |
| 121 | - | if resolver == nil { | |||
| 122 | - | return "", domain.ACMETokenSourceNone | |||
| 123 | - | } | |||
| 124 | - | sv, err := resolver.ResolveCloudflareToken(ctx) | |||
| 125 | - | if err != nil { | |||
| 126 | - | return "", domain.ACMETokenSourceNone | |||
| 127 | - | } | |||
| 128 | - | if sv.Value == "" { | |||
| 129 | - | return "", domain.ACMETokenSourceNone | |||
| 130 | - | } | |||
| 131 | - | return sv.Value, sv.Source | |||
| 132 | - | } | |||
| 133 | - | ||||
| 134 | - | // resolveToken resolves the Cloudflare token via the resolver, propagating errors. | |||
| 135 | - | // Returns empty token and none source if resolver is nil. | |||
| 136 | - | func resolveToken(ctx context.Context, resolver out.SecretResolver) (string, domain.ACMETokenSource, error) { | |||
| 137 | if resolver == nil { | 121 | if resolver == nil { | ||
| 138 | return "", domain.ACMETokenSourceNone, nil | 122 | return "", domain.ACMETokenSourceNone, nil | ||
| 139 | } | 123 | } | ||
| 140 | sv, err := resolver.ResolveCloudflareToken(ctx) | 124 | sv, err := resolver.ResolveCloudflareToken(ctx) | ||
| 141 | if err != nil { | 125 | if err != nil { | ||
| 142 | - | return "", domain.ACMETokenSourceNone, err | 126 | + | if strict { |
| 127 | + | return "", domain.ACMETokenSourceNone, err | |||
| 128 | + | } | |||
| 129 | + | return "", domain.ACMETokenSourceNone, nil | |||
| 143 | } | 130 | } | ||
| 144 | if sv.Value == "" { | 131 | if sv.Value == "" { | ||
| 145 | return "", domain.ACMETokenSourceNone, nil | 132 | return "", domain.ACMETokenSourceNone, nil | ||
M internal/usecase/publictls/http01.go +6 -4
| @@ -23,24 +23,26 @@ func NewHTTP01Challenges() *HTTP01Challenges { | |||
| 23 | 23 | ||
| 24 | 24 | // Present stores the key authorization for the given token. It silently | |
| 25 | 25 | // ignores unsafe tokens and empty keyAuth values. | |
| 26 | - | func (c *HTTP01Challenges) Present(token, keyAuth string) { | |
| 26 | + | func (c *HTTP01Challenges) Present(token, keyAuth string) error { | |
| 27 | 27 | if !domain.IsValidHTTP01Token(token) || keyAuth == "" { | |
| 28 | - | return | |
| 28 | + | return nil | |
| 29 | 29 | } | |
| 30 | 30 | c.mu.Lock() | |
| 31 | 31 | defer c.mu.Unlock() | |
| 32 | 32 | c.data[token] = keyAuth | |
| 33 | + | return nil | |
| 33 | 34 | } | |
| 34 | 35 | ||
| 35 | 36 | // CleanUp removes the stored key authorization for the given token. It silently | |
| 36 | 37 | // ignores unsafe tokens. | |
| 37 | - | func (c *HTTP01Challenges) CleanUp(token string) { | |
| 38 | + | func (c *HTTP01Challenges) CleanUp(token string) error { | |
| 38 | 39 | if !domain.IsValidHTTP01Token(token) { | |
| 39 | - | return | |
| 40 | + | return nil | |
| 40 | 41 | } | |
| 41 | 42 | c.mu.Lock() | |
| 42 | 43 | defer c.mu.Unlock() | |
| 43 | 44 | delete(c.data, token) | |
| 45 | + | return nil | |
| 44 | 46 | } | |
| 45 | 47 | ||
| 46 | 48 | // Get returns the key authorization for the given token, and a boolean | |
| @@ -23,24 +23,26 @@ func NewHTTP01Challenges() *HTTP01Challenges { | |||||
| 23 | 23 | ||||
| 24 | // Present stores the key authorization for the given token. It silently | 24 | // Present stores the key authorization for the given token. It silently | ||
| 25 | // ignores unsafe tokens and empty keyAuth values. | 25 | // ignores unsafe tokens and empty keyAuth values. | ||
| 26 | - | func (c *HTTP01Challenges) Present(token, keyAuth string) { | 26 | + | func (c *HTTP01Challenges) Present(token, keyAuth string) error { |
| 27 | if !domain.IsValidHTTP01Token(token) || keyAuth == "" { | 27 | if !domain.IsValidHTTP01Token(token) || keyAuth == "" { | ||
| 28 | - | return | 28 | + | return nil |
| 29 | } | 29 | } | ||
| 30 | c.mu.Lock() | 30 | c.mu.Lock() | ||
| 31 | defer c.mu.Unlock() | 31 | defer c.mu.Unlock() | ||
| 32 | c.data[token] = keyAuth | 32 | c.data[token] = keyAuth | ||
| 33 | + | return nil | |||
| 33 | } | 34 | } | ||
| 34 | 35 | ||||
| 35 | // CleanUp removes the stored key authorization for the given token. It silently | 36 | // CleanUp removes the stored key authorization for the given token. It silently | ||
| 36 | // ignores unsafe tokens. | 37 | // ignores unsafe tokens. | ||
| 37 | - | func (c *HTTP01Challenges) CleanUp(token string) { | 38 | + | func (c *HTTP01Challenges) CleanUp(token string) error { |
| 38 | if !domain.IsValidHTTP01Token(token) { | 39 | if !domain.IsValidHTTP01Token(token) { | ||
| 39 | - | return | 40 | + | return nil |
| 40 | } | 41 | } | ||
| 41 | c.mu.Lock() | 42 | c.mu.Lock() | ||
| 42 | defer c.mu.Unlock() | 43 | defer c.mu.Unlock() | ||
| 43 | delete(c.data, token) | 44 | delete(c.data, token) | ||
| 45 | + | return nil | |||
| 44 | } | 46 | } | ||
| 45 | 47 | ||||
| 46 | // Get returns the key authorization for the given token, and a boolean | 48 | // Get returns the key authorization for the given token, and a boolean | ||
M internal/usecase/publictls/integration_test.go +1 -3
| @@ -292,7 +292,5 @@ func TestPublicTLSIntegration_MissingRequiredCertReportsCoverageError(t *testing | |||
| 292 | 292 | assert.False(t, rc.Covered, "route should NOT be covered") | |
| 293 | 293 | assert.True(t, rc.RequiredACME) | |
| 294 | 294 | ||
| 295 | - | // The route error may be empty since not all code paths populate routeErr; | |
| 296 | - | // at minimum the covered flag must be false. | |
| 297 | - | t.Logf("Route error (may be empty): %q", rc.Error) | |
| 295 | + | assert.Contains(t, rc.Error, "obtain certificate") | |
| 298 | 296 | } | |
| @@ -292,7 +292,5 @@ func TestPublicTLSIntegration_MissingRequiredCertReportsCoverageError(t *testing | |||||
| 292 | assert.False(t, rc.Covered, "route should NOT be covered") | 292 | assert.False(t, rc.Covered, "route should NOT be covered") | ||
| 293 | assert.True(t, rc.RequiredACME) | 293 | assert.True(t, rc.RequiredACME) | ||
| 294 | 294 | ||||
| 295 | - | // The route error may be empty since not all code paths populate routeErr; | 295 | + | assert.Contains(t, rc.Error, "obtain certificate") |
| 296 | - | // at minimum the covered flag must be false. | |||
| 297 | - | t.Logf("Route error (may be empty): %q", rc.Error) | |||
| 298 | } | 296 | } | ||
M internal/usecase/publictls/renewal.go +9 -5
| @@ -11,16 +11,13 @@ import ( | |||
| 11 | 11 | "github.com/bnema/gordon/internal/domain" | |
| 12 | 12 | ) | |
| 13 | 13 | ||
| 14 | - | // renewalWindow is the window before certificate expiry when renewal should begin. | |
| 15 | - | const renewalWindow = 30 * 24 * time.Hour | |
| 16 | - | ||
| 17 | 14 | // ShouldRenew reports whether a certificate should be renewed at the given time. | |
| 18 | 15 | // A certificate with a zero NotAfter is always considered due for renewal. | |
| 19 | 16 | func ShouldRenew(cert out.StoredCertificate, now time.Time) bool { | |
| 20 | 17 | if cert.NotAfter.IsZero() { | |
| 21 | 18 | return true | |
| 22 | 19 | } | |
| 23 | - | return !now.Before(cert.NotAfter.Add(-renewalWindow)) | |
| 20 | + | return !now.Before(cert.NotAfter.Add(-domain.TLSRenewalWindow)) | |
| 24 | 21 | } | |
| 25 | 22 | ||
| 26 | 23 | // StartRenewalLoop starts a background goroutine that periodically reconciles | |
| @@ -55,7 +52,11 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration) | |||
| 55 | 52 | oldCancel() | |
| 56 | 53 | } | |
| 57 | 54 | if oldDone != nil { | |
| 58 | - | <-oldDone | |
| 55 | + | select { | |
| 56 | + | case <-oldDone: | |
| 57 | + | case <-ctx.Done(): | |
| 58 | + | case <-time.After(30 * time.Second): | |
| 59 | + | } | |
| 59 | 60 | } | |
| 60 | 61 | ||
| 61 | 62 | go func() { | |
| @@ -116,6 +117,9 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||
| 116 | 117 | s.mu.Unlock() | |
| 117 | 118 | ||
| 118 | 119 | for _, cert := range due { | |
| 120 | + | if err := ctx.Err(); err != nil { | |
| 121 | + | return err | |
| 122 | + | } | |
| 119 | 123 | renewed, err := s.deps.Issuer.Renew(ctx, cert) | |
| 120 | 124 | if err != nil { | |
| 121 | 125 | s.mu.Lock() | |
| @@ -11,16 +11,13 @@ import ( | |||||
| 11 | "github.com/bnema/gordon/internal/domain" | 11 | "github.com/bnema/gordon/internal/domain" | ||
| 12 | ) | 12 | ) | ||
| 13 | 13 | ||||
| 14 | - | // renewalWindow is the window before certificate expiry when renewal should begin. | |||
| 15 | - | const renewalWindow = 30 * 24 * time.Hour | |||
| 16 | - | ||||
| 17 | // ShouldRenew reports whether a certificate should be renewed at the given time. | 14 | // ShouldRenew reports whether a certificate should be renewed at the given time. | ||
| 18 | // A certificate with a zero NotAfter is always considered due for renewal. | 15 | // A certificate with a zero NotAfter is always considered due for renewal. | ||
| 19 | func ShouldRenew(cert out.StoredCertificate, now time.Time) bool { | 16 | func ShouldRenew(cert out.StoredCertificate, now time.Time) bool { | ||
| 20 | if cert.NotAfter.IsZero() { | 17 | if cert.NotAfter.IsZero() { | ||
| 21 | return true | 18 | return true | ||
| 22 | } | 19 | } | ||
| 23 | - | return !now.Before(cert.NotAfter.Add(-renewalWindow)) | 20 | + | return !now.Before(cert.NotAfter.Add(-domain.TLSRenewalWindow)) |
| 24 | } | 21 | } | ||
| 25 | 22 | ||||
| 26 | // StartRenewalLoop starts a background goroutine that periodically reconciles | 23 | // StartRenewalLoop starts a background goroutine that periodically reconciles | ||
| @@ -55,7 +52,11 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration) | |||||
| 55 | oldCancel() | 52 | oldCancel() | ||
| 56 | } | 53 | } | ||
| 57 | if oldDone != nil { | 54 | if oldDone != nil { | ||
| 58 | - | <-oldDone | 55 | + | select { |
| 56 | + | case <-oldDone: | |||
| 57 | + | case <-ctx.Done(): | |||
| 58 | + | case <-time.After(30 * time.Second): | |||
| 59 | + | } | |||
| 59 | } | 60 | } | ||
| 60 | 61 | ||||
| 61 | go func() { | 62 | go func() { | ||
| @@ -116,6 +117,9 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error | |||||
| 116 | s.mu.Unlock() | 117 | s.mu.Unlock() | ||
| 117 | 118 | ||||
| 118 | for _, cert := range due { | 119 | for _, cert := range due { | ||
| 120 | + | if err := ctx.Err(); err != nil { | |||
| 121 | + | return err | |||
| 122 | + | } | |||
| 119 | renewed, err := s.deps.Issuer.Renew(ctx, cert) | 123 | renewed, err := s.deps.Issuer.Renew(ctx, cert) | ||
| 120 | if err != nil { | 124 | if err != nil { | ||
| 121 | s.mu.Lock() | 125 | s.mu.Lock() | ||
M internal/usecase/publictls/service.go +81 -31
| @@ -88,6 +88,7 @@ func (s *Service) Load(ctx context.Context) error { | |||
| 88 | 88 | ||
| 89 | 89 | s.certs = make(map[string]*out.StoredCertificate, len(stored)) | |
| 90 | 90 | s.lastErr = make(map[string]string) | |
| 91 | + | s.requiredHosts = make(map[string]struct{}) | |
| 91 | 92 | ||
| 92 | 93 | for i := range stored { | |
| 93 | 94 | cert := &stored[i] | |
| @@ -96,6 +97,7 @@ func (s *Service) Load(ctx context.Context) error { | |||
| 96 | 97 | continue | |
| 97 | 98 | } | |
| 98 | 99 | s.certs[cert.ID] = cert | |
| 100 | + | addNamesToRequiredHosts(s.requiredHosts, cert.Names) | |
| 99 | 101 | if cert.LastError != "" { | |
| 100 | 102 | s.lastErr[cert.ID] = cert.LastError | |
| 101 | 103 | } | |
| @@ -156,6 +158,7 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||
| 156 | 158 | // returns ErrTLSRouteNotCovered if target derivation/issuer fails. | |
| 157 | 159 | s.mu.Lock() | |
| 158 | 160 | s.requiredHosts = required | |
| 161 | + | clearRouteErrorsLocked(s.routeErr, required) | |
| 159 | 162 | s.mu.Unlock() | |
| 160 | 163 | ||
| 161 | 164 | // Derive desired targets. | |
| @@ -164,6 +167,9 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||
| 164 | 167 | s.deps.ZoneResolver, | |
| 165 | 168 | ) | |
| 166 | 169 | if err != nil { | |
| 170 | + | s.mu.Lock() | |
| 171 | + | setRouteErrorsLocked(s.routeErr, required, fmt.Sprintf("derive certificate targets: %v", err)) | |
| 172 | + | s.mu.Unlock() | |
| 167 | 173 | return fmt.Errorf("derive certificate targets: %w", err) | |
| 168 | 174 | } | |
| 169 | 175 | ||
| @@ -172,17 +178,6 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||
| 172 | 178 | // the exact route hosts already present suffice for isRequiredHostLocked. | |
| 173 | 179 | addTargetNames(required, targets) | |
| 174 | 180 | ||
| 175 | - | // Acquire store lock to serialise certificate operations. | |
| 176 | - | unlock, err := s.deps.Store.Lock(ctx) | |
| 177 | - | if err != nil { | |
| 178 | - | return fmt.Errorf("acquire store lock: %w", err) | |
| 179 | - | } | |
| 180 | - | defer func() { | |
| 181 | - | if err := unlock(); err != nil { | |
| 182 | - | log.Warn().Err(err).Msg("failed to release store lock") | |
| 183 | - | } | |
| 184 | - | }() | |
| 185 | - | ||
| 186 | 181 | // Under mu: update requiredHosts (with target names) and compute missing targets. | |
| 187 | 182 | s.mu.Lock() | |
| 188 | 183 | s.requiredHosts = required | |
| @@ -196,9 +191,9 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||
| 196 | 191 | } | |
| 197 | 192 | s.mu.Unlock() | |
| 198 | 193 | ||
| 199 | - | // Process missing targets without holding s.mu so GetCertificate/Status | |
| 200 | - | // are not blocked during network I/O (Obtain) or storage writes (Save). | |
| 201 | - | s.reconcileMissingTargets(ctx, missing) | |
| 194 | + | // Process missing targets without holding s.mu or the store lock so | |
| 195 | + | // GetCertificate/Status are not blocked during network I/O (Obtain). | |
| 196 | + | s.reconcileMissingTargets(ctx, missing, required) | |
| 202 | 197 | ||
| 203 | 198 | log.Debug().Int("missing_count", len(missing)).Msg("reconciled missing targets") | |
| 204 | 199 | return nil | |
| @@ -232,20 +227,53 @@ func canonicalHostSet(hosts []string) map[string]struct{} { | |||
| 232 | 227 | return required | |
| 233 | 228 | } | |
| 234 | 229 | ||
| 230 | + | func addNamesToRequiredHosts(required map[string]struct{}, names []string) { | |
| 231 | + | for _, name := range names { | |
| 232 | + | name = strings.TrimSuffix(strings.ToLower(strings.TrimSpace(name)), ".") | |
| 233 | + | if name != "" { | |
| 234 | + | required[name] = struct{}{} | |
| 235 | + | } | |
| 236 | + | } | |
| 237 | + | } | |
| 238 | + | ||
| 235 | 239 | func addTargetNames(required map[string]struct{}, targets []CertificateTarget) { | |
| 236 | 240 | for _, t := range targets { | |
| 237 | - | for _, name := range t.Names { | |
| 238 | - | name = strings.TrimSuffix(strings.ToLower(strings.TrimSpace(name)), ".") | |
| 239 | - | if name != "" { | |
| 240 | - | required[name] = struct{}{} | |
| 241 | - | } | |
| 241 | + | addNamesToRequiredHosts(required, t.Names) | |
| 242 | + | } | |
| 243 | + | } | |
| 244 | + | ||
| 245 | + | func clearRouteErrorsLocked(routeErr map[string]string, hosts map[string]struct{}) { | |
| 246 | + | for host := range hosts { | |
| 247 | + | delete(routeErr, host) | |
| 248 | + | } | |
| 249 | + | } | |
| 250 | + | ||
| 251 | + | func setRouteErrorsLocked(routeErr map[string]string, hosts map[string]struct{}, message string) { | |
| 252 | + | for host := range hosts { | |
| 253 | + | routeErr[host] = message | |
| 254 | + | } | |
| 255 | + | } | |
| 256 | + | ||
| 257 | + | func setTargetRouteErrorLocked(routeErr map[string]string, required map[string]struct{}, target CertificateTarget, message string) { | |
| 258 | + | for host := range required { | |
| 259 | + | if hostMatchesCert(target.Names, host) { | |
| 260 | + | routeErr[host] = message | |
| 261 | + | } | |
| 262 | + | } | |
| 263 | + | } | |
| 264 | + | ||
| 265 | + | func clearTargetRouteErrorLocked(routeErr map[string]string, required map[string]struct{}, target CertificateTarget) { | |
| 266 | + | for host := range required { | |
| 267 | + | if hostMatchesCert(target.Names, host) { | |
| 268 | + | delete(routeErr, host) | |
| 242 | 269 | } | |
| 243 | 270 | } | |
| 244 | 271 | } | |
| 245 | 272 | ||
| 246 | 273 | // reconcileMissingTargets obtains and saves certificates for each missing | |
| 247 | 274 | // target. Called without s.mu held so network I/O does not block readers. | |
| 248 | - | func (s *Service) reconcileMissingTargets(ctx context.Context, missing []CertificateTarget) { | |
| 275 | + | func (s *Service) reconcileMissingTargets(ctx context.Context, missing []CertificateTarget, required map[string]struct{}) { | |
| 276 | + | log := zerowrap.FromCtx(ctx) | |
| 249 | 277 | for _, target := range missing { | |
| 250 | 278 | order := out.CertificateOrder{ | |
| 251 | 279 | ID: target.ID, | |
| @@ -255,27 +283,48 @@ func (s *Service) reconcileMissingTargets(ctx context.Context, missing []Certifi | |||
| 255 | 283 | ||
| 256 | 284 | stored, err := s.deps.Issuer.Obtain(ctx, order) | |
| 257 | 285 | if err != nil { | |
| 286 | + | errorMessage := fmt.Sprintf("obtain certificate: %v", err) | |
| 258 | 287 | s.mu.Lock() | |
| 259 | - | s.lastErr[target.ID] = fmt.Sprintf("obtain certificate: %v", err) | |
| 288 | + | s.lastErr[target.ID] = errorMessage | |
| 289 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |
| 260 | 290 | s.mu.Unlock() | |
| 261 | 291 | continue | |
| 262 | 292 | } | |
| 263 | 293 | if stored == nil { | |
| 294 | + | errorMessage := "obtain certificate returned nil" | |
| 264 | 295 | s.mu.Lock() | |
| 265 | - | s.lastErr[target.ID] = "obtain certificate returned nil" | |
| 296 | + | s.lastErr[target.ID] = errorMessage | |
| 297 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |
| 266 | 298 | s.mu.Unlock() | |
| 267 | 299 | continue | |
| 268 | 300 | } | |
| 269 | 301 | if err := populateStoredCertificate(stored); err != nil { | |
| 302 | + | errorMessage := err.Error() | |
| 270 | 303 | s.mu.Lock() | |
| 271 | - | s.lastErr[target.ID] = err.Error() | |
| 304 | + | s.lastErr[target.ID] = errorMessage | |
| 305 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |
| 272 | 306 | s.mu.Unlock() | |
| 273 | 307 | continue | |
| 274 | 308 | } | |
| 275 | 309 | ||
| 276 | - | if err := s.deps.Store.Save(ctx, *stored); err != nil { | |
| 310 | + | unlock, err := s.deps.Store.Lock(ctx) | |
| 311 | + | if err != nil { | |
| 312 | + | errorMessage := fmt.Sprintf("acquire store lock: %v", err) | |
| 313 | + | s.mu.Lock() | |
| 314 | + | s.lastErr[target.ID] = errorMessage | |
| 315 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |
| 316 | + | s.mu.Unlock() | |
| 317 | + | continue | |
| 318 | + | } | |
| 319 | + | saveErr := s.deps.Store.Save(ctx, *stored) | |
| 320 | + | if unlockErr := unlock(); unlockErr != nil { | |
| 321 | + | log.Warn().Err(unlockErr).Msg("failed to release store lock") | |
| 322 | + | } | |
| 323 | + | if saveErr != nil { | |
| 324 | + | errorMessage := fmt.Sprintf("save certificate: %v", saveErr) | |
| 277 | 325 | s.mu.Lock() | |
| 278 | - | s.lastErr[target.ID] = fmt.Sprintf("save certificate: %v", err) | |
| 326 | + | s.lastErr[target.ID] = errorMessage | |
| 327 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |
| 279 | 328 | s.mu.Unlock() | |
| 280 | 329 | continue | |
| 281 | 330 | } | |
| @@ -283,6 +332,7 @@ func (s *Service) reconcileMissingTargets(ctx context.Context, missing []Certifi | |||
| 283 | 332 | s.mu.Lock() | |
| 284 | 333 | s.certs[stored.ID] = stored | |
| 285 | 334 | delete(s.lastErr, stored.ID) | |
| 335 | + | clearTargetRouteErrorLocked(s.routeErr, required, target) | |
| 286 | 336 | s.mu.Unlock() | |
| 287 | 337 | } | |
| 288 | 338 | } | |
| @@ -295,15 +345,15 @@ func (s *Service) certificateExistsLocked(target CertificateTarget) bool { | |||
| 295 | 345 | if cert.NotAfter.IsZero() || now.After(cert.NotAfter) { | |
| 296 | 346 | continue // expired or no expiry | |
| 297 | 347 | } | |
| 298 | - | if namesCover(cert.Names, target.Names) { | |
| 348 | + | if exactNamesCoverAll(cert.Names, target.Names) { | |
| 299 | 349 | return true | |
| 300 | 350 | } | |
| 301 | 351 | } | |
| 302 | 352 | return false | |
| 303 | 353 | } | |
| 304 | 354 | ||
| 305 | - | // namesCover reports whether names covers all required names (exact match, case-insensitive). | |
| 306 | - | func namesCover(names, required []string) bool { | |
| 355 | + | // exactNamesCoverAll reports whether names contains every required name exactly, case-insensitively. | |
| 356 | + | func exactNamesCoverAll(names, required []string) bool { | |
| 307 | 357 | covered := make(map[string]bool) | |
| 308 | 358 | for _, n := range names { | |
| 309 | 359 | covered[strings.ToLower(strings.TrimSuffix(n, "."))] = true | |
| @@ -378,9 +428,9 @@ func (s *Service) GetHTTP01Challenge(ctx context.Context, token string) (string, | |||
| 378 | 428 | return s.deps.Challenges.Get(ctx, token) | |
| 379 | 429 | } | |
| 380 | 430 | ||
| 381 | - | // GetStoredCertificate returns a copy of the stored certificate for the given | |
| 382 | - | // ID, or nil if not found. Exposed for testing. | |
| 383 | - | func (s *Service) GetStoredCertificate(id string) *out.StoredCertificate { | |
| 431 | + | // getStoredCertificate returns a copy of the stored certificate for the given | |
| 432 | + | // ID, or nil if not found. | |
| 433 | + | func (s *Service) getStoredCertificate(id string) *out.StoredCertificate { | |
| 384 | 434 | s.mu.RLock() | |
| 385 | 435 | defer s.mu.RUnlock() | |
| 386 | 436 | cert := s.certs[id] | |
| @@ -88,6 +88,7 @@ func (s *Service) Load(ctx context.Context) error { | |||||
| 88 | 88 | ||||
| 89 | s.certs = make(map[string]*out.StoredCertificate, len(stored)) | 89 | s.certs = make(map[string]*out.StoredCertificate, len(stored)) | ||
| 90 | s.lastErr = make(map[string]string) | 90 | s.lastErr = make(map[string]string) | ||
| 91 | + | s.requiredHosts = make(map[string]struct{}) | |||
| 91 | 92 | ||||
| 92 | for i := range stored { | 93 | for i := range stored { | ||
| 93 | cert := &stored[i] | 94 | cert := &stored[i] | ||
| @@ -96,6 +97,7 @@ func (s *Service) Load(ctx context.Context) error { | |||||
| 96 | continue | 97 | continue | ||
| 97 | } | 98 | } | ||
| 98 | s.certs[cert.ID] = cert | 99 | s.certs[cert.ID] = cert | ||
| 100 | + | addNamesToRequiredHosts(s.requiredHosts, cert.Names) | |||
| 99 | if cert.LastError != "" { | 101 | if cert.LastError != "" { | ||
| 100 | s.lastErr[cert.ID] = cert.LastError | 102 | s.lastErr[cert.ID] = cert.LastError | ||
| 101 | } | 103 | } | ||
| @@ -156,6 +158,7 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||||
| 156 | // returns ErrTLSRouteNotCovered if target derivation/issuer fails. | 158 | // returns ErrTLSRouteNotCovered if target derivation/issuer fails. | ||
| 157 | s.mu.Lock() | 159 | s.mu.Lock() | ||
| 158 | s.requiredHosts = required | 160 | s.requiredHosts = required | ||
| 161 | + | clearRouteErrorsLocked(s.routeErr, required) | |||
| 159 | s.mu.Unlock() | 162 | s.mu.Unlock() | ||
| 160 | 163 | ||||
| 161 | // Derive desired targets. | 164 | // Derive desired targets. | ||
| @@ -164,6 +167,9 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||||
| 164 | s.deps.ZoneResolver, | 167 | s.deps.ZoneResolver, | ||
| 165 | ) | 168 | ) | ||
| 166 | if err != nil { | 169 | if err != nil { | ||
| 170 | + | s.mu.Lock() | |||
| 171 | + | setRouteErrorsLocked(s.routeErr, required, fmt.Sprintf("derive certificate targets: %v", err)) | |||
| 172 | + | s.mu.Unlock() | |||
| 167 | return fmt.Errorf("derive certificate targets: %w", err) | 173 | return fmt.Errorf("derive certificate targets: %w", err) | ||
| 168 | } | 174 | } | ||
| 169 | 175 | ||||
| @@ -172,17 +178,6 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||||
| 172 | // the exact route hosts already present suffice for isRequiredHostLocked. | 178 | // the exact route hosts already present suffice for isRequiredHostLocked. | ||
| 173 | addTargetNames(required, targets) | 179 | addTargetNames(required, targets) | ||
| 174 | 180 | ||||
| 175 | - | // Acquire store lock to serialise certificate operations. | |||
| 176 | - | unlock, err := s.deps.Store.Lock(ctx) | |||
| 177 | - | if err != nil { | |||
| 178 | - | return fmt.Errorf("acquire store lock: %w", err) | |||
| 179 | - | } | |||
| 180 | - | defer func() { | |||
| 181 | - | if err := unlock(); err != nil { | |||
| 182 | - | log.Warn().Err(err).Msg("failed to release store lock") | |||
| 183 | - | } | |||
| 184 | - | }() | |||
| 185 | - | ||||
| 186 | // Under mu: update requiredHosts (with target names) and compute missing targets. | 181 | // Under mu: update requiredHosts (with target names) and compute missing targets. | ||
| 187 | s.mu.Lock() | 182 | s.mu.Lock() | ||
| 188 | s.requiredHosts = required | 183 | s.requiredHosts = required | ||
| @@ -196,9 +191,9 @@ func (s *Service) Reconcile(ctx context.Context) error { | |||||
| 196 | } | 191 | } | ||
| 197 | s.mu.Unlock() | 192 | s.mu.Unlock() | ||
| 198 | 193 | ||||
| 199 | - | // Process missing targets without holding s.mu so GetCertificate/Status | 194 | + | // Process missing targets without holding s.mu or the store lock so |
| 200 | - | // are not blocked during network I/O (Obtain) or storage writes (Save). | 195 | + | // GetCertificate/Status are not blocked during network I/O (Obtain). |
| 201 | - | s.reconcileMissingTargets(ctx, missing) | 196 | + | s.reconcileMissingTargets(ctx, missing, required) |
| 202 | 197 | ||||
| 203 | log.Debug().Int("missing_count", len(missing)).Msg("reconciled missing targets") | 198 | log.Debug().Int("missing_count", len(missing)).Msg("reconciled missing targets") | ||
| 204 | return nil | 199 | return nil | ||
| @@ -232,20 +227,53 @@ func canonicalHostSet(hosts []string) map[string]struct{} { | |||||
| 232 | return required | 227 | return required | ||
| 233 | } | 228 | } | ||
| 234 | 229 | ||||
| 230 | + | func addNamesToRequiredHosts(required map[string]struct{}, names []string) { | |||
| 231 | + | for _, name := range names { | |||
| 232 | + | name = strings.TrimSuffix(strings.ToLower(strings.TrimSpace(name)), ".") | |||
| 233 | + | if name != "" { | |||
| 234 | + | required[name] = struct{}{} | |||
| 235 | + | } | |||
| 236 | + | } | |||
| 237 | + | } | |||
| 238 | + | ||||
| 235 | func addTargetNames(required map[string]struct{}, targets []CertificateTarget) { | 239 | func addTargetNames(required map[string]struct{}, targets []CertificateTarget) { | ||
| 236 | for _, t := range targets { | 240 | for _, t := range targets { | ||
| 237 | - | for _, name := range t.Names { | 241 | + | addNamesToRequiredHosts(required, t.Names) |
| 238 | - | name = strings.TrimSuffix(strings.ToLower(strings.TrimSpace(name)), ".") | 242 | + | } |
| 239 | - | if name != "" { | 243 | + | } |
| 240 | - | required[name] = struct{}{} | 244 | + | |
| 241 | - | } | 245 | + | func clearRouteErrorsLocked(routeErr map[string]string, hosts map[string]struct{}) { |
| 246 | + | for host := range hosts { | |||
| 247 | + | delete(routeErr, host) | |||
| 248 | + | } | |||
| 249 | + | } | |||
| 250 | + | ||||
| 251 | + | func setRouteErrorsLocked(routeErr map[string]string, hosts map[string]struct{}, message string) { | |||
| 252 | + | for host := range hosts { | |||
| 253 | + | routeErr[host] = message | |||
| 254 | + | } | |||
| 255 | + | } | |||
| 256 | + | ||||
| 257 | + | func setTargetRouteErrorLocked(routeErr map[string]string, required map[string]struct{}, target CertificateTarget, message string) { | |||
| 258 | + | for host := range required { | |||
| 259 | + | if hostMatchesCert(target.Names, host) { | |||
| 260 | + | routeErr[host] = message | |||
| 261 | + | } | |||
| 262 | + | } | |||
| 263 | + | } | |||
| 264 | + | ||||
| 265 | + | func clearTargetRouteErrorLocked(routeErr map[string]string, required map[string]struct{}, target CertificateTarget) { | |||
| 266 | + | for host := range required { | |||
| 267 | + | if hostMatchesCert(target.Names, host) { | |||
| 268 | + | delete(routeErr, host) | |||
| 242 | } | 269 | } | ||
| 243 | } | 270 | } | ||
| 244 | } | 271 | } | ||
| 245 | 272 | ||||
| 246 | // reconcileMissingTargets obtains and saves certificates for each missing | 273 | // reconcileMissingTargets obtains and saves certificates for each missing | ||
| 247 | // target. Called without s.mu held so network I/O does not block readers. | 274 | // target. Called without s.mu held so network I/O does not block readers. | ||
| 248 | - | func (s *Service) reconcileMissingTargets(ctx context.Context, missing []CertificateTarget) { | 275 | + | func (s *Service) reconcileMissingTargets(ctx context.Context, missing []CertificateTarget, required map[string]struct{}) { |
| 276 | + | log := zerowrap.FromCtx(ctx) | |||
| 249 | for _, target := range missing { | 277 | for _, target := range missing { | ||
| 250 | order := out.CertificateOrder{ | 278 | order := out.CertificateOrder{ | ||
| 251 | ID: target.ID, | 279 | ID: target.ID, | ||
| @@ -255,27 +283,48 @@ func (s *Service) reconcileMissingTargets(ctx context.Context, missing []Certifi | |||||
| 255 | 283 | ||||
| 256 | stored, err := s.deps.Issuer.Obtain(ctx, order) | 284 | stored, err := s.deps.Issuer.Obtain(ctx, order) | ||
| 257 | if err != nil { | 285 | if err != nil { | ||
| 286 | + | errorMessage := fmt.Sprintf("obtain certificate: %v", err) | |||
| 258 | s.mu.Lock() | 287 | s.mu.Lock() | ||
| 259 | - | s.lastErr[target.ID] = fmt.Sprintf("obtain certificate: %v", err) | 288 | + | s.lastErr[target.ID] = errorMessage |
| 289 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |||
| 260 | s.mu.Unlock() | 290 | s.mu.Unlock() | ||
| 261 | continue | 291 | continue | ||
| 262 | } | 292 | } | ||
| 263 | if stored == nil { | 293 | if stored == nil { | ||
| 294 | + | errorMessage := "obtain certificate returned nil" | |||
| 264 | s.mu.Lock() | 295 | s.mu.Lock() | ||
| 265 | - | s.lastErr[target.ID] = "obtain certificate returned nil" | 296 | + | s.lastErr[target.ID] = errorMessage |
| 297 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |||
| 266 | s.mu.Unlock() | 298 | s.mu.Unlock() | ||
| 267 | continue | 299 | continue | ||
| 268 | } | 300 | } | ||
| 269 | if err := populateStoredCertificate(stored); err != nil { | 301 | if err := populateStoredCertificate(stored); err != nil { | ||
| 302 | + | errorMessage := err.Error() | |||
| 270 | s.mu.Lock() | 303 | s.mu.Lock() | ||
| 271 | - | s.lastErr[target.ID] = err.Error() | 304 | + | s.lastErr[target.ID] = errorMessage |
| 305 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |||
| 272 | s.mu.Unlock() | 306 | s.mu.Unlock() | ||
| 273 | continue | 307 | continue | ||
| 274 | } | 308 | } | ||
| 275 | 309 | ||||
| 276 | - | if err := s.deps.Store.Save(ctx, *stored); err != nil { | 310 | + | unlock, err := s.deps.Store.Lock(ctx) |
| 311 | + | if err != nil { | |||
| 312 | + | errorMessage := fmt.Sprintf("acquire store lock: %v", err) | |||
| 313 | + | s.mu.Lock() | |||
| 314 | + | s.lastErr[target.ID] = errorMessage | |||
| 315 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |||
| 316 | + | s.mu.Unlock() | |||
| 317 | + | continue | |||
| 318 | + | } | |||
| 319 | + | saveErr := s.deps.Store.Save(ctx, *stored) | |||
| 320 | + | if unlockErr := unlock(); unlockErr != nil { | |||
| 321 | + | log.Warn().Err(unlockErr).Msg("failed to release store lock") | |||
| 322 | + | } | |||
| 323 | + | if saveErr != nil { | |||
| 324 | + | errorMessage := fmt.Sprintf("save certificate: %v", saveErr) | |||
| 277 | s.mu.Lock() | 325 | s.mu.Lock() | ||
| 278 | - | s.lastErr[target.ID] = fmt.Sprintf("save certificate: %v", err) | 326 | + | s.lastErr[target.ID] = errorMessage |
| 327 | + | setTargetRouteErrorLocked(s.routeErr, required, target, errorMessage) | |||
| 279 | s.mu.Unlock() | 328 | s.mu.Unlock() | ||
| 280 | continue | 329 | continue | ||
| 281 | } | 330 | } | ||
| @@ -283,6 +332,7 @@ func (s *Service) reconcileMissingTargets(ctx context.Context, missing []Certifi | |||||
| 283 | s.mu.Lock() | 332 | s.mu.Lock() | ||
| 284 | s.certs[stored.ID] = stored | 333 | s.certs[stored.ID] = stored | ||
| 285 | delete(s.lastErr, stored.ID) | 334 | delete(s.lastErr, stored.ID) | ||
| 335 | + | clearTargetRouteErrorLocked(s.routeErr, required, target) | |||
| 286 | s.mu.Unlock() | 336 | s.mu.Unlock() | ||
| 287 | } | 337 | } | ||
| 288 | } | 338 | } | ||
| @@ -295,15 +345,15 @@ func (s *Service) certificateExistsLocked(target CertificateTarget) bool { | |||||
| 295 | if cert.NotAfter.IsZero() || now.After(cert.NotAfter) { | 345 | if cert.NotAfter.IsZero() || now.After(cert.NotAfter) { | ||
| 296 | continue // expired or no expiry | 346 | continue // expired or no expiry | ||
| 297 | } | 347 | } | ||
| 298 | - | if namesCover(cert.Names, target.Names) { | 348 | + | if exactNamesCoverAll(cert.Names, target.Names) { |
| 299 | return true | 349 | return true | ||
| 300 | } | 350 | } | ||
| 301 | } | 351 | } | ||
| 302 | return false | 352 | return false | ||
| 303 | } | 353 | } | ||
| 304 | 354 | ||||
| 305 | - | // namesCover reports whether names covers all required names (exact match, case-insensitive). | 355 | + | // exactNamesCoverAll reports whether names contains every required name exactly, case-insensitively. |
| 306 | - | func namesCover(names, required []string) bool { | 356 | + | func exactNamesCoverAll(names, required []string) bool { |
| 307 | covered := make(map[string]bool) | 357 | covered := make(map[string]bool) | ||
| 308 | for _, n := range names { | 358 | for _, n := range names { | ||
| 309 | covered[strings.ToLower(strings.TrimSuffix(n, "."))] = true | 359 | covered[strings.ToLower(strings.TrimSuffix(n, "."))] = true | ||
| @@ -378,9 +428,9 @@ func (s *Service) GetHTTP01Challenge(ctx context.Context, token string) (string, | |||||
| 378 | return s.deps.Challenges.Get(ctx, token) | 428 | return s.deps.Challenges.Get(ctx, token) | ||
| 379 | } | 429 | } | ||
| 380 | 430 | ||||
| 381 | - | // GetStoredCertificate returns a copy of the stored certificate for the given | 431 | + | // getStoredCertificate returns a copy of the stored certificate for the given |
| 382 | - | // ID, or nil if not found. Exposed for testing. | 432 | + | // ID, or nil if not found. |
| 383 | - | func (s *Service) GetStoredCertificate(id string) *out.StoredCertificate { | 433 | + | func (s *Service) getStoredCertificate(id string) *out.StoredCertificate { |
| 384 | s.mu.RLock() | 434 | s.mu.RLock() | ||
| 385 | defer s.mu.RUnlock() | 435 | defer s.mu.RUnlock() | ||
| 386 | cert := s.certs[id] | 436 | cert := s.certs[id] | ||
M internal/usecase/publictls/service_test.go +69 -1
| @@ -542,7 +542,7 @@ func TestServiceLoadParsesPEMIntoTLSCertificate(t *testing.T) { | |||
| 542 | 542 | require.NoError(t, err) | |
| 543 | 543 | ||
| 544 | 544 | // Verify the cached cert has a valid tls.Certificate via the accessor. | |
| 545 | - | cached := svc.GetStoredCertificate("http01-test.example.com") | |
| 545 | + | cached := svc.getStoredCertificate("http01-test.example.com") | |
| 546 | 546 | require.NotNil(t, cached, "cert should be cached") | |
| 547 | 547 | assert.NotEmpty(t, cached.Certificate.Certificate, "tls.Certificate should be populated") | |
| 548 | 548 | assert.NotNil(t, cached.Certificate.PrivateKey, "private key should be populated") | |
| @@ -551,6 +551,74 @@ func TestServiceLoadParsesPEMIntoTLSCertificate(t *testing.T) { | |||
| 551 | 551 | leaf, err := x509.ParseCertificate(cached.Certificate.Certificate[0]) | |
| 552 | 552 | require.NoError(t, err) | |
| 553 | 553 | assert.Contains(t, leaf.DNSNames, "test.example.com") | |
| 554 | + | ||
| 555 | + | served, err := svc.GetCertificateForHost("test.example.com") | |
| 556 | + | require.NoError(t, err) | |
| 557 | + | require.NotNil(t, served, "loaded certificates should seed required hosts") | |
| 558 | + | } | |
| 559 | + | ||
| 560 | + | func TestServiceStatusReportsRouteErrorAfterObtainFailure(t *testing.T) { | |
| 561 | + | ctx := context.Background() | |
| 562 | + | routes := &fakeRoutes{routes: []domain.Route{{Domain: "app.example.com"}}} | |
| 563 | + | issuer, _ := newMockPublicCertificateIssuer(t, func(_ context.Context, _ out.CertificateOrder) (*out.StoredCertificate, error) { | |
| 564 | + | return nil, fmt.Errorf("acme unavailable token=sk-secret") | |
| 565 | + | }, nil) | |
| 566 | + | store, _ := newMockCertificateStore(t) | |
| 567 | + | cfg := Config{ | |
| 568 | + | Enabled: true, | |
| 569 | + | Email: "admin@example.com", | |
| 570 | + | Challenge: "http-01", | |
| 571 | + | HTTPPort: 8088, | |
| 572 | + | TLSPort: 8443, | |
| 573 | + | } | |
| 574 | + | svc := NewService(cfg, ServiceDeps{ | |
| 575 | + | Config: cfg, | |
| 576 | + | Routes: routes, | |
| 577 | + | Issuer: issuer, | |
| 578 | + | Store: store, | |
| 579 | + | Effective: EffectiveChallenge{ | |
| 580 | + | ConfiguredMode: domain.ACMEChallengeHTTP01, | |
| 581 | + | Mode: domain.ACMEChallengeHTTP01, | |
| 582 | + | TokenSource: domain.ACMETokenSourceNone, | |
| 583 | + | }, | |
| 584 | + | }) | |
| 585 | + | ||
| 586 | + | require.NoError(t, svc.Reconcile(ctx)) | |
| 587 | + | status := svc.Status(ctx) | |
| 588 | + | require.Len(t, status.Routes, 1) | |
| 589 | + | assert.False(t, status.Routes[0].Covered) | |
| 590 | + | assert.Contains(t, status.Routes[0].Error, "token=redacted") | |
| 591 | + | assert.NotContains(t, status.Routes[0].Error, "sk-secret") | |
| 592 | + | } | |
| 593 | + | ||
| 594 | + | func TestServiceStatusCoverageUsesServingCertDespiteTransientError(t *testing.T) { | |
| 595 | + | ctx := context.Background() | |
| 596 | + | certPEM, keyPEM, err := generateTestCertPEM([]string{"app.example.com"}) | |
| 597 | + | require.NoError(t, err) | |
| 598 | + | tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) | |
| 599 | + | require.NoError(t, err) | |
| 600 | + | store, _ := newMockCertificateStore(t, out.StoredCertificate{ | |
| 601 | + | ID: "http01-app.example.com", | |
| 602 | + | Names: []string{"app.example.com"}, | |
| 603 | + | Challenge: domain.ACMEChallengeHTTP01, | |
| 604 | + | Certificate: tlsCert, | |
| 605 | + | FullchainPEM: certPEM, | |
| 606 | + | PrivateKeyPEM: keyPEM, | |
| 607 | + | NotAfter: time.Now().Add(90 * 24 * time.Hour), | |
| 608 | + | LastError: "transient renewal error", | |
| 609 | + | }) | |
| 610 | + | cfg := Config{Enabled: true} | |
| 611 | + | svc := NewService(cfg, ServiceDeps{ | |
| 612 | + | Config: cfg, | |
| 613 | + | Routes: &fakeRoutes{routes: []domain.Route{{Domain: "app.example.com"}}}, | |
| 614 | + | Store: store, | |
| 615 | + | }) | |
| 616 | + | require.NoError(t, svc.Load(ctx)) | |
| 617 | + | ||
| 618 | + | status := svc.Status(ctx) | |
| 619 | + | require.Len(t, status.Routes, 1) | |
| 620 | + | assert.True(t, status.Routes[0].Covered) | |
| 621 | + | assert.Equal(t, "http01-app.example.com", status.Routes[0].CoveredBy) | |
| 554 | 622 | } | |
| 555 | 623 | ||
| 556 | 624 | func TestServiceStatusRedactsSensitiveStrings(t *testing.T) { | |
| @@ -542,7 +542,7 @@ func TestServiceLoadParsesPEMIntoTLSCertificate(t *testing.T) { | |||||
| 542 | require.NoError(t, err) | 542 | require.NoError(t, err) | ||
| 543 | 543 | ||||
| 544 | // Verify the cached cert has a valid tls.Certificate via the accessor. | 544 | // Verify the cached cert has a valid tls.Certificate via the accessor. | ||
| 545 | - | cached := svc.GetStoredCertificate("http01-test.example.com") | 545 | + | cached := svc.getStoredCertificate("http01-test.example.com") |
| 546 | require.NotNil(t, cached, "cert should be cached") | 546 | require.NotNil(t, cached, "cert should be cached") | ||
| 547 | assert.NotEmpty(t, cached.Certificate.Certificate, "tls.Certificate should be populated") | 547 | assert.NotEmpty(t, cached.Certificate.Certificate, "tls.Certificate should be populated") | ||
| 548 | assert.NotNil(t, cached.Certificate.PrivateKey, "private key should be populated") | 548 | assert.NotNil(t, cached.Certificate.PrivateKey, "private key should be populated") | ||
| @@ -551,6 +551,74 @@ func TestServiceLoadParsesPEMIntoTLSCertificate(t *testing.T) { | |||||
| 551 | leaf, err := x509.ParseCertificate(cached.Certificate.Certificate[0]) | 551 | leaf, err := x509.ParseCertificate(cached.Certificate.Certificate[0]) | ||
| 552 | require.NoError(t, err) | 552 | require.NoError(t, err) | ||
| 553 | assert.Contains(t, leaf.DNSNames, "test.example.com") | 553 | assert.Contains(t, leaf.DNSNames, "test.example.com") | ||
| 554 | + | ||||
| 555 | + | served, err := svc.GetCertificateForHost("test.example.com") | |||
| 556 | + | require.NoError(t, err) | |||
| 557 | + | require.NotNil(t, served, "loaded certificates should seed required hosts") | |||
| 558 | + | } | |||
| 559 | + | ||||
| 560 | + | func TestServiceStatusReportsRouteErrorAfterObtainFailure(t *testing.T) { | |||
| 561 | + | ctx := context.Background() | |||
| 562 | + | routes := &fakeRoutes{routes: []domain.Route{{Domain: "app.example.com"}}} | |||
| 563 | + | issuer, _ := newMockPublicCertificateIssuer(t, func(_ context.Context, _ out.CertificateOrder) (*out.StoredCertificate, error) { | |||
| 564 | + | return nil, fmt.Errorf("acme unavailable token=sk-secret") | |||
| 565 | + | }, nil) | |||
| 566 | + | store, _ := newMockCertificateStore(t) | |||
| 567 | + | cfg := Config{ | |||
| 568 | + | Enabled: true, | |||
| 569 | + | Email: "admin@example.com", | |||
| 570 | + | Challenge: "http-01", | |||
| 571 | + | HTTPPort: 8088, | |||
| 572 | + | TLSPort: 8443, | |||
| 573 | + | } | |||
| 574 | + | svc := NewService(cfg, ServiceDeps{ | |||
| 575 | + | Config: cfg, | |||
| 576 | + | Routes: routes, | |||
| 577 | + | Issuer: issuer, | |||
| 578 | + | Store: store, | |||
| 579 | + | Effective: EffectiveChallenge{ | |||
| 580 | + | ConfiguredMode: domain.ACMEChallengeHTTP01, | |||
| 581 | + | Mode: domain.ACMEChallengeHTTP01, | |||
| 582 | + | TokenSource: domain.ACMETokenSourceNone, | |||
| 583 | + | }, | |||
| 584 | + | }) | |||
| 585 | + | ||||
| 586 | + | require.NoError(t, svc.Reconcile(ctx)) | |||
| 587 | + | status := svc.Status(ctx) | |||
| 588 | + | require.Len(t, status.Routes, 1) | |||
| 589 | + | assert.False(t, status.Routes[0].Covered) | |||
| 590 | + | assert.Contains(t, status.Routes[0].Error, "token=redacted") | |||
| 591 | + | assert.NotContains(t, status.Routes[0].Error, "sk-secret") | |||
| 592 | + | } | |||
| 593 | + | ||||
| 594 | + | func TestServiceStatusCoverageUsesServingCertDespiteTransientError(t *testing.T) { | |||
| 595 | + | ctx := context.Background() | |||
| 596 | + | certPEM, keyPEM, err := generateTestCertPEM([]string{"app.example.com"}) | |||
| 597 | + | require.NoError(t, err) | |||
| 598 | + | tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) | |||
| 599 | + | require.NoError(t, err) | |||
| 600 | + | store, _ := newMockCertificateStore(t, out.StoredCertificate{ | |||
| 601 | + | ID: "http01-app.example.com", | |||
| 602 | + | Names: []string{"app.example.com"}, | |||
| 603 | + | Challenge: domain.ACMEChallengeHTTP01, | |||
| 604 | + | Certificate: tlsCert, | |||
| 605 | + | FullchainPEM: certPEM, | |||
| 606 | + | PrivateKeyPEM: keyPEM, | |||
| 607 | + | NotAfter: time.Now().Add(90 * 24 * time.Hour), | |||
| 608 | + | LastError: "transient renewal error", | |||
| 609 | + | }) | |||
| 610 | + | cfg := Config{Enabled: true} | |||
| 611 | + | svc := NewService(cfg, ServiceDeps{ | |||
| 612 | + | Config: cfg, | |||
| 613 | + | Routes: &fakeRoutes{routes: []domain.Route{{Domain: "app.example.com"}}}, | |||
| 614 | + | Store: store, | |||
| 615 | + | }) | |||
| 616 | + | require.NoError(t, svc.Load(ctx)) | |||
| 617 | + | ||||
| 618 | + | status := svc.Status(ctx) | |||
| 619 | + | require.Len(t, status.Routes, 1) | |||
| 620 | + | assert.True(t, status.Routes[0].Covered) | |||
| 621 | + | assert.Equal(t, "http01-app.example.com", status.Routes[0].CoveredBy) | |||
| 554 | } | 622 | } | ||
| 555 | 623 | ||||
| 556 | func TestServiceStatusRedactsSensitiveStrings(t *testing.T) { | 624 | func TestServiceStatusRedactsSensitiveStrings(t *testing.T) { | ||
M internal/usecase/publictls/status.go +25 -1
| @@ -31,6 +31,7 @@ func (s *Service) Status(ctx context.Context) domain.PublicTLSStatus { | |||
| 31 | 31 | ||
| 32 | 32 | status.Certificates = s.buildManagedCertificatesLocked() | |
| 33 | 33 | status.Routes = s.buildRouteCoverageLocked(routeDomains, status.Certificates) | |
| 34 | + | status.Errors = buildStatusErrors(status.Certificates, status.Routes) | |
| 34 | 35 | return status | |
| 35 | 36 | } | |
| 36 | 37 | ||
| @@ -60,6 +61,29 @@ func (s *Service) buildManagedCertificatesLocked() []domain.ManagedCertificate { | |||
| 60 | 61 | return certs | |
| 61 | 62 | } | |
| 62 | 63 | ||
| 64 | + | func buildStatusErrors(certs []domain.ManagedCertificate, routes []domain.TLSRouteCoverage) []string { | |
| 65 | + | seen := make(map[string]struct{}) | |
| 66 | + | var errorsList []string | |
| 67 | + | add := func(message string) { | |
| 68 | + | message = sanitizeError(message) | |
| 69 | + | if message == "" { | |
| 70 | + | return | |
| 71 | + | } | |
| 72 | + | if _, ok := seen[message]; ok { | |
| 73 | + | return | |
| 74 | + | } | |
| 75 | + | seen[message] = struct{}{} | |
| 76 | + | errorsList = append(errorsList, message) | |
| 77 | + | } | |
| 78 | + | for _, cert := range certs { | |
| 79 | + | add(cert.LastError) | |
| 80 | + | } | |
| 81 | + | for _, route := range routes { | |
| 82 | + | add(route.Error) | |
| 83 | + | } | |
| 84 | + | return errorsList | |
| 85 | + | } | |
| 86 | + | ||
| 63 | 87 | // buildRouteCoverageLocked returns route coverage for the given domains. | |
| 64 | 88 | // Must be called with s.mu held. | |
| 65 | 89 | func (s *Service) buildRouteCoverageLocked(routeDomains []string, managedCerts []domain.ManagedCertificate) []domain.TLSRouteCoverage { | |
| @@ -116,7 +140,7 @@ func (s *Service) findCoverageLocked(routeDomain string, managedCerts []domain.M | |||
| 116 | 140 | continue | |
| 117 | 141 | } | |
| 118 | 142 | switch mc.Status { | |
| 119 | - | case domain.TLSCertificateStatusExpired, domain.TLSCertificateStatusMissing, domain.TLSCertificateStatusError: | |
| 143 | + | case domain.TLSCertificateStatusExpired, domain.TLSCertificateStatusMissing: | |
| 120 | 144 | continue | |
| 121 | 145 | } | |
| 122 | 146 | // Double-check the raw stored cert expiry to guard against stale | |
| @@ -31,6 +31,7 @@ func (s *Service) Status(ctx context.Context) domain.PublicTLSStatus { | |||||
| 31 | 31 | ||||
| 32 | status.Certificates = s.buildManagedCertificatesLocked() | 32 | status.Certificates = s.buildManagedCertificatesLocked() | ||
| 33 | status.Routes = s.buildRouteCoverageLocked(routeDomains, status.Certificates) | 33 | status.Routes = s.buildRouteCoverageLocked(routeDomains, status.Certificates) | ||
| 34 | + | status.Errors = buildStatusErrors(status.Certificates, status.Routes) | |||
| 34 | return status | 35 | return status | ||
| 35 | } | 36 | } | ||
| 36 | 37 | ||||
| @@ -60,6 +61,29 @@ func (s *Service) buildManagedCertificatesLocked() []domain.ManagedCertificate { | |||||
| 60 | return certs | 61 | return certs | ||
| 61 | } | 62 | } | ||
| 62 | 63 | ||||
| 64 | + | func buildStatusErrors(certs []domain.ManagedCertificate, routes []domain.TLSRouteCoverage) []string { | |||
| 65 | + | seen := make(map[string]struct{}) | |||
| 66 | + | var errorsList []string | |||
| 67 | + | add := func(message string) { | |||
| 68 | + | message = sanitizeError(message) | |||
| 69 | + | if message == "" { | |||
| 70 | + | return | |||
| 71 | + | } | |||
| 72 | + | if _, ok := seen[message]; ok { | |||
| 73 | + | return | |||
| 74 | + | } | |||
| 75 | + | seen[message] = struct{}{} | |||
| 76 | + | errorsList = append(errorsList, message) | |||
| 77 | + | } | |||
| 78 | + | for _, cert := range certs { | |||
| 79 | + | add(cert.LastError) | |||
| 80 | + | } | |||
| 81 | + | for _, route := range routes { | |||
| 82 | + | add(route.Error) | |||
| 83 | + | } | |||
| 84 | + | return errorsList | |||
| 85 | + | } | |||
| 86 | + | ||||
| 63 | // buildRouteCoverageLocked returns route coverage for the given domains. | 87 | // buildRouteCoverageLocked returns route coverage for the given domains. | ||
| 64 | // Must be called with s.mu held. | 88 | // Must be called with s.mu held. | ||
| 65 | func (s *Service) buildRouteCoverageLocked(routeDomains []string, managedCerts []domain.ManagedCertificate) []domain.TLSRouteCoverage { | 89 | func (s *Service) buildRouteCoverageLocked(routeDomains []string, managedCerts []domain.ManagedCertificate) []domain.TLSRouteCoverage { | ||
| @@ -116,7 +140,7 @@ func (s *Service) findCoverageLocked(routeDomain string, managedCerts []domain.M | |||||
| 116 | continue | 140 | continue | ||
| 117 | } | 141 | } | ||
| 118 | switch mc.Status { | 142 | switch mc.Status { | ||
| 119 | - | case domain.TLSCertificateStatusExpired, domain.TLSCertificateStatusMissing, domain.TLSCertificateStatusError: | 143 | + | case domain.TLSCertificateStatusExpired, domain.TLSCertificateStatusMissing: |
| 120 | continue | 144 | continue | ||
| 121 | } | 145 | } | ||
| 122 | // Double-check the raw stored cert expiry to guard against stale | 146 | // Double-check the raw stored cert expiry to guard against stale | ||