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:
shankar0123
2026-05-02 14:32:59 +00:00
parent 4b73344acf
commit e02b5fb80d
5 changed files with 694 additions and 24 deletions
@@ -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,