From 81f63213263c93e8949f999e7b15bd745ca98d0f Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 3 May 2026 20:38:19 +0000 Subject: [PATCH] ejbca: port mTLS keypair to mtlscache (close Bundle M for the last issuer) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes Top-10 fix #1 of the 2026-05-03 issuer-coverage audit (see cowork/issuer-coverage-audit-2026-05-03/RESULTS.md). Pre-fix, ejbca.go::New called tls.LoadX509KeyPair once at construction and configured the keypair into *http.Transport.TLSClientConfig with no mtime watch. mTLS rotation required a server restart — quarterly rotation per any reasonable security policy = quarterly deploy outage. Bundle M from the prior 2026-05-01 audit shipped the mtlscache helper at internal/connector/issuer/mtlscache/cache.go and wired it into Entrust + GlobalSign. EJBCA was missed in Bundle M's scope. This commit ports the same helper onto EJBCA's auth_mode=mtls path. The OAuth2 path is unchanged. Implementation: - New imports internal/connector/issuer/mtlscache. - Connector struct gains an mtls *mtlscache.Cache field (mirroring Entrust + GlobalSign). - New()'s case 'mtls': replaces tls.LoadX509KeyPair + manual *http.Transport with mtlscache.New(certPath, keyPath, Options{HTTPTimeout: 30s}). Cache build happens at construction so misconfigured operators fail fast (matches pre-fix behaviour). - New helper getHTTPClient() returns the cached client; on the mTLS path it calls RefreshIfStale before returning so the next request uses the new keypair if disk has rotated. On OAuth2 / test paths (c.mtls == nil), returns c.httpClient as-is. - All 3 c.httpClient.Do call sites (IssueCertificate enroll, RevokeCertificate revoke, GetOrderStatus cert lookup) replaced with c.getHTTPClient() + client.Do. - crypto/tls import removed (no longer used at this layer). Tests: - TestEJBCA_MTLSKeypairRotation_PicksUpNewCertWithoutRestart (new, ejbca_mtls_rotation_test.go): generates two CAs (caA, caB), signs leafA + leafB, spins up an httptest TLS server that trusts both CAs and records the issuer DN of every presented client cert, writes leafA, makes request 1, writes leafB + advances mtime by 2s, makes request 2. Asserts the server saw caA's DN on req 1 and caB's DN on req 2 — the cache picked up the rotation without ejbca.New re-running. - export_test.go: GetHTTPClientForTest helper exposes the private getHTTPClient so the rotation test drives the production code path. - All existing EJBCA tests still pass (TestNew_MTLSWiresClientCert, TestNew_MTLSCertLoadFailure, TestNew_OAuth2NoTransportTuning, TestNew_InvalidAuthMode). Verified locally: - gofmt clean across the repo. - go vet ./... clean across the repo. - go test -race -count=1 -short ./internal/connector/issuer/ejbca/... ./internal/connector/issuer/mtlscache/... green. Audit reference: cowork/issuer-coverage-audit-2026-05-03/ RESULTS.md Top-10 fix #1. --- internal/connector/issuer/ejbca/ejbca.go | 95 +++++-- .../issuer/ejbca/ejbca_mtls_rotation_test.go | 260 ++++++++++++++++++ .../connector/issuer/ejbca/export_test.go | 9 + 3 files changed, 337 insertions(+), 27 deletions(-) create mode 100644 internal/connector/issuer/ejbca/ejbca_mtls_rotation_test.go diff --git a/internal/connector/issuer/ejbca/ejbca.go b/internal/connector/issuer/ejbca/ejbca.go index b2611af..596354f 100644 --- a/internal/connector/issuer/ejbca/ejbca.go +++ b/internal/connector/issuer/ejbca/ejbca.go @@ -20,7 +20,6 @@ package ejbca import ( "bytes" "context" - "crypto/tls" "crypto/x509" "encoding/base64" "encoding/json" @@ -33,6 +32,7 @@ import ( "time" "github.com/shankar0123/certctl/internal/connector/issuer" + "github.com/shankar0123/certctl/internal/connector/issuer/mtlscache" "github.com/shankar0123/certctl/internal/secret" ) @@ -84,16 +84,32 @@ 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, AND picks up rotated certs + // on the next call without a process restart. nil on the + // OAuth2 path (auth_mode=oauth2) and on the test path + // (NewWithHTTPClient). Closes Top-10 fix #1 of the + // 2026-05-03 issuer-coverage audit. + mtls *mtlscache.Cache } // New creates a new EJBCA connector with the given configuration and logger. // // When config.AuthMode is "mtls" (or empty — mtls is the default), New -// loads config.ClientCertPath + config.ClientKeyPath via tls.LoadX509KeyPair -// and configures *http.Transport.TLSClientConfig so the client presents the -// cert on every request. When AuthMode is "oauth2", New returns a client -// with no transport customization (the OAuth2 Bearer header path is wired -// in setAuthHeaders). Any other AuthMode value returns (nil, error). +// builds an mtlscache.Cache from config.ClientCertPath + config.ClientKeyPath. +// The cache parses the keypair once and configures +// *http.Transport.TLSClientConfig so the client presents the cert on every +// request. Subsequent calls go through getHTTPClient, which calls +// RefreshIfStale on the cache — operators rotating the cert+key on disk +// (e.g. quarterly per security policy) get the new keypair on the next +// API call without a server restart. Closes Top-10 fix #1 of the +// 2026-05-03 issuer-coverage audit. +// +// When AuthMode is "oauth2", New returns a client with no transport +// customization (the OAuth2 Bearer header path is wired in +// setAuthHeaders). Any other AuthMode value returns (nil, error). // // Returns an error if mTLS cert/key load fails (missing file, malformed // PEM, mismatched cert/key) so misconfigured operators get an immediate @@ -110,30 +126,24 @@ func New(config *Config, logger *slog.Logger) (*Connector, error) { switch authMode { case "mtls": - // Build a fresh *http.Transport (do NOT clone http.DefaultTransport - // — mutation would leak across the package boundary). Set - // MinVersion: TLS 1.2 as a compatibility floor for on-prem EJBCA - // installs that may predate TLS 1.3. if config == nil || config.ClientCertPath == "" || config.ClientKeyPath == "" { return nil, fmt.Errorf("EJBCA mTLS requires client_cert_path and client_key_path") } - cert, err := tls.LoadX509KeyPair(config.ClientCertPath, config.ClientKeyPath) + // Build the cache up-front so misconfigured operators fail fast + // at construction rather than discover a broken cert path on + // the first issuance call. mtlscache enforces TLS 1.2 floor + // (compat with on-prem EJBCA installs that predate TLS 1.3). + cache, err := mtlscache.New(config.ClientCertPath, config.ClientKeyPath, mtlscache.Options{ + HTTPTimeout: 30 * time.Second, + }) if err != nil { - return nil, fmt.Errorf("EJBCA mTLS cert load: %w", err) - } - transport := &http.Transport{ - TLSClientConfig: &tls.Config{ - Certificates: []tls.Certificate{cert}, - MinVersion: tls.VersionTLS12, - }, + return nil, fmt.Errorf("EJBCA mTLS cache build: %w", err) } return &Connector{ - config: config, - logger: logger, - httpClient: &http.Client{ - Timeout: 30 * time.Second, - Transport: transport, - }, + config: config, + logger: logger, + httpClient: cache.Client(), + mtls: cache, }, nil case "oauth2": // OAuth2 path uses default transport; setAuthHeaders adds the @@ -159,6 +169,25 @@ func NewWithHTTPClient(config *Config, logger *slog.Logger, client *http.Client) } } +// getHTTPClient returns the HTTP client to use for an EJBCA API call. +// On the mTLS path (auth_mode=mtls), it calls RefreshIfStale on the +// mtlscache so a rotated keypair on disk is picked up before the next +// request — operators rotating their EJBCA client cert quarterly no +// longer need a server restart. On the OAuth2 path (auth_mode=oauth2) +// or the test path (NewWithHTTPClient), it returns c.httpClient as-is +// because there's no keypair to refresh. Closes Top-10 fix #1 of the +// 2026-05-03 issuer-coverage audit. Mirrors the Entrust/GlobalSign +// pattern from Bundle M of the 2026-05-01 audit. +func (c *Connector) getHTTPClient() (*http.Client, error) { + if c.mtls == nil { + return c.httpClient, nil + } + if err := c.mtls.RefreshIfStale(); err != nil { + return nil, fmt.Errorf("EJBCA mTLS cache refresh: %w", err) + } + return c.mtls.Client(), nil +} + // enrollResponse represents the EJBCA /certificate/pkcs10enroll response. type enrollResponse struct { Certificate string `json:"certificate"` @@ -250,7 +279,11 @@ func (c *Connector) IssueCertificate(ctx context.Context, request issuer.Issuanc c.setAuthHeaders(req) req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) + client, err := c.getHTTPClient() + if err != nil { + return nil, err + } + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("EJBCA enroll request failed: %w", err) } @@ -392,7 +425,11 @@ func (c *Connector) RevokeCertificate(ctx context.Context, request issuer.Revoca c.setAuthHeaders(req) req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) + client, err := c.getHTTPClient() + if err != nil { + return err + } + resp, err := client.Do(req) if err != nil { return fmt.Errorf("EJBCA revoke request failed: %w", err) } @@ -438,7 +475,11 @@ func (c *Connector) GetOrderStatus(ctx context.Context, orderID string) (*issuer c.setAuthHeaders(req) - resp, err := c.httpClient.Do(req) + client, err := c.getHTTPClient() + if err != nil { + return nil, err + } + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("EJBCA cert get request failed: %w", err) } diff --git a/internal/connector/issuer/ejbca/ejbca_mtls_rotation_test.go b/internal/connector/issuer/ejbca/ejbca_mtls_rotation_test.go new file mode 100644 index 0000000..ff69e62 --- /dev/null +++ b/internal/connector/issuer/ejbca/ejbca_mtls_rotation_test.go @@ -0,0 +1,260 @@ +package ejbca_test + +// Top-10 fix #1 of the 2026-05-03 issuer-coverage audit. Pre-fix, +// ejbca.New called tls.LoadX509KeyPair once at construction; rotating +// the client cert+key on disk required a server restart to take +// effect. Post-fix, ejbca.New constructs an mtlscache.Cache and the +// hot-path getHTTPClient calls RefreshIfStale before every API +// request — operators rotating quarterly per security policy no +// longer pay the deploy outage. +// +// This test pins the rotation behaviour end-to-end: write certA, +// make one mTLS request, write certB at the same paths, advance +// mtime, make a second request, assert the leaf cert presented on +// the wire flipped from certA to certB. + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "io" + "log/slog" + "math/big" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/shankar0123/certctl/internal/connector/issuer/ejbca" +) + +// TestEJBCA_MTLSKeypairRotation_PicksUpNewCertWithoutRestart verifies +// the mtlscache wiring catches a hot-rotated keypair. Sequence: +// +// 1. Generate caA + caB roots (different CAs so the server can tell +// which client cert was presented by inspecting the issuer DN). +// 2. Sign clientA against caA, clientB against caB. +// 3. Spin up an httptest TLS server that requires a client cert +// signed by caA OR caB (ClientCAs pool with both roots) and +// records which CA actually signed the presented client cert. +// 4. Write clientA's cert+key to {certPath, keyPath}. +// 5. Construct ejbca.Connector via production New (mTLS mode). +// 6. Make request #1 → server records "presented cert from caA". +// 7. Overwrite {certPath, keyPath} with clientB's cert+key. Advance +// mtime via os.Chtimes (ext4 mtime granularity is 1s; advancing +// by 2s defeats the no-op cheap path). +// 8. Make request #2 → server records "presented cert from caB". +// 9. Assert the two recorded issuers differ — the cache picked up +// the rotation without ejbca.New re-running. +func TestEJBCA_MTLSKeypairRotation_PicksUpNewCertWithoutRestart(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + dir := t.TempDir() + certPath := filepath.Join(dir, "client.crt") + keyPath := filepath.Join(dir, "client.key") + + caA, caACert, caAKey := mustCA(t, "EJBCA-RotationTest-CA-A") + caB, caBCert, caBKey := mustCA(t, "EJBCA-RotationTest-CA-B") + + // Sign one leaf cert per CA; both have CN="ejbca-rotation-client" + // so the only distinguishing feature on the wire is the issuer DN. + leafA, leafAKey := mustLeafSignedBy(t, caACert, caAKey) + leafB, leafBKey := mustLeafSignedBy(t, caBCert, caBKey) + + // httptest TLS server with a ClientCAs pool that trusts BOTH + // roots. The handler captures the issuer DN of the presented + // cert into a thread-safe slice the test inspects after the + // requests complete. + pool := x509.NewCertPool() + pool.AddCert(caACert) + pool.AddCert(caBCert) + var ( + mu sync.Mutex + seenIssuerDNs []string + seenCommonNames []string + ) + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 { + http.Error(w, "no client cert", http.StatusUnauthorized) + return + } + leaf := r.TLS.PeerCertificates[0] + mu.Lock() + seenIssuerDNs = append(seenIssuerDNs, leaf.Issuer.String()) + seenCommonNames = append(seenCommonNames, leaf.Subject.CommonName) + mu.Unlock() + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + })) + srv.TLS = &tls.Config{ + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: pool, + MinVersion: tls.VersionTLS12, + } + srv.StartTLS() + defer srv.Close() + + // Step 4 — write clientA initially. + mustWriteKeypair(t, certPath, keyPath, leafA, leafAKey) + + // Step 5 — construct via production New(). + cfg := &ejbca.Config{ + APIUrl: srv.URL, + AuthMode: "mtls", + ClientCertPath: certPath, + ClientKeyPath: keyPath, + CAName: "Management CA", + } + conn, err := ejbca.New(cfg, logger) + if err != nil { + t.Fatalf("ejbca.New: %v", err) + } + + // To talk to httptest's self-signed server cert, mutate the cached + // transport's RootCAs to trust the test server. The Certificates + // field (the client cert) stays intact — that's the field we're + // proving rotates without a New() re-run. + httpClient := ejbca.HTTPClientForTest(conn) + tr := httpClient.Transport.(*http.Transport) + srvPool := x509.NewCertPool() + srvPool.AddCert(srv.Certificate()) + tr.TLSClientConfig.RootCAs = srvPool + + // Step 6 — first request via the production code path. + clientA, err := ejbca.GetHTTPClientForTest(conn) + if err != nil { + t.Fatalf("getHTTPClient (req 1): %v", err) + } + // Re-apply the server-cert trust pool to the freshly-returned + // client's transport (cache rebuild would have wiped any prior + // mutation, but on the cheap path this is a no-op since the same + // transport pointer is returned). + if trA, ok := clientA.Transport.(*http.Transport); ok && trA.TLSClientConfig.RootCAs == nil { + trA.TLSClientConfig.RootCAs = srvPool + } + if _, err := clientA.Get(srv.URL); err != nil { + t.Fatalf("request 1: %v", err) + } + + // Step 7 — overwrite the keypair with leafB, advance mtime. + mustWriteKeypair(t, certPath, keyPath, leafB, leafBKey) + future := time.Now().Add(2 * time.Second) + if err := os.Chtimes(certPath, future, future); err != nil { + t.Fatalf("chtimes cert: %v", err) + } + if err := os.Chtimes(keyPath, future, future); err != nil { + t.Fatalf("chtimes key: %v", err) + } + + // Step 8 — second request. RefreshIfStale should rebuild the + // transport with leafB's keypair. The rebuild creates a new + // transport whose RootCAs is unset, so re-apply. + clientB, err := ejbca.GetHTTPClientForTest(conn) + if err != nil { + t.Fatalf("getHTTPClient (req 2): %v", err) + } + if trB, ok := clientB.Transport.(*http.Transport); ok && trB.TLSClientConfig.RootCAs == nil { + trB.TLSClientConfig.RootCAs = srvPool + } + if _, err := clientB.Get(srv.URL); err != nil { + t.Fatalf("request 2: %v", err) + } + + // Step 9 — assert. + mu.Lock() + defer mu.Unlock() + if len(seenIssuerDNs) != 2 { + t.Fatalf("expected exactly 2 server-side observations, got %d: %v", len(seenIssuerDNs), seenIssuerDNs) + } + if seenIssuerDNs[0] == seenIssuerDNs[1] { + t.Fatalf("issuer DN unchanged across rotation: req1=%q req2=%q — cache did not pick up the new keypair", + seenIssuerDNs[0], seenIssuerDNs[1]) + } + if want := caA.String(); seenIssuerDNs[0] != want { + t.Errorf("req 1 issuer = %q, want %q (caA)", seenIssuerDNs[0], want) + } + if want := caB.String(); seenIssuerDNs[1] != want { + t.Errorf("req 2 issuer = %q, want %q (caB)", seenIssuerDNs[1], want) + } +} + +// --- test helpers ------------------------------------------------------ + +// mustCA generates a self-signed CA cert + key. Returns the parsed CA +// pkix.Name, the parsed cert, and the private key. +func mustCA(t *testing.T, commonName string) (pkix.Name, *x509.Certificate, *ecdsa.PrivateKey) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("CA key gen: %v", err) + } + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: commonName}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + IsCA: true, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + if err != nil { + t.Fatalf("CA cert: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("parse CA: %v", err) + } + return template.Subject, cert, key +} + +// mustLeafSignedBy generates a leaf cert+key, signed by the supplied +// CA. Both rotation halves share the same Subject CN so the +// distinguishing field on the wire is exclusively the Issuer DN. +func mustLeafSignedBy(t *testing.T, caCert *x509.Certificate, caKey *ecdsa.PrivateKey) (*x509.Certificate, *ecdsa.PrivateKey) { + t.Helper() + leafKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("leaf key gen: %v", err) + } + leafTemplate := x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: pkix.Name{CommonName: "ejbca-rotation-client"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + } + der, err := x509.CreateCertificate(rand.Reader, &leafTemplate, caCert, &leafKey.PublicKey, caKey) + if err != nil { + t.Fatalf("leaf cert: %v", err) + } + leaf, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("parse leaf: %v", err) + } + return leaf, leafKey +} + +func mustWriteKeypair(t *testing.T, certPath, keyPath string, leaf *x509.Certificate, leafKey *ecdsa.PrivateKey) { + t.Helper() + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: leaf.Raw}) + if err := os.WriteFile(certPath, certPEM, 0o600); err != nil { + t.Fatalf("write cert: %v", err) + } + keyDER, err := x509.MarshalECPrivateKey(leafKey) + if err != nil { + t.Fatalf("marshal key: %v", err) + } + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) + if err := os.WriteFile(keyPath, keyPEM, 0o600); err != nil { + t.Fatalf("write key: %v", err) + } +} diff --git a/internal/connector/issuer/ejbca/export_test.go b/internal/connector/issuer/ejbca/export_test.go index 88857f5..ce5c221 100644 --- a/internal/connector/issuer/ejbca/export_test.go +++ b/internal/connector/issuer/ejbca/export_test.go @@ -12,3 +12,12 @@ import "net/http" func HTTPClientForTest(c *Connector) *http.Client { return c.httpClient } + +// GetHTTPClientForTest exposes the per-call hot-path getHTTPClient +// helper so the rotation test can drive the production code path +// (which calls RefreshIfStale on the mtls cache before returning +// the client). Production callers reach this same path implicitly +// via IssueCertificate / RevokeCertificate / GetOrderStatus. +func GetHTTPClientForTest(c *Connector) (*http.Client, error) { + return c.getHTTPClient() +}