cloud: carry IAM's refusal reason instead of dropping it

The key resolver read IAM's envelope for a principal and threw the rest away, so a refused key produced a bare nil and every surface downstream could only repeat IAM's generic "the entity does not exist" — which is what told a holder with a REVOKED key to go looking for a deleted org. lookup now records the `code` beside the (nil) principal, and RefusalForKey is the door a user-facing surface asks why. It shares the resolver's existing cache, so asking costs no extra IAM call: the reason is a by-product of the resolution that already happened. Resolution is unchanged. A refused key is still nil, still anonymous, still fails closed — only the diagnosis is added. KeyHint is the ONE way a key is named in a log or an error: prefix only, never the credential. Also makes the zero cache usable — put() creates its map on first write. A partially-constructed iamKeys (any caller naming only the caches it cares about, which every test does) hit a nil-map panic otherwise.
2026-08-01 13:49:26 -07:00
parent a291335081
commit 9caf0eb801
2 changed files with 188 additions and 2 deletions
@@ -41,8 +41,9 @@ type iamKeys struct {
 	base  string
 	auth  string // client_secret_basic, or "" when unconfigured
 	http  *http.Client
-	cache cache[string, *idClaims] // secret key -> principal (get-user?accessKey)
-	orgs  cache[string, string]    // publishable key -> org, and no principal (resolve-key)
+	cache cache[string, *idClaims]  // secret key -> principal (get-user?accessKey)
+	orgs  cache[string, string]     // publishable key -> org, and no principal (resolve-key)
+	why   cache[string, KeyRefusal] // secret key -> WHY it did not resolve, for diagnosis only
 }

 // newIAMKeys reads the same IAM env clients/account does. With no confidential
@@ -55,6 +56,7 @@ func newIAMKeys() *iamKeys {
 		http:  &http.Client{Timeout: 5 * time.Second},
 		cache: newCache[string, *idClaims](60 * time.Second),
 		orgs:  newCache[string, string](60 * time.Second),
+		why:   newCache[string, KeyRefusal](60 * time.Second),
 	}
 }

@@ -208,11 +210,61 @@ func (k *iamKeys) lookupOrg(ctx context.Context, key string) string {
 	return strings.TrimSpace(env.Data.Org)
 }

+// KeyRefusal is the machine-readable reason IAM gives for not resolving a key —
+// `code` on the get-user?accessKey / resolve-key envelope (iam internal/store
+// apikey.go). Cloud does not interpret it; it carries it, so the surface that faces
+// a human can say "revoked, mint a new one" instead of IAM's generic "the entity
+// does not exist". "" means IAM gave no reason (an older IAM, or a store fault,
+// which is NOT a bad credential).
+type KeyRefusal string
+
+// RefusalForKey resolves an opaque secret key and reports WHY it failed, for the
+// surface that must explain the failure to a person. It shares resolve()'s cache, so
+// asking why costs no extra IAM call on the hot path: a resolved key answers ("",
+// true) from the same cached principal the auth path uses.
+func RefusalForKey(ctx context.Context, key string) (KeyRefusal, bool) {
+	key = strings.TrimSpace(key)
+	if !isAPIKey(key) || IsPublishableKey(key) {
+		return "", false
+	}
+	if claims := sharedKeys().resolve(ctx, key); claims != nil {
+		return "", true
+	}
+	return sharedKeys().refusal(ctx, key), false
+}
+
+// refusal reports the cached reason a key did not resolve. lookup records it when it
+// asks IAM, so this never issues a second call — the reason is a by-product of the
+// resolution that already happened, not a separate question.
+func (k *iamKeys) refusal(_ context.Context, key string) KeyRefusal {
+	r, _ := k.why.get(key)
+	return r
+}
+
+// KeyHint is the ONE way a key is named in a log line or an error message: its
+// prefix and nothing else. A credential must never be echoed whole, and "hk-902abd…"
+// is enough for a holder to tell WHICH of their keys failed while being useless to
+// anyone who intercepts it.
+func KeyHint(key string) string {
+	key = strings.TrimSpace(key)
+	const shown = 9 // "hk-" + 6
+	if len(key) <= shown {
+		return "…"
+	}
+	return key[:shown] + "…"
+}
+
 // lookup performs the authenticated get-user?accessKey call and maps the user row
 // to idClaims. Any failure (unreachable, denied, unknown key) yields nil. Name is
 // both the username IAM's owner/name lookups parse and the id fallback: a key has
 // no UUID subject, so userID() falls through to name — the gateway's historical
 // X-User-Id==name behavior the owner/name path expects.
