policy: refuse condition variable suffixes only when saving

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)
 	}
 }
 

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")
 	}

policy/bucket-policy.go

@@ -165,6 +165,19 @@ func (policy BucketPolicy) Validate(bucketName string) error {
 	return nil
 }
 
+// CheckVariables refuses a condition key carrying a /<variable> 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

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())
 		}
 	}

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()
 }
 

policy/condition_variable_test.go

@@ -42,8 +42,8 @@ var conditionVariableCases = []struct {
 }
 
 // Only the tag keys take a /<variable> 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

policy/policy.go

@@ -541,6 +541,19 @@ func (iamp Policy) Validate() error {
 	return iamp.isValid()
 }
 
+// CheckVariables refuses a condition key carrying a /<variable> 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

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)
 	}