feat(insurance)!: make Config.APIKey an opaque Secret

CHANGELOG.md

@@ -75,6 +75,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Changed
 
+- **Breaking (source):** `insurance.Config.APIKey` is now an opaque
+  `insurance.Secret` (build it with `insurance.NewSecret(key)`) instead of a
+  `string`. The key lives in a closure, so a `Config` held in an unexported
+  struct field no longer prints it under any fmt verb, json, xml, yaml or slog.
+  `Config.Format` is removed (`%v` of a `Config` now prints
+  `{[redacted] <org>}`), and two copies of one `Config` are no longer
+  `reflect.DeepEqual`. `Config` is also no longer comparable (`==` stops
+  compiling and a `Config` map key panics as unhashable), and
+  `json.Unmarshal` into a `Config` carrying an `APIKey` now errors where the
+  old `json:"-"` tag ignored it (#330)
 - **Breaking (source):** `common.PurchaseResult` gained an `ExistingCommitment`
   field. Downstream code that builds it with unkeyed composite literals no
   longer compiles; keyed literals are unaffected. The field is true when a

pkg/insurance/client.go

@@ -31,23 +31,13 @@ const maxRetryAfter = 24 * time.Hour
 const maxErrorMessageLen = 200
 
 // Config identifies the vendor organization and credential. APIKey is sent
-// only as the x-api-key header. Format redacts it for every fmt verb except %p
-// and %T on a non-pointer Config, which fmt handles before consulting Format;
-// never pass a Config value to %p. fmt cannot call methods on unexported
-// struct fields, so a Config held by the caller in an unexported field still
-// prints the key raw: drop the Config once NewClient has returned, and never
-// log one. NewClient does not retain the Config.
+// only as the x-api-key header. Secret redacts itself under every printer, so
+// a Config is safe to print or hold in any struct field.
 type Config struct {
-	APIKey string `json:"-"`
+	APIKey Secret
 	OrgID  string
 }
 
-// Format redacts the key for fmt verbs, so %v, %+v, %#v and %s of a Config (or
-// a pointer to one) never print it.
-func (c Config) Format(s fmt.State, _ rune) {
-	_, _ = fmt.Fprintf(s, "insurance.Config{OrgID:%q, APIKey:[redacted]}", c.OrgID)
-}
-
 // HTTPError is a non-2xx vendor response. Only the status, the Retry-After
 // header and a truncated message from the documented error shapes are kept;
 // the raw body is never echoed.
@@ -91,7 +81,7 @@ func NewClient(cfg Config, hc *http.Client) (*Client, error) {
 }
 
 func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) {
-	if strings.TrimSpace(cfg.APIKey) == "" {
+	if strings.TrimSpace(cfg.APIKey.Reveal()) == "" {
 		return nil, errors.New("archera API key is not configured")
 	}
 	if !isUUID(cfg.OrgID) {
@@ -102,7 +92,7 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) {
 	}
 	noRedirect := *hc
 	noRedirect.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }
-	key := cfg.APIKey
+	key := cfg.APIKey.Reveal()
 	return &Client{
 		baseURL: baseURL,
 		orgID:   cfg.OrgID,

pkg/insurance/client_test.go

@@ -1,22 +1,28 @@
 package insurance
 
 import (
+	"bytes"
 	"context"
+	"encoding/gob"
 	"encoding/json"
+	"encoding/xml"
 	"errors"
 	"fmt"
 	"io"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
+	"reflect"
 	"strings"
 	"sync/atomic"
 	"testing"
 	"testing/iotest"
+	"text/template"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v3"
 
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/httpclient"
 )
@@ -32,17 +38,17 @@ func testClient(t *testing.T, h http.Handler) *Client {
 	t.Helper()
 	srv := httptest.NewServer(h)
 	t.Cleanup(srv.Close)
-	c, err := newClient(srv.URL, Config{APIKey: testKey, OrgID: testOrg}, srv.Client())
+	c, err := newClient(srv.URL, Config{APIKey: NewSecret(testKey), OrgID: testOrg}, srv.Client())
 	require.NoError(t, err)
 	return c
 }
 
 func TestNewClient_ValidatesConfig(t *testing.T) {
 	_, err := NewClient(Config{OrgID: testOrg}, nil)
 	assert.Error(t, err)
-	_, err = NewClient(Config{APIKey: testKey, OrgID: "../x"}, nil)
+	_, err = NewClient(Config{APIKey: NewSecret(testKey), OrgID: "../x"}, nil)
 	assert.Error(t, err)
-	c, err := NewClient(Config{APIKey: testKey, OrgID: testOrg}, nil)
+	c, err := NewClient(Config{APIKey: NewSecret(testKey), OrgID: testOrg}, nil)
 	require.NoError(t, err)
 	assert.Equal(t, "https://api.archera.ai", c.baseURL)
 }
@@ -156,7 +162,7 @@ func TestClient_DoesNotMutateCallerHTTPClient(t *testing.T) {
 	srv := httptest.NewServer(http.NotFoundHandler())
 	t.Cleanup(srv.Close)
 	hc := srv.Client()
-	_, err := newClient(srv.URL, Config{APIKey: testKey, OrgID: testOrg}, hc)
+	_, err := newClient(srv.URL, Config{APIKey: NewSecret(testKey), OrgID: testOrg}, hc)
 	require.NoError(t, err)
 	assert.Nil(t, hc.CheckRedirect)
 }
@@ -314,7 +320,7 @@ type holder struct {
 }
 
 func TestConfigAndClient_NeverFormatTheKey(t *testing.T) {
-	cfg := Config{APIKey: testKey, OrgID: testOrg}
+	cfg := Config{APIKey: NewSecret(testKey), OrgID: testOrg}
 	c, err := newClient("http://127.0.0.1:1", cfg, nil)
 	require.NoError(t, err)
 	values := []any{cfg, &cfg, c, *c, holder{cfg, c, cfg, []Config{cfg}, map[string]Config{"k": cfg}},
@@ -334,14 +340,78 @@ func TestConfigAndClient_NeverFormatTheKey(t *testing.T) {
 	b, err := json.Marshal(holder{Cfg: cfg})
 	require.NoError(t, err)
 	assert.NotContains(t, string(b), testKey)
+	assert.Contains(t, string(b), `"APIKey":"[redacted]"`)
 	var buf strings.Builder
 	slog.New(slog.NewTextHandler(&buf, nil)).Info("x", "cfg", cfg, "client", c)
 	slog.New(slog.NewJSONHandler(&buf, nil)).Info("x", "cfg", cfg, "client", c)
 	assert.NotContains(t, buf.String(), testKey)
 }
 
+type secretBox struct{ K Secret }
+
+type unexportedSecretHolders struct {
+	cfg  Config
+	pcfg *Config
+	sec  Secret
+	list []Config
+}
+
+// The exported-field paths must print the exact redaction text; the
+// unexported-field paths (where fmt cannot call methods) must not reach the
+// key at all.
+func TestSecret_NeverRevealedByAnyPrinter(t *testing.T) {
+	sec := NewSecret(testKey)
+	cfg := Config{APIKey: sec, OrgID: testOrg}
+	require.Equal(t, testKey, sec.Reveal())
+	assert.Equal(t, "", Secret{}.Reveal())
+
+	for _, verb := range []string{"%v", "%+v", "%#v", "%s", "%q", "%x", "%d"} {
+		assert.Equal(t, "[redacted]", fmt.Sprintf(verb, sec), verb)
+		assert.Equal(t, "[redacted]", fmt.Sprintf(verb, &sec), verb)
+	}
+	assert.NotContains(t, fmt.Sprintf("%p", &sec), testKey)
+
+	un := unexportedSecretHolders{cfg, &cfg, sec, []Config{cfg}}
+	forbidden := []string{testKey, fmt.Sprintf("%x", testKey), fmt.Sprintf("%q", testKey)}
+	for _, v := range []any{un, &un, []any{un}, map[string]any{"k": un}, fmt.Errorf("w: %w", fmt.Errorf("%v", un))} {
+		for _, verb := range []string{"%v", "%+v", "%#v", "%s", "%q", "%x", "%d", "%p"} {
+			out := fmt.Sprintf(verb, v)
+			for _, bad := range forbidden {
+				assert.NotContains(t, out, bad, "%s of %T", verb, v)
+			}
+		}
+	}
+
+	xmlOut, err := xml.Marshal(secretBox{sec})
+	require.NoError(t, err)
+	assert.Equal(t, "<secretBox><K>[redacted]</K></secretBox>", string(xmlOut))
+	yamlOut, err := yaml.Marshal(struct{ K Secret }{sec})
+	require.NoError(t, err)
+	assert.Equal(t, "k: '[redacted]'\n", string(yamlOut))
+	jsonOut, err := json.Marshal(struct{ K Secret }{sec})
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"K":"[redacted]"}`, string(jsonOut))
+
+	var tpl strings.Builder
+	require.NoError(t, template.Must(template.New("t").Parse("{{.}} {{printf \"%v\" .}}")).Execute(&tpl, cfg))
+	assert.NotContains(t, tpl.String(), testKey)
+
+	var gobBuf bytes.Buffer
+	_ = gob.NewEncoder(&gobBuf).Encode(cfg) // no exported fields on Secret: encodes nothing reachable
+	assert.NotContains(t, gobBuf.String(), testKey)
+
+	var logBuf strings.Builder
+	slog.New(slog.NewTextHandler(&logBuf, nil)).Info("x", "sec", sec, "cfg", cfg)
+	slog.New(slog.NewJSONHandler(&logBuf, nil)).Info("x", "sec", sec, "cfg", cfg)
+	assert.NotContains(t, logBuf.String(), testKey)
+	assert.Contains(t, logBuf.String(), "[redacted]")
+
+	// The key is reachable only through a closure: a copy compares unequal.
+	assert.False(t, reflect.DeepEqual(cfg, Config{APIKey: NewSecret(testKey), OrgID: testOrg}))
+}
+
 func TestNewClient_DefaultHTTPClientIsHardened(t *testing.T) {
-	c, err := NewClient(Config{APIKey: testKey, OrgID: testOrg}, nil)
+	c, err := NewClient(Config{APIKey: NewSecret(testKey), OrgID: testOrg}, nil)
 	require.NoError(t, err)
 	assert.Equal(t, httpclient.New().Timeout, c.hc.Timeout)
 	assert.Equal(t, 30*time.Second, c.hc.Timeout)
@@ -350,7 +420,7 @@ func TestNewClient_DefaultHTTPClientIsHardened(t *testing.T) {
 }
 
 func TestNewClient_RejectsBlankKey(t *testing.T) {
-	_, err := NewClient(Config{APIKey: "   \t", OrgID: testOrg}, nil)
+	_, err := NewClient(Config{APIKey: NewSecret("   \t"), OrgID: testOrg}, nil)
 	assert.Error(t, err)
 }
 

pkg/insurance/key_leak_test.go

@@ -26,7 +26,7 @@ type nestedHolder struct{ inner unexportedPtrHolder }
 // Issue #325: fmt does not call Format on unexported fields, so the key must
 // not be stored in any field fmt can reach.
 func TestClientKeyNeverPrintedWherever(t *testing.T) {
-	c, err := newClient("http://127.0.0.1:1", Config{APIKey: testKey, OrgID: testOrg}, nil)
+	c, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret(testKey), OrgID: testOrg}, nil)
 	require.NoError(t, err)
 
 	holders := map[string]any{
@@ -77,7 +77,7 @@ func TestClientKeyNeverPrintedWherever(t *testing.T) {
 // Client, unexported ones included, and require that no string equals or
 // contains the key.
 func TestClientHoldsNoStringContainingKey(t *testing.T) {
-	c, err := newClient("http://127.0.0.1:1", Config{APIKey: testKey, OrgID: testOrg}, nil)
+	c, err := newClient("http://127.0.0.1:1", Config{APIKey: NewSecret(testKey), OrgID: testOrg}, nil)
 	require.NoError(t, err)
 	seen := map[uintptr]bool{}
 	var walk func(path string, v reflect.Value)

pkg/insurance/secret.go

@@ -0,0 +1,31 @@
+package insurance
+
+import "fmt"
+
+// redactedText is what every printer and marshaler emits for a Secret.
+const redactedText = "[redacted]"
+
+// Secret holds a credential so that no printer can reach it. The value lives
+// only in a closure, so reflection-based printers (fmt on unexported fields,
+// spew, encoders) see a func address, never the string. The zero Secret holds
+// the empty string.
+type Secret struct{ reveal func() string }
+
+// NewSecret wraps a credential.
+func NewSecret(s string) Secret { return Secret{reveal: func() string { return s }} }
+
+// Reveal returns the credential. Call it only where the value is sent, such as
+// the header-set site.
+func (s Secret) Reveal() string {
+	if s.reveal == nil {
+		return ""
+	}
+	return s.reveal()
+}
+
+// Format redacts the value for every fmt verb and for slog text output.
+func (Secret) Format(f fmt.State, _ rune) { _, _ = fmt.Fprint(f, redactedText) }
+
+// MarshalText redacts the value for encoding/json, encoding/xml, yaml and
+// slog's JSON handler.
+func (Secret) MarshalText() ([]byte, error) { return []byte(redactedText), nil }