amadan
objstore: enforcement converges — EnsureUser takes the deny, Grant hoists it
Two changes that are really one bug. EnsureUser gains a deny *DenyTag and stays the single policy writer, so the base and enforced documents cannot drift. policyStatement gains a Condition marshalled omitempty, so a nil deny emits no Condition key. And Grant now calls EnsureUser UNCONDITIONALLY, before the idempotence short-circuit — the fix internal/mail's walk made as C2, for the same reason. EnsureUser is the only PutUserPolicy caller, Grant returned before it for any healthy store, and Rotate never writes a policy at all, so turning enforcement on would have changed the record and nothing else, forever, on every store it could ever be turned on for. Verified by putting the old ordering back: the test fails and the live policy comes back with two Allow statements and no Deny. EnsureBucket stays behind the short-circuit. Only the policy moved; the reap-and-mint that spends IAM's two-key ceiling is still gated. The app's Allow gains s3:GetObjectTagging so it can read the verdict on its own object. s3:PutObjectTagging stays absent and must: the deny only means anything while an app cannot mark its own object clean. assertStorePolicyDocument counted actions, so it failed as a logic error rather than a compile error — updated to four.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9 files changed,
+298
−22
internal/console/deletion_test.go+1 −1internal/objstore/aws.go+40 −5internal/objstore/aws_test.go+11 −4internal/objstore/enforce_test.go+187 −0internal/objstore/memprovisioner.go+24 −2internal/objstore/memprovisioner_test.go+1 −1internal/objstore/provisioner.go+5 −1internal/objstore/walk.go+27 −6internal/objstore/walk_test.go+2 −2
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go| index 824ff7294..a6ff58b4d 100644 |
| --- a/internal/console/deletion_test.go |
| +++ b/internal/console/deletion_test.go |
| @@ -486,7 +486,7 @@ func seedStore(t *testing.T, fx *consoleFixture, fake *objstore.MemProvisioner, |
| } |
| fake.Buckets[bucket]["customer/data.bin"] = []byte("the customer's objects") |
| user := objstore.UserName(fx.sqid, app) |
| - arn, err := fake.EnsureUser(ctx, user, objstore.UserPath, "arn:aws:s3:::"+bucket) |
| + arn, err := fake.EnsureUser(ctx, user, objstore.UserPath, "arn:aws:s3:::"+bucket, nil) |
| if err != nil { |
| t.Fatal(err) |
| } |
diff --git a/internal/objstore/aws.go b/internal/objstore/aws.go| index bfaaa5993..c0db4317d 100644 |
| --- a/internal/objstore/aws.go |
| +++ b/internal/objstore/aws.go |
| @@ -113,6 +113,9 @@ type policyStatement struct { |
| Effect string `json:"Effect"` |
| Action []string `json:"Action"` |
| Resource string `json:"Resource"` |
| + // Condition is omitted entirely when nil, so a store with no enforcement |
| + // carries exactly the two Allow statements and no Condition key at all. |
| + Condition map[string]map[string]string `json:"Condition,omitempty"` |
| } |
| type policyDocument struct { |
| @@ -121,15 +124,47 @@ type policyDocument struct { |
| } |
| // storePolicyJSON is the exact document every store user's inline policy |
| -// carries: ListBucket on the bucket itself, object verbs scoped under it. |
| -func storePolicyJSON(bucketARN string) string { |
| +// carries: ListBucket on the bucket itself, object verbs scoped under it, and |
| +// — when deny is non-nil — a Deny on reading an object the scanner has tagged |
| +// infected (spec §8). |
| +// |
| +// s3:GetObjectTagging is in the Allow so an app can read the verdict on its |
| +// own object. s3:PutObjectTagging is NOT, and MUST NEVER BE: the deny below |
| +// is only meaningful while an app cannot mark its own object clean. That |
| +// absence is the whole anti-forgery property, and it is load-bearing rather |
| +// than an oversight. |
| +// |
| +// Adding GetObjectTagging changes this document for every existing store, |
| +// which is picked up on the next grant re-walk. That is a deliberate one-time |
| +// rewrite, not a regression: PutUserPolicy overwrites wholesale and is |
| +// idempotent, so the cost is one extra IAM call per re-walk. |
| +// |
| +// The deny is weaker than AWS's own tag-based access control example, which |
| +// denies reads UNLESS an object is tagged clean and so blocks unscanned |
| +// objects too. This one blocks only known-infected objects; unscanned ones |
| +// stay readable. That is the right trade when the goal is evidence rather |
| +// than enforcement, but anyone quoting it in a compliance answer needs to |
| +// know which of the two they have. |
| +func storePolicyJSON(bucketARN string, deny *DenyTag) string { |
| doc := policyDocument{ |
| Version: "2012-10-17", |
| Statement: []policyStatement{ |
| {Effect: "Allow", Action: []string{"s3:ListBucket"}, Resource: bucketARN}, |
| - {Effect: "Allow", Action: []string{"s3:GetObject", "s3:PutObject", "s3:DeleteObject"}, Resource: bucketARN + "/*"}, |
| + {Effect: "Allow", Action: []string{ |
| + "s3:GetObject", "s3:PutObject", "s3:DeleteObject", "s3:GetObjectTagging", |
| + }, Resource: bucketARN + "/*"}, |
| }, |
| } |
| + if deny != nil { |
| + doc.Statement = append(doc.Statement, policyStatement{ |
| + Effect: "Deny", |
| + Action: []string{"s3:GetObject"}, |
| + Resource: bucketARN + "/*", |
| + Condition: map[string]map[string]string{ |
| + "StringEquals": {"s3:ExistingObjectTag/" + deny.Key: deny.Value}, |
| + }, |
| + }) |
| + } |
| b, err := json.Marshal(doc) |
| if err != nil { |
| // doc is a static struct of strings/string-slices; Marshal cannot fail. |
| @@ -279,7 +314,7 @@ func (p *awsProvisioner) DeleteBucket(ctx context.Context, name string) error { |
| // a crash between CreateUser and PutUserPolicy) is resolved to its ARN via |
| // GetUser rather than treated as an error; PutUserPolicy is an idempotent |
| // overwrite, so it always runs regardless of which branch produced the ARN. |
| -func (p *awsProvisioner) EnsureUser(ctx context.Context, name, path, bucketARN string) (string, error) { |
| +func (p *awsProvisioner) EnsureUser(ctx context.Context, name, path, bucketARN string, deny *DenyTag) (string, error) { |
| var arn string |
| out, err := p.iam.CreateUser(ctx, &iam.CreateUserInput{ |
| UserName: aws.String(name), |
| @@ -302,7 +337,7 @@ func (p *awsProvisioner) EnsureUser(ctx context.Context, name, path, bucketARN s |
| _, err = p.iam.PutUserPolicy(ctx, &iam.PutUserPolicyInput{ |
| UserName: aws.String(name), |
| PolicyName: aws.String(policyName), |
| - PolicyDocument: aws.String(storePolicyJSON(bucketARN)), |
| + PolicyDocument: aws.String(storePolicyJSON(bucketARN, deny)), |
| }) |
| if err != nil { |
| return "", fmt.Errorf("objstore: put policy for user %q: %w", name, err) |
diff --git a/internal/objstore/aws_test.go b/internal/objstore/aws_test.go| index 20269f4d2..053208e14 100644 |
| --- a/internal/objstore/aws_test.go |
| +++ b/internal/objstore/aws_test.go |
| @@ -479,7 +479,7 @@ func TestEnsureUserAlreadyExists(t *testing.T) { |
| return &iam.PutUserPolicyOutput{}, nil |
| }, |
| }} |
| - arn, err := p.EnsureUser(context.Background(), "u1", UserPath, "arn:aws:s3:::b1") |
| + arn, err := p.EnsureUser(context.Background(), "u1", UserPath, "arn:aws:s3:::b1", nil) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -512,7 +512,7 @@ func TestEnsureUserFresh(t *testing.T) { |
| return &iam.PutUserPolicyOutput{}, nil |
| }, |
| }} |
| - arn, err := p.EnsureUser(context.Background(), "u1", UserPath, "arn:aws:s3:::b1") |
| + arn, err := p.EnsureUser(context.Background(), "u1", UserPath, "arn:aws:s3:::b1", nil) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -547,8 +547,15 @@ func assertStorePolicyDocument(t *testing.T, doc, bucketARN string) { |
| if list.Effect != "Allow" || len(list.Action) != 1 || list.Action[0] != "s3:ListBucket" || list.Resource != bucketARN { |
| t.Fatalf("ListBucket statement = %+v", list) |
| } |
| - wantActions := map[string]bool{"s3:GetObject": true, "s3:PutObject": true, "s3:DeleteObject": true} |
| - if objects.Effect != "Allow" || len(objects.Action) != 3 || objects.Resource != bucketARN+"/*" { |
| + // s3:GetObjectTagging is here so an app can read the scan verdict on its |
| + // own object. s3:PutObjectTagging is deliberately absent and must stay |
| + // absent: the scanning Deny statement is only meaningful while an app |
| + // cannot mark its own object clean. |
| + wantActions := map[string]bool{ |
| + "s3:GetObject": true, "s3:PutObject": true, "s3:DeleteObject": true, |
| + "s3:GetObjectTagging": true, |
| + } |
| + if objects.Effect != "Allow" || len(objects.Action) != 4 || objects.Resource != bucketARN+"/*" { |
| t.Fatalf("object statement = %+v", objects) |
| } |
| for _, a := range objects.Action { |
diff --git a/internal/objstore/enforce_test.go b/internal/objstore/enforce_test.go| new file mode 100644 |
| index 000000000..7f35baaeb |
| --- /dev/null |
| +++ b/internal/objstore/enforce_test.go |
| @@ -0,0 +1,187 @@ |
| +package objstore |
| + |
| +import ( |
| + "context" |
| + "encoding/json" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// With no deny, the document has exactly two statements and no Condition key, |
| +// so an enforcement-off store never carries a Deny. |
| +// |
| +// This is NOT "byte for byte what it was before scanning existed": the same |
| +// change adds s3:GetObjectTagging so an app can read the verdict on its own |
| +// object, which changes the bytes by definition. Every store's policy is |
| +// rewritten once on its next grant re-walk, deliberately. |
| +func TestStorePolicyJSONShapeWithoutDeny(t *testing.T) { |
| + doc := storePolicyJSON("arn:aws:s3:::b", nil) |
| + |
| + var parsed policyDocument |
| + if err := json.Unmarshal([]byte(doc), &parsed); err != nil { |
| + t.Fatalf("not valid JSON: %v\n%s", err, doc) |
| + } |
| + if len(parsed.Statement) != 2 { |
| + t.Fatalf("want exactly two statements without a deny, got %d:\n%s", |
| + len(parsed.Statement), doc) |
| + } |
| + if strings.Contains(doc, "Condition") { |
| + t.Fatalf("a nil deny must emit no Condition key:\n%s", doc) |
| + } |
| + if strings.Contains(doc, `"Effect":"Deny"`) { |
| + t.Fatalf("a nil deny must emit no Deny statement:\n%s", doc) |
| + } |
| + if !strings.Contains(doc, "s3:GetObjectTagging") { |
| + t.Fatalf("an app cannot read its own verdict without GetObjectTagging:\n%s", doc) |
| + } |
| + assertNoPutObjectTagging(t, doc) |
| +} |
| + |
| +// With a deny, a third statement appears, conditioned on the engine's verdict |
| +// tag. |
| +func TestStorePolicyJSONWithDeny(t *testing.T) { |
| + doc := storePolicyJSON("arn:aws:s3:::b", |
| + &DenyTag{Key: TagVerdict, Value: string(VerdictInfected)}) |
| + |
| + var parsed policyDocument |
| + if err := json.Unmarshal([]byte(doc), &parsed); err != nil { |
| + t.Fatalf("not valid JSON: %v\n%s", err, doc) |
| + } |
| + if len(parsed.Statement) != 3 { |
| + t.Fatalf("want three statements with a deny, got %d:\n%s", len(parsed.Statement), doc) |
| + } |
| + deny := parsed.Statement[2] |
| + if deny.Effect != "Deny" || len(deny.Action) != 1 || deny.Action[0] != "s3:GetObject" { |
| + t.Fatalf("deny statement = %+v", deny) |
| + } |
| + if got := deny.Condition["StringEquals"]["s3:ExistingObjectTag/"+TagVerdict]; got != "infected" { |
| + t.Fatalf("deny does not condition on the verdict tag: %+v", deny.Condition) |
| + } |
| + assertNoPutObjectTagging(t, doc) |
| +} |
| + |
| +// The anti-forgery property, asserted in one place: the app must never be able |
| +// to tag its own objects, or it could mark an infected object clean and walk |
| +// straight through the deny. |
| +func assertNoPutObjectTagging(t *testing.T, doc string) { |
| + t.Helper() |
| + if strings.Contains(doc, "s3:PutObjectTagging") { |
| + t.Fatalf("app must never be able to tag its own objects:\n%s", doc) |
| + } |
| +} |
| + |
| +// The fake rewrites an EXISTING user's policy. MemProvisioner.EnsureUser |
| +// early-returns when the principal is already there, and enforcement is only |
| +// ever turned on for a store that already has one — so a fake that recorded |
| +// the policy on the create branch alone would make the next test vacuous. |
| +func TestMemProvisionerRewritesPolicyForExistingUser(t *testing.T) { |
| + ctx := context.Background() |
| + p := NewMemProvisioner("eu-west-1") |
| + |
| + if _, err := p.EnsureUser(ctx, "u", UserPath, "arn:aws:s3:::b", nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + if strings.Contains(p.PolicyFor("u"), `"Effect":"Deny"`) { |
| + t.Fatal("no deny was asked for") |
| + } |
| + if _, err := p.EnsureUser(ctx, "u", UserPath, "arn:aws:s3:::b", |
| + &DenyTag{Key: TagVerdict, Value: string(VerdictInfected)}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.Contains(p.PolicyFor("u"), `"Effect":"Deny"`) { |
| + t.Fatal("EnsureUser did not rewrite an existing user's policy") |
| + } |
| +} |
| + |
| +// Flipping enforcement on a HEALTHY, fully-delivered store must reach the |
| +// backend. |
| +// |
| +// Before EnsureUser was hoisted it could not: EnsureUser is the only |
| +// PutUserPolicy caller, Grant returned at the idempotence short-circuit before |
| +// ever reaching it, and Rotate never writes a policy at all — so enforcement |
| +// would have been permanently inert on every store it was ever turned on for. |
| +// This is the same bug internal/mail's walk fixed as "C2". |
| +func TestGrantWritesDenyOnHealthyStore(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + |
| + // rotateFixture leaves a fully provisioned, fully delivered store — the |
| + // exact state the short-circuit returns early on. |
| + rotateFixture(t, s, pub, p) |
| + user := UserName("bab", "titogo") |
| + if strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) { |
| + t.Fatal("no deny was asked for yet") |
| + } |
| + |
| + // Turn enforcement on. Read-modify-write with the real etag: Put with "" |
| + // is create-only and would return release.ErrCASConflict here. |
| + stored, etag, err := Get(ctx, s, "bab", "titogo") |
| + if err != nil || stored == nil { |
| + t.Fatalf("re-read: %+v, %v", stored, err) |
| + } |
| + stored.Scanning = &ScanBlock{ |
| + Enabled: &ScanEnabled{Engine: "clamav", At: time.Now().UTC(), By: "op@example.com"}, |
| + Enforce: &DenyTag{Key: TagVerdict, Value: string(VerdictInfected)}, |
| + } |
| + if _, err := Put(ctx, s, "bab", "titogo", stored, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, |
| + "bab", "titogo", "op@example.com", "", time.Now().UTC()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) { |
| + t.Fatalf("re-walking a healthy store did not write the deny — the hoist is not working:\n%s", |
| + p.PolicyFor(user)) |
| + } |
| +} |
| + |
| +// The mirror: clearing Enforce and re-walking takes the deny back out. A |
| +// `scan enforce off` that left a live Deny standing would be worse than never |
| +// having offered the switch. |
| +func TestGrantClearsDenyWhenEnforceIsCleared(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + rotateFixture(t, s, pub, p) |
| + user := UserName("bab", "titogo") |
| + |
| + stored, etag, err := Get(ctx, s, "bab", "titogo") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + stored.Scanning = &ScanBlock{Enforce: &DenyTag{Key: TagVerdict, Value: string(VerdictInfected)}} |
| + if _, err := Put(ctx, s, "bab", "titogo", stored, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, |
| + "bab", "titogo", "op@example.com", "", time.Now().UTC()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) { |
| + t.Fatal("setup: the deny was never written") |
| + } |
| + |
| + stored, etag, err = Get(ctx, s, "bab", "titogo") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + stored.Scanning.Enforce = nil |
| + if _, err := Put(ctx, s, "bab", "titogo", stored, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, |
| + "bab", "titogo", "op@example.com", "", time.Now().UTC()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if strings.Contains(p.PolicyFor(user), `"Effect":"Deny"`) { |
| + t.Fatalf("clearing Enforce left a live Deny standing:\n%s", p.PolicyFor(user)) |
| + } |
| +} |
diff --git a/internal/objstore/memprovisioner.go b/internal/objstore/memprovisioner.go| index cb8be9090..8b393d06c 100644 |
| --- a/internal/objstore/memprovisioner.go |
| +++ b/internal/objstore/memprovisioner.go |
| @@ -12,6 +12,11 @@ import ( |
| type MemUser struct { |
| ARN string |
| Keys map[string]string |
| + // Policy is the inline policy document EnsureUser last wrote. The real |
| + // provisioner's PutUserPolicy overwrites wholesale on every call, so the |
| + // fake records it on every call too — including for a user that already |
| + // exists, which is the only case enforcement is ever turned on for. |
| + Policy string |
| } |
| // MemProvisioner is an in-memory Provisioner for walk tests: no AWS, no |
| @@ -71,20 +76,37 @@ func (p *MemProvisioner) EnsureBucket(ctx context.Context, name string) (string, |
| return p.region, nil |
| } |
| -func (p *MemProvisioner) EnsureUser(ctx context.Context, name, path, bucketARN string) (string, error) { |
| +func (p *MemProvisioner) EnsureUser(ctx context.Context, name, path, bucketARN string, deny *DenyTag) (string, error) { |
| p.mu.Lock() |
| defer p.mu.Unlock() |
| if err := p.takeFailure("EnsureUser"); err != nil { |
| return "", err |
| } |
| + policy := storePolicyJSON(bucketARN, deny) |
| if u, ok := p.Users[name]; ok { |
| + // The policy is rewritten for an existing principal too, mirroring |
| + // PutUserPolicy running unconditionally on both of awsProvisioner's |
| + // branches. A fake that recorded it only on the create branch would |
| + // make enforcement look broken exactly where it matters: every store |
| + // this is ever turned on for already has its user. |
| + u.Policy = policy |
| + p.Users[name] = u // MemUser is a value in the map — mutate, store back |
| return u.ARN, nil |
| } |
| arn := "arn:mem:iam::user" + path + name |
| - p.Users[name] = MemUser{ARN: arn, Keys: map[string]string{}} |
| + p.Users[name] = MemUser{ARN: arn, Keys: map[string]string{}, Policy: policy} |
| return arn, nil |
| } |
| +// PolicyFor is the inline policy the fake last wrote for a user, so a walk |
| +// test can assert that enforcement reached the backend rather than only |
| +// reaching the record. |
| +func (p *MemProvisioner) PolicyFor(user string) string { |
| + p.mu.Lock() |
| + defer p.mu.Unlock() |
| + return p.Users[user].Policy |
| +} |
| + |
| func (p *MemProvisioner) MintKey(ctx context.Context, user string) (string, string, error) { |
| p.mu.Lock() |
| defer p.mu.Unlock() |
diff --git a/internal/objstore/memprovisioner_test.go b/internal/objstore/memprovisioner_test.go| index de6e319f2..2e5b1d1a6 100644 |
| --- a/internal/objstore/memprovisioner_test.go |
| +++ b/internal/objstore/memprovisioner_test.go |
| @@ -13,7 +13,7 @@ func TestMemProvisionerIdempotence(t *testing.T) { |
| if region, err := p.EnsureBucket(ctx, "b1"); err != nil || region != "eu-west-1" { |
| t.Fatalf("EnsureBucket #%d: %q %v", i, region, err) |
| } |
| - if arn, err := p.EnsureUser(ctx, "u1", "/carlos-store/", "arn:aws:s3:::b1"); err != nil || arn == "" { |
| + if arn, err := p.EnsureUser(ctx, "u1", "/carlos-store/", "arn:aws:s3:::b1", nil); err != nil || arn == "" { |
| t.Fatalf("EnsureUser #%d: %v", i, err) |
| } |
| } |
diff --git a/internal/objstore/provisioner.go b/internal/objstore/provisioner.go| index 8294ed823..d96d02272 100644 |
| --- a/internal/objstore/provisioner.go |
| +++ b/internal/objstore/provisioner.go |
| @@ -14,7 +14,11 @@ type Provisioner interface { |
| EnsureBucket(ctx context.Context, name string) (region string, err error) |
| // EnsureUser creates the credential principal if absent, scoped to |
| // exactly bucketARN (ListBucket on it; Get/Put/DeleteObject under it). |
| - EnsureUser(ctx context.Context, name, path, bucketARN string) (arn string, err error) |
| + // deny, when non-nil, adds a Deny on reading an object tagged infected. |
| + // It is threaded from the RECORD rather than asked of a Scanner, so a |
| + // repair walk on a deployment with no engine built can still write the |
| + // policy the record asks for instead of silently dropping enforcement. |
| + EnsureUser(ctx context.Context, name, path, bucketARN string, deny *DenyTag) (arn string, err error) |
| MintKey(ctx context.Context, user string) (id, secret string, err error) |
| DeleteKey(ctx context.Context, user, id string) error |
| // ReapKeys deletes every access key of user NOT named in keep. The grant |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index 94475421e..ff596cfc4 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -159,6 +159,32 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| return nil, err |
| } |
| } |
| + // 2. the principal and its policy. EnsureUser runs UNCONDITIONALLY, before |
| + // the idempotence short-circuit below — the same fix internal/mail's walk |
| + // made as "C2", for the same reason. |
| + // |
| + // EnsureUser is the ONLY place this user's policy is written |
| + // (PutUserPolicy overwrites wholesale), and the deny in it is recomputed |
| + // from the CURRENT record on every call. Behind the short-circuit, turning |
| + // enforcement on for an already-provisioned store — which is every store |
| + // it would ever be turned on for — changed the record and nothing else, |
| + // forever. There is no other convergence path: Rotate never writes a |
| + // policy either. |
| + // |
| + // It needs only the bucket ARN string, resolved above, not an existing |
| + // bucket, so EnsureBucket stays behind the short-circuit. EnsureUser is |
| + // cheap to re-run (idempotent CreateUser, one PutUserPolicy); it is the |
| + // reap-and-mint below that spends IAM's two-key ceiling, and THAT is what |
| + // the short-circuit still has to gate. |
| + var deny *DenyTag |
| + if rec.Scanning != nil { |
| + deny = rec.Scanning.Enforce |
| + } |
| + user := UserName(acct, app) |
| + arn, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket, deny) |
| + if err != nil { |
| + return nil, fmt.Errorf("objstore: ensure user %s: %w", user, err) |
| + } |
| // Idempotence short-circuit: "healthy" means the record says provisioned |
| // with THIS deployment's endpoint AND every delivered var is present, of |
| // the right kind, in config.json. Anything less and the walk runs, because |
| @@ -175,16 +201,11 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| return rec, nil |
| } |
| } |
| - // 2–3. backend resources |
| + // 3. the bucket |
| region, err := p.EnsureBucket(ctx, bucket) |
| if err != nil { |
| return nil, fmt.Errorf("objstore: ensure bucket %s: %w", bucket, err) |
| } |
| - user := UserName(acct, app) |
| - arn, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket) |
| - if err != nil { |
| - return nil, fmt.Errorf("objstore: ensure user %s: %w", user, err) |
| - } |
| // 4. key. Reap first: reaching here means delivery is missing, so any |
| // existing CURRENT key is either undelivered (its secret died with a |
| // failed config write — unrecoverable) or stranded by a crash between mint |
diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go| index 518a968c6..994589a6c 100644 |
| --- a/internal/objstore/walk_test.go |
| +++ b/internal/objstore/walk_test.go |
| @@ -1154,7 +1154,7 @@ func TestTeardownConfigObjectAbsent(t *testing.T) { |
| if _, err := p.EnsureBucket(ctx, "carlos-store-tito-bab-titogo"); err != nil { |
| t.Fatal(err) |
| } |
| - arn, err := p.EnsureUser(ctx, "carlos-store-bab-titogo", UserPath, "arn:aws:s3:::carlos-store-tito-bab-titogo") |
| + arn, err := p.EnsureUser(ctx, "carlos-store-bab-titogo", UserPath, "arn:aws:s3:::carlos-store-tito-bab-titogo", nil) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -1194,7 +1194,7 @@ func crashedGrant(t *testing.T, s release.Store, p *MemProvisioner) (bucket, use |
| if _, err := p.EnsureBucket(ctx, bucket); err != nil { |
| t.Fatal(err) |
| } |
| - if _, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket); err != nil { |
| + if _, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket, nil); err != nil { |
| t.Fatal(err) |
| } |
| id, _, err := p.MintKey(ctx, user) |