From 99fb47fbb9dcec5510db07d534a2f7947cad9592 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 11 Nov 2026 12:46:36 +0800 Subject: [PATCH] fix(batch): align variable preflight with policy evaluation Treat unsupported variable spellings as literals, inspect only condition values, and decode BinaryEquals before checking required context. Keep missing request variables fail-closed. Fixes #347 Signed-off-by: Feng Ruohang --- cmd/batch-authz-variables_test.go | 152 ++++++++++++++++++++++++++++++ cmd/batch-authz.go | 48 +++++++++- 2 files changed, 196 insertions(+), 4 deletions(-) create mode 100644 cmd/batch-authz-variables_test.go diff --git a/cmd/batch-authz-variables_test.go b/cmd/batch-authz-variables_test.go new file mode 100644 index 000000000..6d82f3d75 --- /dev/null +++ b/cmd/batch-authz-variables_test.go @@ -0,0 +1,152 @@ +// Copyright (c) 2026 Feng Ruohang +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "encoding/base64" + "fmt" + "maps" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/pgsty/silo-pkg/v3/policy" +) + +// Use explicit condition values to exercise the evaluator, including literal +// spellings that S3 tag validation does not permit in stored tags. +func TestBatchPolicyConditionVariableParity(t *testing.T) { + f := newBatchAuthzFixture(t) + cases := []struct { + value, actual string + matches bool + requestOnly bool + }{ + {"${$}100", "${$}100", true, false}, + {"${$}100", "$100", false, false}, + {"${*}", "${*}", true, false}, + {"${?}", "${?}", true, false}, + {"${custom:unknown}", "${custom:unknown}", true, false}, + {"${aws:Unknown}", "${aws:Unknown}", true, false}, + {"${aws:UserAgent", "${aws:UserAgent", true, false}, + {"${aws:username}", "", true, false}, + {"${aws:UserAgent}", "test-agent", true, true}, + {"${prefix${aws:UserAgent}}", "${prefixtest-agent}", true, true}, + } + for i, tc := range cases { + for opIndex, op := range []string{ + "StringEquals", "StringNotEquals", "StringEqualsIgnoreCase", "StringNotEqualsIgnoreCase", + "StringLike", "StringNotLike", "ForAnyValue:StringEquals", "ForAllValues:StringEquals", "BinaryEquals", + } { + for _, effect := range []string{"Allow", "Deny"} { + t.Run(fmt.Sprintf("%d/%s/%s", i, op, effect), func(t *testing.T) { + name := fmt.Sprintf("var-%d-%d-%s", i, opIndex, effect) + conditionValue := tc.value + if op == "BinaryEquals" { + conditionValue = base64.StdEncoding.EncodeToString([]byte(conditionValue)) + } + statement := fmt.Sprintf(`{"Effect":%q,"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/data/*"],"Condition":{%q:{"s3:ExistingObjectTag/label":%q}}}`, effect, op, conditionValue) + if effect == "Deny" { + statement += `,{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/data/*"]}` + } + cred, sub := batchContextUser(t, f, name, statement) + actual := tc.actual + if tc.value == "${aws:username}" { + actual = cred.AccessKey + } + values := map[string][]string{"ExistingObjectTag/label": {actual}} + ordinaryValues := maps.Clone(values) + ordinaryValues["username"] = []string{cred.AccessKey} + ordinaryValues["UserAgent"] = []string{"test-agent"} + ordinaryAllowed := f.sys.isAllowedLocally(policy.Args{ + AccountName: cred.AccessKey, + Action: policy.GetObjectAction, + BucketName: f.bucket, + ObjectName: "data/" + name, + ConditionValues: ordinaryValues, + }) + matches := tc.matches + if strings.Contains(op, "Not") { + matches = !matches + } + if want := matches == (effect == "Allow"); ordinaryAllowed != want { + t.Fatalf("ordinary evaluator allowed=%v want %v", ordinaryAllowed, want) + } + batchErr := sub.authorize(f.ctx, batchAuthzRequest{ + action: policy.GetObjectAction, bucket: f.bucket, object: "data/" + name, values: values, + }) + if tc.requestOnly { + if !isBatchJobAuthzError(batchErr) { + t.Fatalf("missing request context must deny: %v", batchErr) + } + if (effect == "Deny") != isBatchContextRefusal(batchErr) { + t.Fatalf("only a Deny must refuse missing context: %v", batchErr) + } + return + } + if (batchErr == nil) != ordinaryAllowed { + t.Fatalf("ordinary allowed=%v; batch=%v", ordinaryAllowed, batchErr) + } + }) + } + } + } + + // Condition keys themselves are never expanded. + const statement = `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"],"Condition":{"StringEquals":{"s3:ExistingObjectTag/${aws:UserAgent}":"literal"}}}` + cred, sub := batchContextUser(t, f, "literal-condition-key", statement) + values := map[string][]string{"ExistingObjectTag/${aws:UserAgent}": {"literal"}} + if !f.sys.isAllowedLocally(policy.Args{AccountName: cred.AccessKey, Action: policy.GetObjectAction, BucketName: f.bucket, ObjectName: "data/key", ConditionValues: values}) { + t.Fatal("ordinary evaluator did not match the literal condition key") + } + if err := sub.authorize(f.ctx, batchAuthzRequest{action: policy.GetObjectAction, bucket: f.bucket, object: "data/key", values: values}); err != nil { + t.Fatalf("batch treated a condition key as a variable: %v", err) + } +} + +// Object names permit these spellings, so also compare real S3 GETs with batch. +func TestBatchPolicyResourceVariableParity(t *testing.T) { + f := newBatchAuthzFixture(t) + router := initTestAPIEndPoints(f.obj, []string{"GetObject"}) + for i, raw := range []string{"${$}100", "${*}", "${?}", "${custom:unknown}", "${prefix${aws:UserAgent}}", "${aws:username}", "${aws:UserAgent}"} { + for _, effect := range []string{"Allow", "Deny"} { + t.Run(fmt.Sprintf("%d/%s", i, effect), func(t *testing.T) { + name := fmt.Sprintf("res-%d-%s", i, effect) + statement := fmt.Sprintf(`{"Effect":%q,"Action":["s3:GetObject"],"Resource":[%q]}`, effect, "arn:aws:s3:::batch-authz/"+raw) + if effect == "Deny" { + statement += `,{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::batch-authz/*"]}` + } + cred, sub := batchContextUser(t, f, name, statement) + object := strings.ReplaceAll(raw, "${aws:username}", cred.AccessKey) + if raw == "${aws:UserAgent}" { + object = "test-agent" + } + _, err := f.obj.PutObject(f.ctx, f.bucket, object, mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{}) + mustIAM(t, err) + req, err := newTestSignedRequestV4(http.MethodGet, getGetObjectURL("", f.bucket, object), 0, nil, cred.AccessKey, cred.SecretKey, map[string]string{"User-Agent": "test-agent"}) + mustIAM(t, err) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + wantStatus := http.StatusOK + if effect == "Deny" { + wantStatus = http.StatusForbidden + } + if rec.Code != wantStatus { + t.Fatalf("S3 read: %d want %d: %s", rec.Code, wantStatus, rec.Body.String()) + } + err = sub.authorize(f.ctx, batchAuthzRequest{action: policy.GetObjectAction, bucket: f.bucket, object: object}) + if raw == "${aws:UserAgent}" { + if !isBatchJobAuthzError(err) || (effect == "Deny") != isBatchContextRefusal(err) { + t.Fatalf("batch did not reject missing request context: %v", err) + } + return + } + if (err == nil) != (rec.Code == http.StatusOK) { + t.Fatalf("resource %q %s: S3=%d batch=%v", raw, effect, rec.Code, err) + } + }) + } + } +} diff --git a/cmd/batch-authz.go b/cmd/batch-authz.go index 3e30e2936..59fe0e1b8 100644 --- a/cmd/batch-authz.go +++ b/cmd/batch-authz.go @@ -829,15 +829,15 @@ func batchStatementNeedsRequest(st policy.Statement, args policy.Args, supplied // Policy variables in condition values. if len(st.Conditions) > 0 { data, err := json.Marshal(st.Conditions) - if err != nil || batchUnsuppliedVariable(string(data), supplied) { + if err != nil || batchConditionsNeedVariables(data, supplied) { return true } } return false } -// batchUnsuppliedVariable reports whether s holds a policy variable, ${key}, -// whose value the job does not supply. +// batchUnsuppliedVariable follows Resource.Match: only CommonKeys are expanded, +// and an unsupported ${...} sequence (including its nested text) stays literal. func batchUnsuppliedVariable(s string, supplied map[string]bool) bool { for { i := strings.Index(s, "${") @@ -849,13 +849,53 @@ func batchUnsuppliedVariable(s string, supplied map[string]bool) bool { if j < 0 { return false } - if !batchKeySupplied(supplied, s[:j]) { + if condition.CommonKeysMap[condition.KeyName(s[:j])] && !batchKeySupplied(supplied, s[:j]) { return true } s = s[j+1:] } } +// Conditions replace each recognized variable wherever it occurs in a string +// value, unlike the resource parser. Inspect values only, not condition keys, +// and decode BinaryEquals as the policy evaluator does before substitution. +func batchConditionsNeedVariables(data []byte, supplied map[string]bool) bool { + var conditions map[string]map[string]condition.ValueSet + if err := json.Unmarshal(data, &conditions); err != nil { + return true + } + for name, keys := range conditions { + name = strings.TrimPrefix(strings.TrimPrefix(name, "ForAnyValue:"), "ForAllValues:") + name = strings.TrimSuffix(name, "IfExists") + switch name { + case "StringEquals", "StringNotEquals", "StringEqualsIgnoreCase", "StringNotEqualsIgnoreCase", "StringLike", "StringNotLike", "BinaryEquals": + default: + continue + } + for _, values := range keys { + for value := range values { + s, err := value.GetString() + if err != nil { + return true + } + if name == "BinaryEquals" { + decoded, err := base64.StdEncoding.DecodeString(s) + if err != nil { + return true + } + s = string(decoded) + } + for _, key := range condition.CommonKeys { + if strings.Contains(s, key.VarName()) && !batchKeySupplied(supplied, string(key)) { + return true + } + } + } + } + } + return false +} + // 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