+//
+// A refusal's REASON is recorded beside the (nil) principal rather than discarded.
+// It changes no decision here — a key that does not resolve is anonymous either way,
+// and that stays true — but throwing it away is what left every downstream surface
+// rendering IAM's generic "the entity does not exist" to users whose key had simply
+// been revoked.
 func (k *iamKeys) lookup(ctx context.Context, key string) *idClaims {
 	u := k.base + "/v1/iam/get-user?" + url.Values{"accessKey": {key}}.Encode()
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
@@ -232,6 +284,7 @@ func (k *iamKeys) lookup(ctx context.Context, key string) *idClaims {
 	}
 	var env struct {
 		Status string `json:"status"`
+		Code   string `json:"code"` // WHY, when IAM refused (iam store.KeyFailure)
 		Data   *struct {
 			Owner   string `json:"owner"`
 			Name    string `json:"name"`
@@ -240,9 +293,11 @@ func (k *iamKeys) lookup(ctx context.Context, key string) *idClaims {
 		} `json:"data"`
 	}
 	if json.Unmarshal(raw, &env) != nil || env.Status != "ok" || env.Data == nil {
+		k.why.put(key, KeyRefusal(strings.TrimSpace(env.Code)))
 		return nil
 	}
 	if strings.TrimSpace(env.Data.Owner) == "" {
+		k.why.put(key, KeyRefusal(strings.TrimSpace(env.Code)))
 		return nil
 	}
 	owner := strings.TrimSpace(env.Data.Owner)
@@ -294,8 +349,15 @@ func (c *cache[K, V]) get(k K) (V, bool) {
 	return e.v, true
 }

+// put stores a value. The ZERO cache is usable: its map is created on first write,
+// so a partially-constructed iamKeys (any caller that names only the caches it cares
+// about) records rather than panicking on a nil map. A zero ttl expires immediately,
+// which is the right reading of "no ttl was configured" — never cache forever.
 func (c *cache[K, V]) put(k K, v V) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
+	if c.m == nil {
+		c.m = make(map[K]entry[V])
+	}
 	c.m[k] = entry[V]{v: v, exp: time.Now().Add(c.ttl)}
 }
@@ -6,6 +6,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -188,3 +189,126 @@ func TestResolveOrg_FailsClosed(t *testing.T) {
 		srv.Close()
 	}
 }
