diff --git a/CHANGELOG.md b/CHANGELOG.md index 9796de1..f3cb091 100644 --- a/CHANGELOG.md +++ b/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] }`), 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 diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index 3aad910..c58d077 100644 --- a/pkg/insurance/client.go +++ b/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, diff --git a/pkg/insurance/client_test.go b/pkg/insurance/client_test.go index a6fa9fb..1882200 100644 --- a/pkg/insurance/client_test.go +++ b/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,7 +38,7 @@ 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 } @@ -40,9 +46,9 @@ func testClient(t *testing.T, h http.Handler) *Client { 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, "[redacted]", 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) } diff --git a/pkg/insurance/key_leak_test.go b/pkg/insurance/key_leak_test.go index b7bc93b..1459582 100644 --- a/pkg/insurance/key_leak_test.go +++ b/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) diff --git a/pkg/insurance/secret.go b/pkg/insurance/secret.go new file mode 100644 index 0000000..f8d3d2f --- /dev/null +++ b/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 }