diff --git a/auth/access-control.go b/auth/access-control.go index 15191070..578d1f46 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -291,7 +291,7 @@ func objectsAccessErrors(ctx context.Context, be backend.Backend, opts AccessOpt allDenied := true for i, rd := range resourceDecisions { if rd.Decision == policyDecisionDeny { - errs[i] = s3err.GetExplicitDenyAccessErr(opts.Acc.Access, string(rd.Action), objectPolicyArn(opts.Bucket, keys[i], be.NormalizeObjectKey), "a resource-based policy") + errs[i] = s3err.GetExplicitDenyAccessErr(principalName(opts.Acc), string(rd.Action), objectPolicyArn(opts.Bucket, keys[i], be.NormalizeObjectKey), "a resource-based policy") continue } allDenied = false @@ -329,7 +329,7 @@ func objectsAccessErrors(ctx context.Context, be backend.Backend, opts AccessOpt principal := identity.PrincipalArn if principal == "" { - principal = opts.Acc.Access + principal = principalName(opts.Acc) } for i := range keys { @@ -380,6 +380,16 @@ func objectsAccessErrors(ctx context.Context, be backend.Backend, opts AccessOpt return errs, nil } +// principalName is how a denial message names acc: by its principal ARN +// where the IAM backend gives it one, and by its access key id otherwise — +// the only name the other backends have for it. +func principalName(acc Account) string { + if acc.Arn != "" { + return acc.Arn + } + return acc.Access +} + // decisionForResource is one resource's tri-state decision plus, for // Deny/NoMatch, the specific action responsible — so the caller can build an // AWS-shaped message naming it. @@ -413,7 +423,7 @@ func verifyResourceAccess(ctx context.Context, be backend.Backend, opts AccessOp } for i, object := range objects { - decision, action, err := verifyBucketPolicy(policy, opts.Acc.Access, opts.Bucket, object, condCtx, be.NormalizeObjectKey, opts.Actions...) + decision, action, err := verifyBucketPolicy(policy, opts.Acc, opts.Bucket, object, condCtx, be.NormalizeObjectKey, opts.Actions...) if err != nil { return nil, err } @@ -616,7 +626,7 @@ func verifyIdentityOnlyAccess(ctx fiber.Ctx, pe PolicyEvaluator, acc Account, ac principal := identity.PrincipalArn if principal == "" { - principal = acc.Access + principal = principalName(acc) } // A session policy narrows what the session may do; there is no resource diff --git a/auth/access-control_test.go b/auth/access-control_test.go index 41509449..b57b2883 100644 --- a/auth/access-control_test.go +++ b/auth/access-control_test.go @@ -918,3 +918,186 @@ func TestVerifyObjectsAccess_ResourceDenyNoPolicyEvaluator(t *testing.T) { } func strPtr(s string) *string { return &s } + +// arnPolicyBackend serves one bucket policy, for the ARN-principal tests +// below. It is publicBucketPolicyBackend without the ACL half, which none of +// them reach. +type arnPolicyBackend struct { + backend.BackendUnsupported + policy string +} + +func (b arnPolicyBackend) GetBucketPolicy(_ context.Context, _ string) ([]byte, error) { + return []byte(b.policy), nil +} + +// arnPolicy builds a one-statement bucket policy granting or denying +// s3:GetObject on the test bucket to principal. +func arnPolicy(effect, principal string) string { + return `{"Version":"2012-10-17","Statement":[{"Effect":"` + effect + `","Principal":{"AWS":` + + principal + `},"Action":"s3:GetObject","Resource":"arn:aws:s3:::bucket/*"}]}` +} + +const ( + acPolicyUserArn = `"arn:aws:iam::000000000000:user/alice"` + acPolicyRoleArn = `"arn:aws:iam::000000000000:role/reader"` + acPolicySessionArn = `"arn:aws:sts::000000000000:assumed-role/reader/sess1"` + acPolicyRootArn = `"arn:aws:iam::000000000000:root"` +) + +func acUser() Account { + return Account{Access: "AKIAALICE", Role: RoleUser, Arn: "arn:aws:iam::000000000000:user/alice"} +} + +func acSession() Account { + return Account{ + Access: "ASIASESSION", + Role: RoleUser, + IsSession: true, + Arn: "arn:aws:sts::000000000000:assumed-role/reader/sess1", + RoleArn: "arn:aws:iam::000000000000:role/reader", + } +} + +// TestVerifyAccess_ArnPrincipalMatching walks every combination of principal +// form and caller that a bucket policy can express under an IAM backend +// whose identities have ARNs, with the identity policy silent throughout so +// that what each case measures is the Principal element alone. +func TestVerifyAccess_ArnPrincipalMatching(t *testing.T) { + tests := []struct { + name string + effect string + principal string + acc Account + wantAllow bool + wantDenyBy string + }{ + { + name: "user named by its own arn", effect: "Allow", principal: acPolicyUserArn, + acc: acUser(), wantAllow: true, + }, + { + name: "user not named", effect: "Allow", principal: acPolicyRoleArn, + acc: acUser(), wantDenyBy: "because no identity-based policy allows", + }, + { + name: "access key id is no longer a principal", effect: "Allow", principal: `"AKIAALICE"`, + acc: acUser(), wantDenyBy: "because no identity-based policy allows", + }, + { + name: "session named by its role arn", effect: "Allow", principal: acPolicyRoleArn, + acc: acSession(), wantAllow: true, + }, + { + name: "session named by its own arn", effect: "Allow", principal: acPolicySessionArn, + acc: acSession(), wantAllow: true, + }, + { + name: "another session of the same role", effect: "Allow", + principal: `"arn:aws:sts::000000000000:assumed-role/reader/sess2"`, + acc: acSession(), wantDenyBy: "because no identity-based policy allows", + }, + { + name: "a user is not covered by a role arn", effect: "Allow", principal: acPolicyRoleArn, + acc: acUser(), wantDenyBy: "because no identity-based policy allows", + }, + { + // The account principal delegates to the account's own IAM + // rather than granting, and the identity policy is silent here. + name: "account root arn allows nothing on its own", effect: "Allow", principal: acPolicyRootArn, + acc: acUser(), wantDenyBy: "because no identity-based policy allows", + }, + { + name: "bare account id allows nothing on its own", effect: "Allow", principal: `"000000000000"`, + acc: acUser(), wantDenyBy: "because no identity-based policy allows", + }, + { + name: "wildcard allows everyone", effect: "Allow", principal: `"*"`, + acc: acUser(), wantAllow: true, + }, + { + name: "deny naming the user", effect: "Deny", principal: acPolicyUserArn, + acc: acUser(), wantDenyBy: "with an explicit deny in a resource-based policy", + }, + { + // Deny is not a delegation: naming the account denies every + // principal in it outright. + name: "deny naming the account", effect: "Deny", principal: acPolicyRootArn, + acc: acUser(), wantDenyBy: "with an explicit deny in a resource-based policy", + }, + { + name: "deny naming the account hits a session too", effect: "Deny", principal: acPolicyRootArn, + acc: acSession(), wantDenyBy: "with an explicit deny in a resource-based policy", + }, + { + name: "deny naming the role hits its session", effect: "Deny", principal: acPolicyRoleArn, + acc: acSession(), wantDenyBy: "with an explicit deny in a resource-based policy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + be := arnPolicyBackend{policy: arnPolicy(tt.effect, tt.principal)} + pe := newMockPolicyEvaluator(policyDecisionNoMatch) + pe.principalArn = tt.acc.Arn + + err := VerifyAccess(testFiberCtx(t), be, AccessOptions{ + Acc: tt.acc, + Bucket: "bucket", + Object: "key.txt", + Actions: []Action{GetObjectAction}, + Iam: pe, + }) + + if tt.wantAllow { + assert.NoError(t, err) + return + } + apiErr := requireAccessDeniedAPIError(t, err) + assert.Contains(t, apiErr.Description, tt.wantDenyBy) + assert.Contains(t, apiErr.Description, tt.acc.Arn, + "a denial names the caller by its ARN once the IAM backend gives it one") + }) + } +} + +// TestVerifyAccess_AccountPrincipalDelegatesToIdentityPolicy is the other +// half of the account-principal rule: what it delegates to is the identity +// policy, so the same policy that granted nothing above grants once the +// identity policy allows. +func TestVerifyAccess_AccountPrincipalDelegatesToIdentityPolicy(t *testing.T) { + be := arnPolicyBackend{policy: arnPolicy("Allow", acPolicyRootArn)} + pe := newMockPolicyEvaluator(policyDecisionAllow) + + err := VerifyAccess(testFiberCtx(t), be, AccessOptions{ + Acc: acUser(), + Bucket: "bucket", + Object: "key.txt", + Actions: []Action{GetObjectAction}, + Iam: pe, + }) + + assert.NoError(t, err) +} + +// TestVerifyAccess_AccessKeyPrincipalsStillWorkWithoutArns pins the +// backward-compatible half: an account with no ARN — every IAM backend but +// the standalone service — is still matched by its access key id, and an ARN +// principal means nothing to it. +func TestVerifyAccess_AccessKeyPrincipalsStillWorkWithoutArns(t *testing.T) { + acc := Account{Access: "testuser", Role: RoleUser} + + allowed := arnPolicyBackend{policy: arnPolicy("Allow", `"testuser"`)} + err := VerifyAccess(testFiberCtx(t), allowed, AccessOptions{ + Acc: acc, Bucket: "bucket", Object: "key.txt", + Actions: []Action{GetObjectAction}, Iam: NewIAMServiceSingle(Account{}), + }) + assert.NoError(t, err) + + denied := arnPolicyBackend{policy: arnPolicy("Allow", acPolicyUserArn)} + err = VerifyAccess(testFiberCtx(t), denied, AccessOptions{ + Acc: acc, Bucket: "bucket", Object: "key.txt", + Actions: []Action{GetObjectAction}, Iam: NewIAMServiceSingle(Account{}), + }) + assert.Equal(t, s3err.GetAPIError(s3err.ErrAccessDenied), err) +} diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go index 5f0d2041..cd0d0c3e 100644 --- a/auth/bucket_policy.go +++ b/auth/bucket_policy.go @@ -110,7 +110,7 @@ func (bp *BucketPolicy) Validate(bucket string, iam IAMService) error { return nil } -// decisionFor evaluates a single action against bp for principal/resource, +// decisionFor evaluates a single action against bp for acc/resource, // returning the tri-state policyDecision. A statement whose principal/action/resource // otherwise matches but whose Condition block can't be evaluated // denies the whole decision immediately, regardless of that statement's own @@ -121,10 +121,10 @@ func (bp *BucketPolicy) Validate(bucket string, iam IAMService) error { // Condition write-time validation existed — it only guards a document // stored before that validation existed, or naming a future operator the // gateway doesn't yet recognize. -func (bp *BucketPolicy) decisionFor(principal string, action Action, resource string, condCtx map[string][]string, normalizeObjectKey objectKeyNormalizer) policyDecision { +func (bp *BucketPolicy) decisionFor(acc Account, action Action, resource string, condCtx map[string][]string, normalizeObjectKey objectKeyNormalizer) policyDecision { var isAllowed bool for _, statement := range bp.Statement { - matched, evaluable := statement.findMatch(principal, action, resource, condCtx, bp.Version, normalizeObjectKey) + matched, evaluable := statement.findMatch(acc, action, resource, condCtx, bp.Version, normalizeObjectKey) if !evaluable { return policyDecisionDeny } @@ -236,13 +236,38 @@ func (bpi *BucketPolicyItem) Validate(bucket string, iam IAMService) error { // this request, and — only when they do — whether its Condition block holds // against condCtx. matched is only meaningful when evaluable is true; see // condition.Evaluate and decisionFor's fail-closed handling of evaluable =false. -func (bpi *BucketPolicyItem) findMatch(principal string, action Action, resource string, condCtx map[string][]string, version PolicyVersion, normalizeObjectKey objectKeyNormalizer) (matched bool, evaluable bool) { - if !(bpi.Principals.Contains(principal) && bpi.Actions.FindMatch(action) && bpi.Resources.FindMatch(resource, normalizeObjectKey)) { +func (bpi *BucketPolicyItem) findMatch(acc Account, action Action, resource string, condCtx map[string][]string, version PolicyVersion, normalizeObjectKey objectKeyNormalizer) (matched bool, evaluable bool) { + if !(bpi.matchesPrincipal(acc) && bpi.Actions.FindMatch(action) && bpi.Resources.FindMatch(resource, normalizeObjectKey)) { return false, true } return condition.Evaluate(bpi.Condition, condCtx, string(version)) } +// matchesPrincipal reports whether this statement's Principal element +// covers acc, given what this statement's Effect makes of an account-level +// match. +// +// A statement naming the account — its root ARN, or the bare account id — +// only delegates to the account: it says the account's own IAM may grant +// this, not that this is granted. So it allows nothing by itself, and a +// caller under it is authorized only if an identity policy independently +// allows the request, which VerifyAccess already covers by treating either +// source's Allow as sufficient. Skipping the statement here is what makes +// the account-level Allow contribute nothing. +// +// Deny is not symmetric with that: a statement denying the account denies +// every principal in it outright, delegating nothing. +func (bpi *BucketPolicyItem) matchesPrincipal(acc Account) bool { + switch bpi.Principals.matchFor(acc) { + case principalDirectMatch: + return true + case principalAccountMatch: + return bpi.Effect == BucketPolicyAccessTypeDeny + default: + return false + } +} + // isPublicFor checks if the bucket policy statement grants public access // for given resource and action, and — only when it otherwise matches — // whether its Condition block holds against condCtx. A public statement's @@ -297,6 +322,13 @@ func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) err } if err := policy.Validate(bucket, iam); err != nil { + var lookupErr principalLookupError + if errors.As(err, &lookupErr) { + // Not a defect in the document: the IAM service could not be + // asked whether its principals exist. Report that as itself + // rather than telling the caller their policy is malformed. + return lookupErr.err + } return getMalformedPolicyError(err) } @@ -310,7 +342,7 @@ func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) err // Allow only if every action has a matching Allow; otherwise NoMatch, // paired with the first action that lacked one. Zero actions is // conservatively NoMatch, not vacuously Allow. -func verifyBucketPolicy(policyBytes []byte, access, bucket, object string, condCtx map[string][]string, normalizeObjectKey objectKeyNormalizer, actions ...Action) (policyDecision, Action, error) { +func verifyBucketPolicy(policyBytes []byte, acc Account, bucket, object string, condCtx map[string][]string, normalizeObjectKey objectKeyNormalizer, actions ...Action) (policyDecision, Action, error) { if len(actions) == 0 { return policyDecisionNoMatch, "", nil } @@ -325,7 +357,7 @@ func verifyBucketPolicy(policyBytes []byte, access, bucket, object string, condC result := policyDecisionAllow var blamed Action for _, action := range actions { - switch d := bp.decisionFor(access, action, resource, condCtx, normalizeObjectKey); d { + switch d := bp.decisionFor(acc, action, resource, condCtx, normalizeObjectKey); d { case policyDecisionDeny: return policyDecisionDeny, action, nil case policyDecisionNoMatch: diff --git a/auth/bucket_policy_principals.go b/auth/bucket_policy_principals.go index a4ba83f9..2930d85a 100644 --- a/auth/bucket_policy_principals.go +++ b/auth/bucket_policy_principals.go @@ -16,10 +16,29 @@ package auth import ( "encoding/json" + "strings" ) type Principals map[string]struct{} +// principalMatch is how strongly a Principal element matched a caller. The +// three cases are not interchangeable: a statement naming the account +// (principalAccountMatch) delegates rather than grants, so it means different things +// under Allow and under Deny +type principalMatch int + +const ( + // principalNoMatch means the statement does not cover this caller. + principalNoMatch principalMatch = iota + // principalAccountMatch means the statement names the caller's account + // — its root ARN, or the bare account id — and so covers the caller + // only in the delegating sense. + principalAccountMatch + // principalDirectMatch means the statement names the caller itself, the + // role it assumed, or every principal ("*"). + principalDirectMatch +) + func (p Principals) Add(key string) { p[key] = struct{}{} } @@ -90,7 +109,13 @@ func (p Principals) ToSlice() []string { return principals } -// Validates Principals by checking user account access keys existence +// Validate checks that every principal named here exists, so a policy can +// never be stored naming somebody who cannot be matched. What a principal +// *is* depends on the IAM backend: an AWS-style ARN for a backend that +// implements PrincipalResolver, and an access key id for every other one. +// +// The wildcard is checked the same way either way: "*" is only valid alone, +// never mixed with named principals. func (p Principals) Validate(iam IAMService) error { _, containsWildCard := p["*"] if containsWildCard { @@ -100,6 +125,17 @@ func (p Principals) Validate(iam IAMService) error { return policyErrInvalidPrincipal } + if pr, ok := iam.(PrincipalResolver); ok { + invalid, err := pr.ResolvePrincipals(p.ToSlice()) + if err != nil { + return principalLookupError{err} + } + if len(invalid) > 0 { + return policyErrInvalidPrincipal + } + return nil + } + accs, err := iam.ResolveAccounts(p.ToSlice()) if err != nil { return err @@ -111,15 +147,74 @@ func (p Principals) Validate(iam IAMService) error { return nil } -func (p Principals) Contains(userAccess string) bool { - // "*" means it matches for any user account - _, ok := p["*"] - if ok { - return true +// principalLookupError reports that the IAM service could not be asked +// whether a policy's principals exist, as distinct from its answering that +// one of them does not. Validating an ARN principal is a network call, so +// this is the difference between telling an operator their IAM service is +// unreachable and telling a user their policy is malformed +type principalLookupError struct{ err error } + +func (e principalLookupError) Error() string { return e.err.Error() } +func (e principalLookupError) Unwrap() error { return e.err } + +// matchFor reports how this Principal element covers acc. +// +// An account whose IAM backend gives it an ARN (acc.Arn set, i.e. the +// standalone IAM service) is matched by ARN, the way real S3 does it: +// +// - "*" matches everyone, authenticated or not. +// - The caller's own ARN matches it and nothing else. For an assumed-role +// session that is arn:aws:sts::…:assumed-role//, so a +// statement naming one session does not cover another session of the +// same role. +// - A session's role ARN matches every session of that role, which is the +// only way to name them all: no wildcard is allowed inside an ARN. +// - The account's root ARN, and the bare account id, match as a +// delegation rather than as a grant. The gateway's own root account is +// the exception, and not a special case: the account root ARN is its +// own ARN, so it matches root directly and the account only by +// delegation, which is what the same string means for each of them. +// +// Everything else is left to the IAM backend to have rejected at +// PutBucketPolicy time; matching is a plain string comparison, and +// deliberately so — no case folding, no whitespace trimming, no wildcards +// within an ARN. +// +// An account with no ARN — every other IAM backend — is matched by access +// key id exactly as it always has been. +func (p Principals) matchFor(acc Account) principalMatch { + if _, ok := p["*"]; ok { + return principalDirectMatch } - _, found := p[userAccess] - return found + if acc.Arn == "" { + if _, found := p[acc.Access]; found { + return principalDirectMatch + } + return principalNoMatch + } + + if _, found := p[acc.Arn]; found { + return principalDirectMatch + } + if acc.RoleArn != "" { + if _, found := p[acc.RoleArn]; found { + return principalDirectMatch + } + } + + accountID := accountIDFromArn(acc.Arn) + if accountID == "" { + return principalNoMatch + } + if _, found := p[accountID]; found { + return principalAccountMatch + } + if _, found := p[accountRootArn(accountID)]; found { + return principalAccountMatch + } + + return principalNoMatch } // Bucket policy grants public access, if it contains @@ -128,3 +223,22 @@ func (p Principals) isPublic() bool { _, ok := p["*"] return ok } + +// accountIDFromArn returns the account id field of arn, or "" if arn isn't +// shaped like one. It reads the caller's own ARN, which the IAM service +// built, so it needs to recognize no more than the two shapes that service +// produces: arn:::::. +func accountIDFromArn(arn string) string { + const fields = 6 + parts := strings.SplitN(arn, ":", fields) + if len(parts) != fields || parts[0] != "arn" { + return "" + } + return parts[4] +} + +// accountRootArn builds the ARN naming an account itself, the principal +// form that delegates to it. +func accountRootArn(accountID string) string { + return "arn:aws:iam::" + accountID + ":root" +} diff --git a/auth/bucket_policy_principals_test.go b/auth/bucket_policy_principals_test.go index 766b7628..08715d54 100644 --- a/auth/bucket_policy_principals_test.go +++ b/auth/bucket_policy_principals_test.go @@ -91,16 +91,200 @@ func TestPrincipals_Validate(t *testing.T) { } } -func TestPrincipals_Contains(t *testing.T) { - p := Principals{"user1": {}} - assert.True(t, p.Contains("user1")) - assert.False(t, p.Contains("user2")) +// mockPrincipalResolver is an IAMService that names its identities by ARN, +// standing in for the standalone IAM service client: it reports every +// principal not in valid as unresolvable, the way ResolvePrincipals does. +type mockPrincipalResolver struct { + IAMService + valid map[string]bool + err error + calls [][]string +} - p = Principals{"*": {}} - assert.True(t, p.Contains("anyuser")) +func (m *mockPrincipalResolver) ResolvePrincipals(principals []string) ([]string, error) { + m.calls = append(m.calls, principals) + if m.err != nil { + return nil, m.err + } + invalid := []string{} + for _, p := range principals { + if !m.valid[p] { + invalid = append(invalid, p) + } + } + return invalid, nil +} + +func newMockPrincipalResolver(valid ...string) *mockPrincipalResolver { + set := make(map[string]bool, len(valid)) + for _, v := range valid { + set[v] = true + } + return &mockPrincipalResolver{IAMService: NewIAMServiceSingle(Account{}), valid: set} +} + +const ( + testUserArn = "arn:aws:iam::000000000000:user/alice" + testRoleArn = "arn:aws:iam::000000000000:role/reader" + testSessionArn = "arn:aws:sts::000000000000:assumed-role/reader/sess1" + testRootArn = "arn:aws:iam::000000000000:root" +) + +// TestPrincipals_ValidateWithPrincipalResolver covers the write-time check +// for an ARN-naming backend: the resolver decides, ResolveAccounts is never +// consulted, and the wildcard rules are unchanged. +func TestPrincipals_ValidateWithPrincipalResolver(t *testing.T) { + tests := []struct { + name string + principals Principals + valid []string + err error + }{ + {"resolvable user arn", Principals{testUserArn: {}}, []string{testUserArn}, nil}, + {"unresolvable arn", Principals{testUserArn: {}}, nil, policyErrInvalidPrincipal}, + {"access key id is not a principal", Principals{"AKIAEXAMPLE": {}}, []string{testUserArn}, policyErrInvalidPrincipal}, + {"one of several unresolvable", Principals{testUserArn: {}, testRoleArn: {}}, []string{testUserArn}, policyErrInvalidPrincipal}, + {"all resolvable", Principals{testUserArn: {}, testRoleArn: {}}, []string{testUserArn, testRoleArn}, nil}, + {"only wildcard", Principals{"*": {}}, nil, nil}, + {"wildcard mixed with an arn", Principals{"*": {}, testUserArn: {}}, []string{testUserArn}, policyErrInvalidPrincipal}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resolver := newMockPrincipalResolver(tt.valid...) + assert.EqualValues(t, tt.err, tt.principals.Validate(resolver)) + }) + } +} + +// TestPrincipals_ValidateResolverErrorPropagates pins that a resolver +// failure surfaces as itself rather than as an invalid-principal verdict: +// an unreachable IAM service must not read as "this principal does not +// exist". +func TestPrincipals_ValidateResolverErrorPropagates(t *testing.T) { + resolver := newMockPrincipalResolver() + resolver.err = assert.AnError + + err := Principals{testUserArn: {}}.Validate(resolver) + assert.ErrorIs(t, err, assert.AnError) +} + +// TestPrincipals_ValidateWildcardSkipsResolver pins that a lone "*" costs no +// round trip - it names everyone, so there is nothing to resolve. +func TestPrincipals_ValidateWildcardSkipsResolver(t *testing.T) { + resolver := newMockPrincipalResolver() + + assert.NoError(t, Principals{"*": {}}.Validate(resolver)) + assert.Empty(t, resolver.calls) +} + +func TestPrincipals_matchForAccessKey(t *testing.T) { + user := Account{Access: "user1"} + + assert.Equal(t, principalDirectMatch, Principals{"user1": {}}.matchFor(user)) + assert.Equal(t, principalNoMatch, Principals{"user2": {}}.matchFor(user)) + assert.Equal(t, principalDirectMatch, Principals{"*": {}}.matchFor(user)) + // An ARN means nothing to a backend whose identities are access keys. + assert.Equal(t, principalNoMatch, Principals{testUserArn: {}}.matchFor(user)) +} + +func TestPrincipals_matchForArn(t *testing.T) { + user := Account{Access: "AKIAEXAMPLE", Arn: testUserArn} + session := Account{Access: "ASIAEXAMPLE", Arn: testSessionArn, RoleArn: testRoleArn, IsSession: true} + root := Account{Access: "root", Arn: testRootArn} + + tests := []struct { + name string + principals Principals + acc Account + want principalMatch + }{ + {"user named by its own arn", Principals{testUserArn: {}}, user, principalDirectMatch}, + {"user not named", Principals{testRoleArn: {}}, user, principalNoMatch}, + {"wildcard", Principals{"*": {}}, user, principalDirectMatch}, + {"access key id no longer matches", Principals{"AKIAEXAMPLE": {}}, user, principalNoMatch}, + {"account id delegates", Principals{"000000000000": {}}, user, principalAccountMatch}, + {"account root arn delegates", Principals{testRootArn: {}}, user, principalAccountMatch}, + {"another account does not match", Principals{"arn:aws:iam::111111111111:root": {}}, user, principalNoMatch}, + {"session named by its role arn", Principals{testRoleArn: {}}, session, principalDirectMatch}, + {"session named by its own arn", Principals{testSessionArn: {}}, session, principalDirectMatch}, + {"another session of the same role", Principals{"arn:aws:sts::000000000000:assumed-role/reader/sess2": {}}, session, principalNoMatch}, + {"session under account delegation", Principals{testRootArn: {}}, session, principalAccountMatch}, + {"root matches the account root arn directly", Principals{testRootArn: {}}, root, principalDirectMatch}, + {"matching is case sensitive", Principals{"arn:aws:iam::000000000000:user/ALICE": {}}, user, principalNoMatch}, + {"no wildcard within an arn", Principals{"arn:aws:iam::000000000000:user/*": {}}, user, principalNoMatch}, + {"no whitespace trimming", Principals{" " + testUserArn: {}}, user, principalNoMatch}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.principals.matchFor(tt.acc)) + }) + } +} + +// TestMatchesPrincipal_AccountDelegation pins the asymmetry between Allow +// and Deny for an account-level principal: it delegates to the account's own +// IAM under Allow, granting nothing by itself, but denies the account's +// principals outright under Deny. +func TestMatchesPrincipal_AccountDelegation(t *testing.T) { + user := Account{Access: "AKIAEXAMPLE", Arn: testUserArn} + + allow := &BucketPolicyItem{Effect: BucketPolicyAccessTypeAllow, Principals: Principals{testRootArn: {}}} + deny := &BucketPolicyItem{Effect: BucketPolicyAccessTypeDeny, Principals: Principals{testRootArn: {}}} + + assert.False(t, allow.matchesPrincipal(user)) + assert.True(t, deny.matchesPrincipal(user)) + + // A statement naming the user itself grants under either effect. + allow.Principals = Principals{testUserArn: {}} + deny.Principals = Principals{testUserArn: {}} + assert.True(t, allow.matchesPrincipal(user)) + assert.True(t, deny.matchesPrincipal(user)) +} + +func TestAccountIDFromArn(t *testing.T) { + tests := []struct { + arn string + want string + }{ + {testUserArn, "000000000000"}, + {testSessionArn, "000000000000"}, + {testRootArn, "000000000000"}, + {"arn:aws:iam::123456789012:role/some/path/name", "123456789012"}, + {"", ""}, + {"not-an-arn", ""}, + {"arn:aws:iam::000000000000", ""}, + {"xrn:aws:iam::000000000000:root", ""}, + } + for _, tt := range tests { + t.Run(tt.arn, func(t *testing.T) { + assert.Equal(t, tt.want, accountIDFromArn(tt.arn)) + }) + } } func TestPrincipals_isPublic(t *testing.T) { assert.True(t, Principals{"*": {}}.isPublic()) assert.False(t, Principals{"user1": {}}.isPublic()) } + +// TestValidatePolicyDocument_ResolverFailureIsNotMalformed pins that a +// policy whose principals could not be checked — because the IAM service +// failed, not because the document is wrong — is not reported as a +// malformed policy. Principal validation is a network call for an +// ARN-naming backend, so this is the difference between telling an operator +// their IAM service is down and telling a user their policy is invalid. +func TestValidatePolicyDocument_ResolverFailureIsNotMalformed(t *testing.T) { + resolver := newMockPrincipalResolver() + resolver.err = assert.AnError + + doc := []byte(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":"` + + testUserArn + `"},"Action":"s3:GetObject","Resource":"arn:aws:s3:::bucket/*"}]}`) + + err := ValidatePolicyDocument(doc, "bucket", resolver) + assert.ErrorIs(t, err, assert.AnError) + + // A genuine document defect still reports as MalformedPolicy. + resolver.err = nil + err = ValidatePolicyDocument(doc, "bucket", resolver) + assert.Equal(t, getMalformedPolicyError(policyErrInvalidPrincipal), err) +} diff --git a/auth/bucket_policy_test.go b/auth/bucket_policy_test.go index 28f9cd8e..65aa986f 100644 --- a/auth/bucket_policy_test.go +++ b/auth/bucket_policy_test.go @@ -98,7 +98,7 @@ func TestBucketPolicyDecision_Condition(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - decision, _, err := verifyBucketPolicy([]byte(tt.policy), "someaccess", "mybucket", tt.object, tt.condCtx, nil, tt.action) + decision, _, err := verifyBucketPolicy([]byte(tt.policy), Account{Access: "someaccess"}, "mybucket", tt.object, tt.condCtx, nil, tt.action) assert.NoError(t, err) assert.Equal(t, tt.want, decision) }) @@ -115,7 +115,7 @@ func TestBucketPolicyDecision_UnevaluableConditionFailsClosed(t *testing.T) { "Action":"s3:GetObject","Resource":"arn:aws:s3:::mybucket/*", "Condition":{"SomeFutureOperator":{"aws:UserAgent":"good-agent"}}}]}` - decision, _, err := verifyBucketPolicy([]byte(policy), "someaccess", "mybucket", "key", nil, nil, GetObjectAction) + decision, _, err := verifyBucketPolicy([]byte(policy), Account{Access: "someaccess"}, "mybucket", "key", nil, nil, GetObjectAction) assert.NoError(t, err) assert.Equal(t, policyDecisionDeny, decision) } diff --git a/auth/fixed_bucket_owner.go b/auth/fixed_bucket_owner.go index 2ddebe67..794736c1 100644 --- a/auth/fixed_bucket_owner.go +++ b/auth/fixed_bucket_owner.go @@ -47,6 +47,12 @@ func ResolveFixedBucketOwner(iam IAMService) (Account, bool) { // object writes then land with the same ownership as the buckets root owns. // Backends that do not fix ownership resolve a real per-account uid/gid for // every other account and keep root exactly as it was. +// +// The same backend is also the one that knows root's principal ARN, which +// the S3 request path likewise cannot derive: root is the only identity +// resolved locally rather than through the IAM service, so without this it +// would reach bucket-policy matching unnamed and a statement naming the +// account root ARN would miss it. func rootIdentity(iam IAMService, root Account) Account { owner, fixed := ResolveFixedBucketOwner(iam) if !fixed || owner.Access != root.Access { @@ -56,5 +62,6 @@ func rootIdentity(iam IAMService, root Account) Account { root.UserID = owner.UserID root.GroupID = owner.GroupID root.ProjectID = owner.ProjectID + root.Arn = owner.Arn return root } diff --git a/auth/iam.go b/auth/iam.go index 20ce62a8..210308ae 100644 --- a/auth/iam.go +++ b/auth/iam.go @@ -84,6 +84,21 @@ type Account struct { // IAM backend nor echoed by the admin API. SessionToken string `json:"-"` IsSession bool `json:"-"` + + // Arn and RoleArn name this account the way a bucket policy's Principal + // element does, and are set only by IAM backends whose identities have + // ARNs at all — currently just the standalone IAM service client. Arn is + // the caller's own ARN (an IAM user's, or a session's + // arn:aws:sts::…:assumed-role//); RoleArn is the ARN of + // the role a session assumed, and is empty for everything else. + // + // Both are needed to match a session, because a Principal naming a role + // matches every session of that role while one naming a session matches + // only that session. When Arn is empty the gateway matches principals by + // access key id instead, which is what every other backend has always + // done — see Principals.matchFor. + Arn string `json:"-"` + RoleArn string `json:"-"` } // String elides the two credential-bearing fields so an Account can't leak @@ -91,8 +106,8 @@ type Account struct { // X-Amz-Security-Token *header*, which does nothing for a struct printed // after the token has been parsed out of it. func (a Account) String() string { - return fmt.Sprintf("Account{Access:%s, Secret:REDACTED, Role:%s, UserID:%d, GroupID:%d, ProjectID:%d, SessionToken:REDACTED, IsSession:%t}", - a.Access, a.Role, a.UserID, a.GroupID, a.ProjectID, a.IsSession) + return fmt.Sprintf("Account{Access:%s, Secret:REDACTED, Role:%s, UserID:%d, GroupID:%d, ProjectID:%d, SessionToken:REDACTED, IsSession:%t, Arn:%s, RoleArn:%s}", + a.Access, a.Role, a.UserID, a.GroupID, a.ProjectID, a.IsSession, a.Arn, a.RoleArn) } type ListUserAccountsResult struct { diff --git a/auth/iam_standalone.go b/auth/iam_standalone.go index cb2cee3c..a2a8fc6c 100644 --- a/auth/iam_standalone.go +++ b/auth/iam_standalone.go @@ -122,6 +122,12 @@ type IAMServiceStandalone struct { secret string rootAcc Account cfg IAMServiceStandaloneConfig + // accountID is the AWS account id the IAM service reported at startup, + // the one every ARN it mints belongs to. It is only used to name the + // gateway's own root account, which the service has no record of and so + // cannot name itself. Written once by probeProtocol before this client + // serves anything, and read-only afterwards. + accountID string } var ( @@ -129,6 +135,7 @@ var ( _ SigningKeyProvider = (*IAMServiceStandalone)(nil) _ PolicyEvaluator = (*IAMServiceStandalone)(nil) _ FixedBucketOwner = (*IAMServiceStandalone)(nil) + _ PrincipalResolver = (*IAMServiceStandalone)(nil) ) // NewIAMServiceStandalone constructs the standalone IAM service client. @@ -201,6 +208,7 @@ func (s *IAMServiceStandalone) probeProtocol() error { } if err == nil { + s.accountID = resp.AccountID serverVersion := resp.ServerVersion if serverVersion == "" { serverVersion = "unknown" @@ -437,11 +445,16 @@ func (s *IAMServiceStandalone) DeriveSigningKey(access, sessionToken, date, regi return nil, Account{}, err } - return resp.DerivedKey, s.accountFor(access, sessionToken), nil + acc := s.accountFor(access, sessionToken) + acc.Arn = resp.PrincipalArn + acc.RoleArn = resp.RoleArn + + return resp.DerivedKey, acc, nil } // accountFor builds the Account metadata DeriveSigningKey/GetUserAccount -// return for a resolved non-root identity +// return for a resolved non-root identity. Arn/RoleArn are left to the +// caller: only the endpoints that resolved the identity know them. func (s *IAMServiceStandalone) accountFor(access, sessionToken string) Account { return Account{ Access: access, @@ -454,6 +467,25 @@ func (s *IAMServiceStandalone) accountFor(access, sessionToken string) Account { } } +// ResolvePrincipals implements PrincipalResolver, validating a bucket +// policy's principals in a single round trip regardless of how many it +// names. +func (s *IAMServiceStandalone) ResolvePrincipals(principals []string) ([]string, error) { + if len(principals) == 0 { + return nil, nil + } + + var resp private.ResolvePrincipalsResponse + err := s.doPrivateRequest(private.ResolvePrincipalsPath, private.ResolvePrincipalsRequest{ + Principals: principals, + }, &resp) + if err != nil { + return nil, fmt.Errorf("resolve policy principals: %w", err) + } + + return resp.Invalid, nil +} + // EvaluatePolicy implements PolicyEvaluator, evaluating every action in // actions against resource in a single request rather than one round trip // per action. @@ -622,13 +654,15 @@ func (s *IAMServiceStandalone) resolveAccountDetails(accesses []string) ([]resol if !identity.Found { continue } + acc := s.accountFor(remote[i], "") + acc.Arn = identity.PrincipalArn out[remoteIdx[i]] = resolvedAccount{ Found: true, IsSession: identity.Kind == private.KindSession, // No session token is known here, and none is needed: this // Account answers "who is this" for validation, never // authenticates a request. - Account: s.accountFor(remote[i], ""), + Account: acc, } } return out, nil @@ -665,6 +699,9 @@ func (s *IAMServiceStandalone) rootAccount() Account { acc.UserID = s.cfg.DefaultUserID acc.GroupID = s.cfg.DefaultGroupID acc.ProjectID = s.cfg.DefaultProjectID + if s.accountID != "" { + acc.Arn = accountRootArn(s.accountID) + } return acc } diff --git a/auth/iam_standalone_test.go b/auth/iam_standalone_test.go index 8bdeec60..42b2c74a 100644 --- a/auth/iam_standalone_test.go +++ b/auth/iam_standalone_test.go @@ -107,7 +107,15 @@ func createStandaloneTestUser(t *testing.T, store storage.Storer, userName, acce t.Helper() ctx := context.Background() - if _, err := store.CreateUser(ctx, types.User{UserName: userName, Path: "/", CreateDate: time.Now().UTC()}); err != nil { + // Arn is set explicitly, as the control-plane controller does before + // calling storage.CreateUser: it is what a bucket policy names the user + // by, and storage.CreateUser never populates it. + if _, err := store.CreateUser(ctx, types.User{ + UserName: userName, + Path: "/", + Arn: standaloneUserArn(userName), + CreateDate: time.Now().UTC(), + }); err != nil { t.Fatalf("CreateUser: %v", err) } if _, err := store.CreateAccessKey(ctx, storage.CreateAccessKeyInput{ @@ -689,3 +697,262 @@ func TestIAMServiceStandaloneRootCarriesPosixIdentity(t *testing.T) { t.Errorf("root identity lost its credentials or role: %+v", acc) } } + +// standaloneTestAccountID is the account every ARN the IAM service mints +// belongs to, and the one it reports on the version endpoint. +const standaloneTestAccountID = "000000000000" + +func standaloneUserArn(userName string) string { + return "arn:aws:iam::" + standaloneTestAccountID + ":user/" + userName +} + +func standaloneRoleArn(roleName string) string { + return "arn:aws:iam::" + standaloneTestAccountID + ":role/" + roleName +} + +// createStandaloneTestSession creates a role and a live session of it, +// returning the session's credentials — the shape AssumeRoleWithWebIdentity +// produces, built directly against the store the way +// createStandaloneTestUser does. +func createStandaloneTestSession(t *testing.T, store storage.Storer, roleName, accessKeyID, secret, token string) *types.Session { + t.Helper() + ctx := context.Background() + + role, err := store.CreateRole(ctx, types.Role{ + RoleName: roleName, + Path: "/", + RoleID: "AROA" + roleName, + Arn: standaloneRoleArn(roleName), + CreateDate: time.Now().UTC(), + }) + if err != nil { + t.Fatalf("CreateRole: %v", err) + } + + session, err := store.CreateSession(ctx, types.Session{ + AccessKeyId: accessKeyID, + SecretAccessKey: secret, + SessionToken: token, + RoleArn: role.Arn, + RoleName: role.RoleName, + RoleID: role.RoleID, + RoleSessionName: "sess1", + CreateDate: time.Now().UTC(), + Expiration: time.Now().UTC().Add(time.Hour), + }) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + return session +} + +func newStandaloneTestClient(t *testing.T, sock string) *IAMServiceStandalone { + t.Helper() + + client, err := NewIAMServiceStandalone( + Account{Access: standaloneTestRootAccess, Secret: standaloneTestRootSecret, Role: RoleAdmin}, + IAMServiceStandaloneConfig{Endpoint: sock}) + if err != nil { + t.Fatalf("NewIAMServiceStandalone: %v", err) + } + t.Cleanup(func() { client.Shutdown() }) + return client +} + +// TestIAMServiceStandaloneAuthenticatedAccountsCarryArns covers what a +// bucket policy's Principal element is matched against: a user is named by +// its own ARN, and a session by both its assumed-role ARN and the ARN of the +// role it assumed, since a policy naming either one matches it. +func TestIAMServiceStandaloneAuthenticatedAccountsCarryArns(t *testing.T) { + store, sock := standaloneTestServer(t) + createStandaloneTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + session := createStandaloneTestSession(t, store, "reader", "ASIASESSION", "sesssecret", "tok") + + client := newStandaloneTestClient(t, sock) + yyyymmdd := time.Now().UTC().Format(sigv4auth.YYYYMMDD) + + _, user, err := client.DeriveSigningKey("AKIAALICE", "", yyyymmdd, "us-east-1", "s3") + if err != nil { + t.Fatalf("DeriveSigningKey(user): %v", err) + } + if want := standaloneUserArn("alice"); user.Arn != want { + t.Errorf("user Arn = %q, want %q", user.Arn, want) + } + if user.RoleArn != "" { + t.Errorf("user RoleArn = %q, want it empty: a user assumed no role", user.RoleArn) + } + + _, sess, err := client.DeriveSigningKey(session.AccessKeyId, "tok", yyyymmdd, "us-east-1", "s3") + if err != nil { + t.Fatalf("DeriveSigningKey(session): %v", err) + } + wantSessionArn := "arn:aws:sts::" + standaloneTestAccountID + ":assumed-role/reader/sess1" + if sess.Arn != wantSessionArn { + t.Errorf("session Arn = %q, want %q", sess.Arn, wantSessionArn) + } + if want := standaloneRoleArn("reader"); sess.RoleArn != want { + t.Errorf("session RoleArn = %q, want %q", sess.RoleArn, want) + } +} + +// TestIAMServiceStandaloneRootCarriesAccountArn pins that the gateway's own +// root account is named by the account root ARN. The IAM service holds no +// record of root, so the account id comes from the startup probe. +func TestIAMServiceStandaloneRootCarriesAccountArn(t *testing.T) { + _, sock := standaloneTestServer(t) + client := newStandaloneTestClient(t, sock) + + want := "arn:aws:iam::" + standaloneTestAccountID + ":root" + + owner, fixed := ResolveFixedBucketOwner(client) + if !fixed { + t.Fatal("ResolveFixedBucketOwner: standalone client must fix bucket ownership") + } + if owner.Arn != want { + t.Errorf("BucketOwner().Arn = %q, want %q", owner.Arn, want) + } + + acc, err := client.GetUserAccount(standaloneTestRootAccess) + if err != nil { + t.Fatalf("GetUserAccount(root): %v", err) + } + if acc.Arn != want { + t.Errorf("GetUserAccount(root).Arn = %q, want %q", acc.Arn, want) + } +} + +// TestIAMServiceStandaloneResolvePrincipals covers the write-time principal +// check PutBucketPolicy makes, end to end against a real IAM service: every +// form a bucket policy may name resolves, and an access key id — what this +// gateway's other IAM backends name principals by — does not. +func TestIAMServiceStandaloneResolvePrincipals(t *testing.T) { + store, sock := standaloneTestServer(t) + createStandaloneTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + createStandaloneTestSession(t, store, "reader", "ASIASESSION", "sesssecret", "tok") + + client := newStandaloneTestClient(t, sock) + + valid := []string{ + standaloneTestAccountID, + "arn:aws:iam::" + standaloneTestAccountID + ":root", + standaloneUserArn("alice"), + standaloneRoleArn("reader"), + "arn:aws:sts::" + standaloneTestAccountID + ":assumed-role/reader/sess1", + "arn:aws:sts::" + standaloneTestAccountID + ":assumed-role/reader/never-assumed", + } + invalid := []string{ + "AKIAALICE", + "ASIASESSION", + "alice", + standaloneUserArn("bob"), + standaloneRoleArn("writer"), + standaloneUserArn("*"), + "arn:aws:iam::111111111111:root", + } + + got, err := client.ResolvePrincipals(append(append([]string{}, valid...), invalid...)) + if err != nil { + t.Fatalf("ResolvePrincipals: %v", err) + } + + reported := map[string]bool{} + for _, p := range got { + reported[p] = true + } + for _, p := range valid { + if reported[p] { + t.Errorf("principal %q reported invalid, want it to resolve", p) + } + } + for _, p := range invalid { + if !reported[p] { + t.Errorf("principal %q reported valid, want it rejected", p) + } + } +} + +// TestIAMServiceStandaloneResolvePrincipalsEmpty pins that validating a +// policy whose only principal is the wildcard — which never reaches here — +// costs no round trip. +func TestIAMServiceStandaloneResolvePrincipalsEmpty(t *testing.T) { + _, sock := standaloneTestServer(t) + client := newStandaloneTestClient(t, sock) + + invalid, err := client.ResolvePrincipals(nil) + if err != nil { + t.Fatalf("ResolvePrincipals(nil): %v", err) + } + if len(invalid) != 0 { + t.Errorf("ResolvePrincipals(nil) = %v, want none", invalid) + } +} + +// TestIAMServiceStandalonePrincipalsValidate ties the two halves together: +// a bucket policy validated against the standalone client names principals +// by ARN, and the same document naming an access key id is rejected. +func TestIAMServiceStandalonePrincipalsValidate(t *testing.T) { + store, sock := standaloneTestServer(t) + createStandaloneTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + + client := newStandaloneTestClient(t, sock) + + if err := (Principals{standaloneUserArn("alice"): {}}).Validate(client); err != nil { + t.Errorf("Validate(user arn) = %v, want it accepted", err) + } + if err := (Principals{"AKIAALICE": {}}).Validate(client); err != policyErrInvalidPrincipal { + t.Errorf("Validate(access key id) = %v, want %v", err, policyErrInvalidPrincipal) + } + if err := (Principals{"*": {}}).Validate(client); err != nil { + t.Errorf("Validate(wildcard) = %v, want it accepted", err) + } +} + +// TestIAMServiceStandaloneRootIdentityCarriesArn pins that root reaches the +// S3 request path named. Root is the one identity resolved locally rather +// than through the IAM service, so ResolveDerivedKey never sees an ARN for +// it — rootIdentity has to carry the one the backend defines, or a bucket +// policy naming the account root ARN would miss root entirely. +func TestIAMServiceStandaloneRootIdentityCarriesArn(t *testing.T) { + _, sock := standaloneTestServer(t) + client := newStandaloneTestClient(t, sock) + + rootAcc := Account{Access: standaloneTestRootAccess, Secret: standaloneTestRootSecret, Role: RoleAdmin} + yyyymmdd := time.Now().UTC().Format(sigv4auth.YYYYMMDD) + + _, acc, err := ResolveDerivedKey(client, rootAcc, standaloneTestRootAccess, "", yyyymmdd, "us-east-1", "s3") + if err != nil { + t.Fatalf("ResolveDerivedKey(root): %v", err) + } + + want := "arn:aws:iam::" + standaloneTestAccountID + ":root" + if acc.Arn != want { + t.Errorf("root Arn = %q, want %q", acc.Arn, want) + } + if acc.Secret != standaloneTestRootSecret || acc.Role != RoleAdmin { + t.Errorf("root identity lost its credentials or role: %+v", acc) + } +} + +// TestIAMServiceStandaloneCapabilityInterfaces pins the full set of +// capability interfaces the standalone client implements. Every one is +// resolved by type assertion on the live IAMService, so anything that wraps +// it (auth.IAMCache, cmd/vgwrdma's shutdown-once wrapper) has to re-expose +// all of them — a capability silently dropped by a wrapper is a +// capability switched off gateway-wide. +func TestIAMServiceStandaloneCapabilityInterfaces(t *testing.T) { + _, sock := standaloneTestServer(t) + var iam IAMService = newStandaloneTestClient(t, sock) + + if _, ok := iam.(SigningKeyProvider); !ok { + t.Error("standalone client must implement SigningKeyProvider") + } + if _, ok := iam.(PolicyEvaluator); !ok { + t.Error("standalone client must implement PolicyEvaluator") + } + if _, ok := iam.(FixedBucketOwner); !ok { + t.Error("standalone client must implement FixedBucketOwner") + } + if _, ok := iam.(PrincipalResolver); !ok { + t.Error("standalone client must implement PrincipalResolver") + } +} diff --git a/auth/object_lock.go b/auth/object_lock.go index e6f72521..826510f5 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -291,7 +291,7 @@ func verifyBypassGovernancePermission(ctx context.Context, be backend.Backend, i case err != nil: return err default: - resourceDecision, _, err = verifyBucketPolicy(policy, acc.Access, bucket, key, condCtx, be.NormalizeObjectKey, BypassGovernanceRetentionAction) + resourceDecision, _, err = verifyBucketPolicy(policy, acc, bucket, key, condCtx, be.NormalizeObjectKey, BypassGovernanceRetentionAction) if err != nil { return err } @@ -300,7 +300,7 @@ func verifyBypassGovernancePermission(ctx context.Context, be backend.Backend, i resourceArn := objectPolicyArn(bucket, key, be.NormalizeObjectKey) if resourceDecision == policyDecisionDeny { - return s3err.GetExplicitDenyAccessErr(acc.Access, string(BypassGovernanceRetentionAction), resourceArn, "a resource-based policy") + return s3err.GetExplicitDenyAccessErr(principalName(acc), string(BypassGovernanceRetentionAction), resourceArn, "a resource-based policy") } pe, hasPolicyEvaluator := iam.(PolicyEvaluator) @@ -340,7 +340,7 @@ func verifyBypassGovernancePermission(ctx context.Context, be backend.Backend, i if identityDecision == policyDecisionDeny || sessionDenies { principal := identity.PrincipalArn if principal == "" { - principal = acc.Access + principal = principalName(acc) } return s3err.GetExplicitDenyAccessErr(principal, string(BypassGovernanceRetentionAction), resourceArn, "an identity-based policy") } @@ -351,7 +351,7 @@ func verifyBypassGovernancePermission(ctx context.Context, be backend.Backend, i principal := identity.PrincipalArn if principal == "" { - principal = acc.Access + principal = principalName(acc) } return s3err.GetImplicitDenyAccessErr(principal, string(BypassGovernanceRetentionAction), resourceArn) } diff --git a/auth/object_lock_test.go b/auth/object_lock_test.go index 65d54107..1e7b59db 100644 --- a/auth/object_lock_test.go +++ b/auth/object_lock_test.go @@ -316,3 +316,66 @@ func (b *objectRetentionBackend) GetObjectRetention(_ context.Context, _, _, _ s func (b *objectRetentionBackend) GetBucketPolicy(_ context.Context, _ string) ([]byte, error) { return nil, s3err.GetAPIError(s3err.ErrNoSuchBucketPolicy) } + +// TestVerifyBypassGovernancePermission_ArnPrincipals covers the +// governance-bypass path's own bucket-policy evaluation under an IAM backend +// whose identities have ARNs. It is a separate evaluation from VerifyAccess's +// and has to agree with it: the same principal forms match, the account +// principal delegates under Allow but not under Deny, and a denial names the +// caller by its ARN. +func TestVerifyBypassGovernancePermission_ArnPrincipals(t *testing.T) { + const ( + userArn = "arn:aws:iam::000000000000:user/alice" + rootArn = "arn:aws:iam::000000000000:root" + ) + user := Account{Access: "AKIAALICE", Role: RoleUser, Arn: userArn} + + bypassPolicy := func(effect, principal string) []byte { + return []byte(`{"Statement":[{"Effect":"` + effect + `","Principal":{"AWS":"` + principal + + `"},"Action":"s3:BypassGovernanceRetention","Resource":"arn:aws:s3:::bucket/*"}]}`) + } + + t.Run("user arn allows the bypass", func(t *testing.T) { + be := &publicBucketPolicyBackend{policy: bypassPolicy("Allow", userArn)} + err := verifyBypassGovernancePermission(context.Background(), be, + newMockPolicyEvaluator(policyDecisionNoMatch), user, "bucket", "key.txt", BypassRequested, false, nil) + assert.NoError(t, err) + }) + + t.Run("account arn delegates and so allows nothing", func(t *testing.T) { + be := &publicBucketPolicyBackend{policy: bypassPolicy("Allow", rootArn)} + err := verifyBypassGovernancePermission(context.Background(), be, + newMockPolicyEvaluator(policyDecisionNoMatch), user, "bucket", "key.txt", BypassRequested, false, nil) + + apiErr, ok := err.(s3err.APIError) + assert.True(t, ok, "err = %#v, want s3err.APIError", err) + assert.Contains(t, apiErr.Description, "because no identity-based policy allows") + }) + + t.Run("account arn deny names the caller by arn", func(t *testing.T) { + be := &publicBucketPolicyBackend{policy: bypassPolicy("Deny", rootArn)} + err := verifyBypassGovernancePermission(context.Background(), be, + newMockPolicyEvaluator(policyDecisionAllow), user, "bucket", "key.txt", BypassRequested, false, nil) + + apiErr, ok := err.(s3err.APIError) + assert.True(t, ok, "err = %#v, want s3err.APIError", err) + assert.Contains(t, apiErr.Description, userArn) + assert.Contains(t, apiErr.Description, "with an explicit deny in a resource-based policy") + }) + + t.Run("root is named by the account arn it carries", func(t *testing.T) { + // Root reaches here only for BypassOverwrite; a requested bypass + // returns earlier. rootIdentity gives it the account root ARN, which + // is how a Deny naming the account reaches it at all. + root := Account{Access: "root", Role: RoleAdmin, Arn: rootArn} + be := &publicBucketPolicyBackend{policy: bypassPolicy("Deny", rootArn)} + + err := verifyBypassGovernancePermission(context.Background(), be, + newMockPolicyEvaluator(policyDecisionAllow), root, "bucket", "key.txt", BypassOverwrite, false, nil) + + apiErr, ok := err.(s3err.APIError) + assert.True(t, ok, "err = %#v, want s3err.APIError", err) + assert.Contains(t, apiErr.Description, rootArn) + assert.Contains(t, apiErr.Description, "with an explicit deny in a resource-based policy") + }) +} diff --git a/auth/principal_resolver.go b/auth/principal_resolver.go new file mode 100644 index 00000000..4f434158 --- /dev/null +++ b/auth/principal_resolver.go @@ -0,0 +1,32 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +// PrincipalResolver is implemented by IAM backends whose identities are +// named by AWS-style principal ARNs — currently only the standalone IAM +// service client. Its presence is what switches a bucket policy's Principal +// element from naming access key ids to naming ARNs, the way real S3 does. +// +// Every other backend (internal, LDAP, Vault, IPA, S3, single) has no ARNs +// to name anything by: its accounts are access keys and nothing else. Those +// backends do not implement this, and their bucket policies keep naming +// access key ids exactly as before. +type PrincipalResolver interface { + // ResolvePrincipals returns the subset of principals that do not name + // anything — the write-time check behind PutBucketPolicy, mirroring + // ResolveAccounts' "return what does not exist" contract for access + // keys. The wildcard "*" is handled by the caller and never reaches + // here. + ResolvePrincipals(principals []string) ([]string, error) +} diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index 88795087..45975089 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -134,6 +134,7 @@ type standaloneIAMExtensions interface { auth.SigningKeyProvider auth.PolicyEvaluator auth.FixedBucketOwner + auth.PrincipalResolver } type shutdownOnceService struct { diff --git a/iamapi/internal/iammiddleware/policy.go b/iamapi/internal/iammiddleware/policy.go index b5c1f36c..cf1739b9 100644 --- a/iamapi/internal/iammiddleware/policy.go +++ b/iamapi/internal/iammiddleware/policy.go @@ -406,17 +406,23 @@ func requestConditionContext(ctx fiber.Ctx, identity types.Identity, action stri // though it authenticates as root. func IdentityConditionContext(identity types.Identity) map[string][]string { condCtx := map[string][]string{} - if arn := CallerArn(identity); arn != "" { + if arn := PrincipalConditionArn(identity); arn != "" { condCtx["aws:PrincipalArn"] = []string{arn} - condCtx["aws:PrincipalAccount"] = []string{iamutil.DefaultAccountID} } + // aws:PrincipalAccount describes the caller's account rather than the + // caller, so it is set for any non-root identity — deliberately not + // keyed off aws:PrincipalArn above, which answers a different question + // and could in principle come back empty for an identity that still has + // an account. switch { case identity.User != nil: + condCtx["aws:PrincipalAccount"] = []string{iamutil.DefaultAccountID} condCtx["aws:PrincipalType"] = []string{"User"} condCtx["aws:username"] = []string{identity.User.UserName} condCtx["aws:userid"] = []string{identity.User.UserID} addPrincipalTagContext(condCtx, identity.User.Tags) case identity.Session != nil: + condCtx["aws:PrincipalAccount"] = []string{iamutil.DefaultAccountID} condCtx["aws:PrincipalType"] = []string{"AssumedRole"} condCtx["aws:userid"] = []string{identity.Session.RoleID + ":" + identity.Session.RoleSessionName} if identity.Role != nil { @@ -497,3 +503,19 @@ func CallerArn(identity types.Identity) string { } return "" } + +// PrincipalConditionArn identifies identity the way the aws:PrincipalArn +// condition key does, which is not the way an error message does: for a +// session it is the assumed *role's* ARN, not the session's own. A +// Condition on aws:PrincipalArn therefore applies to every session of a +// role and can never single one out — aws:userid, which carries +// :, is the key that can. +func PrincipalConditionArn(identity types.Identity) string { + if identity.Session != nil { + return identity.Session.RoleArn + } + if identity.User != nil { + return identity.User.Arn + } + return "" +} diff --git a/iamapi/internal/iammiddleware/policy_test.go b/iamapi/internal/iammiddleware/policy_test.go new file mode 100644 index 00000000..6e7f2504 --- /dev/null +++ b/iamapi/internal/iammiddleware/policy_test.go @@ -0,0 +1,155 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iammiddleware + +import ( + "testing" + + "github.com/versity/versitygw/iamapi/internal/iamutil" + "github.com/versity/versitygw/iamapi/types" +) + +func testIdentityUser() types.Identity { + return types.Identity{User: &types.User{ + UserName: "alice", + UserID: "AIDAALICE", + Arn: iamutil.BuildUserArn(iamutil.DefaultAccountID, "/", "alice"), + }} +} + +func testIdentitySession() types.Identity { + role := &types.Role{ + RoleName: "reader", + RoleID: "AROAREADER", + Arn: iamutil.BuildRoleArn(iamutil.DefaultAccountID, "/", "reader"), + } + return types.Identity{ + Role: role, + Session: &types.Session{ + RoleArn: role.Arn, + RoleName: role.RoleName, + RoleID: role.RoleID, + RoleSessionName: "sess1", + }, + } +} + +// TestCallerArnAndPrincipalConditionArn pins the one place the two differ: a +// session is named by its assumed-role ARN in an error message, and by its +// role's own ARN as aws:PrincipalArn. +func TestCallerArnAndPrincipalConditionArn(t *testing.T) { + tests := []struct { + name string + identity types.Identity + wantCaller string + wantCondition string + }{ + { + name: "user", + identity: testIdentityUser(), + wantCaller: "arn:aws:iam::000000000000:user/alice", + wantCondition: "arn:aws:iam::000000000000:user/alice", + }, + { + name: "session", + identity: testIdentitySession(), + wantCaller: "arn:aws:sts::000000000000:assumed-role/reader/sess1", + wantCondition: "arn:aws:iam::000000000000:role/reader", + }, + { + name: "root", + identity: types.Identity{IsRoot: true}, + wantCaller: "", + wantCondition: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := CallerArn(tt.identity); got != tt.wantCaller { + t.Errorf("CallerArn() = %q, want %q", got, tt.wantCaller) + } + if got := PrincipalConditionArn(tt.identity); got != tt.wantCondition { + t.Errorf("PrincipalConditionArn() = %q, want %q", got, tt.wantCondition) + } + }) + } +} + +// TestPrincipalConditionArnOutlivesRole pins that a session whose role has +// since been deleted is still named by the role it was minted against: the +// session carries the ARN, so nothing has to be looked up to name it. +func TestPrincipalConditionArnOutlivesRole(t *testing.T) { + identity := testIdentitySession() + identity.Role = nil + + want := "arn:aws:iam::000000000000:role/reader" + if got := PrincipalConditionArn(identity); got != want { + t.Errorf("PrincipalConditionArn() = %q, want %q", got, want) + } +} + +// TestIdentityConditionContext covers the identity-derived condition keys +// only this service can supply. aws:PrincipalArn for a session is the role +// ARN, so a Condition on it covers every session of the role and can never +// single one out — aws:userid, which carries :, is +// the key that can. +func TestIdentityConditionContext(t *testing.T) { + tests := []struct { + name string + identity types.Identity + want map[string]string + }{ + { + name: "user", + identity: testIdentityUser(), + want: map[string]string{ + "aws:PrincipalArn": "arn:aws:iam::000000000000:user/alice", + "aws:PrincipalAccount": iamutil.DefaultAccountID, + "aws:PrincipalType": "User", + "aws:username": "alice", + "aws:userid": "AIDAALICE", + }, + }, + { + name: "session", + identity: testIdentitySession(), + want: map[string]string{ + "aws:PrincipalArn": "arn:aws:iam::000000000000:role/reader", + "aws:PrincipalAccount": iamutil.DefaultAccountID, + "aws:PrincipalType": "AssumedRole", + "aws:userid": "AROAREADER:sess1", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IdentityConditionContext(tt.identity) + for key, want := range tt.want { + vals, ok := got[key] + if !ok { + t.Errorf("%s missing, want %q", key, want) + continue + } + if len(vals) != 1 || vals[0] != want { + t.Errorf("%s = %v, want [%q]", key, vals, want) + } + } + if _, ok := got["aws:username"]; ok && tt.identity.Session != nil { + t.Error("aws:username set for a session, which has no user name") + } + }) + } +} diff --git a/iamapi/private/handlers.go b/iamapi/private/handlers.go index 641e8bfa..e4c137ae 100644 --- a/iamapi/private/handlers.go +++ b/iamapi/private/handlers.go @@ -22,6 +22,7 @@ import ( "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/iamapi/internal/iammiddleware" + "github.com/versity/versitygw/iamapi/internal/iamutil" "github.com/versity/versitygw/iamapi/policy" "github.com/versity/versitygw/iamapi/types" "github.com/versity/versitygw/internal/sigv4auth" @@ -38,6 +39,7 @@ func (p *PrivateAPI) handleVersion(ctx fiber.Ctx) error { Protocol: ProtocolVersion, MinClient: MinClientProtocol, ServerVersion: p.serverVersion, + AccountID: iamutil.DefaultAccountID, }) } @@ -47,14 +49,25 @@ func (p *PrivateAPI) handleDeriveSigningKey(ctx fiber.Ctx) error { return errMalformedRequestBody } - _, secret, err := resolvePrivateIdentity(ctx.Context(), p.store, req.AccessKeyID, req.SessionToken) + identity, secret, err := resolvePrivateIdentity(ctx.Context(), p.store, req.AccessKeyID, req.SessionToken) if err != nil { return mapResolveError(err) } derivedKey := sigv4auth.DeriveKey(secret, req.Date, req.Region, req.Service) - return ctx.JSON(DeriveSigningKeyResponse{DerivedKey: derivedKey}) + resp := DeriveSigningKeyResponse{ + DerivedKey: derivedKey, + PrincipalArn: iammiddleware.CallerArn(*identity), + } + // The role ARN comes from the session rather than from the role the + // store holds now: a session outliving its role keeps authenticating, + // and it still belongs to the role it was minted against. + if identity.Session != nil { + resp.RoleArn = identity.Session.RoleArn + } + + return ctx.JSON(resp) } // recordDataPlaneUsage records this S3 request as a use of the credential @@ -120,6 +133,32 @@ func (p *PrivateAPI) handleResolveIdentity(ctx fiber.Ctx) error { return ctx.JSON(ResolveIdentityResponse{Identities: identities}) } +// handleResolvePrincipals answers, for each string a bucket policy names as +// a Principal, whether it resolves to something that exists — reporting only +// the ones that do not, so a valid policy's principals disclose nothing. +func (p *PrivateAPI) handleResolvePrincipals(ctx fiber.Ctx) error { + var req ResolvePrincipalsRequest + if err := json.Unmarshal(ctx.Body(), &req); err != nil { + return errMalformedRequestBody + } + + invalid := []string{} + for _, principal := range req.Principals { + resolves, err := principalResolves(ctx.Context(), p.store, iamutil.DefaultAccountID, principal) + if err != nil { + // A store fault, not a verdict on the principal — errorHandler + // renders it as a 500 so the gateway reports the service as + // broken rather than the policy as malformed. + return err + } + if !resolves { + invalid = append(invalid, principal) + } + } + + return ctx.JSON(ResolvePrincipalsResponse{Invalid: invalid}) +} + // identityKindWireValue converts identityKind to its wire representation. func identityKindWireValue(k identityKind) string { if k == identityKindSession { diff --git a/iamapi/private/principal.go b/iamapi/private/principal.go new file mode 100644 index 00000000..157ac2f2 --- /dev/null +++ b/iamapi/private/principal.go @@ -0,0 +1,167 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package private + +import ( + "context" + "errors" + "strings" + + "github.com/versity/versitygw/iamapi/iamerr" + "github.com/versity/versitygw/iamapi/internal/iamutil" +) + +// accountIDLen is the length of an AWS account id, and the length the bare +// account-id principal form must have. +const accountIDLen = 12 + +// principalResolves reports whether principal names an identity that exists +// right now, in the form an S3 bucket policy's Principal element may take. +// It is the write-time check behind PutBucketPolicy: a statement naming +// something that cannot be resolved is rejected rather than stored as one +// that can never match. +// +// The accepted forms, all scoped to accountID — this service serves exactly +// one account, and an ARN naming another cannot name anything it knows: +// +// arn:aws:iam:::root the account itself +// the same, in the bare account-id form +// arn:aws:iam:::user an existing IAM user +// arn:aws:iam:::role an existing IAM role +// arn:aws:sts:::assumed-role// +// any session of an existing role +// +// A user's or role's ARN embeds its IAM path, so an ARN that omits the path +// of a path-bearing identity does not resolve — the comparison below is +// against the identity's own stored ARN, which enforces that, and the +// case-sensitivity of the whole string, without either being a rule of its +// own. +// +// The session name in an assumed-role ARN is not checked for EXISTENCE — +// it names a session that need not have been minted yet, and commonly +// won't have been when the policy is written — but it must still be a +// session name that could exist: the same grammar +// AssumeRoleWithWebIdentity enforces on RoleSessionName. Without that, +// "assumed-role/reader/*" would be stored as a statement that no session +// can ever match, since a caller's session ARN carries a literal name and +// nothing here pattern-matches. +// +// Everything else — a wildcard inside an ARN, a group/policy/ +// instance-profile ARN, another partition, another account, an access key +// id — does not resolve. Wildcards in particular: only a whole Principal of +// "*" is special, and that never reaches here. +func principalResolves(ctx context.Context, store iamutil.IdentityStore, accountID, principal string) (bool, error) { + if principal == accountID && isAccountID(principal) { + return true, nil + } + + resource, ok := principalArnResource(principal, "iam", accountID) + if ok { + switch { + case resource == "root": + return true, nil + case strings.HasPrefix(resource, "user/"): + user, err := store.GetUser(ctx, lastArnSegment(resource)) + if err != nil { + return false, ignoreNoSuchEntity(err) + } + return user != nil && user.Arn == principal, nil + case strings.HasPrefix(resource, "role/"): + role, err := store.GetRole(ctx, lastArnSegment(resource)) + if err != nil { + return false, ignoreNoSuchEntity(err) + } + return role != nil && role.Arn == principal, nil + } + return false, nil + } + + resource, ok = principalArnResource(principal, "sts", accountID) + if !ok || !strings.HasPrefix(resource, "assumed-role/") { + return false, nil + } + // An assumed-role ARN is arn:aws:sts:::assumed-role// + // with exactly those two segments: unlike the role's own ARN it never + // carries the role's path, so a third segment is not a deeper path but a + // malformed principal. + roleName, sessionName, found := strings.Cut(strings.TrimPrefix(resource, "assumed-role/"), "/") + if !found || roleName == "" { + return false, nil + } + if err := iamutil.ValidateRoleSessionName(sessionName); err != nil { + return false, nil + } + role, err := store.GetRole(ctx, roleName) + if err != nil { + return false, ignoreNoSuchEntity(err) + } + // Role lookup is case-insensitive, so compare against the ARN a session + // of that role would actually authenticate as. Without this a + // wrong-case role name would be accepted and then match nothing — the + // same reason the user and role branches above compare the stored ARN + // rather than trusting the lookup. + return role != nil && iamutil.BuildAssumedRoleArn(accountID, role.RoleName, sessionName) == principal, nil +} + +// ignoreNoSuchEntity returns nil for the store's "this identity does not +// exist" error — the answer principalResolves is asking for — and err +// itself for anything else, which is a fault in the service rather than a +// verdict on the principal. +func ignoreNoSuchEntity(err error) error { + var apiErr iamerr.Error + if errors.As(err, &apiErr) && apiErr.Code == "NoSuchEntity" { + return nil + } + return err +} + +// principalArnResource splits principal into the resource part of an +// arn:aws:::: ARN, reporting ok=false for any +// other shape. The region field must be empty, as it is in every IAM and STS +// ARN, and the partition must be "aws": this service has no other. +func principalArnResource(principal, service, accountID string) (resource string, ok bool) { + prefix := "arn:aws:" + service + "::" + accountID + ":" + if !strings.HasPrefix(principal, prefix) { + return "", false + } + resource = strings.TrimPrefix(principal, prefix) + if resource == "" { + return "", false + } + return resource, true +} + +// lastArnSegment returns the identity name from an ARN resource such as +// "user/team/sub/alice" — the last segment, everything before it being the +// identity's IAM path. +func lastArnSegment(resource string) string { + if idx := strings.LastIndex(resource, "/"); idx >= 0 { + return resource[idx+1:] + } + return resource +} + +// isAccountID reports whether s is an AWS account id: exactly twelve +// digits. +func isAccountID(s string) bool { + if len(s) != accountIDLen { + return false + } + for i := 0; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return false + } + } + return true +} diff --git a/iamapi/private/private_test.go b/iamapi/private/private_test.go index cff9d827..540e55ca 100644 --- a/iamapi/private/private_test.go +++ b/iamapi/private/private_test.go @@ -636,6 +636,24 @@ func TestPrivateAPIRejectsNonRootCredential(t *testing.T) { } } +// TestPrivateAPIEveryRouteRequiresRootCredential walks the whole route +// table rather than one route: every endpoint here is root-signed by +// construction, and the way that breaks is a new route registered without +// the auth middleware wrapped around it. +func TestPrivateAPIEveryRouteRequiresRootCredential(t *testing.T) { + p, store := newTestServer(t) + createTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + + for _, path := range []string{VersionPath, DerivePath, EvaluatePath, ResolveIdentityPath, ResolvePrincipalsPath} { + t.Run(path, func(t *testing.T) { + resp := doPrivateRequest(t, p, http.MethodPost, path, "AKIAALICE", "alicesecret", []byte("{}")) + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want %d; body=%s", resp.StatusCode, http.StatusForbidden, readBody(t, resp)) + } + }) + } +} + func TestPrivateAPIRejectsMalformedBody(t *testing.T) { p, _ := newTestServer(t) @@ -1020,3 +1038,197 @@ func createTestSessionForRole(t *testing.T, store storage.Storer, role *types.Ro } return session } + +// TestPrivateAPIDeriveSigningKeyPrincipalArn covers the identity a derived +// key belongs to, which the S3 gateway matches bucket-policy principals +// against: a user carries only its own ARN, while a session carries both its +// assumed-role ARN and the role's, since a policy naming either matches it. +func TestPrivateAPIDeriveSigningKeyPrincipalArn(t *testing.T) { + p, store := newTestServer(t) + createTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + role := createTestRole(t, store, "testrole", "") + session := createTestSessionForRole(t, store, role, "ASIASESSION", "sessionsecret", "tok", "") + + yyyymmdd := time.Now().UTC().Format(sigv4auth.YYYYMMDD) + + for _, tc := range []struct { + name string + req DeriveSigningKeyRequest + principalArn string + roleArn string + }{ + { + name: "user", + req: DeriveSigningKeyRequest{AccessKeyID: "AKIAALICE"}, + principalArn: iamutil.BuildUserArn(iamutil.DefaultAccountID, "/", "alice"), + }, + { + name: "session", + req: DeriveSigningKeyRequest{AccessKeyID: session.AccessKeyId, SessionToken: "tok"}, + principalArn: iamutil.BuildAssumedRoleArn(iamutil.DefaultAccountID, role.RoleName, session.RoleSessionName), + roleArn: role.Arn, + }, + } { + t.Run(tc.name, func(t *testing.T) { + tc.req.Date, tc.req.Region, tc.req.Service = yyyymmdd, "us-east-1", "s3" + body, _ := json.Marshal(tc.req) + + resp := doPrivateRequest(t, p, http.MethodPost, DerivePath, testRoot.Access, testRoot.Secret, body) + raw := readBody(t, resp) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, body=%s", resp.StatusCode, raw) + } + + var out DeriveSigningKeyResponse + if err := json.Unmarshal([]byte(raw), &out); err != nil { + t.Fatalf("unmarshal %s: %v", raw, err) + } + if len(out.DerivedKey) == 0 { + t.Error("DerivedKey is empty") + } + if out.PrincipalArn != tc.principalArn { + t.Errorf("PrincipalArn = %q, want %q", out.PrincipalArn, tc.principalArn) + } + if out.RoleArn != tc.roleArn { + t.Errorf("RoleArn = %q, want %q", out.RoleArn, tc.roleArn) + } + }) + } +} + +// TestPrivateAPIResolvePrincipals covers the write-time principal check +// behind PutBucketPolicy across every form a bucket policy may name, and the +// forms it may not. Only the unresolvable ones come back. +func TestPrivateAPIResolvePrincipals(t *testing.T) { + p, store := newTestServer(t) + createTestUser(t, store, "alice", "AKIAALICE", "alicesecret", "") + createTestRole(t, store, "testrole", "") + + acct := iamutil.DefaultAccountID + valid := []string{ + acct, + "arn:aws:iam::" + acct + ":root", + "arn:aws:iam::" + acct + ":user/alice", + "arn:aws:iam::" + acct + ":role/testrole", + // The session name is not checked against anything: it names a + // session that need not exist when the policy is written. + "arn:aws:sts::" + acct + ":assumed-role/testrole/never-assumed", + } + // A session name must be one that could exist — the same grammar + // AssumeRoleWithWebIdentity enforces — or the statement could never + // match anything. + invalidSessions := []string{ + "arn:aws:sts::" + acct + ":assumed-role/testrole/*", + "arn:aws:sts::" + acct + ":assumed-role/testrole/s", + "arn:aws:sts::" + acct + ":assumed-role/testrole/sess with spaces", + "arn:aws:sts::" + acct + ":assumed-role/testrole/" + strings.Repeat("s", 65), + // Role lookup is case-insensitive, so a wrong-case role name would + // otherwise resolve and then match no session. + "arn:aws:sts::" + acct + ":assumed-role/TESTROLE/sess", + } + invalid := []string{ + "AKIAALICE", + "alice", + "arn:aws:iam::" + acct + ":user/bob", + "arn:aws:iam::" + acct + ":role/norole", + "arn:aws:iam::" + acct + ":user/ALICE", + "arn:aws:iam::" + acct + ":user/*", + "arn:aws:iam::" + acct + ":user/team/alice", + "arn:aws:iam::" + acct + ":group/admins", + "arn:aws:iam::" + acct + ":user/alice ", + "arn:aws:iam::111111111111:root", + "arn:aws:iam:us-east-1:" + acct + ":root", + "arn:aws-cn:iam::" + acct + ":root", + "arn:aws:sts::" + acct + ":assumed-role/norole/sess", + "arn:aws:sts::" + acct + ":assumed-role/testrole", + "arn:aws:sts::" + acct + ":assumed-role/testrole/a/b", + "arn:aws:sts::" + acct + ":federated-user/alice", + "arn:aws:s3:::mybucket", + "", + } + invalid = append(invalid, invalidSessions...) + + body, _ := json.Marshal(ResolvePrincipalsRequest{Principals: append(append([]string{}, valid...), invalid...)}) + resp := doPrivateRequest(t, p, http.MethodPost, ResolvePrincipalsPath, testRoot.Access, testRoot.Secret, body) + raw := readBody(t, resp) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.StatusCode, raw) + } + + var out ResolvePrincipalsResponse + if err := json.Unmarshal([]byte(raw), &out); err != nil { + t.Fatalf("unmarshal %s: %v", raw, err) + } + + got := map[string]bool{} + for _, p := range out.Invalid { + got[p] = true + } + for _, p := range valid { + if got[p] { + t.Errorf("principal %q reported invalid, want it to resolve", p) + } + } + for _, p := range invalid { + if !got[p] { + t.Errorf("principal %q reported valid, want it rejected", p) + } + } +} + +// TestPrivateAPIResolvePrincipalsPathedIdentity pins that a user's or role's +// IAM path is part of the ARN naming it: the path-less form of a path-bearing +// identity resolves to nothing, and the full form resolves. +func TestPrivateAPIResolvePrincipalsPathedIdentity(t *testing.T) { + p, store := newTestServer(t) + + acct := iamutil.DefaultAccountID + if _, err := store.CreateUser(context.Background(), types.User{ + UserName: "pathed", + Path: "/team/sub/", + Arn: iamutil.BuildUserArn(acct, "/team/sub/", "pathed"), + CreateDate: time.Now().UTC(), + }); err != nil { + t.Fatalf("CreateUser: %v", err) + } + + body, _ := json.Marshal(ResolvePrincipalsRequest{Principals: []string{ + "arn:aws:iam::" + acct + ":user/team/sub/pathed", + "arn:aws:iam::" + acct + ":user/pathed", + }}) + resp := doPrivateRequest(t, p, http.MethodPost, ResolvePrincipalsPath, testRoot.Access, testRoot.Secret, body) + raw := readBody(t, resp) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.StatusCode, raw) + } + + var out ResolvePrincipalsResponse + if err := json.Unmarshal([]byte(raw), &out); err != nil { + t.Fatalf("unmarshal %s: %v", raw, err) + } + want := []string{"arn:aws:iam::" + acct + ":user/pathed"} + if len(out.Invalid) != 1 || out.Invalid[0] != want[0] { + t.Errorf("Invalid = %v, want %v", out.Invalid, want) + } +} + +// TestPrivateAPIVersionReportsAccountID pins that the version endpoint +// carries the account id every ARN this service mints belongs to — the +// gateway names its own root account with it, having no other source for it. +func TestPrivateAPIVersionReportsAccountID(t *testing.T) { + p, _ := newTestServer(t) + + resp := doPrivateRequest(t, p, http.MethodPost, VersionPath, testRoot.Access, testRoot.Secret, []byte("{}")) + raw := readBody(t, resp) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.StatusCode, raw) + } + + var out VersionResponse + if err := json.Unmarshal([]byte(raw), &out); err != nil { + t.Fatalf("unmarshal %s: %v", raw, err) + } + if out.AccountID != iamutil.DefaultAccountID { + t.Errorf("AccountID = %q, want %q", out.AccountID, iamutil.DefaultAccountID) + } +} diff --git a/iamapi/private/server.go b/iamapi/private/server.go index b035c4a7..d3900630 100644 --- a/iamapi/private/server.go +++ b/iamapi/private/server.go @@ -42,10 +42,11 @@ const ( // These are exported so auth.IAMServiceStandalone (the S3-side client) // shares one source of truth for the routes rather than duplicating the // literal path strings. - DerivePath = "/private/derive-signing-key" - EvaluatePath = "/private/evaluate-policy" - ResolveIdentityPath = "/private/resolve-identity" - VersionPath = "/private/version" + DerivePath = "/private/derive-signing-key" + EvaluatePath = "/private/evaluate-policy" + ResolveIdentityPath = "/private/resolve-identity" + ResolvePrincipalsPath = "/private/resolve-principals" + VersionPath = "/private/version" // ProtocolHeader carries the private protocol version each peer speaks. // Both send it: the S3 gateway on every request, this service on every @@ -152,6 +153,7 @@ func New(store storage.Storer, root iammiddleware.RootCredentials, opts ...Priva app.Post(DerivePath, chainHandlers(rootAuth, p.handleDeriveSigningKey)) app.Post(EvaluatePath, chainHandlers(rootAuth, p.handleEvaluatePolicy)) app.Post(ResolveIdentityPath, chainHandlers(rootAuth, p.handleResolveIdentity)) + app.Post(ResolvePrincipalsPath, chainHandlers(rootAuth, p.handleResolvePrincipals)) return p, nil } diff --git a/iamapi/private/types.go b/iamapi/private/types.go index b40d605e..9e3d1abd 100644 --- a/iamapi/private/types.go +++ b/iamapi/private/types.go @@ -30,9 +30,32 @@ type DeriveSigningKeyRequest struct { } // DeriveSigningKeyResponse carries the derived signing key (kSigning) — -// never the underlying secret. +// never the underlying secret — and the identity it belongs to, named the +// way a bucket policy's Principal element names it. +// +// PrincipalArn is the caller's own ARN: an IAM user's, or an assumed-role +// session's arn:aws:sts::…:assumed-role//. RoleArn is that +// session's role ARN and is set only for a session, because a bucket policy +// Principal naming a role matches every session of it type DeriveSigningKeyResponse struct { - DerivedKey []byte `json:"derivedKey"` + DerivedKey []byte `json:"derivedKey"` + PrincipalArn string `json:"principalArn,omitempty"` + RoleArn string `json:"roleArn,omitempty"` +} + +// ResolvePrincipalsRequest asks whether each string is a principal an S3 +// bucket policy may name — the write-time check behind PutBucketPolicy. +type ResolvePrincipalsRequest struct { + Principals []string `json:"principals"` +} + +// ResolvePrincipalsResponse answers with only the principals that do not +// resolve, so an empty list means the whole policy's principals are valid. +// It reports no detail about the ones that do: a principal's existence is +// all PutBucketPolicy validation needs, and answering more would make this +// endpoint an identity enumerator. +type ResolvePrincipalsResponse struct { + Invalid []string `json:"invalid"` } // EvaluatePolicyRequest is the evaluate-policy request body. @@ -132,8 +155,15 @@ type EvaluatePolicyResponse struct { // gateway to draw that conclusion itself from this field. Protocol duplicates // the ProtocolHeader every response carries, and ServerVersion is the build // tag — what maps a protocol number back to an image during a rollout. +// AccountID is the single AWS account id every identity this service holds +// belongs to. The gateway learns it here rather than assuming a compile-time +// constant the two builds happen to share: it is what a bucket policy's +// account-level Principal forms (the account root ARN, and the bare account +// id) are matched against, and getting it from the service that mints the +// ARNs keeps one source of truth for it. type VersionResponse struct { Protocol int `json:"protocol"` MinClient int `json:"minClient"` ServerVersion string `json:"serverVersion,omitempty"` + AccountID string `json:"accountId,omitempty"` } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index af8cbebc..d132cf8e 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1745,8 +1745,16 @@ func TestS3IAMAccessControl(ts *TestState) { ts.Run(S3IAMAccessControl_condition_on_deny_statement) ts.Run(S3IAMAccessControl_condition_multiple_keys_anded) ts.Run(S3IAMAccessControl_inactive_and_deleted_credentials) - ts.Run(S3IAMAccessControl_bucket_policy_unknown_principal_rejected) ts.Run(S3IAMAccessControl_access_key_last_used_records_s3) + ts.Run(S3IAMPrincipal_accepted_forms) + ts.Run(S3IAMPrincipal_rejected_forms) + ts.Run(S3IAMPrincipal_empty_forms) + ts.Run(S3IAMPrincipal_user_arn_names_only_that_user) + ts.Run(S3IAMPrincipal_pathed_user_arn) + ts.Run(S3IAMPrincipal_account_delegates_but_does_not_grant) + ts.Run(S3IAMPrincipal_role_arn_does_not_match_a_user) + ts.Run(S3IAMPrincipal_deleted_user_arn_rejected) + ts.Run(S3IAMPrincipal_wildcard_grants_everyone) } // TestS3IAMSessionAccessControl is the one group the OIDC workflow runs, so @@ -1765,6 +1773,10 @@ func TestS3IAMSessionAccessControl(ts *TestState) { ts.Run(S3IAMSession_role_policy_deny_overrides_session_allow) ts.Run(S3IAMSession_session_policy_without_role_policy_denied) ts.Run(S3IAMSession_bucket_policy_allows_without_role_policy) + ts.Run(S3IAMSession_bucket_policy_role_arn_covers_every_session) + ts.Run(S3IAMSession_bucket_policy_names_one_session) + ts.Run(S3IAMSession_bucket_policy_account_delegates) + ts.Run(S3IAMSession_bucket_policy_session_principal_forms) ts.Run(S3IAMSession_session_policy_filters_bucket_policy_grant) ts.Run(S3IAMSession_bucket_policy_deny_overrides_role_allow) ts.Run(S3IAMSession_missing_and_wrong_security_token) @@ -2190,7 +2202,19 @@ func GetIntTests() IntTests { "S3IAMAccessControl_condition_on_deny_statement": S3IAMAccessControl_condition_on_deny_statement, "S3IAMAccessControl_condition_multiple_keys_anded": S3IAMAccessControl_condition_multiple_keys_anded, "S3IAMAccessControl_inactive_and_deleted_credentials": S3IAMAccessControl_inactive_and_deleted_credentials, - "S3IAMAccessControl_bucket_policy_unknown_principal_rejected": S3IAMAccessControl_bucket_policy_unknown_principal_rejected, + "S3IAMSession_bucket_policy_role_arn_covers_every_session": S3IAMSession_bucket_policy_role_arn_covers_every_session, + "S3IAMSession_bucket_policy_names_one_session": S3IAMSession_bucket_policy_names_one_session, + "S3IAMSession_bucket_policy_account_delegates": S3IAMSession_bucket_policy_account_delegates, + "S3IAMSession_bucket_policy_session_principal_forms": S3IAMSession_bucket_policy_session_principal_forms, + "S3IAMPrincipal_accepted_forms": S3IAMPrincipal_accepted_forms, + "S3IAMPrincipal_rejected_forms": S3IAMPrincipal_rejected_forms, + "S3IAMPrincipal_empty_forms": S3IAMPrincipal_empty_forms, + "S3IAMPrincipal_user_arn_names_only_that_user": S3IAMPrincipal_user_arn_names_only_that_user, + "S3IAMPrincipal_pathed_user_arn": S3IAMPrincipal_pathed_user_arn, + "S3IAMPrincipal_account_delegates_but_does_not_grant": S3IAMPrincipal_account_delegates_but_does_not_grant, + "S3IAMPrincipal_role_arn_does_not_match_a_user": S3IAMPrincipal_role_arn_does_not_match_a_user, + "S3IAMPrincipal_deleted_user_arn_rejected": S3IAMPrincipal_deleted_user_arn_rejected, + "S3IAMPrincipal_wildcard_grants_everyone": S3IAMPrincipal_wildcard_grants_everyone, "S3IAMAccessControl_access_key_last_used_records_s3": S3IAMAccessControl_access_key_last_used_records_s3, "Authentication_invalid_auth_header": Authentication_invalid_auth_header, "Authentication_unsupported_signature_version": Authentication_unsupported_signature_version, diff --git a/tests/integration/s3_iam_access_control.go b/tests/integration/s3_iam_access_control.go index b94f1151..a7d19e60 100644 --- a/tests/integration/s3_iam_access_control.go +++ b/tests/integration/s3_iam_access_control.go @@ -374,7 +374,7 @@ func S3IAMAccessControl_multiple_inline_policies_combine(s *S3Conf) error { // S3IAMAccessControl_bucket_policy_allows_without_identity_policy verifies // the resource side is independently sufficient too: a bucket policy naming -// the user's access key grants the request with no identity policy at all. +// the user's ARN grants the request with no identity policy at all. func S3IAMAccessControl_bucket_policy_allows_without_identity_policy(s *S3Conf) error { testName := "S3IAMAccessControl_bucket_policy_allows_without_identity_policy" return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { @@ -385,7 +385,7 @@ func S3IAMAccessControl_bucket_policy_allows_without_identity_policy(s *S3Conf) defer cleanup() if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Allow", Principal: user.conf.awsID, Action: actS3PutObject, Resource: objectsArn(bucket), + Effect: "Allow", Principal: user.arn, Action: actS3PutObject, Resource: objectsArn(bucket), }); err != nil { return err } @@ -416,7 +416,7 @@ func S3IAMAccessControl_bucket_policy_explicit_deny(s *S3Conf) error { defer cleanup() if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Deny", Principal: user.conf.awsID, Action: actS3GetObject, Resource: objectsArn(bucket), + Effect: "Deny", Principal: user.arn, Action: actS3GetObject, Resource: objectsArn(bucket), }); err != nil { return err } @@ -424,10 +424,7 @@ func S3IAMAccessControl_bucket_policy_explicit_deny(s *S3Conf) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err = user.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) cancel() - // The resource-based denial names the access key, not the ARN: - // bucket-policy principals are access-key-based for every backend, - // so the gateway has no ARN in hand at that point. - return checkApiErr(err, wantExplicitResourceDeny(user.conf.awsID, actS3GetObject, objectArn(bucket, "obj"))) + return checkApiErr(err, wantExplicitResourceDeny(user.arn, actS3GetObject, objectArn(bucket, "obj"))) }) } @@ -470,16 +467,16 @@ func S3IAMAccessControl_policy_combinations(s *S3Conf) error { return wantExplicitIdentityDeny(u.arn, actS3GetObject, objectArn(bucket, "obj")) }}, {identity: silent, resource: deny, wantErr: func(u *s3IAMPrincipal) s3err.S3Error { - return wantExplicitResourceDeny(u.conf.awsID, actS3GetObject, objectArn(bucket, "obj")) + return wantExplicitResourceDeny(u.arn, actS3GetObject, objectArn(bucket, "obj")) }}, {identity: allow, resource: deny, wantErr: func(u *s3IAMPrincipal) s3err.S3Error { - return wantExplicitResourceDeny(u.conf.awsID, actS3GetObject, objectArn(bucket, "obj")) + return wantExplicitResourceDeny(u.arn, actS3GetObject, objectArn(bucket, "obj")) }}, // A Deny on both sides is reported as the resource-based one: // VerifyAccess evaluates the bucket policy first and returns // immediately, which also saves an IAM round trip. {identity: deny, resource: deny, wantErr: func(u *s3IAMPrincipal) s3err.S3Error { - return wantExplicitResourceDeny(u.conf.awsID, actS3GetObject, objectArn(bucket, "obj")) + return wantExplicitResourceDeny(u.arn, actS3GetObject, objectArn(bucket, "obj")) }}, } @@ -512,7 +509,7 @@ func S3IAMAccessControl_policy_combinations(s *S3Conf) error { effect = "Deny" } if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: effect, Principal: user.conf.awsID, Action: actS3GetObject, Resource: objectsArn(bucket), + Effect: effect, Principal: user.arn, Action: actS3GetObject, Resource: objectsArn(bucket), }); err != nil { return err } @@ -857,7 +854,7 @@ func S3IAMAccessControl_governance_bypass_sources(s *S3Conf) error { effect = "Deny" } if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: effect, Principal: user.conf.awsID, + Effect: effect, Principal: user.arn, Action: actS3BypassGovernance, Resource: objectsArn(bucket), }); err != nil { return err @@ -1123,11 +1120,11 @@ func S3IAMAccessControl_delete_objects_bucket_policy_deny_per_key(s *S3Conf) err // keys — the second one proves the first didn't end the evaluation. if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Deny", Principal: user.conf.awsID, + Effect: "Deny", Principal: user.arn, Action: actS3DeleteObject, Resource: objectArn(bucket, "protected/*"), }, bucketStatement{ - Effect: "Deny", Principal: user.conf.awsID, + Effect: "Deny", Principal: user.arn, Action: actS3DeleteObject, Resource: objectArn(bucket, "vault/*"), }, ); err != nil { @@ -1139,7 +1136,7 @@ func S3IAMAccessControl_delete_objects_bucket_policy_deny_per_key(s *S3Conf) err // is denied for want of any grant, and "allowed/" is the only prefix // the identity policy permits. resourceDeny := func(key string) keyDenial { - return keyDenial{key, wantExplicitResourceDeny(user.conf.awsID, actS3DeleteObject, objectArn(bucket, key))} + return keyDenial{key, wantExplicitResourceDeny(user.arn, actS3DeleteObject, objectArn(bucket, key))} } implicitDeny := func(key string) keyDenial { return keyDenial{key, wantImplicitDeny(user.arn, actS3DeleteObject, objectArn(bucket, key))} @@ -1236,7 +1233,7 @@ func S3IAMAccessControl_delete_objects_deny_across_versioned_split(s *S3Conf) er defer cleanup() if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Deny", Principal: user.conf.awsID, + Effect: "Deny", Principal: user.arn, Action: []string{actS3DeleteObject, actS3DeleteObjectVersion}, Resource: objectArn(bucket, "protected/*"), }); err != nil { @@ -1266,8 +1263,8 @@ func S3IAMAccessControl_delete_objects_deny_across_versioned_split(s *S3Conf) er return err } return checkDeleteObjectsErrsInOrder(out.Errors, []keyDenial{ - {"protected/x", wantExplicitResourceDeny(user.conf.awsID, actS3DeleteObject, objectArn(bucket, "protected/x"))}, - {"protected/v", wantExplicitResourceDeny(user.conf.awsID, actS3DeleteObjectVersion, objectArn(bucket, "protected/v"))}, + {"protected/x", wantExplicitResourceDeny(user.arn, actS3DeleteObject, objectArn(bucket, "protected/x"))}, + {"protected/v", wantExplicitResourceDeny(user.arn, actS3DeleteObjectVersion, objectArn(bucket, "protected/v"))}, {"secret/z", wantImplicitDeny(user.arn, actS3DeleteObject, objectArn(bucket, "secret/z"))}, {"other/v", wantImplicitDeny(user.arn, actS3DeleteObjectVersion, objectArn(bucket, "other/v"))}, }) @@ -2050,24 +2047,6 @@ func S3IAMAccessControl_inactive_and_deleted_credentials(s *S3Conf) error { }) } -// S3IAMAccessControl_bucket_policy_unknown_principal_rejected verifies -// PutBucketPolicy validates its principals against the IAM service, so a -// policy naming somebody who doesn't exist is rejected instead of being -// stored as a statement that can never match. -func S3IAMAccessControl_bucket_policy_unknown_principal_rejected(s *S3Conf) error { - testName := "S3IAMAccessControl_bucket_policy_unknown_principal_rejected" - return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { - err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Allow", Principal: "AKIADOESNOTEXIST", Action: actS3GetObject, Resource: objectsArn(bucket), - }) - return checkApiErr(err, s3err.APIError{ - Code: "MalformedPolicy", - Description: "Invalid principal in policy", - HTTPStatusCode: 400, - }) - }) -} - // S3IAMAccessControl_access_key_last_used_records_s3 covers last-used // tracking for the S3 data plane: an IAM user's S3 request is recorded // against the access key that signed it, with the "s3" service name and the diff --git a/tests/integration/s3_iam_principals.go b/tests/integration/s3_iam_principals.go new file mode 100644 index 00000000..e99b572a --- /dev/null +++ b/tests/integration/s3_iam_principals.go @@ -0,0 +1,475 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package integration + +import ( + "context" + "fmt" + "strings" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/iam" + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/versity/versitygw/s3err" +) + +// wantInvalidPrincipal is the error PutBucketPolicy reports for a Principal +// that names nothing this IAM service knows. +func wantInvalidPrincipal() s3err.APIError { + return getMalformedPolicyError("Invalid principal in policy") +} + +// S3IAMPrincipal_accepted_forms covers every Principal form PutBucketPolicy +// accepts: the wildcard in both its shapes, an existing user's ARN, an +// existing role's ARN, an assumed-role ARN naming a session of an existing +// role, the account root ARN, the bare account id, and a list mixing them. +func S3IAMPrincipal_accepted_forms(s *S3Conf) error { + testName := "S3IAMPrincipal_accepted_forms" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + roleName, roleCleanup, err := newS3IAMBareRole(root) + if err != nil { + return err + } + defer roleCleanup() + + cases := []struct { + name string + principal any + }{ + {"wildcard string", "*"}, + {"wildcard aws struct", map[string]any{"AWS": "*"}}, + {"user arn", user.arn}, + {"user arn in an aws struct", map[string]any{"AWS": user.arn}}, + {"role arn", roleArnFor(roleName)}, + // The session name is never validated: it names a session that + // need not have been minted when the policy is written. + {"assumed role arn", assumedRoleArnFor(roleName, "any-session-name")}, + {"account root arn", accountArn()}, + {"bare account id", testAccountID}, + {"list of arns", []string{user.arn, roleArnFor(roleName)}}, + {"aws struct with a list", map[string]any{"AWS": []string{user.arn, accountArn()}}}, + } + + for _, tc := range cases { + err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: tc.principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }) + if err != nil { + return fmt.Errorf("%s: expected the policy to be accepted: %w", tc.name, err) + } + } + return nil + }) +} + +// S3IAMPrincipal_rejected_forms covers what PutBucketPolicy refuses to +// store, all of it with the same MalformedPolicy error real S3 reports. The +// two that matter most are an access key id — what every other IAM backend +// names principals by, and what this one no longer accepts — and any +// wildcard inside an ARN: only a whole Principal of "*" is a wildcard, so +// there is no pattern form of a principal at all. +func S3IAMPrincipal_rejected_forms(s *S3Conf) error { + testName := "S3IAMPrincipal_rejected_forms" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + roleName, roleCleanup, err := newS3IAMBareRole(root) + if err != nil { + return err + } + defer roleCleanup() + + cases := []struct { + name string + principal any + }{ + {"access key id", user.conf.awsID}, + {"bare user name", user.name}, + {"non existing user arn", userArnFor("no-such-user")}, + {"non existing role arn", roleArnFor("no-such-role")}, + {"assumed role arn of a non existing role", assumedRoleArnFor("no-such-role", "sess")}, + {"assumed role arn missing the session name", "arn:aws:sts::" + testAccountID + ":assumed-role/" + roleName}, + // A session name is not checked for existence, but it must be + // one that could exist: a form no session can ever carry would + // be stored as a statement that can never match. + {"wildcard session name", assumedRoleArnFor(roleName, "*")}, + {"session name too short", assumedRoleArnFor(roleName, "s")}, + {"session name with a space", assumedRoleArnFor(roleName, "sess name")}, + {"session name too long", assumedRoleArnFor(roleName, strings.Repeat("s", 65))}, + {"wrong case role name in an assumed role arn", assumedRoleArnFor(strings.ToUpper(roleName), "sess")}, + {"wildcard within an arn", userArnFor("*")}, + {"wildcard suffix within an arn", userArnFor(user.name[:4] + "*")}, + {"wildcard account in an arn", "arn:aws:iam::*:user/" + user.name}, + {"wrong case resource type", "arn:aws:iam::" + testAccountID + ":USER/" + user.name}, + {"wrong case user name", userArnFor(strings.ToUpper(user.name))}, + {"trailing slash after the user name", user.arn + "/"}, + {"leading whitespace", " " + user.arn}, + {"trailing whitespace", user.arn + " "}, + {"another account", "arn:aws:iam::111111111111:root"}, + {"another account user", "arn:aws:iam::111111111111:user/" + user.name}, + {"account id of another account", "111111111111"}, + {"region bearing arn", "arn:aws:iam:us-east-1:" + testAccountID + ":root"}, + {"wrong partition", "arn:aws-cn:iam::" + testAccountID + ":root"}, + {"group arn", "arn:aws:iam::" + testAccountID + ":group/admins"}, + {"non iam arn", bucketArn(bucket)}, + {"not an arn", "not-an-arn"}, + {"wildcard mixed with an arn", []string{"*", user.arn}}, + {"list with one unresolvable entry", []string{user.arn, userArnFor("no-such-user")}}, + } + + for _, tc := range cases { + err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: tc.principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }) + if err := checkApiErr(err, wantInvalidPrincipal()); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// S3IAMPrincipal_empty_forms covers the Principal shapes that carry no +// principal at all. They are rejected the same way they were before +// principals were ARNs — the check is on the element's shape, not on what it +// names — so this pins that the ARN change left them alone. +func S3IAMPrincipal_empty_forms(s *S3Conf) error { + testName := "S3IAMPrincipal_empty_forms" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + cases := []struct { + name string + principal any + }{ + {"empty string", ""}, + {"empty list", []string{}}, + {"empty aws struct", map[string]any{"AWS": ""}}, + {"empty aws list", map[string]any{"AWS": []string{}}}, + } + + for _, tc := range cases { + err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: tc.principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }) + if err := checkApiErr(err, wantInvalidPrincipal()); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} + +// S3IAMPrincipal_user_arn_names_only_that_user verifies a user ARN grants +// the user it names and nobody else — neither another user, nor a caller +// named by the access key that used to be the principal form. +func S3IAMPrincipal_user_arn_names_only_that_user(s *S3Conf) error { + testName := "S3IAMPrincipal_user_arn_names_only_that_user" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if err := putObjectAsRoot(s, bucket, "obj"); err != nil { + return err + } + + granted, cleanupGranted, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanupGranted() + + other, cleanupOther, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanupOther() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: granted.arn, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + if err := getObjectAllowed(granted, bucket, "obj"); err != nil { + return err + } + return getObjectDenied(other, bucket, "obj", wantImplicitDeny(other.arn, actS3GetObject, objectArn(bucket, "obj"))) + }) +} + +// S3IAMPrincipal_pathed_user_arn verifies a user's IAM path is part of the +// ARN naming it: only the full-path form resolves at all, and it matches the +// user it names. +func S3IAMPrincipal_pathed_user_arn(s *S3Conf) error { + testName := "S3IAMPrincipal_pathed_user_arn" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if err := putObjectAsRoot(s, bucket, "obj"); err != nil { + return err + } + + userName := newIAMUserName() + createOut, err := createIAMUser(root, &iam.CreateUserInput{ + UserName: aws.String(userName), + Path: aws.String("/team/sub/"), + }) + if err != nil { + return err + } + defer deleteS3IAMUser(root, userName) + + user, err := s3IAMUserWithNewKey(root, s, userName, aws.ToString(createOut.User.Arn)) + if err != nil { + return err + } + + // The path-less form names no identity, so it never reaches + // storage. + err = putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: userArnFor(userName), Action: actS3GetObject, Resource: objectsArn(bucket), + }) + if err := checkApiErr(err, wantInvalidPrincipal()); err != nil { + return fmt.Errorf("path-less user arn: %w", err) + } + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: user.arn, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return fmt.Errorf("full user arn: %w", err) + } + return getObjectAllowed(user, bucket, "obj") + }) +} + +// S3IAMPrincipal_account_delegates_but_does_not_grant is the asymmetry the +// account-level principal forms carry: naming the account in an Allow +// delegates to the account's own IAM rather than granting anything, so a +// user with no identity policy is still denied and the same user with one is +// allowed. Naming it in a Deny delegates nothing and denies outright. +func S3IAMPrincipal_account_delegates_but_does_not_grant(s *S3Conf) error { + testName := "S3IAMPrincipal_account_delegates_but_does_not_grant" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if err := putObjectAsRoot(s, bucket, "obj"); err != nil { + return err + } + + // Both account forms are the same principal and must behave + // identically. + for _, principal := range []string{accountArn(), testAccountID} { + if err := func() error { + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + // Delegation grants nothing on its own. + if err := getObjectDenied(user, bucket, "obj", + wantImplicitDeny(user.arn, actS3GetObject, objectArn(bucket, "obj"))); err != nil { + return fmt.Errorf("allow without an identity policy: %w", err) + } + + // ...and the identity policy it delegates to does. + if err := putS3IAMUserPolicy(root, user, "p", policyDoc(accessStatement{ + Effect: "Allow", Action: actS3GetObject, Resource: objectsArn(bucket), + })); err != nil { + return err + } + if err := getObjectAllowed(user, bucket, "obj"); err != nil { + return fmt.Errorf("allow with an identity policy: %w", err) + } + + // A Deny naming the account is not a delegation: it denies + // the same user its own identity policy just allowed. + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Deny", Principal: principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + return getObjectDenied(user, bucket, "obj", + wantExplicitResourceDeny(user.arn, actS3GetObject, objectArn(bucket, "obj"))) + }(); err != nil { + return fmt.Errorf("principal %q: %w", principal, err) + } + } + return nil + }) +} + +// S3IAMPrincipal_role_arn_does_not_match_a_user verifies a role ARN names +// sessions of that role and nothing else: a long-term user is not covered by +// it, however the role is otherwise configured. The session side — that a +// role ARN does match every session of the role — needs a real OIDC token +// and lives in the s3-iam-session group. +func S3IAMPrincipal_role_arn_does_not_match_a_user(s *S3Conf) error { + testName := "S3IAMPrincipal_role_arn_does_not_match_a_user" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if err := putObjectAsRoot(s, bucket, "obj"); err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + roleName, roleCleanup, err := newS3IAMBareRole(root) + if err != nil { + return err + } + defer roleCleanup() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: roleArnFor(roleName), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + return getObjectDenied(user, bucket, "obj", + wantImplicitDeny(user.arn, actS3GetObject, objectArn(bucket, "obj"))) + }) +} + +// S3IAMPrincipal_deleted_user_arn_rejected verifies the write-time check +// tracks the IAM service rather than a snapshot of it: an ARN that resolved +// when a policy naming it was written no longer resolves once the user is +// gone, so the same document can no longer be stored. +func S3IAMPrincipal_deleted_user_arn_rejected(s *S3Conf) error { + testName := "S3IAMPrincipal_deleted_user_arn_rejected" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + // The test deletes the user itself; cleanup tolerates that and + // still runs if an earlier step fails first. + defer cleanup() + + statement := bucketStatement{ + Effect: "Allow", Principal: user.arn, Action: actS3GetObject, Resource: objectsArn(bucket), + } + if err := putBucketPolicyDoc(s, bucket, statement); err != nil { + return fmt.Errorf("expected a live user's arn to be accepted: %w", err) + } + + if err := deleteS3IAMUser(root, user.name); err != nil { + return err + } + + err = putBucketPolicyDoc(s, bucket, statement) + return checkApiErr(err, wantInvalidPrincipal()) + }) +} + +// S3IAMPrincipal_wildcard_grants_everyone verifies "*" still names every +// caller, the one principal form that does not have to resolve to anything. +func S3IAMPrincipal_wildcard_grants_everyone(s *S3Conf) error { + testName := "S3IAMPrincipal_wildcard_grants_everyone" + return s3IAMActionHandler(s, testName, func(root *iam.Client, bucket string) error { + if err := putObjectAsRoot(s, bucket, "obj"); err != nil { + return err + } + + user, cleanup, err := newS3IAMUser(root, s, nil) + if err != nil { + return err + } + defer cleanup() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: "*", Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + return getObjectAllowed(user, bucket, "obj") + }) +} + +// newS3IAMBareRole creates a role nothing ever assumes: these tests only +// need one to exist, so that a Principal naming it resolves. Its trust +// policy names the account rather than an OIDC provider, which keeps the +// fixture free of the whole web-identity setup a session needs — those tests +// live in the s3-iam-session group. +func newS3IAMBareRole(root *iam.Client) (roleName string, cleanup func(), err error) { + roleName = newIAMRoleName() + trust := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":"` + + accountArn() + `"},"Action":"sts:AssumeRole"}]}` + + if _, err := createIAMRole(root, &iam.CreateRoleInput{ + RoleName: aws.String(roleName), + AssumeRolePolicyDocument: aws.String(trust), + }); err != nil { + return "", nil, fmt.Errorf("create role: %w", err) + } + return roleName, func() { deleteIAMRole(root, roleName) }, nil +} + +// s3IAMUserWithNewKey mints an access key for an already-created user and +// returns it as an s3IAMPrincipal, for the tests that need a user +// newS3IAMUser cannot create — one with an IAM path, for instance. +func s3IAMUserWithNewKey(root *iam.Client, s *S3Conf, userName, userArn string) (*s3IAMPrincipal, error) { + keyOut, err := createIAMAccessKey(root, &iam.CreateAccessKeyInput{UserName: aws.String(userName)}) + if err != nil { + return nil, fmt.Errorf("create access key: %w", err) + } + + conf := *s + conf.awsID = aws.ToString(keyOut.AccessKey.AccessKeyId) + conf.awsSecret = aws.ToString(keyOut.AccessKey.SecretAccessKey) + + return &s3IAMPrincipal{name: userName, arn: userArn, conf: conf, client: conf.GetClient()}, nil +} + +// putObjectAsRoot puts an object the test's principals then read, so what a +// test measures is their authorization rather than their ability to set the +// scene. +func putObjectAsRoot(s *S3Conf, bucket, key string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + _, err := s.GetClient().PutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: &key}) + return err +} + +// getObjectAllowed and getObjectDenied assert the two outcomes a principal +// test cares about, so each case reads as the one line it is. +func getObjectAllowed(p *s3IAMPrincipal, bucket, key string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + if _, err := p.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: &key}); err != nil { + return fmt.Errorf("expected GetObject to be allowed for %v: %w", p.arn, err) + } + return nil +} + +func getObjectDenied(p *s3IAMPrincipal, bucket, key string, want s3err.S3Error) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + + _, err := p.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: &key}) + return checkApiErr(err, want) +} diff --git a/tests/integration/s3_iam_session_access_control.go b/tests/integration/s3_iam_session_access_control.go index 74a51739..43c77ad3 100644 --- a/tests/integration/s3_iam_session_access_control.go +++ b/tests/integration/s3_iam_session_access_control.go @@ -309,11 +309,11 @@ func S3IAMSession_session_policy_without_role_policy_denied(s *S3Conf) error { // policy is independently sufficient for a session too, exactly as it is for // a long-term user. // -// The bucket policy names "*" rather than the session: this gateway matches -// bucket-policy principals against the caller's access key, and a session's -// key is ephemeral, so auth.CheckIfAccountsExist rejects one as a principal -// outright rather than let a policy come to reference a principal that stops -// existing. See bucketStatement. +// The policy names the session's role ARN, which is how a bucket policy +// names every session of a role: no wildcard is allowed inside a principal +// ARN, so the role ARN is the only form that covers sessions the policy was +// written before. Naming one specific session is +// S3IAMSession_bucket_policy_names_one_session. func S3IAMSession_bucket_policy_allows_without_role_policy(s *S3Conf) error { testName := "S3IAMSession_bucket_policy_allows_without_role_policy" return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { @@ -323,11 +323,6 @@ func S3IAMSession_bucket_policy_allows_without_role_policy(s *S3Conf) error { if err != nil { return err } - if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Allow", Principal: "*", Action: actS3GetObject, Resource: objectsArn(bucket), - }); err != nil { - return err - } session, cleanup, err := newGitHubSession(root, s, nil, "") if err != nil { @@ -335,6 +330,12 @@ func S3IAMSession_bucket_policy_allows_without_role_policy(s *S3Conf) error { } defer cleanup() + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: roleArnFor(session.name), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) cancel() @@ -367,13 +368,6 @@ func S3IAMSession_session_policy_filters_bucket_policy_grant(s *S3Conf) error { if err != nil { return err } - if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Allow", Principal: "*", - Action: []string{actS3GetObject, actS3PutObject}, Resource: objectsArn(bucket), - }); err != nil { - return err - } - session, cleanup, err := newGitHubSession(root, s, nil, policyDoc(accessStatement{Effect: "Allow", Action: actS3GetObject, Resource: objectsArn(bucket)})) if err != nil { @@ -381,6 +375,13 @@ func S3IAMSession_session_policy_filters_bucket_policy_grant(s *S3Conf) error { } defer cleanup() + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: roleArnFor(session.name), + Action: []string{actS3GetObject, actS3PutObject}, Resource: objectsArn(bucket), + }); err != nil { + return err + } + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) cancel() @@ -407,12 +408,6 @@ func S3IAMSession_bucket_policy_deny_overrides_role_allow(s *S3Conf) error { if err != nil { return err } - if err := putBucketPolicyDoc(s, bucket, bucketStatement{ - Effect: "Deny", Principal: "*", Action: actS3GetObject, Resource: objectsArn(bucket), - }); err != nil { - return err - } - session, cleanup, err := newGitHubSession(root, s, map[string]string{ "p": policyDoc(accessStatement{Effect: "Allow", Action: "s3:*", Resource: objectsArn(bucket)}), }, "") @@ -421,13 +416,18 @@ func S3IAMSession_bucket_policy_deny_overrides_role_allow(s *S3Conf) error { } defer cleanup() + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Deny", Principal: roleArnFor(session.name), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) cancel() - // A resource-based denial names the raw access key: bucket-policy - // principals are access-key-based for every backend, so no ARN is in - // hand at that point. - return checkApiErr(err, wantExplicitResourceDeny(session.conf.awsID, actS3GetObject, objectArn(bucket, "obj"))) + // A session is named by its assumed-role ARN in a denial message, + // which is what real S3 reports too. + return checkApiErr(err, wantExplicitResourceDeny(session.arn, actS3GetObject, objectArn(bucket, "obj"))) }) } @@ -685,9 +685,12 @@ func S3IAMSession_delete_objects_authorizes_each_key(s *S3Conf) error { } // S3IAMSession_condition_identity_keys verifies the identity-derived -// condition keys describe the *session*, not the underlying role: aws:userid -// carries the role id and session name, and aws:PrincipalArn the -// assumed-role ARN. +// condition keys for a session. They do not all describe the same thing: +// aws:userid carries the role id and the session name, and so pins one +// session, while aws:PrincipalArn is the assumed *role's* ARN and therefore +// covers every session of it — a Condition on it can never single one out. +// A denial message names the session by its assumed-role ARN, which is a +// different thing from aws:PrincipalArn and deliberately so. func S3IAMSession_condition_identity_keys(s *S3Conf) error { testName := "S3IAMSession_condition_identity_keys" return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { @@ -704,10 +707,14 @@ func S3IAMSession_condition_identity_keys(s *S3Conf) error { wantAllowed bool }{ { - name: "principal arn matches the assumed-role session", - condition: func(p *s3IAMPrincipal) []byte { return cond("StringEquals", "aws:PrincipalArn", p.arn) }, + name: "principal arn is the assumed role's arn", + condition: func(p *s3IAMPrincipal) []byte { return cond("StringEquals", "aws:PrincipalArn", roleArnFor(p.name)) }, wantAllowed: true, }, + { + name: "principal arn is not the assumed-role session arn", + condition: func(p *s3IAMPrincipal) []byte { return cond("StringEquals", "aws:PrincipalArn", p.arn) }, + }, { name: "principal type is AssumedRole", condition: func(p *s3IAMPrincipal) []byte { return cond("StringEquals", "aws:PrincipalType", "AssumedRole") }, @@ -1031,3 +1038,210 @@ func S3IAMSession_role_last_used_records_s3(s *S3Conf) error { return nil }) } + +// S3IAMSession_bucket_policy_role_arn_covers_every_session verifies a +// Principal naming a role covers sessions of it that did not exist when the +// policy was written. That is the only way to express "any session of this +// role": no wildcard is allowed inside a principal ARN. +func S3IAMSession_bucket_policy_role_arn_covers_every_session(s *S3Conf) error { + testName := "S3IAMSession_bucket_policy_role_arn_covers_every_session" + return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s.GetClient().PutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return err + } + + session, cleanup, err := newGitHubSession(root, s, nil, "") + if err != nil { + return err + } + defer cleanup() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: roleArnFor(session.name), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + // A session minted after the policy was written is covered by it + // just as the first one is. + later, err := anotherSessionOfRole(s, session.name) + if err != nil { + return err + } + + for _, p := range []*s3IAMPrincipal{session, later} { + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = p.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return fmt.Errorf("expected GetObject to be allowed for %v: %w", p.arn, err) + } + } + return nil + }) +} + +// S3IAMSession_bucket_policy_names_one_session verifies the other half: +// a Principal naming one assumed-role session covers that session and no +// other session of the same role. +func S3IAMSession_bucket_policy_names_one_session(s *S3Conf) error { + testName := "S3IAMSession_bucket_policy_names_one_session" + return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s.GetClient().PutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return err + } + + named, cleanup, err := newGitHubSession(root, s, nil, "") + if err != nil { + return err + } + defer cleanup() + + other, err := anotherSessionOfRole(s, named.name) + if err != nil { + return err + } + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: named.arn, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = named.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return fmt.Errorf("expected GetObject to be allowed for the named session: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = other.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + return checkApiErr(err, wantImplicitDeny(other.arn, actS3GetObject, objectArn(bucket, "obj"))) + }) +} + +// S3IAMSession_bucket_policy_account_delegates verifies the account-level +// principal forms delegate rather than grant for a session too: naming the +// account allows nothing without a role policy, and denies everything under +// a Deny. +func S3IAMSession_bucket_policy_account_delegates(s *S3Conf) error { + testName := "S3IAMSession_bucket_policy_account_delegates" + return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s.GetClient().PutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return err + } + + session, cleanup, err := newGitHubSession(root, s, nil, "") + if err != nil { + return err + } + defer cleanup() + + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: accountArn(), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err := checkApiErr(err, wantImplicitDeny(session.arn, actS3GetObject, objectArn(bucket, "obj"))); err != nil { + return fmt.Errorf("allow naming the account: %w", err) + } + + // The role policy the account principal delegates to is what + // actually grants. It has to be removed again before teardown: + // DeleteRole refuses a role that still carries an inline policy. + if _, err := putIAMRolePolicy(root, &iam.PutRolePolicyInput{ + RoleName: aws.String(session.name), + PolicyName: aws.String("p"), + PolicyDocument: aws.String(policyDoc(accessStatement{ + Effect: "Allow", Action: actS3GetObject, Resource: objectsArn(bucket), + })), + }); err != nil { + return err + } + defer deleteIAMRolePolicy(root, session.name, "p") + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + if err != nil { + return fmt.Errorf("expected the role policy to grant what the account principal delegated: %w", err) + } + + // A Deny naming the account is not a delegation. + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Deny", Principal: accountArn(), Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = session.client.GetObject(ctx, &s3.GetObjectInput{Bucket: &bucket, Key: getPtr("obj")}) + cancel() + return checkApiErr(err, wantExplicitResourceDeny(session.arn, actS3GetObject, objectArn(bucket, "obj"))) + }) +} + +// S3IAMSession_bucket_policy_session_principal_forms covers what +// PutBucketPolicy makes of the session-shaped principal forms: an +// assumed-role ARN resolves as long as its role does, whatever session name +// it carries, and the forms that name no role do not resolve at all. +func S3IAMSession_bucket_policy_session_principal_forms(s *S3Conf) error { + testName := "S3IAMSession_bucket_policy_session_principal_forms" + return s3IAMSessionActionHandler(s, testName, func(root *iam.Client, bucket string) error { + session, cleanup, err := newGitHubSession(root, s, nil, "") + if err != nil { + return err + } + defer cleanup() + + accepted := []struct { + name string + principal string + }{ + {"the live session's arn", session.arn}, + {"a session name never assumed", assumedRoleArnFor(session.name, "never-assumed")}, + } + for _, tc := range accepted { + if err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: tc.principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }); err != nil { + return fmt.Errorf("%s: expected the policy to be accepted: %w", tc.name, err) + } + } + + rejected := []struct { + name string + principal string + }{ + {"session access key id", session.conf.awsID}, + {"wildcard session name", assumedRoleArnFor(session.name, "*")}, + {"assumed-role arn of a non existing role", assumedRoleArnFor("no-such-role", "sess")}, + {"assumed-role arn with the iam service", "arn:aws:iam::" + testAccountID + ":assumed-role/" + session.name + "/sess"}, + {"role arn with the sts service", "arn:aws:sts::" + testAccountID + ":role/" + session.name}, + } + for _, tc := range rejected { + err := putBucketPolicyDoc(s, bucket, bucketStatement{ + Effect: "Allow", Principal: tc.principal, Action: actS3GetObject, Resource: objectsArn(bucket), + }) + if err := checkApiErr(err, wantInvalidPrincipal()); err != nil { + return fmt.Errorf("%s: %w", tc.name, err) + } + } + return nil + }) +} diff --git a/tests/integration/s3_iam_utils.go b/tests/integration/s3_iam_utils.go index b56ae295..ee8b6979 100644 --- a/tests/integration/s3_iam_utils.go +++ b/tests/integration/s3_iam_utils.go @@ -227,14 +227,14 @@ func putBucketPolicyDoc(s *S3Conf, bucket string, statements ...bucketStatement) // the difference is Principal, which bucket policies require and identity // policies forbid. // -// Principal is matched against the caller's raw access key by this gateway -// (auth.Principals.Contains) — deliberately not against an ARN, for -// compatibility with the non-IAM backends that have no ARNs at all. A -// long-term user is therefore named by its AKIA… access key. An assumed-role -// session cannot be named at all: its ASIA… key is ephemeral, so -// auth.IAMService.ResolveAccounts rejects it outright rather than let a bucket -// policy come to reference a principal that stops existing. Session tests -// use "*" for that reason. +// Under the standalone IAM service — the only backend these groups run +// against — Principal names an AWS-style ARN, the way real S3 does +// (auth.Principals.matchFor): a user by its own ARN, a role by its role ARN, +// which covers every session of it, and one session by its +// arn:aws:sts::…:assumed-role// ARN. The account root ARN and +// the bare account id name the account, which delegates rather than grants. +// The gateway's other IAM backends have no ARNs at all and keep naming +// principals by access key id; those are the Access_Control tests, not these. type bucketStatement struct { Sid string `json:"Sid,omitempty"` Effect string `json:"Effect"` @@ -545,6 +545,39 @@ func newGitHubSession(root *iam.Client, s *S3Conf, rolePolicies map[string]strin return principal, cleanup, nil } +// anotherSessionOfRole mints a second, independent session of a role a test +// already holds one of — the fixture for the difference between a bucket +// policy naming a role, which covers every session of it, and one naming a +// single session, which covers only that one. +func anotherSessionOfRole(s *S3Conf, roleName string) (*s3IAMPrincipal, error) { + token, ok := gitHubOIDCToken() + if !ok { + return nil, fmt.Errorf("no GitHub OIDC token available") + } + + sessionName := "s3-sess-" + genRandString(8) + out, err := assumeRoleWithWebIdentitySessionPolicy(s, roleArnFor(roleName), sessionName, token, "") + if err != nil { + return nil, fmt.Errorf("AssumeRoleWithWebIdentity: %w", err) + } + + access := aws.ToString(out.Credentials.AccessKeyId) + secret := aws.ToString(out.Credentials.SecretAccessKey) + sessionToken := aws.ToString(out.Credentials.SessionToken) + + conf := *s + conf.awsID = access + conf.awsSecret = secret + + return &s3IAMPrincipal{ + name: roleName, + arn: aws.ToString(out.AssumedRoleUser.Arn), + conf: conf, + client: s3ClientWithSessionCreds(s, access, secret, sessionToken), + sessionToken: sessionToken, + }, nil +} + // assumeRoleWithWebIdentitySessionPolicy is assumeRoleWithWebIdentity with // the optional inline session-policy parameter, which no other test in this // package needs. @@ -569,6 +602,24 @@ func roleArnFor(roleName string) string { return "arn:aws:iam::" + testAccountID + ":role/" + roleName } +// userArnFor and assumedRoleArnFor build the remaining principal ARNs a +// bucket policy can name in this gateway's single fixed account. +// assumedRoleArnFor deliberately takes the role's plain name: unlike a role's +// own ARN, an assumed-role ARN never carries the role's IAM path. +func userArnFor(userName string) string { + return "arn:aws:iam::" + testAccountID + ":user/" + userName +} + +func assumedRoleArnFor(roleName, sessionName string) string { + return "arn:aws:sts::" + testAccountID + ":assumed-role/" + roleName + "/" + sessionName +} + +// accountArn is the principal naming the account itself — the delegating +// form, which grants nothing on its own but denies everything under Deny. +func accountArn() string { + return "arn:aws:iam::" + testAccountID + ":root" +} + // sessionNameFor recovers the session name from an assumed-role ARN, whose // last path element it is. func sessionNameFor(p *s3IAMPrincipal) string { diff --git a/webui/web/explorer.html b/webui/web/explorer.html index a2e19685..5309c72f 100644 --- a/webui/web/explorer.html +++ b/webui/web/explorer.html @@ -878,7 +878,9 @@ under the License.

About Bucket Policies

-

Bucket policies define who can access your bucket and what actions they can perform. The Principal is the access key of the user account, and the Resource uses standard bucket ARN format.

+

Bucket policies define who can access your bucket and what actions they can perform. The Resource uses standard bucket ARN format.

+

The Principal is a principal ARN — a user (arn:aws:iam::000000000000:user/alice), a role, which covers every session of it (arn:aws:iam::000000000000:role/reader), one assumed-role session (arn:aws:sts::000000000000:assumed-role/reader/session), or the account itself (arn:aws:iam::000000000000:root), which delegates to the account’s IAM rather than granting on its own. "*" names everyone. Wildcards are not allowed inside an ARN.

+

The Principal is the access key of the user account.

@@ -976,7 +978,8 @@ under the License.
  • Sid: Statement identifier (optional, but recommended)
  • Effect: "Allow" or "Deny"
    • -
  • Principal: Array of access keys (e.g., ["user001", "user002"])
    • +
  • Principal: Array of principal ARNs (e.g., ["arn:aws:iam::000000000000:user/alice"]), or "*"
    • +
  • Principal: Array of access keys (e.g., ["user001", "user002"])
  • Action: Array of S3 actions (e.g., ["s3:GetObject", "s3:PutObject"])
  • Resource: Array of ARNs (e.g., ["arn:aws:s3:::bucket/*"])
@@ -4205,16 +4208,20 @@ under the License. function loadExamplePolicy() { if (!currentPolicyBucket) return; + // The Principal form depends on the IAM backend behind the gateway: a + // standalone IAM service names principals by ARN, the gateway's own + // account store by access key. + const examplePrincipals = api.hasIAM() + ? ["arn:aws:iam::000000000000:user/user001", "arn:aws:iam::000000000000:user/user002"] + : ["user001", "user002"]; + const examplePolicy = { "Version": "2012-10-17", "Statement": [ { "Sid": "FullS3Access", "Effect": "Allow", - "Principal": [ - "user001", - "user002" - ], + "Principal": examplePrincipals, "Action": [ "s3:AbortMultipartUpload", "s3:DeleteObject",