refactor(blobmanager/s3accesspoint): slim AssumeRole session policy (#3143)

Author: jiparis

pkg/blobmanager/s3accesspoint/backend.go

@@ -296,13 +296,14 @@ func (p *sessionCredentialsProvider) Retrieve(ctx context.Context) (aws.Credenti
 		return p.ambientCreds.Retrieve(ctx)
 	}
 
-	// Session policy intersects with the base role's permissions; even
-	// if the role grants accesspoint/*, this session can only touch the
-	// caller's AP and prefix. The prefix is the requesting-org UUID
-	// straight from ctx — same source as the session name — so a
-	// tampered AccessPointARN in the secret blob can't widen the prefix
-	// scope to escape into another tenant's namespace.
-	sessionPolicy := buildSessionPolicy(p.creds.AccessPointARN, info.OrgID)
+	// Session policy intersects with the base role's permissions and
+	// pins this session to the caller's AP ARN. Cross-tenant defense
+	// against a tampered AccessPointARN in the secret blob lives in the
+	// AP's resource policy (aws:userid StringEquals on the role session
+	// name minted from the request-context org UUID), not here — keeping
+	// the inline policy small leaves headroom in STS's packed-policy
+	// budget for tags inherited from the caller principal.
+	sessionPolicy := buildSessionPolicy(p.creds.AccessPointARN)
 
 	out, err := p.stsClient.AssumeRole(ctx, &sts.AssumeRoleInput{
 		RoleArn:         aws.String(p.creds.BaseRoleARN),
@@ -334,15 +335,15 @@ func roleSessionName(orgUUID string) string {
 	return "cas-" + orgUUID
 }
 
-// buildSessionPolicy returns an IAM policy document that allows only the
-// operations the backend actually performs, and only against this
-// tenant's AP + key prefix. The Resource ARNs use the AP form
-// "${apARN}/object/${keyPrefix}/*".
-func buildSessionPolicy(apARN, keyPrefix string) string {
-	// Minimal, hand-written JSON — keeping it small reduces request
-	// payload (STS limits session policies to 2048 chars by default).
-	return fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject","s3:PutObject","s3:DeleteObject","s3:GetObjectAttributes"],"Resource":"%s/object/%s/*"}]}`,
-		apARN, keyPrefix)
+// buildSessionPolicy returns an IAM policy document allowing only the
+// S3 actions the backend actually performs (Get/Head/Put — HeadObject
+// is authorized as s3:GetObject) and scoped to this tenant's AP ARN.
+// Cross-tenant isolation is enforced by the AP resource policy's
+// aws:userid check against the role session name, not by this policy;
+// keeping the inline document minimal preserves headroom in the STS
+// packed-policy budget against tags inherited from the caller.
+func buildSessionPolicy(apARN string) string {
+	return fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject","s3:PutObject"],"Resource":"%s/object/*"}]}`, apARN)
 }
 
 // hexSha256ToBinaryB64 decodes the hex sha and re-encodes as base64. S3

pkg/blobmanager/s3accesspoint/backend_test.go

@@ -90,21 +90,27 @@ func TestBackend_KeyDerivedFromRequestingOrg(t *testing.T) {
 	require.ErrorIs(t, err, ErrMissingRequestingOrg)
 }
 
-// TestSessionPolicy_ScopesToTenantPrefix locks down the session-policy
-// generator: the IAM policy minted at AssumeRole time must reference
-// both the AP ARN and the tenant key prefix, so a leaked token can't
-// touch keys outside its tenant's namespace.
-func TestSessionPolicy_ScopesToTenantPrefix(t *testing.T) {
+// TestSessionPolicy_ScopesToAccessPoint locks down the session-policy
+// generator: the IAM policy minted at AssumeRole time must scope to the
+// AP ARN (the tenant boundary lives in the AP resource policy, not
+// here), and must not include S3 actions the backend never calls — that
+// keeps the inline policy small so STS's packed-policy budget has
+// headroom for tags inherited from the caller principal.
+func TestSessionPolicy_ScopesToAccessPoint(t *testing.T) {
 	t.Parallel()
 
-	policy := buildSessionPolicy("arn:aws:s3:us-east-1:111:accesspoint/ap-a", "org/A")
+	policy := buildSessionPolicy("arn:aws:s3:us-east-1:111:accesspoint/ap-a")
 
-	assert.Contains(t, policy, `"arn:aws:s3:us-east-1:111:accesspoint/ap-a/object/org/A/*"`,
-		"policy Resource must be the AP ARN + tenant prefix")
+	assert.Contains(t, policy, `"arn:aws:s3:us-east-1:111:accesspoint/ap-a/object/*"`,
+		"policy Resource must be the AP ARN scoped to /object/*")
 	assert.NotContains(t, policy, `"*"`,
 		"session policy must not wildcard the Resource")
 	assert.Contains(t, policy, `"s3:GetObject"`)
 	assert.Contains(t, policy, `"s3:PutObject"`)
+	assert.NotContains(t, policy, `"s3:DeleteObject"`,
+		"backend never deletes — re-adding s3:DeleteObject grows the packed-policy footprint without need")
+	assert.NotContains(t, policy, `"s3:GetObjectAttributes"`,
+		"backend never calls GetObjectAttributes — re-adding it grows the packed-policy footprint without need")
 }
 
 // TestRoleSessionName_DerivedFromOrg pins the session-name shape that

pkg/blobmanager/s3accesspoint/provider.go

@@ -24,12 +24,16 @@
 //     resource policy enforces a StringEquals on aws:userid so that a
 //     session minted for org A cannot read or write to org B's AP — even if
 //     org A's secret blob has been tampered with to point at org B's ARN.
+//     This is the actual cross-tenant boundary.
 //  3. A per-tenant key prefix derived from the requesting org UUID: every
-//     object is keyed as <orgUUID>/sha256:<digest> and the AssumeRole
-//     session policy's Resource is scoped to ${apARN}/object/<orgUUID>/*.
-//     The prefix shares its source of truth with the session name, so a
-//     tampered secret cannot reroute a tenant's writes into a different
-//     namespace.
+//     object is keyed as <orgUUID>/sha256:<digest>, sharing its source of
+//     truth (the request-context org claim) with the role session name.
+//     This avoids cross-tenant collisions at the bucket layer; it is not
+//     load-bearing for tenant isolation, which is enforced by (1) and (2).
+//     The AssumeRole session policy's Resource scopes to the AP ARN only
+//     (${apARN}/object/*), keeping the inline document small so STS's
+//     packed-policy budget isn't consumed by chainloop before tags
+//     inherited from the caller principal are even accounted for.
 //
 // The session name MUST come from the request context, not from the secret
 // blob: a secrets-store compromise alone must not let an attacker reroute