diff --git a/pkg/blobmanager/s3accesspoint/backend.go b/pkg/blobmanager/s3accesspoint/backend.go index 33a42d829..1897eeb92 100644 --- a/pkg/blobmanager/s3accesspoint/backend.go +++ b/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 diff --git a/pkg/blobmanager/s3accesspoint/backend_test.go b/pkg/blobmanager/s3accesspoint/backend_test.go index 587101373..8aeb12e42 100644 --- a/pkg/blobmanager/s3accesspoint/backend_test.go +++ b/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 diff --git a/pkg/blobmanager/s3accesspoint/provider.go b/pkg/blobmanager/s3accesspoint/provider.go index 6dab7d1d6..378ae9a89 100644 --- a/pkg/blobmanager/s3accesspoint/provider.go +++ b/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 /sha256: and the AssumeRole -// session policy's Resource is scoped to ${apARN}/object//*. -// 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 /sha256:, 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