awsacmpca: thread ctx through factory + registry — fix CI contextcheck

Follow-up to 590f654 (awsacmpca: replace stub client with AWS SDK v2
implementation). CI's golangci-lint contextcheck rule flagged six
violations in awsacmpca_test.go where mustNew/awsacmpca.New were
called from test functions that had ctx in scope but didn't thread it
through New(). The previous commit used context.Background() inside
New() with the rationale that "the audit allows either threading or
documenting the limitation"; CI made that choice for us.

Threading ctx is the right shape per the audit's stated preference.
The fix cascades from awsacmpca.New through issuerfactory.NewFromConfig
and IssuerRegistry.Rebuild because the contextcheck rule propagates
upward through every caller that has ctx in scope.

This commit:
- Changes awsacmpca.New(config, logger) to
  awsacmpca.New(ctx, config, logger). The ctx is passed to
  buildSDKClient → awsconfig.LoadDefaultConfig so SDK credential chain
  resolution honors caller deadlines (LoadDefaultConfig may probe IMDS
  or remote credential sources). The doc-comment on New explains that
  callers without a useful deadline should pass context.Background()
  and that the SDK has internal credential-resolution timeouts.
- Adds ctx as the first parameter of issuerfactory.NewFromConfig.
  Currently only the AWSACMPCA branch uses ctx (it's threaded into
  awsacmpca.New); the other 11 branches accept ctx without using it.
  This is a contractual change that lets callers thread ctx through
  without contextcheck warnings, even though most issuer constructors
  do no ctx-aware work today.
- Adds ctx as the first parameter of IssuerRegistry.Rebuild. Rebuild
  iterates over configs and calls NewFromConfig per issuer; the same
  ctx flows through every connector instantiation.
- Updates the two production call sites in internal/service:
  - issuer.go:279 (TestIssuer connection test) now passes its
    method-scoped ctx
  - issuer.go:303 (BuildRegistry) now passes its method-scoped ctx
    to Rebuild
- Updates 13 test sites in internal/connector/issuerfactory/factory_test.go
  via a new testCtx() helper that returns context.Background(). Helper
  is dedicated to this file so contextcheck's "you have a ctx in scope,
  pass it" rule doesn't fire on test functions that don't otherwise
  need ctx.
- Updates 6 test sites in internal/service/issuer_registry_test.go
  to pass context.Background() to Rebuild.
- Removes the now-stale "// NewFromConfig has no ctx parameter
  (preserved across all 12 connectors); pass context.Background() ..."
  comment from the awsacmpca branch in factory.go — that workaround
  is no longer the design.

Verified locally:
- gofmt -l . clean
- go vet ./... clean
- staticcheck ./... clean
- golangci-lint run --timeout 5m ./... clean (was failing with 6
  contextcheck issues before the cascade; now 0 issues)
- go test -short -count=1 across all changed packages green

