From 18e09ca2190aeebd402ec3c2cc175706e1ad4cc9 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sun, 8 Nov 2026 11:39:01 +0800 Subject: [PATCH] fix(batch): refuse a job only for a Deny that needs request context A batch job is authorized without a request, so a policy statement that depends on request context the job does not have (source address, transport, headers) was taken to refuse the action. Two parts of that rule refused jobs that no request would refuse: - The scan for unsupplied policy variables ran over the whole statement before its resources were matched. A Deny on an unrelated bucket whose condition value held a variable such as ${aws:UserAgent} refused every job of the user. Resources are now matched first; a variable in a resource still means the statement may apply. - An Allow that needs absent context, such as an IP-restricted read, refused the job even when another Allow granted the action. Such an Allow now grants nothing to the job: it is dropped from the policy that is evaluated, not merely exempted, since IfExists and Null conditions hold for an absent key and would grant. Dropping an Allow can only narrow the decision. A Deny that needs the context still refuses. The narrowed policy is a new policy.Policy, because the statement index of the original points into its own statements and the original may be a cached document. For a session policy, whether one applies and its first decision still come from the claims as before; only an allow is re-evaluated on the narrowed policy. Tests cover an unrelated Deny with a variable, a Deny with an unsupplied variable in its resource, an IP-restricted Allow next to a plain one, a Null Allow that plain evaluation without the address would grant, and the same for session policies. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/batch-authz-context_test.go | 118 +++++++++++++++++++++ cmd/batch-authz.go | 178 ++++++++++++++++++++++---------- 2 files changed, 243 insertions(+), 53 deletions(-) create mode 100644 cmd/batch-authz-context_test.go 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