amadan

carlos / platform Public

console: store operator allowlist matches exactly, never folds

strings.EqualFold applies Unicode simple folding, in which U+017F (ſ) folds to
"s" — and ſ is already lowercase, so it survives the ASCII lowercasing every
address entry point does, and ValidAddress accepts it. A member signing in as
paul@rſlw.com matched an allowlist entry of paul@rslw.com and was handed the
authority to spend the deployment's money; the test added here fails with a
200 (bucket, key and delivered credentials) against the old comparison.

Compare the trimmed, lowercased list entry to the address exactly instead.

Also pinned: the allowlist refuses BEFORE the backend/bucket 503s, so the
grant door is not a deployment-config oracle for any member; rotate's lost-CAS
409 sentence, driven by a real competing rotate-shaped write through a racing
store; and grant's 502 sentence, driven by a failing provisioner, asserted to
carry no bucket name, ARN, status code or request id.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 146d7af1225cb3700cbad4d65c204e442144c7f9 parent 7dcd46b
2 files changed, +186 −6
  • internal/console/apistore.go +17 −6
  • internal/console/apistore_test.go +169 −0
diff --git a/internal/console/apistore.go b/internal/console/apistore.go
index e7303d20f..ddcee5d89 100644
--- a/internal/console/apistore.go
+++ b/internal/console/apistore.go
@@ -344,14 +344,25 @@ const storeRotateTimeout = 30 * time.Second
// storeOperator reports whether this address may drive the grant door.
//
-// Case-insensitive because the list is typed by a human into a unit file and
-// addresses are not case-sensitive; an operator whose keymail address renders
-// as Alice@… on one screen and alice@… on another must not silently lose the
-// authority. Empty list is fail-closed — the loop simply never matches — which
-// is the right direction for a gate on spending money.
+// EXACT MATCH ON THE CANONICAL FORM, never a fold. Every address entry point
+// in this codebase ASCII-lowercases on the way in (sign-in, team-add, the
+// materialize pass, `console member`), so the address on a CLI token is
+// already canonical; the list entry is lowercased and trimmed here so an
+// operator who typed Alice@Keymail.dev into a unit file still matches.
+//
+// strings.EqualFold would be the obvious way to write that and is WRONG for
+// the highest-authority gate in the API: it applies Unicode simple folding, in
+// which U+017F (ſ, long s) folds to "s" — and ſ is already lowercase, so it
+// survives every ToLower on the way in. ValidAddress permits it (anything
+// without whitespace and with an @). With a fold, paul@rſlw.com would match an
+// allowlist entry of paul@rslw.com and be handed the authority to spend the
+// deployment's money.
+//
+// Empty list is fail-closed — the loop simply never matches — which is the
+// right direction for a gate on spending.
func (s *Server) storeOperator(addr string) bool {
for _, op := range s.cfg.StoreOperators {
- if strings.EqualFold(strings.TrimSpace(op), addr) {
+ if strings.ToLower(strings.TrimSpace(op)) == addr {
return true
}
}
diff --git a/internal/console/apistore_test.go b/internal/console/apistore_test.go
index 10a36d848..d168e667d 100644
--- a/internal/console/apistore_test.go
+++ b/internal/console/apistore_test.go
@@ -11,6 +11,7 @@ package console
import (
"context"
+ "errors"
"net/http"
"strings"
"testing"
@@ -590,6 +591,56 @@ func TestAPIStoreGrantNeedsADeploymentOperator(t *testing.T) {
if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusForbidden {
t.Fatalf("grant with no operator list = %d %v, want 403", status, out)
}
+
+ // The allowlist runs BEFORE the deployment-shape 503s. A non-operator who
+ // could tell "this deployment has no object storage" from "you may not
+ // grant" would have turned the grant door into a config oracle for every
+ // member of every account.
+ fx.srv.cfg.StoreProvisioner = nil
+ if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusForbidden {
+ t.Fatalf("grant by a non-operator on a backend-less console = %d %v, want 403 — not the 503", status, out)
+ }
+ fx.srv.cfg.Releases = nil
+ if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusForbidden {
+ t.Fatalf("grant by a non-operator on a bucket-less console = %d %v, want 403", status, out)
+ }
+}
+
+// TestAPIStoreGrantAllowlistDoesNotFold: the allowlist is an EXACT match on
+// the canonical (ASCII-lowercased) address every entry point in this codebase
+// writes — not a Unicode fold. U+017F (ſ) folds to "s" and is already
+// lowercase, so it sails through every ToLower on the way in and ValidAddress
+// accepts it; under strings.EqualFold, paul@rſlw.com would be handed the
+// authority to spend this deployment's money on behalf of paul@rslw.com.
+func TestAPIStoreGrantAllowlistDoesNotFold(t *testing.T) {
+ fx, fake, token, _ := storeArmed(t, "code-store-grant-fold")
+ declareStore(t, fx, token)
+ fx.srv.cfg.StoreOperators = []string{"paul@rslw.com"}
+ const homoglyph = "paul@rſlw.com" // paul@rſlw.com
+ if err := fx.st.AddMember(OpsAccountID, homoglyph, RoleMember, time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ impostor := cliToken(t, fx, homoglyph, "code-store-grant-fold-imp")
+
+ // The member gate passes for this address — so a 403 below is the
+ // allowlist refusing, not the account resolution failing by accident.
+ if status, out := getJSON(t, storeURL(fx, "titogo"), impostor); status != http.StatusOK {
+ t.Fatalf("status as the ſ member = %d %v, want 200 — the member gate must be what passed", status, out)
+ }
+
+ status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, impostor)
+ if status != http.StatusForbidden {
+ t.Fatalf("grant as %q against an allowlist of paul@rslw.com = %d %v, want 403", homoglyph, status, out)
+ }
+ untouched(t, fake, "a fold-matched grant")
+ if rec := storedRecord(t, fx, "titogo"); rec.Grant != nil {
+ t.Fatalf("a fold-matched grant wrote a grant block: %+v", rec.Grant)
+ }
+ // The address the list actually names still works, however it was typed.
+ fx.srv.cfg.StoreOperators = []string{" Alice@Keymail.dev "}
+ if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK {
+ t.Fatalf("grant by the listed operator = %d %v, want 200", status, out)
+ }
}
// TestAPIStoreOperatorDoorsGateIsByteIdentical: the member gate runs BEFORE
@@ -968,3 +1019,121 @@ func TestAPIStoreRotateWithoutAProvisionedStoreIs409(t *testing.T) {
t.Fatalf("serial = %d, want 0", got)
}
}
+
+// racingStore lands a competing write on one key immediately BEFORE the
+// handler's own Put reaches the bucket — the interleaving that turns a walk's
+// etag stale. One shot, so only the write under test races. Mirrors the
+// wrapper objstore's walk tests use, at the console's own seam: the fixture
+// owns Config.Releases, so swapping it in is all it takes to make a
+// two-console race reproducible in one process.
+type racingStore struct {
+ release.Store
+ raceKeyOnce string
+ race func(inner release.Store)
+}
+
+func (r *racingStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) {
+ if key != "" && key == r.raceKeyOnce {
+ r.raceKeyOnce = ""
+ r.race(r.Store)
+ }
+ return r.Store.Put(ctx, key, body, ifMatch)
+}
+
+// TestAPIStoreRotateConflictSaysReRun: the highest-stakes sentence in this
+// file. A rotation that loses its final CAS to ANOTHER rotation has already
+// minted a key and delivered it into the app's config — the walk refuses to
+// overwrite the competitor's record precisely because that record names a key
+// somebody else delivered. So the answer must never read as "nothing
+// happened": a member told their rotation failed would go looking for a key
+// that is already live, and might reach for a grant, which reaps.
+func TestAPIStoreRotateConflictSaysReRun(t *testing.T) {
+ fx, _, token, _ := storeArmed(t, "code-store-rotate-conflict")
+ declareStore(t, fx, token)
+ if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK {
+ t.Fatalf("grant = %d %v", status, out)
+ }
+ firstKey := storedRecord(t, fx, "titogo").Provisioned.KeyID
+
+ // From here on, the console writes through a store that lets a competing
+ // rotation land in the window between this walk's Get and its Put.
+ racer := &racingStore{Store: fx.rel, raceKeyOnce: objstore.Key(release.OpsAccount, "titogo")}
+ racer.race = func(in release.Store) {
+ other, etag, err := objstore.Get(context.Background(), in, release.OpsAccount, "titogo")
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Another rotation got there first and delivered a key of its own.
+ other.Provisioned.KeyID = "MEMKEY99"
+ other.Provisioned.PrevKeyID = firstKey
+ if _, err := objstore.Put(context.Background(), in, release.OpsAccount, "titogo", other, etag); err != nil {
+ t.Fatal(err)
+ }
+ }
+ fx.srv.cfg.Releases = racer
+
+ status, out := postJSON(t, storeURL(fx, "titogo")+"/rotate", map[string]any{}, token)
+ if status != http.StatusConflict {
+ t.Fatalf("rotate over a concurrent rotation = %d %v, want 409", status, out)
+ }
+ if msg := str(t, out, "error"); msg != "another rotation is in progress — re-run to converge" {
+ t.Fatalf("error = %q, want the re-run-to-converge sentence", msg)
+ }
+ // The words that would be a lie: this caller HAS minted and delivered.
+ for _, forbidden := range []string{"nothing", "failed", "try again"} {
+ if strings.Contains(strings.ToLower(str(t, out, "error")), forbidden) {
+ t.Fatalf("error = %q, must not suggest the rotation left nothing behind", out["error"])
+ }
+ }
+ // The competitor's record stands — the whole reason the walk gave up.
+ if rec := storedRecord(t, fx, "titogo"); rec.Provisioned.KeyID != "MEMKEY99" {
+ t.Fatalf("the competing rotation was clobbered: %+v", rec.Provisioned)
+ }
+}
+
+// TestAPIStoreGrantFailureSaysResume: a backend that refuses gets ONE fixed
+// sentence, and the answer carries no bucket name, ARN, endpoint or request id
+// — those go to the console log, where the operator can read them and a
+// tenant cannot. The sentence's promise is what the walk's ordering
+// guarantees: the grant block naming the bucket is written BEFORE anything is
+// created, so nothing exists that only a bill would find, and the same verb
+// run again resumes from where it stopped.
+func TestAPIStoreGrantFailureSaysResume(t *testing.T) {
+ fx, fake, token, _ := storeArmed(t, "code-store-grant-failure")
+ declareStore(t, fx, token)
+ fake.FailNext["EnsureBucket"] = errors.New(
+ "AccessDenied: arn:aws:s3:::carlos-bab-titogo — status code 403, request id 8EXAMPLE7")
+
+ status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token)
+ if status != http.StatusBadGateway {
+ t.Fatalf("grant against a refusing backend = %d %v, want 502", status, out)
+ }
+ msg := str(t, out, "error")
+ if msg != "could not provision the store — nothing unrecorded was left behind; re-run the grant to resume" {
+ t.Fatalf("error = %q, want the fixed resume sentence", msg)
+ }
+ for _, leak := range []string{"carlos-bab-titogo", "AccessDenied", "arn:aws", "403", "8EXAMPLE7"} {
+ if strings.Contains(msg, leak) {
+ t.Fatalf("error = %q, leaks backend detail %q", msg, leak)
+ }
+ }
+ // Recorded before created: the bucket the walk was going to make is named
+ // in the record, which is exactly why the sentence above is true.
+ rec := storedRecord(t, fx, "titogo")
+ if rec.Grant == nil || rec.Grant.Bucket != "carlos-bab-titogo" {
+ t.Fatalf("grant block after a failed walk = %+v, want the bucket recorded", rec.Grant)
+ }
+ if rec.Provisioned != nil {
+ t.Fatalf("provisioned = %+v after a failed walk", rec.Provisioned)
+ }
+ if got := serial(t, fx); got != 0 {
+ t.Fatalf("serial after a failed grant = %d, want 0 — nothing was delivered", got)
+ }
+ // The one-shot failure is spent; re-running resumes and completes.
+ if status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{}, token); status != http.StatusOK {
+ t.Fatalf("re-run grant = %d %v, want 200", status, out)
+ }
+ if _, ok := fake.Buckets["carlos-bab-titogo"]; !ok {
+ t.Fatalf("buckets after the resume = %v", fake.Buckets)
+ }
+}