+
+// ── why a key was refused ────────────────────────────────────────────────────
+
+// IAM's refusal REASON survives the resolver instead of being discarded.
+//
+// "the entity does not exist" is IAM's generic answer for several distinct causes,
+// and cloud dropped everything but the (nil) principal — so every surface downstream
+// could only repeat that sentence. A holder whose key had been REVOKED was sent
+// looking for a deleted organization instead of minting a new key. Resolution is
+// unchanged: a refused key is still nil, still anonymous. Only the diagnosis is added.
+func TestIAMKeys_RefusalReasonIsCarried(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		switch r.URL.Query().Get("accessKey") {
+		case "hk-live-GOOD":
+			_, _ = w.Write([]byte(`{"status":"ok","data":{"owner":"hanzo","name":"z"}}`))
+		case "hk-live-REVOKED":
+			_, _ = w.Write([]byte(`{"status":"error","msg":"the entity does not exist","code":"key_unknown"}`))
+		case "hk-live-FORGED":
+			_, _ = w.Write([]byte(`{"status":"error","msg":"the entity does not exist","code":"key_foreign_user"}`))
+		default:
+			// An older IAM that gives no reason at all.
+			_, _ = w.Write([]byte(`{"status":"error","msg":"the entity does not exist"}`))
+		}
+	}))
+	defer srv.Close()
+
+	k := &iamKeys{base: srv.URL, auth: "Basic test", http: srv.Client(),
+		cache: newCache[string, *idClaims](time.Minute),
+		why:   newCache[string, KeyRefusal](time.Minute)}
+
+	// A refused key is STILL nil — the reason changes no decision.
+	for _, tc := range []struct {
+		key  string
+		want KeyRefusal
+	}{
+		{"hk-live-REVOKED", "key_unknown"},
+		{"hk-live-FORGED", "key_foreign_user"},
+		{"hk-live-SILENT", ""}, // an IAM that sends no code yields no invented reason
+	} {
+		if c := k.resolve(context.Background(), tc.key); c != nil {
+			t.Fatalf("%s resolved to %+v — a refused key must stay anonymous", tc.key, c)
+		}
+		if got := k.refusal(context.Background(), tc.key); got != tc.want {
+			t.Errorf("%s: refusal = %q, want %q", tc.key, got, tc.want)
+		}
+	}
+
+	// A key that RESOLVES records no refusal.
+	if c := k.resolve(context.Background(), "hk-live-GOOD"); c == nil {
+		t.Fatal("a valid key must still resolve")
+	}
+	if got := k.refusal(context.Background(), "hk-live-GOOD"); got != "" {
+		t.Errorf("a resolved key recorded refusal %q, want none", got)
+	}
+}
+
+// RefusalForKey is the door a user-facing surface asks "why did this fail?", and it
+// answers from the SAME cache the auth path already filled — so diagnosing a failure
+// costs no extra IAM call.
+func TestRefusalForKey(t *testing.T) {
+	var calls int
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		calls++
+		w.Header().Set("Content-Type", "application/json")
+		if r.URL.Query().Get("accessKey") == "hk-live-GOOD" {
+			_, _ = w.Write([]byte(`{"status":"ok","data":{"owner":"hanzo","name":"z"}}`))
+			return
+		}
+		_, _ = w.Write([]byte(`{"status":"error","msg":"the entity does not exist","code":"key_unknown"}`))
+	}))
+	defer srv.Close()
+
+	sharedKeysOnce = sync.Once{}
+	sharedKeysInst = nil
+	t.Setenv("IAM_URL", srv.URL)
+	t.Setenv("IAM_MINT_CLIENT_ID", "hanzo-console")
+	t.Setenv("IAM_MINT_CLIENT_SECRET", "s3cr3t")
+	t.Cleanup(func() { sharedKeysOnce = sync.Once{}; sharedKeysInst = nil })
+
+	if reason, ok := RefusalForKey(context.Background(), "hk-live-REVOKED"); ok || reason != "key_unknown" {
+		t.Fatalf("RefusalForKey(revoked) = (%q,%v), want (key_unknown,false)", reason, ok)
+	}
+	if reason, ok := RefusalForKey(context.Background(), "hk-live-GOOD"); !ok || reason != "" {
+		t.Fatalf("RefusalForKey(valid) = (%q,%v), want (\"\",true)", reason, ok)
+	}
+	// A non-key string never reaches IAM at all.
+	before := calls
+	if reason, ok := RefusalForKey(context.Background(), "not-a-key"); ok || reason != "" {
+		t.Fatalf("RefusalForKey(garbage) = (%q,%v), want (\"\",false)", reason, ok)
+	}
+	if calls != before {
+		t.Errorf("a non-key string cost %d IAM call(s), want 0", calls-before)
+	}
+	// Asking again is free — the reason rides the cache the auth path already filled.
+	before = calls
+	if _, _ = RefusalForKey(context.Background(), "hk-live-REVOKED"); calls != before {
+		t.Errorf("re-asking why cost %d extra IAM call(s), want 0", calls-before)
+	}
+}
+
+// KeyHint names a key without disclosing it — enough for a holder to tell WHICH key
+// failed, useless to anyone who reads the log.
+func TestKeyHint_NeverDisclosesTheKey(t *testing.T) {
+	const key = "hk-902abd8e-dead-beef-cafe-000000000000"
+	hint := KeyHint(key)
+	if hint != "hk-902abd…" {
+		t.Fatalf("KeyHint = %q, want hk-902abd…", hint)
+	}
+	if strings.Contains(key, hint) {
+		t.Fatalf("the hint %q is a literal prefix long enough to be a substring test failure", hint)
+	}
+	// Nothing beyond the first 9 characters ever appears.
+	if strings.Contains(hint, "beef") || strings.Contains(hint, "dead") || len(hint) > 12 {
+		t.Fatalf("KeyHint leaked key material: %q", hint)
+	}
+	// A short/empty value discloses nothing at all rather than the whole string.
+	for _, short := range []string{"", "hk-", "hk-abc"} {
+		if h := KeyHint(short); h != "…" {
+			t.Errorf("KeyHint(%q) = %q, want …", short, h)
+		}
+	}
+}