Sandbox couldn't run the existing CI's full make verify due to
disk pressure on /sessions and a virtiofs concurrent-open-file
ceiling on go mod tidy; operator should run `make verify` on
the workstation to confirm.

Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md
Top-10 fix #1 (CI follow-up; behavior unchanged from 590f654).
This commit is contained in:
shankar0123
2026-05-01 23:27:25 +00:00
parent 590f654b0d
commit 804a1b05ce
7 changed files with 68 additions and 45 deletions
@@ -176,7 +176,12 @@ type Connector struct {
//
// Callers wanting to inject a mock client (tests, fake CAs) should use
// NewWithClient instead, which bypasses the SDK loading path entirely.
func New(config *Config, logger *slog.Logger) (*Connector, error) {
//
// ctx is used only for the SDK config load (LoadDefaultConfig may probe IMDS
// or remote credential sources). Callers that don't have a useful deadline
// should pass context.Background(); the SDK has its own internal timeouts
// for credential resolution.
func New(ctx context.Context, config *Config, logger *slog.Logger) (*Connector, error) {
if config != nil {
if config.SigningAlgorithm == "" {
config.SigningAlgorithm = "SHA256WITHRSA"
@@ -192,7 +197,7 @@ func New(config *Config, logger *slog.Logger) (*Connector, error) {
}
if config != nil && config.Region != "" {
client, err := buildSDKClient(context.Background(), config.Region)
client, err := buildSDKClient(ctx, config.Region)
if err != nil {
return nil, fmt.Errorf("AWS ACM PCA SDK init: %w", err)
}
@@ -72,11 +72,14 @@ func (m *mockACMPCAClient) GetCACertificate(ctx context.Context, input *awsacmpc
// mustNew is a test helper that calls awsacmpca.New and fails the test if
// New returns an error. Use this for the ValidateConfig-only test sites
// where config is nil; New(nil, ...) skips SDK loading and never errors,
// so this helper is just to keep the call sites terse.
func mustNew(t *testing.T, config *awsacmpca.Config, logger *slog.Logger) *awsacmpca.Connector {
// where config is nil; New(ctx, nil, ...) skips SDK loading and never
// errors, so this helper is just to keep the call sites terse. The ctx
// parameter exists for contextcheck-lint cleanliness — when callers have
// ctx in scope, they should pass it through to New, which threads it
// into the SDK config load.
func mustNew(t *testing.T, ctx context.Context, config *awsacmpca.Config, logger *slog.Logger) *awsacmpca.Connector {
t.Helper()
c, err := awsacmpca.New(config, logger)
c, err := awsacmpca.New(ctx, config, logger)
if err != nil {
t.Fatalf("awsacmpca.New: %v", err)
}
@@ -155,7 +158,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
ValidityDays: 365,
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err != nil {
@@ -172,7 +175,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
TemplateArn: "arn:aws:acm-pca:eu-west-1:123456789012:template/WebServer",
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err != nil {
@@ -181,7 +184,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
})
t.Run("ValidateConfig_InvalidJSON", func(t *testing.T) {
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
err := connector.ValidateConfig(ctx, []byte(`{invalid json}`))
if err == nil {
t.Fatal("Expected error for invalid JSON")
@@ -196,7 +199,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
CAArn: "arn:aws:acm-pca:us-east-1:123456789012:certificate-authority/12345678-1234-1234-1234-123456789012",
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err == nil {
@@ -212,7 +215,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
Region: "us-east-1",
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err == nil {
@@ -229,7 +232,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
CAArn: "not-an-arn",
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err == nil {
@@ -247,7 +250,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
SigningAlgorithm: "INVALID_ALGO",
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err == nil {
@@ -265,7 +268,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
ValidityDays: -1,
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err == nil {
@@ -595,7 +598,7 @@ func TestAWSACMPCAConnector(t *testing.T) {
// SigningAlgorithm and ValidityDays not set
}
connector := mustNew(t, nil, logger)
connector := mustNew(t, ctx, nil, logger)
rawConfig, _ := json.Marshal(config)
err := connector.ValidateConfig(ctx, rawConfig)
if err != nil {
@@ -668,7 +671,7 @@ func TestNew_ProductionPath(t *testing.T) {
Region: "us-east-1",
CAArn: "arn:aws:acm-pca:us-east-1:123456789012:certificate-authority/12345678-1234-1234-1234-123456789012",
}
c, err := awsacmpca.New(cfg, logger)
c, err := awsacmpca.New(ctx, cfg, logger)
if err != nil {
t.Fatalf("New with valid config returned error: %v", err)
}
@@ -702,7 +705,7 @@ func TestNew_ProductionPath(t *testing.T) {
// the connector is constructed with no client and ValidateConfig
// must be called before any operation. This documents the lazy
// initialization contract.
c, err := awsacmpca.New(nil, logger)
c, err := awsacmpca.New(ctx, nil, logger)
if err != nil {
t.Fatalf("New(nil, logger) returned error: %v", err)
}
@@ -728,7 +731,7 @@ func TestNew_ProductionPath(t *testing.T) {
// New(nil, ...) leaves client nil; ValidateConfig with a valid
// config should build it. After ValidateConfig succeeds, client-
// using methods should work end-to-end (modulo network errors).
c, err := awsacmpca.New(nil, logger)
c, err := awsacmpca.New(ctx, nil, logger)
if err != nil {
t.Fatalf("New(nil, logger): %v", err)
}
@@ -760,7 +763,7 @@ func TestNew_ProductionPath(t *testing.T) {
t.Run("RevokeBeforeInitFailsFast", func(t *testing.T) {
// The audit also flagged RevokeCertificate as part of the stub
// blocker. Verify the nil-client guard fires for revoke too.
c, err := awsacmpca.New(nil, logger)
c, err := awsacmpca.New(ctx, nil, logger)
if err != nil {
t.Fatalf("New(nil, logger): %v", err)
}
@@ -776,7 +779,7 @@ func TestNew_ProductionPath(t *testing.T) {
})
t.Run("GetCAPEMBeforeInitFailsFast", func(t *testing.T) {
c, err := awsacmpca.New(nil, logger)
c, err := awsacmpca.New(ctx, nil, logger)
if err != nil {
t.Fatalf("New(nil, logger): %v", err)
}
+9 -2
View File
@@ -1,6 +1,7 @@
package issuerfactory
import (
"context"
"encoding/json"
"fmt"
"log/slog"
@@ -23,7 +24,13 @@ import (
// NewFromConfig instantiates an issuer connector from its type string and config JSON.
// The config JSON keys use snake_case matching the connector Config struct json tags.
// This replaces the manual wiring in cmd/server/main.go.
func NewFromConfig(issuerType string, configJSON json.RawMessage, logger *slog.Logger) (issuer.Connector, error) {
//
// ctx is currently used only by the AWSACMPCA branch (passed to
// awsconfig.LoadDefaultConfig for SDK credential chain resolution). Other
// connectors take no context at construction; the parameter is kept on the
// signature so callers that have a ctx in scope thread it through cleanly
// (contextcheck linter).
func NewFromConfig(ctx context.Context, issuerType string, configJSON json.RawMessage, logger *slog.Logger) (issuer.Connector, error) {
if len(configJSON) == 0 {
configJSON = []byte("{}")
}
@@ -90,7 +97,7 @@ func NewFromConfig(issuerType string, configJSON json.RawMessage, logger *slog.L
if err := json.Unmarshal(configJSON, &cfg); err != nil {
return nil, fmt.Errorf("invalid AWS ACM PCA config: %w", err)
}
conn, err := awsacmpca.New(&cfg, logger)
conn, err := awsacmpca.New(ctx, &cfg, logger)
if err != nil {
return nil, fmt.Errorf("AWS ACM PCA init: %w", err)
}
@@ -1,6 +1,7 @@
package issuerfactory
import (
"context"
"encoding/json"
"log/slog"
"os"
@@ -11,9 +12,14 @@ func testLogger() *slog.Logger {
return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError}))
}
// testCtx is a fresh background context per test. The factory takes ctx
// for the AWSACMPCA SDK config load; other connectors ignore it. Tests
// use a dedicated helper so contextcheck doesn't cascade.
func testCtx() context.Context { return context.Background() }
func TestNewFromConfig_LocalCA(t *testing.T) {
cfg := json.RawMessage(`{"ca_common_name":"Test CA"}`)
conn, err := NewFromConfig("local", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "local", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(local) failed: %v", err)
}
@@ -24,7 +30,7 @@ func TestNewFromConfig_LocalCA(t *testing.T) {
func TestNewFromConfig_GenericCA_Alias(t *testing.T) {
cfg := json.RawMessage(`{}`)
conn, err := NewFromConfig("GenericCA", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "GenericCA", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(GenericCA) failed: %v", err)
}
@@ -35,7 +41,7 @@ func TestNewFromConfig_GenericCA_Alias(t *testing.T) {
func TestNewFromConfig_ACME(t *testing.T) {
cfg := json.RawMessage(`{"directory_url":"https://acme-staging-v02.api.letsencrypt.org/directory","email":"test@example.com"}`)
conn, err := NewFromConfig("ACME", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "ACME", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(ACME) failed: %v", err)
}
@@ -46,7 +52,7 @@ func TestNewFromConfig_ACME(t *testing.T) {
func TestNewFromConfig_StepCA(t *testing.T) {
cfg := json.RawMessage(`{"ca_url":"https://ca.internal:9000","provisioner_name":"test"}`)
conn, err := NewFromConfig("StepCA", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "StepCA", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(StepCA) failed: %v", err)
}
@@ -57,7 +63,7 @@ func TestNewFromConfig_StepCA(t *testing.T) {
func TestNewFromConfig_OpenSSL(t *testing.T) {
cfg := json.RawMessage(`{"sign_script":"/path/to/sign.sh"}`)
conn, err := NewFromConfig("OpenSSL", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "OpenSSL", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(OpenSSL) failed: %v", err)
}
@@ -68,7 +74,7 @@ func TestNewFromConfig_OpenSSL(t *testing.T) {
func TestNewFromConfig_VaultPKI(t *testing.T) {
cfg := json.RawMessage(`{"addr":"https://vault:8200","token":"hvs.test","mount":"pki","role":"web","ttl":"8760h"}`)
conn, err := NewFromConfig("VaultPKI", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "VaultPKI", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(VaultPKI) failed: %v", err)
}
@@ -79,7 +85,7 @@ func TestNewFromConfig_VaultPKI(t *testing.T) {
func TestNewFromConfig_DigiCert(t *testing.T) {
cfg := json.RawMessage(`{"api_key":"test-key","org_id":"123","product_type":"ssl_basic"}`)
conn, err := NewFromConfig("DigiCert", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "DigiCert", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(DigiCert) failed: %v", err)
}
@@ -90,7 +96,7 @@ func TestNewFromConfig_DigiCert(t *testing.T) {
func TestNewFromConfig_Sectigo(t *testing.T) {
cfg := json.RawMessage(`{"customer_uri":"test-org","login":"api-user","password":"secret","org_id":1}`)
conn, err := NewFromConfig("Sectigo", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "Sectigo", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(Sectigo) failed: %v", err)
}
@@ -101,7 +107,7 @@ func TestNewFromConfig_Sectigo(t *testing.T) {
func TestNewFromConfig_GoogleCAS(t *testing.T) {
cfg := json.RawMessage(`{"project":"my-project","location":"us-central1","ca_pool":"my-pool","credentials":"/path/to/creds.json"}`)
conn, err := NewFromConfig("GoogleCAS", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "GoogleCAS", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(GoogleCAS) failed: %v", err)
}
@@ -112,7 +118,7 @@ func TestNewFromConfig_GoogleCAS(t *testing.T) {
func TestNewFromConfig_UnknownType(t *testing.T) {
cfg := json.RawMessage(`{}`)
_, err := NewFromConfig("UnknownCA", cfg, testLogger())
_, err := NewFromConfig(testCtx(), "UnknownCA", cfg, testLogger())
if err == nil {
t.Fatal("expected error for unknown type")
}
@@ -120,7 +126,7 @@ func TestNewFromConfig_UnknownType(t *testing.T) {
func TestNewFromConfig_MalformedJSON(t *testing.T) {
cfg := json.RawMessage(`{invalid json}`)
_, err := NewFromConfig("ACME", cfg, testLogger())
_, err := NewFromConfig(testCtx(), "ACME", cfg, testLogger())
if err == nil {
t.Fatal("expected error for malformed JSON")
}
@@ -128,7 +134,7 @@ func TestNewFromConfig_MalformedJSON(t *testing.T) {
func TestNewFromConfig_EmptyConfig(t *testing.T) {
// Empty config should work — connectors have defaults
conn, err := NewFromConfig("local", nil, testLogger())
conn, err := NewFromConfig(testCtx(), "local", nil, testLogger())
if err != nil {
t.Fatalf("NewFromConfig with nil config failed: %v", err)
}
@@ -139,7 +145,7 @@ func TestNewFromConfig_EmptyConfig(t *testing.T) {
func TestNewFromConfig_AWSACMPCA(t *testing.T) {
cfg := json.RawMessage(`{"project":"my-project","location":"us-central1","ca_pool":"my-pool","credentials":"/path/to/creds.json"}`)
conn, err := NewFromConfig("AWSACMPCA", cfg, testLogger())
conn, err := NewFromConfig(testCtx(), "AWSACMPCA", cfg, testLogger())
if err != nil {
t.Fatalf("NewFromConfig(AWSACMPCA) failed: %v", err)
}
+2 -2
View File
@@ -276,7 +276,7 @@ func (s *IssuerService) TestConnection(ctx context.Context, id string) error {
}
// Instantiate a throwaway connector and validate
connector, err := issuerfactory.NewFromConfig(string(iss.Type), configJSON, s.logger)
connector, err := issuerfactory.NewFromConfig(ctx, string(iss.Type), configJSON, s.logger)
if err != nil {
s.updateTestStatus(ctx, iss, "failed")
return fmt.Errorf("failed to create connector: %w", err)
@@ -300,7 +300,7 @@ func (s *IssuerService) BuildRegistry(ctx context.Context) error {
return fmt.Errorf("failed to load issuers from database: %w", err)
}
if err := s.registry.Rebuild(issuers, s.encryptionKey); err != nil {
if err := s.registry.Rebuild(ctx, issuers, s.encryptionKey); err != nil {
// Log the error but don't fail — some issuers loaded successfully.
s.logger.Warn("issuer registry rebuilt with errors", "error", err)
}
+3 -2
View File
@@ -1,6 +1,7 @@
package service
import (
"context"
"encoding/json"
"fmt"
"log/slog"
@@ -115,7 +116,7 @@ func (r *IssuerRegistry) Len() int {
// for v2 blobs is performed inside [crypto.DecryptIfKeySet]. Empty passphrase
// fails closed via [crypto.ErrEncryptionKeyRequired] when encrypted configs
// are encountered. See M-8 in certctl-audit-report.md.
func (r *IssuerRegistry) Rebuild(configs []*domain.Issuer, encryptionKey string) error {
func (r *IssuerRegistry) Rebuild(ctx context.Context, configs []*domain.Issuer, encryptionKey string) error {
newIssuers := make(map[string]IssuerConnector)
var errors []string
@@ -141,7 +142,7 @@ func (r *IssuerRegistry) Rebuild(configs []*domain.Issuer, encryptionKey string)
configJSON = json.RawMessage("{}")
}
connector, err := issuerfactory.NewFromConfig(string(cfg.Type), configJSON, r.logger)
connector, err := issuerfactory.NewFromConfig(ctx, string(cfg.Type), configJSON, r.logger)
if err != nil {
errors = append(errors, fmt.Sprintf("issuer %s: factory error: %v", cfg.ID, err))
continue
+7 -6
View File
@@ -1,6 +1,7 @@
package service
import (
"context"
"encoding/json"
"log/slog"
"os"
@@ -101,7 +102,7 @@ func TestIssuerRegistry_Rebuild_Enabled(t *testing.T) {
},
}
err := reg.Rebuild(configs, "")
err := reg.Rebuild(context.Background(), configs, "")
if err != nil {
t.Fatalf("Rebuild failed: %v", err)
}
@@ -142,7 +143,7 @@ func TestIssuerRegistry_Rebuild_WithEncryption(t *testing.T) {
},
}
err = reg.Rebuild(configs, "test-key")
err = reg.Rebuild(context.Background(), configs, "test-key")
if err != nil {
t.Fatalf("Rebuild with encryption failed: %v", err)
}
@@ -168,7 +169,7 @@ func TestIssuerRegistry_Rebuild_NilKeyFallback(t *testing.T) {
// Empty passphrase is safe when no EncryptedConfig is present — falls back to config column.
// The C-2 fail-closed sentinel only fires when EncryptedConfig is non-empty.
err := reg.Rebuild(configs, "")
err := reg.Rebuild(context.Background(), configs, "")
if err != nil {
t.Fatalf("Rebuild with empty key failed: %v", err)
}
@@ -200,7 +201,7 @@ func TestIssuerRegistry_Rebuild_InvalidConfig(t *testing.T) {
}
// Should return an error indicating partial failure, but still load valid issuers
err := reg.Rebuild(configs, "")
err := reg.Rebuild(context.Background(), configs, "")
if err == nil {
t.Fatal("Rebuild should return error when some issuers fail to load")
}
@@ -232,7 +233,7 @@ func TestIssuerRegistry_Rebuild_ReplacesExisting(t *testing.T) {
},
}
err := reg.Rebuild(configs, "")
err := reg.Rebuild(context.Background(), configs, "")
if err != nil {
t.Fatalf("Rebuild failed: %v", err)
}
@@ -277,7 +278,7 @@ func TestIssuerRegistry_Rebuild_Empty(t *testing.T) {
reg.Set("iss-existing", &mockIssuerConnector{})
err := reg.Rebuild([]*domain.Issuer{}, "")
err := reg.Rebuild(context.Background(), []*domain.Issuer{}, "")
if err != nil {
t.Fatalf("Rebuild with empty configs failed: %v", err)
}