diff --git a/cmd/batch-authz-context_test.go b/cmd/batch-authz-context_test.go new file mode 100644 index 000000000..2e3cb7390 --- /dev/null +++ b/cmd/batch-authz-context_test.go @@ -0,0 +1,118 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "strings" + "testing" + + "github.com/minio/minio/internal/auth" + "github.com/pgsty/silo-pkg/v3/policy" +) + +// batchContextRead is the object read the tests below authorize. +func batchContextRead(f *batchAuthzFixture) batchAuthzRequest { + return batchAuthzRequest{action: policy.GetObjectAction, bucket: f.bucket, object: "data/a"} +} + +// batchContextUser creates a user with statements as its only policy and +// returns its submitter binding. +func batchContextUser(t *testing.T, f *batchAuthzFixture, name, statements string) (auth.Credentials, *batchJobSubmitter) { + t.Helper() + batchAuthzPolicy(f.ctx, t, f.sys, name, statements) + cred := iamAdminUser(f.ctx, t, f.sys, name+"-user", name) + sub, err := newBatchJobSubmitter(cred) + mustIAM(t, err) + return cred, sub +} + +func isBatchContextRefusal(err error) bool { + return isBatchJobAuthzError(err) && strings.Contains(err.Error(), "request context") +} + +// A statement whose resources rule the object out does not apply, whatever +// its conditions need, policy variables in condition values included. One +// whose resources match still refuses. +func TestBatchAuthzContextResourceFirst(t *testing.T) { + f := newBatchAuthzFixture(t) + const unrelated = `{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::other-bucket/*"],"Condition":{"StringEquals":{"s3:ExistingObjectTag/owner":"${aws:UserAgent}"}}}` + _, sub := batchContextUser(t, f, "context-unrelated", batchAuthzFullAccess+",\n"+unrelated) + if err := sub.authorize(f.ctx, batchContextRead(f)); err != nil { + t.Errorf("a Deny on another bucket refused an allowed read: %v", err) + } + + related := strings.Replace(unrelated, "other-bucket", f.bucket, 1) + _, sub = batchContextUser(t, f, "context-related", batchAuthzFullAccess+",\n"+related) + if err := sub.authorize(f.ctx, batchContextRead(f)); !isBatchContextRefusal(err) { + t.Errorf("a Deny on the object with an unsupplied variable must refuse, got %v", err) + } + + // A variable in the resource leaves open whether it matches. + const resourceVar = `{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/${aws:UserAgent}/*"]}` + _, sub = batchContextUser(t, f, "context-resource-var", batchAuthzFullAccess+",\n"+resourceVar) + if err := sub.authorize(f.ctx, batchContextRead(f)); !isBatchContextRefusal(err) { + t.Errorf("a Deny whose resource holds an unsupplied variable must refuse, got %v", err) + } +} + +// An Allow that needs request context the job does not have grants nothing +// to the job; it neither refuses the job nor grants by an absent key. +func TestBatchAuthzContextAllowDropped(t *testing.T) { + f := newBatchAuthzFixture(t) + const ipAllow = `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::*"],"Condition":{"IpAddress":{"aws:SourceIp":["10.0.0.0/8"]}}}` + _, sub := batchContextUser(t, f, "context-ip-allow", batchAuthzFullAccess+",\n"+ipAllow) + if err := sub.authorize(f.ctx, batchContextRead(f)); err != nil { + t.Errorf("a conditional Allow refused a read another Allow grants: %v", err) + } + + // A Null condition holds for an absent key: evaluated without the + // context, this Allow would grant what no request with an address gets. + const nullAllow = `{"Effect":"Allow","Action":["admin:StartBatchJob"]}, +{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"],"Condition":{"Null":{"aws:SourceIp":"true"}}}` + cred, sub := batchContextUser(t, f, "context-null-allow", nullAllow) + if !f.sys.isAllowedLocally(policy.Args{AccountName: cred.AccessKey, Action: policy.GetObjectAction, BucketName: f.bucket, ObjectName: "data/a", ConditionValues: map[string][]string{}}) { + t.Fatal("control: without the address the Null Allow grants") + } + err := sub.authorize(f.ctx, batchContextRead(f)) + if !isBatchJobAuthzError(err) || isBatchContextRefusal(err) { + t.Errorf("an Allow that needs the address must grant nothing, and deny rather than refuse; got %v", err) + } +} + +// The same holds for the Allows of a session policy, which only narrows. +func TestBatchAuthzContextSessionAllow(t *testing.T) { + f := newBatchAuthzFixture(t) + batchAuthzPolicy(f.ctx, t, f.sys, "context-session-parent", batchAuthzFullAccess) + parent := iamAdminUser(f.ctx, t, f.sys, "context-session-parent", "context-session-parent") + for _, tc := range []struct { + name, statements string + allowed bool + }{ + {"null-only", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"],"Condition":{"Null":{"aws:SourceIp":"true"}}}`, false}, + {"with-plain", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"],"Condition":{"Null":{"aws:SourceIp":"true"}}}, +{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/data/*"]}`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + sp, err := policy.ParseConfig(strings.NewReader(`{"Version":"2012-10-17","Statement":[` + tc.statements + `]}`)) + mustIAM(t, err) + created, _, err := f.sys.NewServiceAccount(f.ctx, parent.AccessKey, nil, newServiceAccountOpts{sessionPolicy: sp}) + mustIAM(t, err) + stored, ok := f.sys.GetUser(f.ctx, created.AccessKey) + if !ok { + t.Fatal("service account missing") + } + sub, err := newBatchJobSubmitter(stored.Credentials) + mustIAM(t, err) + err = sub.authorize(f.ctx, batchContextRead(f)) + switch { + case isBatchContextRefusal(err): + t.Errorf("a session-policy Allow refused the job: %v", err) + case tc.allowed && err != nil: + t.Errorf("the plain session Allow must still grant: %v", err) + case !tc.allowed && !isBatchJobAuthzError(err): + t.Errorf("a session policy whose only Allow needs the address must deny, got %v", err) + } + }) + } +} diff --git a/cmd/batch-authz.go b/cmd/batch-authz.go index 59c74262d..0ce940212 100644 --- a/cmd/batch-authz.go +++ b/cmd/batch-authz.go @@ -37,9 +37,10 @@ import ( // expired, revoked or edited since submission fails the job; a reused // access key does not match the binding. // - Each action is evaluated with the submitter's policies and session -// policy but without a request. A policy statement that would need +// policy but without a request. A Deny that may apply and would need // request context the job does not have (source address, transport, -// headers and so on) refuses the action instead of being skipped. +// headers and so on) refuses the action instead of being skipped; an +// Allow that would need it grants nothing to the job. // // A job without a binding was queued by an earlier release. It is never // authorized, whoever submitted it, and has to be submitted again. @@ -474,22 +475,36 @@ func (p batchJobPrincipal) authorizeArgs(args policy.Args, req *batchAuthzReques snap = globalIAMSys.store.batchSnapshot(ctx) defer snap.release() } - policies, err := globalIAMSys.batchLocalPolicies(p, args, plugin == nil, snap) + mapped, session, err := globalIAMSys.batchLocalPolicies(p, args, plugin == nil, snap) if err != nil { if errors.Is(err, errNoSuchPolicy) || errors.Is(err, errAuthentication) || errors.Is(err, errNoSuchUser) || errors.Is(err, errNoSuchGroup) { return denied } return batchUnavailable("IAM policy lookup", err) } + // sessionNarrowed: the session policy lost an Allow below, so the + // narrowed policy decides in its place. + sessionNarrowed := false if req != nil { supplied := req.suppliedConditionKeys() - for _, pol := range policies { - for _, st := range pol.Statements { - if batchStatementNeedsRequest(st, args, supplied) { - return batchJobAuthzError{reason: denied.reason + ": a policy statement depends on request context that a batch job does not have"} - } - } + var refuse bool + if mapped, _, refuse = batchContextFree(mapped, args, supplied); !refuse { + session, sessionNarrowed, refuse = batchContextFree(session, args, supplied) } + if refuse { + return batchJobAuthzError{reason: denied.reason + ": a policy statement depends on request context that a batch job does not have"} + } + } + sessionDenies := func() bool { + has, allowed := p.sessionPolicyDecision(args) + if has && allowed && sessionNarrowed { + // As sessionPolicyDecision evaluates it, without its dropped + // Allows. This can only turn an allow into a deny. + spArgs := args + spArgs.IsOwner, spArgs.DenyOnly = false, false + allowed = session.IsAllowed(spArgs) + } + return has && !allowed } if plugin != nil { ok, err := plugin.IsAllowedContext(ctx, args) @@ -500,7 +515,7 @@ func (p batchJobPrincipal) authorizeArgs(args policy.Args, req *batchAuthzReques return denied } // The plugin does not see a session policy; it only limits further. - if has, allowed := p.sessionPolicyDecision(args); has && !allowed { + if sessionDenies() { return denied } return nil @@ -516,7 +531,7 @@ func (p batchJobPrincipal) authorizeArgs(args policy.Args, req *batchAuthzReques return denied } } - if has, allowed := p.sessionPolicyDecision(args); has && !allowed { + if sessionDenies() { return denied } ownerDerived := (p.temp || p.service) && p.parentUser == globalActiveCred.AccessKey @@ -524,13 +539,50 @@ func (p batchJobPrincipal) authorizeArgs(args policy.Args, req *batchAuthzReques if p.service { args.AccountName = p.parentUser } - if len(policies) == 0 || !policies[0].IsAllowed(args) { + if mapped == nil || !mapped.IsAllowed(args) { return denied } } return nil } +// batchContextFree returns pol without the Allow statements that may apply +// to args but need request context the job does not supply (see +// batchStatementNeedsRequest), and whether it dropped any. Such an Allow +// grants nothing to the job. It is dropped rather than evaluated, because +// evaluated without the context it could grant what a request would not: +// IfExists and Null conditions hold for an absent key. Dropping an Allow can +// only narrow the decision. Dropping a Deny that needs the context could +// allow what a request with that context would not, so refuse reports one, +// and the job refuses the action. +func batchContextFree(pol *policy.Policy, args policy.Args, supplied map[string]bool) (out *policy.Policy, dropped, refuse bool) { + if pol == nil { + return nil, false, false + } + var kept []policy.Statement + for i, st := range pol.Statements { + if !batchStatementNeedsRequest(st, args, supplied) { + if dropped { + kept = append(kept, st) + } + continue + } + if st.Effect != policy.Allow { + return pol, false, true + } + if !dropped { + kept = append(make([]policy.Statement, 0, len(pol.Statements)-1), pol.Statements[:i]...) + dropped = true + } + } + if !dropped { + return pol, false, false + } + // A new policy: pol's statement index refers to pol's own statements, and + // pol may be a cached policy document. + return &policy.Policy{ID: pol.ID, Version: pol.Version, Statements: kept}, true, false +} + func batchDeniedReason(req batchAuthzRequest) string { resource := req.bucket if req.object != "" { @@ -636,13 +688,13 @@ func (s *batchPolicySnapshot) loadPolicyDoc(name string, out map[string]PolicyDo } // batchLocalPolicies returns every local policy that can take part in the -// decision on args: the mapped policies that IsAllowed would merge when -// withMapped is set (no authorization plugin), and the session policy. -func (sys *IAMSys) batchLocalPolicies(p batchJobPrincipal, args policy.Args, withMapped bool, snap *batchPolicySnapshot) ([]policy.Policy, error) { - var out []policy.Policy +// decision on args: mapped, the mapped policies merged as IsAllowed would +// merge them, when withMapped is set (no authorization plugin), and session, +// the session policy. Each is nil when it takes no part. +func (sys *IAMSys) batchLocalPolicies(p batchJobPrincipal, args policy.Args, withMapped bool, snap *batchPolicySnapshot) (mapped, session *policy.Policy, err error) { if withMapped && args.IsOwner { // IsAllowed does not evaluate any policy for the owner. - return nil, nil + return nil, nil, nil } if withMapped { // The caller holds snap through evaluation. Lazy mapping loads and @@ -664,28 +716,28 @@ func (sys *IAMSys) batchLocalPolicies(p batchJobPrincipal, args policy.Args, wit if roleArn := args.GetRoleArn(); roleArn != "" { a, err := arn.Parse(roleArn) if err != nil { - return nil, err + return nil, nil, err } names = append(names, newMappedPolicy(sys.rolesMap[a]).toSlice()...) } else { - mapped, err := mappedPolicies(p.parentUser) + parent, err := mappedPolicies(p.parentUser) if err != nil { - return nil, err + return nil, nil, err } - names = append(names, mapped...) + names = append(names, parent...) // The claimed policies apply only when nothing is mapped. - if len(mapped) == 0 { + if len(parent) == 0 { if claimed, ok := policy.GetPoliciesFromClaims(args.Claims, iamPolicyClaimNameOpenID()); ok { names = append(names, claimed.ToSlice()...) } } } default: - mapped, err := mappedPolicies(p.accessKey) + own, err := mappedPolicies(p.accessKey) if err != nil { - return nil, err + return nil, nil, err } - names = append(names, mapped...) + names = append(names, own...) } var selected []policy.Policy for _, name := range names { @@ -700,63 +752,56 @@ func (sys *IAMSys) batchLocalPolicies(p batchJobPrincipal, args policy.Args, wit continue } if err != nil { - return nil, err + return nil, nil, err } doc, ok = loaded[name] if !ok { - return nil, fmt.Errorf("load IAM policy %q returned no document", name) + return nil, nil, fmt.Errorf("load IAM policy %q returned no document", name) } } selected = append(selected, doc.Policy) } - out = append(out, policy.MergePolicies(selected...)) + merged := policy.MergePolicies(selected...) + mapped = &merged } if p.temp || p.service { if p.service && p.claims[iamPolicyClaimNameSA()] == inheritedPolicyType { - return out, nil + return mapped, nil, nil } if sp, ok := p.claims[sessionPolicyNameExtracted]; ok { s, ok := sp.(string) if !ok { - return nil, errAuthentication + return nil, nil, errAuthentication } parsed, err := policy.ParseConfig(strings.NewReader(s)) if err != nil { - return nil, err + return nil, nil, err } - out = append(out, *parsed) + session = parsed } } - return out, nil + return mapped, session, nil } -// batchStatementNeedsRequest reports whether a statement that applies to -// args depends on a condition key that the job does not supply. Applying +// batchStatementNeedsRequest reports whether a statement may apply to args +// and depends on request context that the job does not supply: a condition +// key, or a policy variable in a resource or a condition value. Applying // such a statement without its value could grant (Allow) or drop (Deny) -// what a request with that context would not, so the job refuses the action. -// Resources are matched without conditions; a statement is taken to apply -// whenever it might. +// what a request with that context would not (see batchContextFree). +// +// Resources are matched first and without conditions; a statement is taken +// to apply whenever it might. A statement whose resources rule args out +// does not apply, whatever its conditions need. One whose resources hold a +// variable the job does not supply might match, so it may apply. func batchStatementNeedsRequest(st policy.Statement, args policy.Args, supplied map[string]bool) bool { if (!st.Actions.Match(args.Action) && !st.Actions.IsEmpty()) || st.NotActions.Match(args.Action) { return false } - // Policy variables in resources or condition values. - if data, err := json.Marshal(st); err == nil { - s := string(data) - for { - i := strings.Index(s, "${") - if i < 0 { - break - } - s = s[i+2:] - j := strings.IndexByte(s, '}') - if j < 0 { - break - } - if !batchKeySupplied(supplied, s[:j]) { + for _, resources := range []policy.ResourceSet{st.Resources, st.NotResources} { + for r := range resources { + if batchUnsuppliedVariable(r.Pattern, supplied) { return true } - s = s[j+1:] } } if !batchStatementMayApply(st, args) { @@ -767,9 +812,36 @@ func batchStatementNeedsRequest(st policy.Statement, args policy.Args, supplied return true } } + // Policy variables in condition values. + if len(st.Conditions) > 0 { + data, err := json.Marshal(st.Conditions) + if err != nil || batchUnsuppliedVariable(string(data), supplied) { + return true + } + } return false } +// batchUnsuppliedVariable reports whether s holds a policy variable, ${key}, +// whose value the job does not supply. +func batchUnsuppliedVariable(s string, supplied map[string]bool) bool { + for { + i := strings.Index(s, "${") + if i < 0 { + return false + } + s = s[i+2:] + j := strings.IndexByte(s, '}') + if j < 0 { + return false + } + if !batchKeySupplied(supplied, s[:j]) { + return true + } + s = s[j+1:] + } +} + // batchStatementMayApply matches the resources of a statement, in both forms // a bucket request can take. A statement whose resources the evaluator // translates (S3 Tables and vectors actions cover S3 data actions) is taken