mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-14 21:38:51 +00:00
globalsign,entrust: cache mTLS keypair with mtime-based reload
Closes the #10 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Pre-fix, GlobalSign reloaded the mTLS cert/key from disk on every API call (globalsign.go::getHTTPClient) and Entrust loaded once in ValidateConfig with no rotation handling — both shapes were broken for different reasons. Per-call disk reads under a 100- cert renewal sweep meant 200 file opens / parses / tls.X509KeyPair calls in flight, each adding 5–50ms of latency for nothing; the single-load Entrust shape served stale credentials forever after a cert rotation, requiring a process restart. This commit: - Adds a new shared package internal/connector/issuer/mtlscache/ with a Cache type holding a parsed tls.Certificate plus a precomputed *http.Transport. RWMutex serialises reloads; reads are lock-free in the hot path (read lock briefly held to copy out the *http.Client pointer, then released — the HTTP request itself happens with no lock held, per the audit prompt's anti- pattern about holding the write lock across an API call). - RefreshIfStale stats the cert file; if mtime advanced beyond the last load, the keypair is re-parsed and the transport is rebuilt. The fast path (mtime unchanged) takes the read lock for the comparison and returns immediately. Double-checked-lock pattern (read lock → stat → release → write lock → re-stat) prevents two callers who observed the same stale mtime from both reloading. - Options.TLSConfigBuilder lets the caller customise the *tls.Config built around the parsed leaf certificate. GlobalSign uses this to inject the ServerCAPath-pinning RootCAs pool that buildServerTLSConfig already produces; entrust uses the default builder. - New() performs the initial load so a broken cert path fails fast at construction rather than at first API call. - GlobalSign.Connector gains an mtls field. getHTTPClient now: (1) preserves the test-mode short-circuit when httpClient has a non-nil Transport; (2) preserves the bare-default-client short-circuit when cert paths aren't configured; (3) lazy-builds the cache on the first call so the constructor stays cheap; (4) calls RefreshIfStale on every subsequent call. The error wrap preserves the substring "client certificate" so existing TestGlobalsign_GetHTTPClient_MTLSPathConfigured_LoadsKeyPair keeps its assertion. - Entrust.Connector gains an mtls field plus a new getHTTPClient helper mirroring GlobalSign's shape. The three IssueCertificate / RevokeCertificate / pollEnrollmentOnce sites that previously hit c.httpClient.Do(req) directly now route through getHTTPClient, which falls through to the test-injected client (same logic as GlobalSign) and otherwise serves the cached mTLS client. The legacy ValidateConfig flow that pre-built c.httpClient with its own transport stays intact — its transport wins because getHTTPClient short-circuits when c.httpClient.Transport != nil. - Tests at internal/connector/issuer/mtlscache/cache_test.go cover: * fail-fast on missing paths (constructor input validation) * load on construction (positive + negative) * NoReloadWhenMtimeStable — 100 RefreshIfStale calls, LoadedAt must stay equal to the constructor's stamp (the load-bearing regression guard against per-call disk reads) * ReloadsOnMtimeAdvance — os.Chtimes forward, next refresh must observe the new LoadedAt (the load-bearing regression guard for rotation-without-process-restart) * StatErrorBubbles — missing cert file surfaces as an error rather than silently serving stale credentials * ConcurrentNoRace — 100 goroutines × 50 iterations under -race; no race detected, all calls succeed * TLSConfigBuilderUsed — custom builder is invoked at New AND on reload; verifies MinVersion=TLS1.3 takes effect * ClientHonoursTimeout — Options.HTTPTimeout reaches the constructed *http.Client - docs/connectors.md GlobalSign + Entrust sections each gain an "mTLS keypair caching (audit fix #10)" paragraph documenting the steady-state caching, mtime-based rotation contract, and operator workflow (mv -f new.crt /etc/certctl/.../client.crt). Acquirer impact: removes the per-call disk-read latency floor and makes operator-driven cert rotation a no-restart event. Combined with audit fix #9's bounded scheduler concurrency, the renewal sweep's hot path now has predictable steady-state cost: capN concurrent goroutines, each reusing the cached keypair, no per- call file I/O. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - go test -race -count=1 ./internal/connector/issuer/mtlscache/... green (8 tests) - go test -count=1 -short across globalsign / entrust / sectigo / ejbca / mtlscache / connector packages: green Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #10. Closes the audit's full Top-10 list (fixes #1-10 all shipped to master).
This commit is contained in:
@@ -40,6 +40,7 @@ import (
|
||||
|
||||
"github.com/shankar0123/certctl/internal/connector/issuer"
|
||||
"github.com/shankar0123/certctl/internal/connector/issuer/asyncpoll"
|
||||
"github.com/shankar0123/certctl/internal/connector/issuer/mtlscache"
|
||||
"github.com/shankar0123/certctl/internal/secret"
|
||||
)
|
||||
|
||||
@@ -105,6 +106,14 @@ type Connector struct {
|
||||
config *Config
|
||||
logger *slog.Logger
|
||||
httpClient *http.Client
|
||||
|
||||
// mtls caches the parsed client keypair + a precomputed
|
||||
// *http.Transport so steady-state API calls don't re-parse the
|
||||
// keypair on every request. Audit fix #10. nil in test mode
|
||||
// (NewWithHTTPClient) and on the first ValidateConfig call
|
||||
// before the cache is wired; getHTTPClient falls through to
|
||||
// httpClient when nil so test paths keep their behaviour.
|
||||
mtls *mtlscache.Cache
|
||||
}
|
||||
|
||||
// New creates a new GlobalSign Atlas HVCA connector with the given configuration and logger.
|
||||
@@ -236,36 +245,52 @@ func (c *Connector) ValidateConfig(ctx context.Context, rawConfig json.RawMessag
|
||||
|
||||
// getHTTPClient returns the HTTP client to use, creating one with mTLS if needed.
|
||||
// If the connector was created with NewWithHTTPClient (test mode), uses that client directly.
|
||||
// Otherwise, creates a fresh mTLS client with the configured certificate.
|
||||
// Otherwise, returns the cached mTLS client (audit fix #10), refreshing it
|
||||
// from disk if the cert file's mtime has advanced since the last load —
|
||||
// rotation-via-mv-f takes effect on the next call without a process restart.
|
||||
func (c *Connector) getHTTPClient(ctx context.Context) (*http.Client, error) {
|
||||
// Check if we're in test mode (httpClient was explicitly provided and has non-nil transport)
|
||||
// Test mode: NewWithHTTPClient supplied a pre-built client with a
|
||||
// non-nil Transport. The cache layer must NOT intercept this
|
||||
// branch — tests need their httptest-backed transport, not an
|
||||
// mTLS one against a (probably non-existent) cert file.
|
||||
if c.httpClient != nil && c.httpClient.Transport != nil {
|
||||
return c.httpClient, nil
|
||||
}
|
||||
|
||||
// For tests with default client (nil or minimal), check if cert paths are available
|
||||
// Test mode 2: bare default client + no cert paths configured.
|
||||
// Same rationale — return what the caller supplied as-is.
|
||||
if c.config.ClientCertPath == "" || c.config.ClientKeyPath == "" {
|
||||
// Test mode: use httpClient as-is (won't load certs)
|
||||
return c.httpClient, nil
|
||||
}
|
||||
|
||||
// Production mode: load mTLS certificate
|
||||
cert, err := tls.LoadX509KeyPair(c.config.ClientCertPath, c.config.ClientKeyPath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to load GlobalSign client certificate: %w", err)
|
||||
// Production mode: lazy-build the cache on the first call so the
|
||||
// constructor stays cheap (no disk I/O). Subsequent calls take
|
||||
// the fast path through the cache's RWMutex.
|
||||
if c.mtls == nil {
|
||||
// Capture the config pointer so the TLSConfigBuilder closure
|
||||
// reads the current ServerCAPath. The cache itself owns the
|
||||
// rebuild on rotation.
|
||||
cfg := c.config
|
||||
cache, err := mtlscache.New(c.config.ClientCertPath, c.config.ClientKeyPath, mtlscache.Options{
|
||||
TLSConfigBuilder: func(cert tls.Certificate) (*tls.Config, error) {
|
||||
return buildServerTLSConfig(cfg, cert)
|
||||
},
|
||||
HTTPTimeout: 30 * time.Second,
|
||||
})
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to load GlobalSign client certificate (mTLS cache build): %w", err)
|
||||
}
|
||||
c.mtls = cache
|
||||
} else if err := c.mtls.RefreshIfStale(); err != nil {
|
||||
// stat / parse failure on rotation should bubble up — a
|
||||
// missing cert file is a real outage signal. The cache
|
||||
// keeps serving the previous keypair on parse error
|
||||
// because reload only commits on success, but stat error
|
||||
// is surfaced to the caller.
|
||||
return nil, fmt.Errorf("failed to refresh GlobalSign mTLS cache: %w", err)
|
||||
}
|
||||
|
||||
tlsConfig, err := buildServerTLSConfig(c.config, cert)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to build GlobalSign TLS config: %w", err)
|
||||
}
|
||||
|
||||
return &http.Client{
|
||||
Transport: &http.Transport{
|
||||
TLSClientConfig: tlsConfig,
|
||||
},
|
||||
Timeout: 30 * time.Second,
|
||||
}, nil
|
||||
return c.mtls.Client(), nil
|
||||
}
|
||||
|
||||
// setAuthHeaders writes the GlobalSign double-auth headers (ApiKey,
|
||||
|
||||
Reference in New Issue
Block a user