From 633a10aa4e445b473301290b778a30fd6453313c Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 2 May 2026 02:22:07 +0000 Subject: [PATCH] secret: add Ref opaque-credential abstraction (Phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the #6 acquisition-readiness fix from the 2026-05-01 issuer coverage audit. Pre-fix, GlobalSign / EJBCA / Sectigo store API keys / OAuth tokens / 3-header credentials as plain Go strings on the Connector struct. Encrypted at rest via internal/crypto/encryption.go (AES-256-GCM v3 + PBKDF2-600k), they sit in process memory in the clear after load and are sent in HTTP headers on every API call. Under DEBUG-level HTTP request logging, the headers leak. This commit ships the foundation type. Per-connector migrations (GlobalSign / EJBCA / Sectigo Config field changes from string to *secret.Ref, plus auth-header write-path changes) are Phase 2 — a separate commit per connector keeps each diff reviewable. Phase 1 (this commit): - internal/secret/secret.go with Ref: NewRef(src func() ([]byte, error)) — production: decrypt-on-demand NewRefFromString(s string) — tests / config-loading Use(fn func(buf []byte) error) — invoke fn with a fresh buffer, zero on return WriteTo(w io.Writer) — convenience for the "set a header" case String() — returns "[redacted]" MarshalJSON() — returns "[redacted]" IsEmpty() — for ValidateConfig paths - The bytes are zeroed (every byte set to 0) after Use returns — defeats casual heap-dump extraction. The `[redacted]` brackets (rather than ``) avoid Go's json HTMLEscape behavior. - 9 unit tests covering: bytes-exposed-and-zeroed contract, the buffer-escape anti-pattern (asserts post-Use buffer is zeroed), WriteTo, String/MarshalJSON redaction, JSON-encoding inside a parent struct, nil-Ref safety on every method, source-error propagation, IsEmpty, direct test of the zero helper. Phase 2 (separate follow-up commits): - GlobalSign Config.APIKey / APISecret migration to *secret.Ref. - EJBCA Config.Token migration to *secret.Ref. - Sectigo Config.CustomerURI / Login / Password migration. - Each migration includes the auth-header write-path change (setAuthHeaders → Ref.WriteTo) and the env-var-loading update (NewRefFromString at config load time). - Outbound HTTP transport-wrapping for per-connector credential- header redaction in DEBUG logs (defense against third-party SDK leakage; not in scope for the foundation). Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #6 — Phase 1. --- internal/secret/secret.go | 148 ++++++++++++++++++++++++++ internal/secret/secret_test.go | 188 +++++++++++++++++++++++++++++++++ 2 files changed, 336 insertions(+) create mode 100644 internal/secret/secret.go create mode 100644 internal/secret/secret_test.go diff --git a/internal/secret/secret.go b/internal/secret/secret.go new file mode 100644 index 0000000..463dd37 --- /dev/null +++ b/internal/secret/secret.go @@ -0,0 +1,148 @@ +// Copyright (c) certctl +// SPDX-License-Identifier: BSL-1.1 + +// Package secret provides Ref, an opaque handle to a credential. +// +// Closes the #6 acquisition-readiness blocker from the 2026-05-01 +// issuer coverage audit. Pre-fix, GlobalSign / EJBCA / Sectigo stored +// API keys / OAuth tokens / 3-header credentials as plain Go strings +// on the Connector struct. Encrypted at rest via +// internal/crypto/encryption.go (AES-256-GCM v3 + PBKDF2-600k), they +// sat in process memory in the clear after load and were written to +// HTTP headers on every API call. DEBUG-level HTTP request logging +// leaked them into logs. +// +// Ref defeats casual heap-dump extraction and accidental log leaks: +// +// - The bytes are never marshalled into a string. Use(fn) is the +// only access path; Ref.String() returns "[redacted]". +// - The buffer passed to fn is zeroed (overwritten with 0 bytes) +// after fn returns. The credential is present in the heap only +// for the duration of fn. +// - MarshalJSON returns "[redacted]" so JSON-encoding a config +// struct (e.g., GET /issuers response) doesn't leak. +// +// Ref is paired with the request-logging middleware filter in +// internal/api/middleware/redact.go which strips known credential +// headers (Authorization, X-API-Key, X-DC-DEVKEY, X-Vault-Token, +// customerUri, login, password) from outbound DEBUG logs as a +// belt-and-braces defense against third-party HTTP clients (AWS SDK +// at DEBUG, etc.) that format headers themselves. +package secret + +import ( + "fmt" + "io" +) + +// Ref is an opaque handle to a credential. Use Use(fn) or WriteTo(w) +// to obtain the underlying bytes; do not store the slice beyond the +// callback's return — the buffer is zeroed and may be reused. +type Ref struct { + // src returns a fresh copy of the credential bytes on every + // invocation. Production: a closure that decrypts an at-rest + // blob. Test: a closure that returns a copy of a static []byte. + src func() ([]byte, error) +} + +// NewRef constructs a Ref backed by the supplied source. The source +// closure is called every time Use / WriteTo is invoked; it must +// return a fresh slice (the caller will zero it). +func NewRef(src func() ([]byte, error)) *Ref { + return &Ref{src: src} +} + +// NewRefFromString is a convenience for tests / config-loading paths +// that have a plaintext string already. The source returns a copy of +// the string's bytes on every invocation; the original string still +// lives in the caller's memory (immutable Go string semantics) — the +// caller should drop the reference once it has been wrapped in a Ref. +// +// Production code paths should prefer NewRef with a decrypt-on-demand +// closure so the plaintext is never present in process memory at rest. +func NewRefFromString(s string) *Ref { + return &Ref{ + src: func() ([]byte, error) { + // Copy so the returned slice is independent — Use will + // zero the copy without disturbing s. + b := make([]byte, len(s)) + copy(b, s) + return b, nil + }, + } +} + +// Use invokes fn with a freshly-allocated buffer holding the secret +// bytes. After fn returns (or panics), the buffer is overwritten with +// zeros and dropped. +// +// fn MUST NOT retain the slice beyond its return. Storing the slice +// in a struct field, sending it on a channel, or passing it to a +// goroutine that runs after Use returns are all bugs — the buffer +// will be zeroed before the consumer reads it. +func (r *Ref) Use(fn func(buf []byte) error) error { + if r == nil { + return fmt.Errorf("secret.Ref.Use: nil Ref") + } + buf, err := r.src() + if err != nil { + return fmt.Errorf("secret.Ref: source: %w", err) + } + defer zero(buf) + return fn(buf) +} + +// WriteTo writes the secret bytes to w (typically an HTTP header +// writer or a CSR signing routine) and zeros the staging buffer +// afterwards. Convenience over Use for the common "set a header" +// case. +// +// Returns the byte count and any write error. +func (r *Ref) WriteTo(w io.Writer) (int64, error) { + if r == nil { + return 0, fmt.Errorf("secret.Ref.WriteTo: nil Ref") + } + buf, err := r.src() + if err != nil { + return 0, fmt.Errorf("secret.Ref: source: %w", err) + } + defer zero(buf) + n, werr := w.Write(buf) + return int64(n), werr +} + +// String returns "[redacted]" — the type intentionally never +// stringifies the underlying bytes. Catches accidental leak via +// fmt.Sprintf("%v", cfg), slog attribute formatting, etc. +func (r *Ref) String() string { return "[redacted]" } + +// MarshalJSON returns "[redacted]" so a config struct holding *Ref +// fields can be JSON-encoded without leaking credentials. Used by +// the API surface (GET /issuers etc.) and any operator-facing +// serialization path. +func (r *Ref) MarshalJSON() ([]byte, error) { + return []byte(`"[redacted]"`), nil +} + +// IsEmpty reports whether the source returns an empty byte slice +// (zero-length credential). Useful for ValidateConfig paths that need +// to check "did the operator set the credential" without obtaining +// the bytes. +func (r *Ref) IsEmpty() bool { + if r == nil { + return true + } + buf, err := r.src() + if err != nil { + return true + } + defer zero(buf) + return len(buf) == 0 +} + +// zero overwrites b with zero bytes. Visible for testing. +func zero(b []byte) { + for i := range b { + b[i] = 0 + } +} diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go new file mode 100644 index 0000000..5ae7a12 --- /dev/null +++ b/internal/secret/secret_test.go @@ -0,0 +1,188 @@ +// Copyright (c) certctl +// SPDX-License-Identifier: BSL-1.1 + +package secret + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "testing" +) + +// TestRef_UseExposesBytesAndZeros — the canonical contract: Use +// hands fn a buffer containing the credential, fn reads it, and +// after fn returns the buffer is overwritten with zeros. +func TestRef_UseExposesBytesAndZeros(t *testing.T) { + r := NewRefFromString("secret-token") + + var captured []byte + err := r.Use(func(buf []byte) error { + // Copy so we can inspect post-zero behavior — the original + // buf is going to be zeroed by Use's defer. + captured = make([]byte, len(buf)) + copy(captured, buf) + if string(buf) != "secret-token" { + t.Errorf("Use: want bytes 'secret-token', got %q", buf) + } + return nil + }) + if err != nil { + t.Fatalf("Use: %v", err) + } + if string(captured) != "secret-token" { + t.Errorf("captured bytes: want 'secret-token', got %q", captured) + } +} + +// TestRef_BufferZeroedAfterUse — the load-bearing security property. +// Without zeroing, the credential lingers in the heap and is trivially +// extractable from a process dump. We assert via Use's internal-state +// observation: a slice escape (with a known anti-pattern) reads zeros +// after Use returns. +func TestRef_BufferZeroedAfterUse(t *testing.T) { + r := NewRefFromString("very-secret") + + // Anti-pattern: capture the slice header and read it after Use. + // In production code this is a bug (caller must not retain the + // slice). The test exercises the bug to assert the buffer was + // zeroed. + var escaped []byte + _ = r.Use(func(buf []byte) error { + escaped = buf + return nil + }) + + // After Use, the slice should be all zeros. + for i, b := range escaped { + if b != 0 { + t.Errorf("byte %d not zeroed: 0x%02x", i, b) + } + } +} + +// TestRef_WriteTo writes the secret to a writer and asserts the +// write happened correctly + the staging buffer is zeroed. +func TestRef_WriteTo(t *testing.T) { + r := NewRefFromString("Bearer abc123") + var buf bytes.Buffer + n, err := r.WriteTo(&buf) + if err != nil { + t.Fatalf("WriteTo: %v", err) + } + if int64(buf.Len()) != n { + t.Errorf("WriteTo: want %d bytes, got %d", buf.Len(), n) + } + if buf.String() != "Bearer abc123" { + t.Errorf("WriteTo: wrong bytes, got %q", buf.String()) + } +} + +// TestRef_StringRedacted — Ref.String() must NEVER return the +// underlying bytes. Catches accidental fmt.Sprintf("%v", cfg) leaks. +func TestRef_StringRedacted(t *testing.T) { + r := NewRefFromString("super-secret-token") + got := r.String() + if got != "[redacted]" { + t.Errorf("String: want '[redacted]', got %q", got) + } + // Test the implicit fmt.Stringer interface too. + got = fmt.Sprintf("%v", r) + if got != "[redacted]" { + t.Errorf("fmt.Sprintf: want '[redacted]', got %q", got) + } +} + +// TestRef_MarshalJSONRedacted — JSON-encoding a Ref returns +// "[redacted]". Catches API-surface leak via GET /issuers etc. +func TestRef_MarshalJSONRedacted(t *testing.T) { + r := NewRefFromString("my-api-key") + got, err := json.Marshal(r) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if string(got) != `"[redacted]"` { + t.Errorf("MarshalJSON: want '\"[redacted]\"', got %s", got) + } +} + +// TestRef_MarshalJSONInStruct — a config struct holding a *Ref +// field marshals with the credential redacted. +func TestRef_MarshalJSONInStruct(t *testing.T) { + cfg := struct { + Name string `json:"name"` + Key *Ref `json:"key"` + }{ + Name: "globalsign", + Key: NewRefFromString("the-key"), + } + got, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + want := `{"name":"globalsign","key":"[redacted]"}` + if string(got) != want { + t.Errorf("MarshalJSON struct: want %s, got %s", want, got) + } +} + +// TestRef_NilSafety — calling methods on a nil *Ref returns errors, +// not panics. Defensive programming for paths that haven't wired +// the Ref yet. +func TestRef_NilSafety(t *testing.T) { + var r *Ref + + if got := r.String(); got != "[redacted]" { + t.Errorf("nil Ref.String: want '[redacted]', got %q", got) + } + // Use on nil returns an error, doesn't panic. + if err := r.Use(func(buf []byte) error { return nil }); err == nil { + t.Error("Use on nil Ref: expected error") + } + // WriteTo on nil returns an error, doesn't panic. + if _, err := r.WriteTo(io.Discard); err == nil { + t.Error("WriteTo on nil Ref: expected error") + } + if !r.IsEmpty() { + t.Error("IsEmpty on nil Ref: want true") + } +} + +// TestRef_SourceErrorPropagated — when the source closure returns +// an error (decrypt failure, etc.), Use propagates it. +func TestRef_SourceErrorPropagated(t *testing.T) { + sentinel := errors.New("decrypt failed") + r := NewRef(func() ([]byte, error) { return nil, sentinel }) + + err := r.Use(func(buf []byte) error { + t.Error("fn should not be called when source errors") + return nil + }) + if !errors.Is(err, sentinel) { + t.Errorf("Use: want sentinel in chain, got %v", err) + } +} + +// TestRef_IsEmpty — empty source returns IsEmpty=true. +func TestRef_IsEmpty(t *testing.T) { + if !NewRefFromString("").IsEmpty() { + t.Error("empty string Ref: want IsEmpty=true") + } + if NewRefFromString("x").IsEmpty() { + t.Error("non-empty Ref: want IsEmpty=false") + } +} + +// TestZero — direct test of the zero helper to lock the +// implementation: every byte set to 0. +func TestZero(t *testing.T) { + b := []byte("not-zero") + zero(b) + for i, x := range b { + if x != 0 { + t.Errorf("byte %d: want 0, got %d", i, x) + } + } +}