diff --git a/pkg/insurance/client.go b/pkg/insurance/client.go index c7af007..3aad910 100644 --- a/pkg/insurance/client.go +++ b/pkg/insurance/client.go @@ -33,9 +33,10 @@ 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 also cannot call methods on unexported -// struct fields, so a consumer must hold a *Client, or a Config only in an -// exported field, and never log a Config stored in an unexported field. +// 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. type Config struct { APIKey string `json:"-"` OrgID string @@ -67,20 +68,22 @@ func (e *HTTPError) Error() string { // Client is the real read-only QuoteClient. It issues single-shot GETs and // never follows redirects, so the API key cannot be forwarded to another host. +// +// The API key is held only inside the setAuth and redact closures, never in a +// field fmt or reflection can read, so no fmt verb prints it wherever the +// Client (or a pointer to it) is stored. type Client struct { baseURL string - cfg Config + orgID string hc *http.Client + // setAuth adds the credential to one outgoing request. + setAuth func(*http.Request) + // redact masks the credential in a vendor-supplied message. + redact func(string) string } var _ QuoteClient = (*Client)(nil) -// Format keeps the embedded configuration, and with it the key, out of any -// fmt output of a Client (same %p/%T limit as Config). -func (c Client) Format(s fmt.State, _ rune) { - _, _ = fmt.Fprintf(s, "insurance.Client{OrgID:%q, APIKey:[redacted]}", c.cfg.OrgID) -} - // NewClient returns a Client for the fixed Archera origin. hc is optional; nil // uses the shared SSRF-hardened client with a 30-second timeout. func NewClient(cfg Config, hc *http.Client) (*Client, error) { @@ -99,7 +102,19 @@ func newClient(baseURL string, cfg Config, hc *http.Client) (*Client, error) { } noRedirect := *hc noRedirect.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } - return &Client{baseURL: baseURL, cfg: cfg, hc: &noRedirect}, nil + key := cfg.APIKey + return &Client{ + baseURL: baseURL, + orgID: cfg.OrgID, + hc: &noRedirect, + setAuth: func(req *http.Request) { req.Header.Set("x-api-key", key) }, + redact: func(msg string) string { + if key == "" { + return msg + } + return strings.ReplaceAll(msg, key, "[redacted]") + }, + }, nil } // Comparison reads the documented comparison operation. Empty request slices @@ -132,7 +147,7 @@ func (c *Client) Comparison(ctx context.Context, req ComparisonRequest) (*Compar return nil, err } defer func() { _ = body.Close() }() - return DecodeComparison(&boundedReader{r: body, n: maxResponseBytes}, c.cfg.OrgID, req.PlanID, fetchedAt) + return DecodeComparison(&boundedReader{r: body, n: maxResponseBytes}, c.orgID, req.PlanID, fetchedAt) } // Plan reads the documented CommitmentPlan operation. @@ -149,7 +164,7 @@ func (c *Client) Plan(ctx context.Context, planID string) (*Plan, error) { } func (c *Client) planPath(planID string) string { - return "/v1/org/" + c.cfg.OrgID + "/commitment-plans/" + planID + return "/v1/org/" + c.orgID + "/commitment-plans/" + planID } var errResponseTooLarge = fmt.Errorf("response exceeds %d bytes", maxResponseBytes) @@ -192,7 +207,7 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (io.ReadClo if err != nil { return nil, time.Time{}, fmt.Errorf("building archera request: %w", err) } - req.Header.Set("x-api-key", c.cfg.APIKey) + c.setAuth(req) req.Header.Set("Accept", "application/json") // baseURL is the fixed BaseURL constant; only in-package tests override it. resp, err := c.hc.Do(req) //nolint:gosec // G704: see comment above @@ -202,7 +217,7 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (io.ReadClo fetchedAt := time.Now().UTC() if resp.StatusCode < 200 || resp.StatusCode > 299 { defer func() { _ = resp.Body.Close() }() - return nil, fetchedAt, newHTTPError(resp, c.cfg.APIKey) + return nil, fetchedAt, newHTTPError(resp, c.redact) } return resp.Body, fetchedAt, nil } @@ -217,7 +232,7 @@ func sanitizeTransportError(err error) error { return err } -func newHTTPError(resp *http.Response, apiKey string) *HTTPError { +func newHTTPError(resp *http.Response, redact func(string) string) *HTTPError { e := &HTTPError{StatusCode: resp.StatusCode} if s, err := strconv.Atoi(strings.TrimSpace(resp.Header.Get("Retry-After"))); err == nil && s > 0 { // Compare before multiplying: s*time.Second overflows int64 for large s. @@ -234,24 +249,19 @@ func newHTTPError(resp *http.Response, apiKey string) *HTTPError { } raw, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) if err == nil && json.Unmarshal(raw, &shape) == nil { - e.Message = truncate(cleanMessage(shape.Message, apiKey), maxErrorMessageLen) + e.Message = truncate(redact(cleanMessage(shape.Message)), maxErrorMessageLen) } return e } -// cleanMessage drops control characters (log forging, ANSI injection) and -// masks the API key if the vendor echoes it back. -func cleanMessage(msg, apiKey string) string { - msg = strings.Map(func(r rune) rune { +// cleanMessage drops control characters (log forging, ANSI injection). +func cleanMessage(msg string) string { + return strings.Map(func(r rune) rune { if unicode.IsControl(r) { return -1 } return r }, msg) - if apiKey != "" { - msg = strings.ReplaceAll(msg, apiKey, "[redacted]") - } - return msg } func truncate(s string, n int) string { diff --git a/pkg/insurance/key_leak_test.go b/pkg/insurance/key_leak_test.go new file mode 100644 index 0000000..b7bc93b --- /dev/null +++ b/pkg/insurance/key_leak_test.go @@ -0,0 +1,120 @@ +package insurance + +import ( + "encoding/base64" + "encoding/json" + "fmt" + "reflect" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type unexportedPtrHolder struct { + n int + client *Client +} + +type unexportedValHolder struct{ client Client } + +type ExportedPtrHolder struct{ Client *Client } + +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) + require.NoError(t, err) + + holders := map[string]any{ + "unexported *Client": unexportedPtrHolder{1, c}, + "&unexported *Client": &unexportedPtrHolder{1, c}, + "unexported Client value": unexportedValHolder{*c}, + "&unexported Client value": &unexportedValHolder{*c}, + "exported *Client": ExportedPtrHolder{c}, + "nested": nestedHolder{unexportedPtrHolder{1, c}}, + "[]*Client": []*Client{c}, + "map": map[string]*Client{"a": c}, + "[]any": []any{c, unexportedValHolder{*c}}, + "wrapped error": fmt.Errorf("x: %w", fmt.Errorf("%v", unexportedValHolder{*c})), + } + forbidden := []string{ + testKey, + fmt.Sprintf("%x", testKey), fmt.Sprintf("%X", testKey), + base64.StdEncoding.EncodeToString([]byte(testKey)), + fmt.Sprintf("%q", testKey), + } + verbs := []string{"%v", "%+v", "%#v", "%s", "%q", "%x", "%X", "%d", "%T", "%p"} + for name, h := range holders { + for _, verb := range append([]string{"Sprint", "Sprintln"}, verbs...) { + t.Run(name+"/"+verb, func(t *testing.T) { + var out string + switch verb { + case "Sprint": + out = fmt.Sprint(h) + case "Sprintln": + out = fmt.Sprintln(h) + default: + out = fmt.Sprintf(verb, h) + } + for _, bad := range forbidden { + assert.NotContains(t, out, bad) + } + }) + } + } + b, err := json.Marshal(ExportedPtrHolder{c}) + require.NoError(t, err) + for _, bad := range forbidden { + require.NotContains(t, string(b), bad) + } +} + +// The invariant must not depend on fmt behavior: walk every field of a +// 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) + require.NoError(t, err) + seen := map[uintptr]bool{} + var walk func(path string, v reflect.Value) + walk = func(path string, v reflect.Value) { + switch v.Kind() { + case reflect.String: + require.NotContains(t, v.String(), testKey, path) + case reflect.Pointer, reflect.Interface: + if v.IsNil() { + return + } + if v.Kind() == reflect.Pointer { + if seen[v.Pointer()] { + return + } + seen[v.Pointer()] = true + } + walk(path+"*", v.Elem()) + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + walk(path+"."+v.Type().Field(i).Name, v.Field(i)) + } + case reflect.Slice, reflect.Array: + if v.Kind() == reflect.Slice && v.Type().Elem().Kind() == reflect.Uint8 { + require.NotContains(t, string(v.Bytes()), testKey, path) + return + } + for i := 0; i < v.Len(); i++ { + walk(fmt.Sprintf("%s[%d]", path, i), v.Index(i)) + } + case reflect.Map: + for _, k := range v.MapKeys() { + walk(path+"{key}", k) + walk(path+"{val}", v.MapIndex(k)) + } + } + } + walk("Client", reflect.ValueOf(c)) + require.True(t, strings.HasPrefix(c.baseURL, "http"), "walk reached the struct") +}