fix: address all findings from deep code review
CRITICAL: - GetCertificate now returns (nil, nil) for unrouted domains, allowing Go's TLS stack to fall back to static certs in tls.Config.Certificates HIGH: - Add singleflight to deduplicate concurrent cert issuance for the same domain on cache miss - Remove pkiadapter import from onboarding handler (layer violation); mobileconfig is now pre-computed in run.go - CLI ca.go uses shared cliLogger() instead of ad-hoc zerowrap.Default() MEDIUM: - HTTPSRedirect validates RequestURI starts with / (open redirect fix) and uses 308 instead of 301 (preserves HTTP method) - Onboarding normalizeHTTPSURL validates https:// scheme - Leaf cert NotAfter capped to intermediate expiry - sweepExpiredCerts threads parent context for shutdown cancellation - sweepExpiredCerts fetches routes once instead of per-entry - Remove dead SetAIAURL code LOW: - Clearer error for non-ECDSA keys in parseCertAndKey - Log warning when intermediate cert/key files are partially present - Consistent key-first write ordering in bootstrapRoot - Document dual-mutex lock ordering (renewMu → mu) - Comment explaining inline HTML and JS cookie decisions
- commit
- 67e87d28181b2e26cedbb2240bd4d93a45022074
- author
- brice · · 2026-03-31 03:43 UTC
- parent
- a6947b2
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
10 files · +133 -84
M internal/adapters/in/cli/ca.go +4 -3
| @@ -10,7 +10,6 @@ import ( | |||
| 10 | 10 | "path/filepath" | |
| 11 | 11 | "time" | |
| 12 | 12 | ||
| 13 | - | "github.com/bnema/zerowrap" | |
| 14 | 13 | "github.com/spf13/cobra" | |
| 15 | 14 | ||
| 16 | 15 | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | |
| @@ -103,9 +102,11 @@ func resolveCADataDir() (string, error) { | |||
| 103 | 102 | return local.GetDataDir(), nil | |
| 104 | 103 | } | |
| 105 | 104 | ||
| 105 | + | // loadCAFromDataDir creates a CA adapter directly (bypassing the usecase layer). | |
| 106 | + | // This is intentional: CLI commands like 'ca export' and 'ca info' are standalone | |
| 107 | + | // admin utilities that run outside the server lifecycle and don't need business logic. | |
| 106 | 108 | func loadCAFromDataDir(dataDir string) (*pkiadapter.CA, error) { | |
| 107 | - | log := zerowrap.Default() | |
| 108 | - | ca, err := pkiadapter.NewCA(dataDir, log) | |
| 109 | + | ca, err := pkiadapter.NewCA(dataDir, cliLogger()) | |
| 109 | 110 | if err != nil { | |
| 110 | 111 | return nil, fmt.Errorf("failed to load CA from %s: %w", dataDir, err) | |
| 111 | 112 | } | |
| @@ -10,7 +10,6 @@ import ( | |||||
| 10 | "path/filepath" | 10 | "path/filepath" | ||
| 11 | "time" | 11 | "time" | ||
| 12 | 12 | ||||
| 13 | - | "github.com/bnema/zerowrap" | |||
| 14 | "github.com/spf13/cobra" | 13 | "github.com/spf13/cobra" | ||
| 15 | 14 | ||||
| 16 | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | 15 | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | ||
| @@ -103,9 +102,11 @@ func resolveCADataDir() (string, error) { | |||||
| 103 | return local.GetDataDir(), nil | 102 | return local.GetDataDir(), nil | ||
| 104 | } | 103 | } | ||
| 105 | 104 | ||||
| 105 | + | // loadCAFromDataDir creates a CA adapter directly (bypassing the usecase layer). | |||
| 106 | + | // This is intentional: CLI commands like 'ca export' and 'ca info' are standalone | |||
| 107 | + | // admin utilities that run outside the server lifecycle and don't need business logic. | |||
| 106 | func loadCAFromDataDir(dataDir string) (*pkiadapter.CA, error) { | 108 | func loadCAFromDataDir(dataDir string) (*pkiadapter.CA, error) { | ||
| 107 | - | log := zerowrap.Default() | 109 | + | ca, err := pkiadapter.NewCA(dataDir, cliLogger()) |
| 108 | - | ca, err := pkiadapter.NewCA(dataDir, log) | |||
| 109 | if err != nil { | 110 | if err != nil { | ||
| 110 | return nil, fmt.Errorf("failed to load CA from %s: %w", dataDir, err) | 111 | return nil, fmt.Errorf("failed to load CA from %s: %w", dataDir, err) | ||
| 111 | } | 112 | } | ||
M internal/adapters/in/cli/controlplane_resolver.go +1 -3
| @@ -3,8 +3,6 @@ package cli | |||
| 3 | 3 | import ( | |
| 4 | 4 | "fmt" | |
| 5 | 5 | ||
| 6 | - | "github.com/bnema/zerowrap" | |
| 7 | - | ||
| 8 | 6 | "github.com/bnema/gordon/internal/app" | |
| 9 | 7 | ) | |
| 10 | 8 | ||
| @@ -19,7 +17,7 @@ func (h *controlPlaneHandle) close() { | |||
| 19 | 17 | return | |
| 20 | 18 | } | |
| 21 | 19 | if err := h.closeFn(); err != nil { | |
| 22 | - | log := zerowrap.Default() | |
| 20 | + | log := cliLogger() | |
| 23 | 21 | log.Warn().Err(err).Msg("failed to close control-plane resources") | |
| 24 | 22 | } | |
| 25 | 23 | } | |
| @@ -3,8 +3,6 @@ package cli | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "fmt" | 4 | "fmt" | ||
| 5 | 5 | ||||
| 6 | - | "github.com/bnema/zerowrap" | |||
| 7 | - | ||||
| 8 | "github.com/bnema/gordon/internal/app" | 6 | "github.com/bnema/gordon/internal/app" | ||
| 9 | ) | 7 | ) | ||
| 10 | 8 | ||||
| @@ -19,7 +17,7 @@ func (h *controlPlaneHandle) close() { | |||||
| 19 | return | 17 | return | ||
| 20 | } | 18 | } | ||
| 21 | if err := h.closeFn(); err != nil { | 19 | if err := h.closeFn(); err != nil { | ||
| 22 | - | log := zerowrap.Default() | 20 | + | log := cliLogger() |
| 23 | log.Warn().Err(err).Msg("failed to close control-plane resources") | 21 | log.Warn().Err(err).Msg("failed to close control-plane resources") | ||
| 24 | } | 22 | } | ||
| 25 | } | 23 | } | ||
M internal/adapters/in/cli/presentation.go +12 -0
| @@ -4,6 +4,8 @@ import ( | |||
| 4 | 4 | "fmt" | |
| 5 | 5 | "io" | |
| 6 | 6 | ||
| 7 | + | "github.com/bnema/zerowrap" | |
| 8 | + | ||
| 7 | 9 | "github.com/bnema/gordon/internal/adapters/in/cli/ui/styles" | |
| 8 | 10 | ) | |
| 9 | 11 | ||
| @@ -17,6 +19,16 @@ var cliWritef = func(w io.Writer, format string, args ...any) error { | |||
| 17 | 19 | return err | |
| 18 | 20 | } | |
| 19 | 21 | ||
| 22 | + | // cliLogger returns a quiet logger for CLI commands that run outside the | |
| 23 | + | // server lifecycle. Level "warn" keeps normal output clean while still | |
| 24 | + | // surfacing actionable warnings from adapters. | |
| 25 | + | func cliLogger() zerowrap.Logger { | |
| 26 | + | return zerowrap.New(zerowrap.Config{ | |
| 27 | + | Level: "warn", | |
| 28 | + | Format: "console", | |
| 29 | + | }) | |
| 30 | + | } | |
| 31 | + | ||
| 20 | 32 | func cliRenderTitle(msg string) string { | |
| 21 | 33 | return styles.Theme.Title.Render(msg) | |
| 22 | 34 | } | |
| @@ -4,6 +4,8 @@ import ( | |||||
| 4 | "fmt" | 4 | "fmt" | ||
| 5 | "io" | 5 | "io" | ||
| 6 | 6 | ||||
| 7 | + | "github.com/bnema/zerowrap" | |||
| 8 | + | ||||
| 7 | "github.com/bnema/gordon/internal/adapters/in/cli/ui/styles" | 9 | "github.com/bnema/gordon/internal/adapters/in/cli/ui/styles" | ||
| 8 | ) | 10 | ) | ||
| 9 | 11 | ||||
| @@ -17,6 +19,16 @@ var cliWritef = func(w io.Writer, format string, args ...any) error { | |||||
| 17 | return err | 19 | return err | ||
| 18 | } | 20 | } | ||
| 19 | 21 | ||||
| 22 | + | // cliLogger returns a quiet logger for CLI commands that run outside the | |||
| 23 | + | // server lifecycle. Level "warn" keeps normal output clean while still | |||
| 24 | + | // surfacing actionable warnings from adapters. | |||
| 25 | + | func cliLogger() zerowrap.Logger { | |||
| 26 | + | return zerowrap.New(zerowrap.Config{ | |||
| 27 | + | Level: "warn", | |||
| 28 | + | Format: "console", | |||
| 29 | + | }) | |||
| 30 | + | } | |||
| 31 | + | ||||
| 20 | func cliRenderTitle(msg string) string { | 32 | func cliRenderTitle(msg string) string { | ||
| 21 | return styles.Theme.Title.Render(msg) | 33 | return styles.Theme.Title.Render(msg) | ||
| 22 | } | 34 | } | ||
M internal/adapters/in/http/middleware/cidr.go +18 -11
| @@ -5,6 +5,7 @@ import ( | |||
| 5 | 5 | "fmt" | |
| 6 | 6 | "net" | |
| 7 | 7 | "net/http" | |
| 8 | + | "strings" | |
| 8 | 9 | ||
| 9 | 10 | "github.com/bnema/zerowrap" | |
| 10 | 11 | ||
| @@ -49,6 +50,15 @@ func cidrAllowlist(allowedNets []*net.IPNet, ipExtractor func(*http.Request) str | |||
| 49 | 50 | } | |
| 50 | 51 | } | |
| 51 | 52 | ||
| 53 | + | // extractRemoteIP returns the IP portion of r.RemoteAddr, stripping the port. | |
| 54 | + | func extractRemoteIP(remoteAddr string) string { | |
| 55 | + | host, _, err := net.SplitHostPort(remoteAddr) | |
| 56 | + | if err != nil { | |
| 57 | + | return remoteAddr | |
| 58 | + | } | |
| 59 | + | return host | |
| 60 | + | } | |
| 61 | + | ||
| 52 | 62 | // RegistryCIDRAllowlist returns middleware that restricts access to the given CIDR ranges. | |
| 53 | 63 | // Localhost is always allowed so Gordon can pull from its own registry. | |
| 54 | 64 | // An empty allowedNets slice is a no-op (all traffic passes through). | |
| @@ -74,10 +84,7 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||
| 74 | 84 | } | |
| 75 | 85 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 76 | 86 | if !forceAll { | |
| 77 | - | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) | |
| 78 | - | if err != nil { | |
| 79 | - | remoteIP = r.RemoteAddr | |
| 80 | - | } | |
| 87 | + | remoteIP := extractRemoteIP(r.RemoteAddr) | |
| 81 | 88 | if IsTrustedProxy(remoteIP, localhostNets) || IsTrustedProxy(remoteIP, proxyNets) { | |
| 82 | 89 | next.ServeHTTP(w, r) | |
| 83 | 90 | return | |
| @@ -88,13 +95,17 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||
| 88 | 95 | if h, _, err := net.SplitHostPort(host); err == nil { | |
| 89 | 96 | host = h | |
| 90 | 97 | } | |
| 91 | - | target := fmt.Sprintf("https://%s:%d%s", host, tlsPort, r.RequestURI) | |
| 98 | + | path := r.RequestURI | |
| 99 | + | if !strings.HasPrefix(path, "/") { | |
| 100 | + | path = "/" | |
| 101 | + | } | |
| 102 | + | target := fmt.Sprintf("https://%s:%d%s", host, tlsPort, path) | |
| 92 | 103 | ||
| 93 | 104 | log.Debug(). | |
| 94 | 105 | Str("target", target). | |
| 95 | 106 | Msg("redirecting HTTP client to HTTPS") | |
| 96 | 107 | ||
| 97 | - | http.Redirect(w, r, target, http.StatusMovedPermanently) | |
| 108 | + | http.Redirect(w, r, target, http.StatusPermanentRedirect) | |
| 98 | 109 | }) | |
| 99 | 110 | } | |
| 100 | 111 | } | |
| @@ -105,10 +116,6 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||
| 105 | 116 | // An empty allowedNets slice is a no-op (all traffic passes through). | |
| 106 | 117 | func ProxyCIDRAllowlist(allowedNets []*net.IPNet, log zerowrap.Logger) func(http.Handler) http.Handler { | |
| 107 | 118 | return cidrAllowlist(allowedNets, func(r *http.Request) string { | |
| 108 | - | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) | |
| 109 | - | if err != nil { | |
| 110 | - | return r.RemoteAddr | |
| 111 | - | } | |
| 112 | - | return remoteIP | |
| 119 | + | return extractRemoteIP(r.RemoteAddr) | |
| 113 | 120 | }, "proxy origin", log) | |
| 114 | 121 | } | |
| @@ -5,6 +5,7 @@ import ( | |||||
| 5 | "fmt" | 5 | "fmt" | ||
| 6 | "net" | 6 | "net" | ||
| 7 | "net/http" | 7 | "net/http" | ||
| 8 | + | "strings" | |||
| 8 | 9 | ||||
| 9 | "github.com/bnema/zerowrap" | 10 | "github.com/bnema/zerowrap" | ||
| 10 | 11 | ||||
| @@ -49,6 +50,15 @@ func cidrAllowlist(allowedNets []*net.IPNet, ipExtractor func(*http.Request) str | |||||
| 49 | } | 50 | } | ||
| 50 | } | 51 | } | ||
| 51 | 52 | ||||
| 53 | + | // extractRemoteIP returns the IP portion of r.RemoteAddr, stripping the port. | |||
| 54 | + | func extractRemoteIP(remoteAddr string) string { | |||
| 55 | + | host, _, err := net.SplitHostPort(remoteAddr) | |||
| 56 | + | if err != nil { | |||
| 57 | + | return remoteAddr | |||
| 58 | + | } | |||
| 59 | + | return host | |||
| 60 | + | } | |||
| 61 | + | ||||
| 52 | // RegistryCIDRAllowlist returns middleware that restricts access to the given CIDR ranges. | 62 | // RegistryCIDRAllowlist returns middleware that restricts access to the given CIDR ranges. | ||
| 53 | // Localhost is always allowed so Gordon can pull from its own registry. | 63 | // Localhost is always allowed so Gordon can pull from its own registry. | ||
| 54 | // An empty allowedNets slice is a no-op (all traffic passes through). | 64 | // An empty allowedNets slice is a no-op (all traffic passes through). | ||
| @@ -74,10 +84,7 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||||
| 74 | } | 84 | } | ||
| 75 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 85 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 76 | if !forceAll { | 86 | if !forceAll { | ||
| 77 | - | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) | 87 | + | remoteIP := extractRemoteIP(r.RemoteAddr) |
| 78 | - | if err != nil { | |||
| 79 | - | remoteIP = r.RemoteAddr | |||
| 80 | - | } | |||
| 81 | if IsTrustedProxy(remoteIP, localhostNets) || IsTrustedProxy(remoteIP, proxyNets) { | 88 | if IsTrustedProxy(remoteIP, localhostNets) || IsTrustedProxy(remoteIP, proxyNets) { | ||
| 82 | next.ServeHTTP(w, r) | 89 | next.ServeHTTP(w, r) | ||
| 83 | return | 90 | return | ||
| @@ -88,13 +95,17 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||||
| 88 | if h, _, err := net.SplitHostPort(host); err == nil { | 95 | if h, _, err := net.SplitHostPort(host); err == nil { | ||
| 89 | host = h | 96 | host = h | ||
| 90 | } | 97 | } | ||
| 91 | - | target := fmt.Sprintf("https://%s:%d%s", host, tlsPort, r.RequestURI) | 98 | + | path := r.RequestURI |
| 99 | + | if !strings.HasPrefix(path, "/") { | |||
| 100 | + | path = "/" | |||
| 101 | + | } | |||
| 102 | + | target := fmt.Sprintf("https://%s:%d%s", host, tlsPort, path) | |||
| 92 | 103 | ||||
| 93 | log.Debug(). | 104 | log.Debug(). | ||
| 94 | Str("target", target). | 105 | Str("target", target). | ||
| 95 | Msg("redirecting HTTP client to HTTPS") | 106 | Msg("redirecting HTTP client to HTTPS") | ||
| 96 | 107 | ||||
| 97 | - | http.Redirect(w, r, target, http.StatusMovedPermanently) | 108 | + | http.Redirect(w, r, target, http.StatusPermanentRedirect) |
| 98 | }) | 109 | }) | ||
| 99 | } | 110 | } | ||
| 100 | } | 111 | } | ||
| @@ -105,10 +116,6 @@ func HTTPSRedirect(proxyNets []*net.IPNet, tlsPort int, forceAll bool, log zerow | |||||
| 105 | // An empty allowedNets slice is a no-op (all traffic passes through). | 116 | // An empty allowedNets slice is a no-op (all traffic passes through). | ||
| 106 | func ProxyCIDRAllowlist(allowedNets []*net.IPNet, log zerowrap.Logger) func(http.Handler) http.Handler { | 117 | func ProxyCIDRAllowlist(allowedNets []*net.IPNet, log zerowrap.Logger) func(http.Handler) http.Handler { | ||
| 107 | return cidrAllowlist(allowedNets, func(r *http.Request) string { | 118 | return cidrAllowlist(allowedNets, func(r *http.Request) string { | ||
| 108 | - | remoteIP, _, err := net.SplitHostPort(r.RemoteAddr) | 119 | + | return extractRemoteIP(r.RemoteAddr) |
| 109 | - | if err != nil { | |||
| 110 | - | return r.RemoteAddr | |||
| 111 | - | } | |||
| 112 | - | return remoteIP | |||
| 113 | }, "proxy origin", log) | 120 | }, "proxy origin", log) | ||
| 114 | } | 121 | } | ||
M internal/adapters/in/http/onboarding/handler.go +11 -5
| @@ -5,8 +5,7 @@ import ( | |||
| 5 | 5 | "html" | |
| 6 | 6 | "net" | |
| 7 | 7 | "net/http" | |
| 8 | - | ||
| 9 | - | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | |
| 8 | + | "strings" | |
| 10 | 9 | ) | |
| 11 | 10 | ||
| 12 | 11 | // Handler serves CA onboarding endpoints on the TLS port for direct/Tailnet clients. | |
| @@ -18,10 +17,10 @@ type Handler struct { | |||
| 18 | 17 | ||
| 19 | 18 | // NewHandler creates an onboarding handler. | |
| 20 | 19 | // tlsPort is the HTTPS port used to build redirect URLs on the onboarding page. | |
| 21 | - | func NewHandler(rootPEM, rootDER []byte, rootCN string, tlsPort int) *Handler { | |
| 20 | + | func NewHandler(rootPEM, mobileconfig []byte, tlsPort int) *Handler { | |
| 22 | 21 | return &Handler{ | |
| 23 | 22 | rootPEM: rootPEM, | |
| 24 | - | mobileconfig: pkiadapter.GenerateMobileconfig(rootDER, rootCN), | |
| 23 | + | mobileconfig: mobileconfig, | |
| 25 | 24 | tlsPort: tlsPort, | |
| 26 | 25 | } | |
| 27 | 26 | } | |
| @@ -51,15 +50,22 @@ func (h *Handler) normalizeHTTPSURL(r *http.Request) string { | |||
| 51 | 50 | if h.tlsPort != 0 && h.tlsPort != 443 { | |
| 52 | 51 | hostname = fmt.Sprintf("%s:%d", hostname, h.tlsPort) | |
| 53 | 52 | } | |
| 54 | - | return html.EscapeString(fmt.Sprintf("https://%s/", hostname)) | |
| 53 | + | u := fmt.Sprintf("https://%s/", hostname) | |
| 54 | + | if !strings.HasPrefix(u, "https://") { | |
| 55 | + | return "https://localhost/" | |
| 56 | + | } | |
| 57 | + | return html.EscapeString(u) | |
| 55 | 58 | } | |
| 56 | 59 | ||
| 57 | 60 | // ServeOnboardingPage serves the CA trust onboarding HTML page. | |
| 61 | + | // Static onboarding page -- kept inline for simplicity. | |
| 58 | 62 | func (h *Handler) ServeOnboardingPage(w http.ResponseWriter, r *http.Request) { | |
| 59 | 63 | w.Header().Set("Content-Type", "text/html; charset=utf-8") | |
| 60 | 64 | ||
| 61 | 65 | redirectURL := h.normalizeHTTPSURL(r) | |
| 62 | 66 | ||
| 67 | + | // Cookie is set client-side on click (not on page load) to mark CA installation complete. | |
| 68 | + | // HttpOnly cannot be applied to JS-set cookies -- acceptable since this is a UI hint, not a session token. | |
| 63 | 69 | _, _ = fmt.Fprintf(w, `<!DOCTYPE html> | |
| 64 | 70 | <html lang="en"> | |
| 65 | 71 | <head> | |
| @@ -5,8 +5,7 @@ import ( | |||||
| 5 | "html" | 5 | "html" | ||
| 6 | "net" | 6 | "net" | ||
| 7 | "net/http" | 7 | "net/http" | ||
| 8 | - | 8 | + | "strings" | |
| 9 | - | pkiadapter "github.com/bnema/gordon/internal/adapters/out/pki" | |||
| 10 | ) | 9 | ) | ||
| 11 | 10 | ||||
| 12 | // Handler serves CA onboarding endpoints on the TLS port for direct/Tailnet clients. | 11 | // Handler serves CA onboarding endpoints on the TLS port for direct/Tailnet clients. | ||
| @@ -18,10 +17,10 @@ type Handler struct { | |||||
| 18 | 17 | ||||
| 19 | // NewHandler creates an onboarding handler. | 18 | // NewHandler creates an onboarding handler. | ||
| 20 | // tlsPort is the HTTPS port used to build redirect URLs on the onboarding page. | 19 | // tlsPort is the HTTPS port used to build redirect URLs on the onboarding page. | ||
| 21 | - | func NewHandler(rootPEM, rootDER []byte, rootCN string, tlsPort int) *Handler { | 20 | + | func NewHandler(rootPEM, mobileconfig []byte, tlsPort int) *Handler { |
| 22 | return &Handler{ | 21 | return &Handler{ | ||
| 23 | rootPEM: rootPEM, | 22 | rootPEM: rootPEM, | ||
| 24 | - | mobileconfig: pkiadapter.GenerateMobileconfig(rootDER, rootCN), | 23 | + | mobileconfig: mobileconfig, |
| 25 | tlsPort: tlsPort, | 24 | tlsPort: tlsPort, | ||
| 26 | } | 25 | } | ||
| 27 | } | 26 | } | ||
| @@ -51,15 +50,22 @@ func (h *Handler) normalizeHTTPSURL(r *http.Request) string { | |||||
| 51 | if h.tlsPort != 0 && h.tlsPort != 443 { | 50 | if h.tlsPort != 0 && h.tlsPort != 443 { | ||
| 52 | hostname = fmt.Sprintf("%s:%d", hostname, h.tlsPort) | 51 | hostname = fmt.Sprintf("%s:%d", hostname, h.tlsPort) | ||
| 53 | } | 52 | } | ||
| 54 | - | return html.EscapeString(fmt.Sprintf("https://%s/", hostname)) | 53 | + | u := fmt.Sprintf("https://%s/", hostname) |
| 54 | + | if !strings.HasPrefix(u, "https://") { | |||
| 55 | + | return "https://localhost/" | |||
| 56 | + | } | |||
| 57 | + | return html.EscapeString(u) | |||
| 55 | } | 58 | } | ||
| 56 | 59 | ||||
| 57 | // ServeOnboardingPage serves the CA trust onboarding HTML page. | 60 | // ServeOnboardingPage serves the CA trust onboarding HTML page. | ||
| 61 | + | // Static onboarding page -- kept inline for simplicity. | |||
| 58 | func (h *Handler) ServeOnboardingPage(w http.ResponseWriter, r *http.Request) { | 62 | func (h *Handler) ServeOnboardingPage(w http.ResponseWriter, r *http.Request) { | ||
| 59 | w.Header().Set("Content-Type", "text/html; charset=utf-8") | 63 | w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| 60 | 64 | ||||
| 61 | redirectURL := h.normalizeHTTPSURL(r) | 65 | redirectURL := h.normalizeHTTPSURL(r) | ||
| 62 | 66 | ||||
| 67 | + | // Cookie is set client-side on click (not on page load) to mark CA installation complete. | |||
| 68 | + | // HttpOnly cannot be applied to JS-set cookies -- acceptable since this is a UI hint, not a session token. | |||
| 63 | _, _ = fmt.Fprintf(w, `<!DOCTYPE html> | 69 | _, _ = fmt.Fprintf(w, `<!DOCTYPE html> | ||
| 64 | <html lang="en"> | 70 | <html lang="en"> | ||
| 65 | <head> | 71 | <head> | ||
M internal/adapters/in/http/onboarding/handler_test.go +6 -6
| @@ -12,8 +12,8 @@ import ( | |||
| 12 | 12 | ||
| 13 | 13 | func TestHandler_CACert(t *testing.T) { | |
| 14 | 14 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | |
| 15 | - | rootDER := []byte("fakeDER") | |
| 16 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | |
| 15 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") | |
| 16 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) | |
| 17 | 17 | ||
| 18 | 18 | req := httptest.NewRequest("GET", "/ca.crt", nil) | |
| 19 | 19 | w := httptest.NewRecorder() | |
| @@ -26,8 +26,8 @@ func TestHandler_CACert(t *testing.T) { | |||
| 26 | 26 | ||
| 27 | 27 | func TestHandler_Mobileconfig(t *testing.T) { | |
| 28 | 28 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | |
| 29 | - | rootDER := []byte("fakeDER") | |
| 30 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | |
| 29 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") | |
| 30 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) | |
| 31 | 31 | ||
| 32 | 32 | req := httptest.NewRequest("GET", "/ca.mobileconfig", nil) | |
| 33 | 33 | w := httptest.NewRecorder() | |
| @@ -40,8 +40,8 @@ func TestHandler_Mobileconfig(t *testing.T) { | |||
| 40 | 40 | ||
| 41 | 41 | func TestHandler_OnboardingPage(t *testing.T) { | |
| 42 | 42 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | |
| 43 | - | rootDER := []byte("fakeDER") | |
| 44 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | |
| 43 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") | |
| 44 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) | |
| 45 | 45 | ||
| 46 | 46 | req := httptest.NewRequest("GET", "/ca", nil) | |
| 47 | 47 | w := httptest.NewRecorder() | |
| @@ -12,8 +12,8 @@ import ( | |||||
| 12 | 12 | ||||
| 13 | func TestHandler_CACert(t *testing.T) { | 13 | func TestHandler_CACert(t *testing.T) { | ||
| 14 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | 14 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | ||
| 15 | - | rootDER := []byte("fakeDER") | 15 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") |
| 16 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | 16 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) |
| 17 | 17 | ||||
| 18 | req := httptest.NewRequest("GET", "/ca.crt", nil) | 18 | req := httptest.NewRequest("GET", "/ca.crt", nil) | ||
| 19 | w := httptest.NewRecorder() | 19 | w := httptest.NewRecorder() | ||
| @@ -26,8 +26,8 @@ func TestHandler_CACert(t *testing.T) { | |||||
| 26 | 26 | ||||
| 27 | func TestHandler_Mobileconfig(t *testing.T) { | 27 | func TestHandler_Mobileconfig(t *testing.T) { | ||
| 28 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | 28 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | ||
| 29 | - | rootDER := []byte("fakeDER") | 29 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") |
| 30 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | 30 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) |
| 31 | 31 | ||||
| 32 | req := httptest.NewRequest("GET", "/ca.mobileconfig", nil) | 32 | req := httptest.NewRequest("GET", "/ca.mobileconfig", nil) | ||
| 33 | w := httptest.NewRecorder() | 33 | w := httptest.NewRecorder() | ||
| @@ -40,8 +40,8 @@ func TestHandler_Mobileconfig(t *testing.T) { | |||||
| 40 | 40 | ||||
| 41 | func TestHandler_OnboardingPage(t *testing.T) { | 41 | func TestHandler_OnboardingPage(t *testing.T) { | ||
| 42 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | 42 | rootPEM := []byte("-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----\n") | ||
| 43 | - | rootDER := []byte("fakeDER") | 43 | + | mobileconfig := []byte("<plist>com.apple.security.root</plist>") |
| 44 | - | h := onboarding.NewHandler(rootPEM, rootDER, "Gordon Test CA", 443) | 44 | + | h := onboarding.NewHandler(rootPEM, mobileconfig, 443) |
| 45 | 45 | ||||
| 46 | req := httptest.NewRequest("GET", "/ca", nil) | 46 | req := httptest.NewRequest("GET", "/ca", nil) | ||
| 47 | w := httptest.NewRecorder() | 47 | w := httptest.NewRecorder() | ||
M internal/adapters/out/pki/ca.go +26 -36
| @@ -42,23 +42,21 @@ const ( | |||
| 42 | 42 | type CA struct { | |
| 43 | 43 | dataDir string | |
| 44 | 44 | log zerowrap.Logger | |
| 45 | - | aiaURL string // optional: URL for AIA extension in leaf certs | |
| 46 | 45 | ||
| 47 | 46 | rootCert *x509.Certificate | |
| 48 | 47 | rootKey crypto.Signer | |
| 49 | 48 | rootPEM []byte | |
| 50 | 49 | ||
| 50 | + | // mu guards interCert/interKey for concurrent reads during cert issuance. | |
| 51 | + | // renewMu serialises intermediate CA renewal. | |
| 52 | + | // Lock ordering: renewMu → mu (never acquire mu before renewMu). | |
| 51 | 53 | mu sync.RWMutex | |
| 52 | 54 | interCert *x509.Certificate | |
| 53 | 55 | interKey crypto.Signer | |
| 54 | 56 | ||
| 55 | - | renewMu sync.Mutex // serialises intermediate renewal | |
| 57 | + | renewMu sync.Mutex | |
| 56 | 58 | } | |
| 57 | 59 | ||
| 58 | - | // SetAIAURL sets the Authority Information Access URL embedded in leaf certs. | |
| 59 | - | // Format: http://{gordon_domain}[:{port}]/ca.crt | |
| 60 | - | func (ca *CA) SetAIAURL(url string) { ca.aiaURL = url } | |
| 61 | - | ||
| 62 | 60 | // NewCA loads or generates the root and intermediate CA certificates. | |
| 63 | 61 | func NewCA(dataDir string, log zerowrap.Logger) (*CA, error) { | |
| 64 | 62 | ca := &CA{ | |
| @@ -214,10 +212,11 @@ func (ca *CA) bootstrapRoot(certPath, keyPath string) error { | |||
| 214 | 212 | } | |
| 215 | 213 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | |
| 216 | 214 | ||
| 217 | - | if err := writeSecure(certPath, certPEM, 0644); err != nil { | |
| 215 | + | // Write key first — orphaned key is harmless; missing cert triggers regeneration. | |
| 216 | + | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { | |
| 218 | 217 | return err | |
| 219 | 218 | } | |
| 220 | - | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { | |
| 219 | + | if err := writeSecure(certPath, certPEM, 0644); err != nil { | |
| 221 | 220 | return err | |
| 222 | 221 | } | |
| 223 | 222 | ||
| @@ -256,6 +255,13 @@ func (ca *CA) loadOrGenerateIntermediate() error { | |||
| 256 | 255 | } | |
| 257 | 256 | } | |
| 258 | 257 | ||
| 258 | + | if (certErr == nil) != (keyErr == nil) { | |
| 259 | + | ca.log.Warn(). | |
| 260 | + | Bool("cert_missing", certErr != nil). | |
| 261 | + | Bool("key_missing", keyErr != nil). | |
| 262 | + | Msg("intermediate CA files incomplete, regenerating") | |
| 263 | + | } | |
| 264 | + | ||
| 259 | 265 | cert, key, err := ca.generateIntermediate() | |
| 260 | 266 | if err != nil { | |
| 261 | 267 | return err | |
| @@ -323,30 +329,13 @@ func (ca *CA) storeIntermediate(cert *x509.Certificate, key *ecdsa.PrivateKey) e | |||
| 323 | 329 | } | |
| 324 | 330 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | |
| 325 | 331 | ||
| 326 | - | // Write both files to temps first, then atomically rename together | |
| 327 | - | // so a crash between writes cannot leave a dangling cert without its key. | |
| 328 | - | certTmp := certPath + ".tmp" | |
| 329 | - | keyTmp := keyPath + ".tmp" | |
| 330 | - | ||
| 331 | - | if err := writeSecure(certTmp, certPEM, 0644); err != nil { | |
| 332 | - | return err | |
| 333 | - | } | |
| 334 | - | if err := writeSecure(keyTmp, keyPEM, 0600); err != nil { | |
| 335 | - | os.Remove(certTmp) | |
| 332 | + | // Write key first — if cert write fails afterward, the orphaned key | |
| 333 | + | // is harmless: loadOrGenerateIntermediate requires both files and | |
| 334 | + | // regenerates when either is missing. | |
| 335 | + | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { | |
| 336 | 336 | return err | |
| 337 | 337 | } | |
| 338 | - | ||
| 339 | - | if err := os.Rename(certTmp, certPath); err != nil { | |
| 340 | - | os.Remove(certTmp) | |
| 341 | - | os.Remove(keyTmp) | |
| 342 | - | return fmt.Errorf("rename intermediate cert: %w", err) | |
| 343 | - | } | |
| 344 | - | if err := os.Rename(keyTmp, keyPath); err != nil { | |
| 345 | - | os.Remove(keyTmp) | |
| 346 | - | return fmt.Errorf("rename intermediate key: %w", err) | |
| 347 | - | } | |
| 348 | - | ||
| 349 | - | return nil | |
| 338 | + | return writeSecure(certPath, certPEM, 0644) | |
| 350 | 339 | } | |
| 351 | 340 | ||
| 352 | 341 | // IssueCertificate generates a leaf cert for the given domain, | |
| @@ -372,6 +361,11 @@ func (ca *CA) IssueCertificate(domain string) (*tls.Certificate, error) { | |||
| 372 | 361 | } | |
| 373 | 362 | ||
| 374 | 363 | now := time.Now() | |
| 364 | + | expiry := now.Add(leafLifetime) | |
| 365 | + | if expiry.After(interCert.NotAfter) { | |
| 366 | + | expiry = interCert.NotAfter.Add(-1 * time.Minute) | |
| 367 | + | } | |
| 368 | + | ||
| 375 | 369 | template := &x509.Certificate{ | |
| 376 | 370 | SerialNumber: serial, | |
| 377 | 371 | Subject: pkix.Name{ | |
| @@ -379,15 +373,11 @@ func (ca *CA) IssueCertificate(domain string) (*tls.Certificate, error) { | |||
| 379 | 373 | }, | |
| 380 | 374 | DNSNames: []string{domain}, | |
| 381 | 375 | NotBefore: now.Add(-5 * time.Minute), | |
| 382 | - | NotAfter: now.Add(leafLifetime), | |
| 376 | + | NotAfter: expiry, | |
| 383 | 377 | KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, | |
| 384 | 378 | ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | |
| 385 | 379 | } | |
| 386 | 380 | ||
| 387 | - | if ca.aiaURL != "" { | |
| 388 | - | template.IssuingCertificateURL = []string{ca.aiaURL} | |
| 389 | - | } | |
| 390 | - | ||
| 391 | 381 | certDER, err := x509.CreateCertificate(rand.Reader, template, interCert, key.Public(), interKey) | |
| 392 | 382 | if err != nil { | |
| 393 | 383 | return nil, fmt.Errorf("create leaf cert: %w", err) | |
| @@ -447,7 +437,7 @@ func parseCertAndKey(certPEM, keyPEM []byte) (*x509.Certificate, crypto.Signer, | |||
| 447 | 437 | } | |
| 448 | 438 | key, err := x509.ParseECPrivateKey(keyBlock.Bytes) | |
| 449 | 439 | if err != nil { | |
| 450 | - | return nil, nil, fmt.Errorf("parse key: %w", err) | |
| 440 | + | return nil, nil, fmt.Errorf("parse private key (only ECDSA P-256 is supported): %w", err) | |
| 451 | 441 | } | |
| 452 | 442 | ||
| 453 | 443 | return cert, key, nil | |
| @@ -42,23 +42,21 @@ const ( | |||||
| 42 | type CA struct { | 42 | type CA struct { | ||
| 43 | dataDir string | 43 | dataDir string | ||
| 44 | log zerowrap.Logger | 44 | log zerowrap.Logger | ||
| 45 | - | aiaURL string // optional: URL for AIA extension in leaf certs | |||
| 46 | 45 | ||||
| 47 | rootCert *x509.Certificate | 46 | rootCert *x509.Certificate | ||
| 48 | rootKey crypto.Signer | 47 | rootKey crypto.Signer | ||
| 49 | rootPEM []byte | 48 | rootPEM []byte | ||
| 50 | 49 | ||||
| 50 | + | // mu guards interCert/interKey for concurrent reads during cert issuance. | |||
| 51 | + | // renewMu serialises intermediate CA renewal. | |||
| 52 | + | // Lock ordering: renewMu → mu (never acquire mu before renewMu). | |||
| 51 | mu sync.RWMutex | 53 | mu sync.RWMutex | ||
| 52 | interCert *x509.Certificate | 54 | interCert *x509.Certificate | ||
| 53 | interKey crypto.Signer | 55 | interKey crypto.Signer | ||
| 54 | 56 | ||||
| 55 | - | renewMu sync.Mutex // serialises intermediate renewal | 57 | + | renewMu sync.Mutex |
| 56 | } | 58 | } | ||
| 57 | 59 | ||||
| 58 | - | // SetAIAURL sets the Authority Information Access URL embedded in leaf certs. | |||
| 59 | - | // Format: http://{gordon_domain}[:{port}]/ca.crt | |||
| 60 | - | func (ca *CA) SetAIAURL(url string) { ca.aiaURL = url } | |||
| 61 | - | ||||
| 62 | // NewCA loads or generates the root and intermediate CA certificates. | 60 | // NewCA loads or generates the root and intermediate CA certificates. | ||
| 63 | func NewCA(dataDir string, log zerowrap.Logger) (*CA, error) { | 61 | func NewCA(dataDir string, log zerowrap.Logger) (*CA, error) { | ||
| 64 | ca := &CA{ | 62 | ca := &CA{ | ||
| @@ -214,10 +212,11 @@ func (ca *CA) bootstrapRoot(certPath, keyPath string) error { | |||||
| 214 | } | 212 | } | ||
| 215 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | 213 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | ||
| 216 | 214 | ||||
| 217 | - | if err := writeSecure(certPath, certPEM, 0644); err != nil { | 215 | + | // Write key first — orphaned key is harmless; missing cert triggers regeneration. |
| 216 | + | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { | |||
| 218 | return err | 217 | return err | ||
| 219 | } | 218 | } | ||
| 220 | - | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { | 219 | + | if err := writeSecure(certPath, certPEM, 0644); err != nil { |
| 221 | return err | 220 | return err | ||
| 222 | } | 221 | } | ||
| 223 | 222 | ||||
| @@ -256,6 +255,13 @@ func (ca *CA) loadOrGenerateIntermediate() error { | |||||
| 256 | } | 255 | } | ||
| 257 | } | 256 | } | ||
| 258 | 257 | ||||
| 258 | + | if (certErr == nil) != (keyErr == nil) { | |||
| 259 | + | ca.log.Warn(). | |||
| 260 | + | Bool("cert_missing", certErr != nil). | |||
| 261 | + | Bool("key_missing", keyErr != nil). | |||
| 262 | + | Msg("intermediate CA files incomplete, regenerating") | |||
| 263 | + | } | |||
| 264 | + | ||||
| 259 | cert, key, err := ca.generateIntermediate() | 265 | cert, key, err := ca.generateIntermediate() | ||
| 260 | if err != nil { | 266 | if err != nil { | ||
| 261 | return err | 267 | return err | ||
| @@ -323,30 +329,13 @@ func (ca *CA) storeIntermediate(cert *x509.Certificate, key *ecdsa.PrivateKey) e | |||||
| 323 | } | 329 | } | ||
| 324 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | 330 | keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) | ||
| 325 | 331 | ||||
| 326 | - | // Write both files to temps first, then atomically rename together | 332 | + | // Write key first — if cert write fails afterward, the orphaned key |
| 327 | - | // so a crash between writes cannot leave a dangling cert without its key. | 333 | + | // is harmless: loadOrGenerateIntermediate requires both files and |
| 328 | - | certTmp := certPath + ".tmp" | 334 | + | // regenerates when either is missing. |
| 329 | - | keyTmp := keyPath + ".tmp" | 335 | + | if err := writeSecure(keyPath, keyPEM, 0600); err != nil { |
| 330 | - | ||||
| 331 | - | if err := writeSecure(certTmp, certPEM, 0644); err != nil { | |||
| 332 | - | return err | |||
| 333 | - | } | |||
| 334 | - | if err := writeSecure(keyTmp, keyPEM, 0600); err != nil { | |||
| 335 | - | os.Remove(certTmp) | |||
| 336 | return err | 336 | return err | ||
| 337 | } | 337 | } | ||
| 338 | - | 338 | + | return writeSecure(certPath, certPEM, 0644) | |
| 339 | - | if err := os.Rename(certTmp, certPath); err != nil { | |||
| 340 | - | os.Remove(certTmp) | |||
| 341 | - | os.Remove(keyTmp) | |||
| 342 | - | return fmt.Errorf("rename intermediate cert: %w", err) | |||
| 343 | - | } | |||
| 344 | - | if err := os.Rename(keyTmp, keyPath); err != nil { | |||
| 345 | - | os.Remove(keyTmp) | |||
| 346 | - | return fmt.Errorf("rename intermediate key: %w", err) | |||
| 347 | - | } | |||
| 348 | - | ||||
| 349 | - | return nil | |||
| 350 | } | 339 | } | ||
| 351 | 340 | ||||
| 352 | // IssueCertificate generates a leaf cert for the given domain, | 341 | // IssueCertificate generates a leaf cert for the given domain, | ||
| @@ -372,6 +361,11 @@ func (ca *CA) IssueCertificate(domain string) (*tls.Certificate, error) { | |||||
| 372 | } | 361 | } | ||
| 373 | 362 | ||||
| 374 | now := time.Now() | 363 | now := time.Now() | ||
| 364 | + | expiry := now.Add(leafLifetime) | |||
| 365 | + | if expiry.After(interCert.NotAfter) { | |||
| 366 | + | expiry = interCert.NotAfter.Add(-1 * time.Minute) | |||
| 367 | + | } | |||
| 368 | + | ||||
| 375 | template := &x509.Certificate{ | 369 | template := &x509.Certificate{ | ||
| 376 | SerialNumber: serial, | 370 | SerialNumber: serial, | ||
| 377 | Subject: pkix.Name{ | 371 | Subject: pkix.Name{ | ||
| @@ -379,15 +373,11 @@ func (ca *CA) IssueCertificate(domain string) (*tls.Certificate, error) { | |||||
| 379 | }, | 373 | }, | ||
| 380 | DNSNames: []string{domain}, | 374 | DNSNames: []string{domain}, | ||
| 381 | NotBefore: now.Add(-5 * time.Minute), | 375 | NotBefore: now.Add(-5 * time.Minute), | ||
| 382 | - | NotAfter: now.Add(leafLifetime), | 376 | + | NotAfter: expiry, |
| 383 | KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, | 377 | KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, | ||
| 384 | ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | 378 | ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | ||
| 385 | } | 379 | } | ||
| 386 | 380 | ||||
| 387 | - | if ca.aiaURL != "" { | |||
| 388 | - | template.IssuingCertificateURL = []string{ca.aiaURL} | |||
| 389 | - | } | |||
| 390 | - | ||||
| 391 | certDER, err := x509.CreateCertificate(rand.Reader, template, interCert, key.Public(), interKey) | 381 | certDER, err := x509.CreateCertificate(rand.Reader, template, interCert, key.Public(), interKey) | ||
| 392 | if err != nil { | 382 | if err != nil { | ||
| 393 | return nil, fmt.Errorf("create leaf cert: %w", err) | 383 | return nil, fmt.Errorf("create leaf cert: %w", err) | ||
| @@ -447,7 +437,7 @@ func parseCertAndKey(certPEM, keyPEM []byte) (*x509.Certificate, crypto.Signer, | |||||
| 447 | } | 437 | } | ||
| 448 | key, err := x509.ParseECPrivateKey(keyBlock.Bytes) | 438 | key, err := x509.ParseECPrivateKey(keyBlock.Bytes) | ||
| 449 | if err != nil { | 439 | if err != nil { | ||
| 450 | - | return nil, nil, fmt.Errorf("parse key: %w", err) | 440 | + | return nil, nil, fmt.Errorf("parse private key (only ECDSA P-256 is supported): %w", err) |
| 451 | } | 441 | } | ||
| 452 | 442 | ||||
| 453 | return cert, key, nil | 443 | return cert, key, nil | ||
M internal/app/run.go +5 -2
| @@ -1624,10 +1624,13 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 1624 | 1624 | ||
| 1625 | 1625 | httpsMux := http.NewServeMux() | |
| 1626 | 1626 | if svc.caAdapter != nil { | |
| 1627 | - | onboardingHandler := onboarding.NewHandler( | |
| 1628 | - | svc.caAdapter.RootCertificate(), | |
| 1627 | + | mobileconfigBytes := pkiadapter.GenerateMobileconfig( | |
| 1629 | 1628 | svc.caAdapter.RootCertificateDER(), | |
| 1630 | 1629 | svc.caAdapter.RootCommonName(), | |
| 1630 | + | ) | |
| 1631 | + | onboardingHandler := onboarding.NewHandler( | |
| 1632 | + | svc.caAdapter.RootCertificate(), | |
| 1633 | + | mobileconfigBytes, | |
| 1631 | 1634 | cfg.Server.TLSPort, | |
| 1632 | 1635 | ) | |
| 1633 | 1636 | httpsMux.HandleFunc("GET /ca", onboardingHandler.ServeOnboardingPage) | |
| @@ -1624,10 +1624,13 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 1624 | 1624 | ||||
| 1625 | httpsMux := http.NewServeMux() | 1625 | httpsMux := http.NewServeMux() | ||
| 1626 | if svc.caAdapter != nil { | 1626 | if svc.caAdapter != nil { | ||
| 1627 | - | onboardingHandler := onboarding.NewHandler( | 1627 | + | mobileconfigBytes := pkiadapter.GenerateMobileconfig( |
| 1628 | - | svc.caAdapter.RootCertificate(), | |||
| 1629 | svc.caAdapter.RootCertificateDER(), | 1628 | svc.caAdapter.RootCertificateDER(), | ||
| 1630 | svc.caAdapter.RootCommonName(), | 1629 | svc.caAdapter.RootCommonName(), | ||
| 1630 | + | ) | |||
| 1631 | + | onboardingHandler := onboarding.NewHandler( | |||
| 1632 | + | svc.caAdapter.RootCertificate(), | |||
| 1633 | + | mobileconfigBytes, | |||
| 1631 | cfg.Server.TLSPort, | 1634 | cfg.Server.TLSPort, | ||
| 1632 | ) | 1635 | ) | ||
| 1633 | httpsMux.HandleFunc("GET /ca", onboardingHandler.ServeOnboardingPage) | 1636 | httpsMux.HandleFunc("GET /ca", onboardingHandler.ServeOnboardingPage) | ||
M internal/usecase/pki/service.go +49 -17
| @@ -3,11 +3,11 @@ package pki | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "crypto/tls" | |
| 6 | - | "fmt" | |
| 7 | 6 | "sync" | |
| 8 | 7 | "time" | |
| 9 | 8 | ||
| 10 | 9 | "github.com/bnema/zerowrap" | |
| 10 | + | "golang.org/x/sync/singleflight" | |
| 11 | 11 | ||
| 12 | 12 | "github.com/bnema/gordon/internal/boundaries/out" | |
| 13 | 13 | ) | |
| @@ -33,6 +33,7 @@ type Service struct { | |||
| 33 | 33 | log zerowrap.Logger | |
| 34 | 34 | ||
| 35 | 35 | cache sync.Map // map[string]*cachedCert | |
| 36 | + | flight singleflight.Group | |
| 36 | 37 | cancel context.CancelFunc | |
| 37 | 38 | done chan struct{} | |
| 38 | 39 | } | |
| @@ -68,10 +69,12 @@ func (s *Service) CachedCertCount() int { | |||
| 68 | 69 | } | |
| 69 | 70 | ||
| 70 | 71 | // GetCertificate is the tls.Config.GetCertificate callback. | |
| 72 | + | // It returns (nil, nil) for unknown domains so Go's TLS stack | |
| 73 | + | // falls back to tls.Config.Certificates. | |
| 71 | 74 | func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { | |
| 72 | 75 | domain := hello.ServerName | |
| 73 | 76 | if domain == "" { | |
| 74 | - | return nil, fmt.Errorf("no SNI in ClientHello") | |
| 77 | + | return nil, nil | |
| 75 | 78 | } | |
| 76 | 79 | ||
| 77 | 80 | if entry, ok := s.cache.Load(domain); ok { | |
| @@ -84,22 +87,38 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||
| 84 | 87 | } | |
| 85 | 88 | ||
| 86 | 89 | if !s.isDomainAllowed(hello.Context(), domain) { | |
| 87 | - | return nil, fmt.Errorf("domain %q not in route table", domain) | |
| 90 | + | return nil, nil | |
| 88 | 91 | } | |
| 89 | 92 | ||
| 90 | - | cert, err := s.ca.IssueCertificate(domain) | |
| 93 | + | result, err, _ := s.flight.Do(domain, func() (any, error) { | |
| 94 | + | // Re-check cache; another goroutine in the same flight may have populated it. | |
| 95 | + | if entry, ok := s.cache.Load(domain); ok { | |
| 96 | + | if cached, ok := entry.(*cachedCert); ok { | |
| 97 | + | if time.Now().Before(cached.expiresAt) { | |
| 98 | + | return cached.cert, nil | |
| 99 | + | } | |
| 100 | + | } | |
| 101 | + | s.cache.Delete(domain) | |
| 102 | + | } | |
| 103 | + | ||
| 104 | + | cert, err := s.ca.IssueCertificate(domain) | |
| 105 | + | if err != nil { | |
| 106 | + | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | |
| 107 | + | return nil, err | |
| 108 | + | } | |
| 109 | + | ||
| 110 | + | s.cache.Store(domain, &cachedCert{ | |
| 111 | + | cert: cert, | |
| 112 | + | expiresAt: time.Now().Add(s.leafLifetime()), | |
| 113 | + | }) | |
| 114 | + | ||
| 115 | + | s.log.Debug().Str("domain", domain).Msg("issued new leaf certificate") | |
| 116 | + | return cert, nil | |
| 117 | + | }) | |
| 91 | 118 | if err != nil { | |
| 92 | - | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | |
| 93 | 119 | return nil, err | |
| 94 | 120 | } | |
| 95 | - | ||
| 96 | - | s.cache.Store(domain, &cachedCert{ | |
| 97 | - | cert: cert, | |
| 98 | - | expiresAt: time.Now().Add(s.leafLifetime()), | |
| 99 | - | }) | |
| 100 | - | ||
| 101 | - | s.log.Debug().Str("domain", domain).Msg("issued new leaf certificate") | |
| 102 | - | return cert, nil | |
| 121 | + | return result.(*tls.Certificate), nil | |
| 103 | 122 | } | |
| 104 | 123 | ||
| 105 | 124 | func (s *Service) isDomainAllowed(ctx context.Context, domain string) bool { | |
| @@ -129,7 +148,7 @@ func (s *Service) maintenanceLoop(ctx context.Context) { | |||
| 129 | 148 | return | |
| 130 | 149 | case <-ticker.C: | |
| 131 | 150 | s.renewIntermediateIfNeeded() | |
| 132 | - | s.sweepExpiredCerts() | |
| 151 | + | s.sweepExpiredCerts(ctx) | |
| 133 | 152 | } | |
| 134 | 153 | } | |
| 135 | 154 | } | |
| @@ -147,14 +166,27 @@ func (s *Service) renewIntermediateIfNeeded() { | |||
| 147 | 166 | } | |
| 148 | 167 | } | |
| 149 | 168 | ||
| 150 | - | func (s *Service) sweepExpiredCerts() { | |
| 169 | + | func (s *Service) sweepExpiredCerts(ctx context.Context) { | |
| 151 | 170 | now := time.Now() | |
| 152 | - | ctx := context.Background() | |
| 171 | + | ||
| 172 | + | // Fetch routes once for all cache entries. | |
| 173 | + | routes := s.routes.GetRoutes(ctx) | |
| 174 | + | extRoutes := s.routes.GetExternalRoutes() | |
| 175 | + | ||
| 176 | + | allowed := make(map[string]struct{}, len(routes)+len(extRoutes)) | |
| 177 | + | for _, r := range routes { | |
| 178 | + | allowed[r.Domain] = struct{}{} | |
| 179 | + | } | |
| 180 | + | for d := range extRoutes { | |
| 181 | + | allowed[d] = struct{}{} | |
| 182 | + | } | |
| 183 | + | ||
| 153 | 184 | swept := 0 | |
| 154 | 185 | s.cache.Range(func(key, value any) bool { | |
| 155 | 186 | domain := key.(string) | |
| 156 | 187 | cached := value.(*cachedCert) | |
| 157 | - | if now.After(cached.expiresAt) || !s.isDomainAllowed(ctx, domain) { | |
| 188 | + | _, ok := allowed[domain] | |
| 189 | + | if now.After(cached.expiresAt) || !ok { | |
| 158 | 190 | s.cache.Delete(key) | |
| 159 | 191 | swept++ | |
| 160 | 192 | } | |
| @@ -3,11 +3,11 @@ package pki | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "crypto/tls" | 5 | "crypto/tls" | ||
| 6 | - | "fmt" | |||
| 7 | "sync" | 6 | "sync" | ||
| 8 | "time" | 7 | "time" | ||
| 9 | 8 | ||||
| 10 | "github.com/bnema/zerowrap" | 9 | "github.com/bnema/zerowrap" | ||
| 10 | + | "golang.org/x/sync/singleflight" | |||
| 11 | 11 | ||||
| 12 | "github.com/bnema/gordon/internal/boundaries/out" | 12 | "github.com/bnema/gordon/internal/boundaries/out" | ||
| 13 | ) | 13 | ) | ||
| @@ -33,6 +33,7 @@ type Service struct { | |||||
| 33 | log zerowrap.Logger | 33 | log zerowrap.Logger | ||
| 34 | 34 | ||||
| 35 | cache sync.Map // map[string]*cachedCert | 35 | cache sync.Map // map[string]*cachedCert | ||
| 36 | + | flight singleflight.Group | |||
| 36 | cancel context.CancelFunc | 37 | cancel context.CancelFunc | ||
| 37 | done chan struct{} | 38 | done chan struct{} | ||
| 38 | } | 39 | } | ||
| @@ -68,10 +69,12 @@ func (s *Service) CachedCertCount() int { | |||||
| 68 | } | 69 | } | ||
| 69 | 70 | ||||
| 70 | // GetCertificate is the tls.Config.GetCertificate callback. | 71 | // GetCertificate is the tls.Config.GetCertificate callback. | ||
| 72 | + | // It returns (nil, nil) for unknown domains so Go's TLS stack | |||
| 73 | + | // falls back to tls.Config.Certificates. | |||
| 71 | func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { | 74 | func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
| 72 | domain := hello.ServerName | 75 | domain := hello.ServerName | ||
| 73 | if domain == "" { | 76 | if domain == "" { | ||
| 74 | - | return nil, fmt.Errorf("no SNI in ClientHello") | 77 | + | return nil, nil |
| 75 | } | 78 | } | ||
| 76 | 79 | ||||
| 77 | if entry, ok := s.cache.Load(domain); ok { | 80 | if entry, ok := s.cache.Load(domain); ok { | ||
| @@ -84,22 +87,38 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||||
| 84 | } | 87 | } | ||
| 85 | 88 | ||||
| 86 | if !s.isDomainAllowed(hello.Context(), domain) { | 89 | if !s.isDomainAllowed(hello.Context(), domain) { | ||
| 87 | - | return nil, fmt.Errorf("domain %q not in route table", domain) | 90 | + | return nil, nil |
| 88 | } | 91 | } | ||
| 89 | 92 | ||||
| 90 | - | cert, err := s.ca.IssueCertificate(domain) | 93 | + | result, err, _ := s.flight.Do(domain, func() (any, error) { |
| 94 | + | // Re-check cache; another goroutine in the same flight may have populated it. | |||
| 95 | + | if entry, ok := s.cache.Load(domain); ok { | |||
| 96 | + | if cached, ok := entry.(*cachedCert); ok { | |||
| 97 | + | if time.Now().Before(cached.expiresAt) { | |||
| 98 | + | return cached.cert, nil | |||
| 99 | + | } | |||
| 100 | + | } | |||
| 101 | + | s.cache.Delete(domain) | |||
| 102 | + | } | |||
| 103 | + | ||||
| 104 | + | cert, err := s.ca.IssueCertificate(domain) | |||
| 105 | + | if err != nil { | |||
| 106 | + | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | |||
| 107 | + | return nil, err | |||
| 108 | + | } | |||
| 109 | + | ||||
| 110 | + | s.cache.Store(domain, &cachedCert{ | |||
| 111 | + | cert: cert, | |||
| 112 | + | expiresAt: time.Now().Add(s.leafLifetime()), | |||
| 113 | + | }) | |||
| 114 | + | ||||
| 115 | + | s.log.Debug().Str("domain", domain).Msg("issued new leaf certificate") | |||
| 116 | + | return cert, nil | |||
| 117 | + | }) | |||
| 91 | if err != nil { | 118 | if err != nil { | ||
| 92 | - | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | |||
| 93 | return nil, err | 119 | return nil, err | ||
| 94 | } | 120 | } | ||
| 95 | - | 121 | + | return result.(*tls.Certificate), nil | |
| 96 | - | s.cache.Store(domain, &cachedCert{ | |||
| 97 | - | cert: cert, | |||
| 98 | - | expiresAt: time.Now().Add(s.leafLifetime()), | |||
| 99 | - | }) | |||
| 100 | - | ||||
| 101 | - | s.log.Debug().Str("domain", domain).Msg("issued new leaf certificate") | |||
| 102 | - | return cert, nil | |||
| 103 | } | 122 | } | ||
| 104 | 123 | ||||
| 105 | func (s *Service) isDomainAllowed(ctx context.Context, domain string) bool { | 124 | func (s *Service) isDomainAllowed(ctx context.Context, domain string) bool { | ||
| @@ -129,7 +148,7 @@ func (s *Service) maintenanceLoop(ctx context.Context) { | |||||
| 129 | return | 148 | return | ||
| 130 | case <-ticker.C: | 149 | case <-ticker.C: | ||
| 131 | s.renewIntermediateIfNeeded() | 150 | s.renewIntermediateIfNeeded() | ||
| 132 | - | s.sweepExpiredCerts() | 151 | + | s.sweepExpiredCerts(ctx) |
| 133 | } | 152 | } | ||
| 134 | } | 153 | } | ||
| 135 | } | 154 | } | ||
| @@ -147,14 +166,27 @@ func (s *Service) renewIntermediateIfNeeded() { | |||||
| 147 | } | 166 | } | ||
| 148 | } | 167 | } | ||
| 149 | 168 | ||||
| 150 | - | func (s *Service) sweepExpiredCerts() { | 169 | + | func (s *Service) sweepExpiredCerts(ctx context.Context) { |
| 151 | now := time.Now() | 170 | now := time.Now() | ||
| 152 | - | ctx := context.Background() | 171 | + | |
| 172 | + | // Fetch routes once for all cache entries. | |||
| 173 | + | routes := s.routes.GetRoutes(ctx) | |||
| 174 | + | extRoutes := s.routes.GetExternalRoutes() | |||
| 175 | + | ||||
| 176 | + | allowed := make(map[string]struct{}, len(routes)+len(extRoutes)) | |||
| 177 | + | for _, r := range routes { | |||
| 178 | + | allowed[r.Domain] = struct{}{} | |||
| 179 | + | } | |||
| 180 | + | for d := range extRoutes { | |||
| 181 | + | allowed[d] = struct{}{} | |||
| 182 | + | } | |||
| 183 | + | ||||
| 153 | swept := 0 | 184 | swept := 0 | ||
| 154 | s.cache.Range(func(key, value any) bool { | 185 | s.cache.Range(func(key, value any) bool { | ||
| 155 | domain := key.(string) | 186 | domain := key.(string) | ||
| 156 | cached := value.(*cachedCert) | 187 | cached := value.(*cachedCert) | ||
| 157 | - | if now.After(cached.expiresAt) || !s.isDomainAllowed(ctx, domain) { | 188 | + | _, ok := allowed[domain] |
| 189 | + | if now.After(cached.expiresAt) || !ok { | |||
| 158 | s.cache.Delete(key) | 190 | s.cache.Delete(key) | ||
| 159 | swept++ | 191 | swept++ | ||
| 160 | } | 192 | } | ||
M internal/usecase/pki/service_test.go +1 -1
| @@ -67,7 +67,7 @@ func TestService_GetCertificate_UnknownDomain(t *testing.T) { | |||
| 67 | 67 | ||
| 68 | 68 | hello := &tls.ClientHelloInfo{ServerName: "unknown.example.com"} | |
| 69 | 69 | cert, err := svc.GetCertificate(hello) | |
| 70 | - | assert.Error(t, err) | |
| 70 | + | assert.NoError(t, err) | |
| 71 | 71 | assert.Nil(t, cert) | |
| 72 | 72 | } | |
| 73 | 73 | ||
| @@ -67,7 +67,7 @@ func TestService_GetCertificate_UnknownDomain(t *testing.T) { | |||||
| 67 | 67 | ||||
| 68 | hello := &tls.ClientHelloInfo{ServerName: "unknown.example.com"} | 68 | hello := &tls.ClientHelloInfo{ServerName: "unknown.example.com"} | ||
| 69 | cert, err := svc.GetCertificate(hello) | 69 | cert, err := svc.GetCertificate(hello) | ||
| 70 | - | assert.Error(t, err) | 70 | + | assert.NoError(t, err) |
| 71 | assert.Nil(t, cert) | 71 | assert.Nil(t, cert) | ||
| 72 | } | 72 | } | ||
| 73 | 73 | ||||