From fdf132abd25b466bd3644a9a7dee20083faec884 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 28 Sep 2026 15:45:25 -0700 Subject: [PATCH] policy: refuse condition variable suffixes only when saving #276 ran the suffix check in isValid, so it also ran when a stored bucket policy was decoded and on every request that parses a session policy: a policy saved before #276 failed to load. Loading now accepts it and its suffixed conditions fail closed; CheckVariables refuses it before a save. A trailing slash round-trips instead of failing to parse. --- policy/admin_policy_name_test.go | 22 ++++-- policy/bucket-policy-statement.go | 4 - policy/bucket-policy.go | 13 ++++ policy/condition/func.go | 6 +- policy/condition/key.go | 20 +++-- policy/condition_variable_test.go | 123 +++++++++++++++++++++++------- policy/policy.go | 13 ++++ policy/statement.go | 4 - 8 files changed, 156 insertions(+), 49 deletions(-) diff --git a/policy/admin_policy_name_test.go b/policy/admin_policy_name_test.go index 874ac89..90c6fe0 100644 --- a/policy/admin_policy_name_test.go +++ b/policy/admin_policy_name_test.go @@ -223,23 +223,33 @@ func TestAdminPolicyNameNotAction(t *testing.T) { } // An admin key takes no /variable suffix: admin:PolicyName/x would read the -// value PolicyName/x, which the server never sets and a caller might. Keys that -// do take a variable keep parsing. +// value PolicyName/x, which the server never sets and a caller might. Such a +// policy loads but is refused before saving. Keys that do take a variable keep +// parsing and saving. func TestAdminPolicyNameRefusesVariableSuffix(t *testing.T) { for _, key := range []string{"admin:PolicyName/target", "admin:PolicyName/", "admin:PolicyName/a/b"} { doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": ["admin:CreatePolicy"], "Condition": {"StringLike": {"` + key + `": ["app-*"]}}}]}` - if _, err := ParseConfig(strings.NewReader(doc)); err == nil { - t.Errorf("condition key %q must be refused", key) + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Errorf("condition key %q must load: %v", key, err) + continue + } + if err := p.CheckVariables(); err == nil { + t.Errorf("condition key %q must be refused before saving", key) } } doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::bucket/*"], "Condition": {"StringEquals": {"s3:ExistingObjectTag/team": ["a"]}}}]}` - if _, err := ParseConfig(strings.NewReader(doc)); err != nil { - t.Errorf("a key that takes a variable must still parse: %v", err) + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("a key that takes a variable must still parse: %v", err) + } + if err := p.CheckVariables(); err != nil { + t.Errorf("a key that takes a variable must still be saved: %v", err) } } diff --git a/policy/bucket-policy-statement.go b/policy/bucket-policy-statement.go index 29a9758..40057fc 100644 --- a/policy/bucket-policy-statement.go +++ b/policy/bucket-policy-statement.go @@ -80,10 +80,6 @@ func (statement BPStatement) isValid() error { return Errorf("invalid Principal %v", statement.Principal) } - if err := statement.Conditions.CheckVariables(); err != nil { - return Errorf("%w", err) - } - if len(statement.Actions) == 0 && len(statement.NotActions) == 0 { return Errorf("Action must not be empty") } diff --git a/policy/bucket-policy.go b/policy/bucket-policy.go index fd76738..b468558 100644 --- a/policy/bucket-policy.go +++ b/policy/bucket-policy.go @@ -165,6 +165,19 @@ func (policy BucketPolicy) Validate(bucketName string) error { return nil } +// CheckVariables refuses a condition key carrying a / suffix its +// name does not take. Call it before saving a bucket policy: loading does not +// check, so a policy stored before this rule keeps loading, and its suffixed +// conditions fail closed. +func (policy BucketPolicy) CheckVariables() error { + for _, statement := range policy.Statements { + if err := statement.Conditions.CheckVariables(); err != nil { + return Errorf("%w", err) + } + } + return nil +} + // ParseBucketPolicyConfig - parses data in given reader to Policy. func ParseBucketPolicyConfig(reader io.Reader, bucketName string) (*BucketPolicy, error) { var policy BucketPolicy diff --git a/policy/condition/func.go b/policy/condition/func.go index 4e9b544..89f96cd 100644 --- a/policy/condition/func.go +++ b/policy/condition/func.go @@ -97,7 +97,11 @@ func (functions Functions) VariablesAllowed() bool { // take one. func (functions Functions) CheckVariables() error { for _, f := range functions { - if key := f.key(); !key.VariableAllowed() { + key := f.key() + if key.emptyVariable { + return fmt.Errorf("condition key '%v' ends in a slash that names no variable", key) + } + if !key.VariableAllowed() { return fmt.Errorf("condition key '%v' takes no variable; only %v do", key, variableKeyNames()) } } diff --git a/policy/condition/key.go b/policy/condition/key.go index 4bbd39c..08e8838 100644 --- a/policy/condition/key.go +++ b/policy/condition/key.go @@ -27,6 +27,10 @@ import ( type Key struct { name KeyName variable string + // emptyVariable marks a key written with a trailing slash, such as + // aws:username/. It names no variable, so saving refuses it, but a policy + // stored before that check must still load and marshal back unchanged. + emptyVariable bool } // IsValid - checks if key is valid or not. @@ -46,7 +50,7 @@ func (key Key) Is(name KeyName) bool { } func (key Key) String() string { - if key.variable != "" { + if key.variable != "" || key.emptyVariable { return string(key.name) + "/" + key.variable } return string(key.name) @@ -69,7 +73,7 @@ func (key Key) VarName() string { // Name - returns key name which is stripped value of prefixes "aws:" and "s3:" func (key Key) Name() string { name := key.name.Name() - if key.variable != "" { + if key.variable != "" || key.emptyVariable { return name + "/" + key.variable } return name @@ -99,13 +103,12 @@ func parseKey(s string) (Key, error) { } key := Key{ - name: KeyName(name), - variable: variable, + name: KeyName(name), + variable: variable, + emptyVariable: name != s && variable == "", } - // A trailing slash names no variable, so the key is malformed whatever - // its name. - if key.IsValid() && (name == s || variable != "") { + if key.IsValid() { return key, nil } @@ -116,6 +119,9 @@ func parseKey(s string) (Key, error) { // its name takes. A policy naming a key with a suffix it does not take is // refused, and such a key evaluates with no value. func (key Key) VariableAllowed() bool { + if key.emptyVariable { + return false + } return key.variable == "" || key.name.TakesVariable() } diff --git a/policy/condition_variable_test.go b/policy/condition_variable_test.go index c9c8d62..1aa37af 100644 --- a/policy/condition_variable_test.go +++ b/policy/condition_variable_test.go @@ -42,8 +42,8 @@ var conditionVariableCases = []struct { } // Only the tag keys take a / suffix. A policy putting one on any -// other key is refused with an error rather than parsed into a condition that -// reads a value no server sets. +// other key loads, so a stored one keeps working, but is refused before it is +// saved rather than saved as a condition that reads a value no server sets. func TestConditionVariableRefusedOnKeysThatTakeNone(t *testing.T) { for _, tc := range []struct { key, actions, resources string @@ -58,9 +58,14 @@ func TestConditionVariableRefusedOnKeysThatTakeNone(t *testing.T) { {"jwt:groups/x", `"s3:GetObject"`, `"arn:aws:s3:::bucket/*"`}, } { doc := `{"Version": "2012-10-17", "Statement": [` + conditionStatement(tc.actions, tc.resources, tc.key) + `]}` - _, err := ParseConfig(strings.NewReader(doc)) + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { + t.Errorf("%s must load: %v", tc.key, err) + continue + } + err = p.CheckVariables() if err == nil { - t.Errorf("%s must be refused", tc.key) + t.Errorf("%s must be refused before saving", tc.key) continue } if !strings.Contains(err.Error(), "takes no variable") { @@ -80,19 +85,48 @@ func TestConditionVariableAllowedOnTagKeys(t *testing.T) { {"s3tables:WarehouseTag/team", `"s3tables:GetTable"`, `"arn:aws:s3tables:::bucket/*"`}, } { doc := `{"Version": "2012-10-17", "Statement": [` + conditionStatement(tc.actions, tc.resources, tc.key) + `]}` - if _, err := ParseConfig(strings.NewReader(doc)); err != nil { + p, err := ParseConfig(strings.NewReader(doc)) + if err != nil { t.Errorf("%s must parse: %v", tc.key, err) + continue + } + if err := p.CheckVariables(); err != nil { + t.Errorf("%s must be saved: %v", tc.key, err) } } } -// An empty suffix names no variable, so the key is malformed whatever it is. +// An empty suffix names no variable, so the key is malformed whatever it is: +// it is refused before saving. A stored one still loads and marshals back with +// its slash, so a re-save or replication does not rewrite it into a different +// condition. func TestConditionVariableEmptyRefused(t *testing.T) { for _, key := range []string{"s3:ExistingObjectTag/", "aws:SourceIp/", "admin:PolicyName/"} { for _, c := range conditionVariableCases { doc := `{"Version": "2012-10-17", "Statement": [` + conditionStatement(c.actions, c.resources, key) + `]}` - if _, err := ParseConfig(strings.NewReader(doc)); err == nil { - t.Errorf("%s on %s must be refused", key, c.actions) + p, err := ParseConfig(strings.NewReader(doc)) + bare := `{"Version": "2012-10-17", "Statement": [` + conditionStatement(c.actions, c.resources, strings.TrimSuffix(key, "/")) + `]}` + if _, bareErr := ParseConfig(strings.NewReader(bare)); bareErr != nil { + // The key does not apply to these actions at all, so the + // policy is refused whatever its suffix. + if err == nil { + t.Errorf("%s on %s must be refused, as %s is", key, c.actions, strings.TrimSuffix(key, "/")) + } + continue + } + if err != nil { + t.Errorf("%s on %s must load: %v", key, c.actions, err) + continue + } + if err := p.CheckVariables(); err == nil || !strings.Contains(err.Error(), "names no variable") { + t.Errorf("%s on %s must be refused before saving, got %v", key, c.actions, err) + } + buf, err := json.Marshal(p) + if err != nil { + t.Fatalf("%s on %s must marshal: %v", key, c.actions, err) + } + if !strings.Contains(string(buf), `"`+key+`"`) { + t.Errorf("%s on %s lost its slash on marshal: %s", key, c.actions, buf) } } } @@ -105,38 +139,73 @@ func TestConditionVariableBucketPolicy(t *testing.T) { "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::bucket/*"], "Condition": {"StringEquals": {"` + key + `": ["v"]}}}]}` } - if _, err := ParseBucketPolicyConfig(strings.NewReader(doc("aws:SourceIp/x")), "bucket"); err == nil || - !strings.Contains(err.Error(), "takes no variable") { - t.Errorf("aws:SourceIp/x must be refused in a bucket policy, got %v", err) + bp, err := ParseBucketPolicyConfig(strings.NewReader(doc("aws:SourceIp/x")), "bucket") + if err != nil { + t.Fatalf("a stored bucket policy with aws:SourceIp/x must load: %v", err) } - if _, err := ParseBucketPolicyConfig(strings.NewReader(doc("s3:ExistingObjectTag/team")), "bucket"); err != nil { - t.Errorf("s3:ExistingObjectTag/team must parse in a bucket policy: %v", err) + if err := bp.CheckVariables(); err == nil || !strings.Contains(err.Error(), "takes no variable") { + t.Errorf("aws:SourceIp/x must be refused before saving a bucket policy, got %v", err) + } + var decoded BucketPolicy + if err := json.Unmarshal([]byte(doc("aws:SourceIp/")), &decoded); err != nil { + t.Errorf("a stored bucket policy with aws:SourceIp/ must decode: %v", err) + } + bp, err = ParseBucketPolicyConfig(strings.NewReader(doc("s3:ExistingObjectTag/team")), "bucket") + if err != nil { + t.Fatalf("s3:ExistingObjectTag/team must parse in a bucket policy: %v", err) + } + if err := bp.CheckVariables(); err != nil { + t.Errorf("s3:ExistingObjectTag/team must be saved in a bucket policy: %v", err) } } -// A policy stored before this rule still loads: loading decodes without -// validating, and refusing it there would break IAM on upgrade. Its suffixed -// condition reads no value, so a caller who supplies aws:username/x cannot -// satisfy it. -func TestConditionVariableStoredPolicyFailsClosed(t *testing.T) { - doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": ["s3:GetObject"], - "Resource": ["arn:aws:s3:::bucket/*"], - "Condition": {"StringEquals": {"aws:username/x": ["mallory"]}}}]}` - var p Policy - if err := json.Unmarshal([]byte(doc), &p); err != nil { - t.Fatalf("a stored policy must still load: %v", err) +// A stored bucket policy's suffixed condition reads no value, so an anonymous +// caller who supplies SourceIp/x cannot satisfy it. +func TestConditionVariableStoredBucketPolicyFailsClosed(t *testing.T) { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Principal": {"AWS": ["*"]}, + "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::bucket/*"], + "Condition": {"StringEquals": {"aws:SourceIp/x": ["v"]}}}]}` + bp, err := ParseBucketPolicyConfig(strings.NewReader(doc), "bucket") + if err != nil { + t.Fatalf("a stored bucket policy must load: %v", err) } - if p.IsAllowed(Args{ - AccountName: "mallory", + if bp.IsAllowed(BucketPolicyArgs{ Action: GetObjectAction, BucketName: "bucket", ObjectName: "o", - ConditionValues: map[string][]string{"username/x": {"mallory"}}, + ConditionValues: map[string][]string{"SourceIp/x": {"v"}}, }) { t.Error("a caller-supplied value must not satisfy a suffix the key does not take") } } +// A policy stored before this rule still loads: loading decodes without +// validating, and refusing it there would break IAM on upgrade. Its suffixed +// condition reads no value, so a caller who supplies aws:username/x cannot +// satisfy it. +func TestConditionVariableStoredPolicyFailsClosed(t *testing.T) { + for _, key := range []string{"aws:username/x", "aws:username/"} { + doc := `{"Version": "2012-10-17", "Statement": [{"Effect": "Allow", "Action": ["s3:GetObject"], + "Resource": ["arn:aws:s3:::bucket/*"], + "Condition": {"StringEquals": {"` + key + `": ["mallory"]}}}]}` + var p Policy + if err := json.Unmarshal([]byte(doc), &p); err != nil { + t.Fatalf("a stored policy with %s must still load: %v", key, err) + } + if p.IsAllowed(Args{ + AccountName: "mallory", + Action: GetObjectAction, + BucketName: "bucket", + ObjectName: "o", + ConditionValues: map[string][]string{ + "username/x": {"mallory"}, "username/": {"mallory"}, "username": {"mallory"}, + }, + }) { + t.Errorf("%s: a caller-supplied value must not satisfy a suffix the key does not take", key) + } + } +} + // A condition on a suffix its key does not take can be neither checked nor // satisfied, whatever the operator: an Allow carrying one never grants, and a // Deny carrying one always applies. A negated operator reads an absent value as diff --git a/policy/policy.go b/policy/policy.go index 28268ca..52c9ca5 100644 --- a/policy/policy.go +++ b/policy/policy.go @@ -541,6 +541,19 @@ func (iamp Policy) Validate() error { return iamp.isValid() } +// CheckVariables refuses a condition key carrying a / suffix its +// name does not take. Call it before saving a policy: loading does not check, +// so a policy stored before this rule keeps loading, and its suffixed +// conditions fail closed. +func (iamp Policy) CheckVariables() error { + for _, statement := range iamp.Statements { + if err := statement.Conditions.CheckVariables(); err != nil { + return Errorf("%w", err) + } + } + return nil +} + // ValidateStrict applies strict validation rules suitable for new // policy creation. It rejects policies that would be accepted by // Validate for backward compatibility but are invalid going forward diff --git a/policy/statement.go b/policy/statement.go index d8a2b5f..664955c 100644 --- a/policy/statement.go +++ b/policy/statement.go @@ -334,10 +334,6 @@ func (statement Statement) isValid() error { return err } - if err := statement.Conditions.CheckVariables(); err != nil { - return Errorf("%w", err) - } - if statement.isAdmin() { return statement.validateAdmin(false) }