fix: support IAM policy handling for wildcard actions (#11530)

This PR fixes - allow 's3:versionid` as a valid conditional for Get,Put,Tags,Object locking APIs - allow additional headers missing for object APIs - allow wildcard based action matching
2021-02-12 23:05:09 -08:00 · 2021-02-12 23:05:09 -08:00 · a94a9c37fa
parent 79b6a43467
commit a94a9c37fa
6 changed files with 365 additions and 25 deletions
--- a/cmd/admin-router.go
+++ b/cmd/admin-router.go
@ -110,10 +110,9 @@ func registerAdminRouter(router *mux.Router, enableConfigOps, enableIAMOps bool)
 			// -- IAM APIs --

 			// Add policy IAM
-			adminRouter.Methods(http.MethodPut).Path(adminVersion+"/add-canned-policy").HandlerFunc(httpTraceHdrs(adminAPI.AddCannedPolicy)).Queries("name", "{name:.*}")
+			adminRouter.Methods(http.MethodPut).Path(adminVersion+"/add-canned-policy").HandlerFunc(httpTraceAll(adminAPI.AddCannedPolicy)).Queries("name", "{name:.*}")

 			// Add user IAM
-
 			adminRouter.Methods(http.MethodGet).Path(adminVersion + "/accountinfo").HandlerFunc(httpTraceAll(adminAPI.AccountInfoHandler))

 			adminRouter.Methods(http.MethodPut).Path(adminVersion+"/add-user").HandlerFunc(httpTraceHdrs(adminAPI.AddUser)).Queries("accessKey", "{accessKey:.*}")
--- a/pkg/bucket/policy/condition/key.go
+++ b/pkg/bucket/policy/condition/key.go
@ -124,6 +124,7 @@ var AllSupportedKeys = append([]Key{
 	S3Prefix,
 	S3Delimiter,
 	S3MaxKeys,
+	S3VersionID,
 	S3ObjectLockRemainingRetentionDays,
 	S3ObjectLockMode,
 	S3ObjectLockLegalHold,
@ -143,6 +144,8 @@ var AllSupportedKeys = append([]Key{

 // CommonKeys - is list of all common condition keys.
 var CommonKeys = append([]Key{
+	S3XAmzContentSha256,
+	S3LocationConstraint,
 	AWSReferer,
 	AWSSourceIP,
 	AWSUserAgent,
@ -152,7 +155,6 @@ var CommonKeys = append([]Key{
 	AWSPrincipalType,
 	AWSUserID,
 	AWSUsername,
-	S3XAmzContentSha256,
 	LDAPUser,
 }, JWTKeys...)

@ -241,6 +243,13 @@ func (set KeySet) Add(key Key) {
 	set[key] = struct{}{}
 }

+// Merge merges two key sets, duplicates are overwritten
+func (set KeySet) Merge(mset KeySet) {
+	for k, v := range mset {
+		set[k] = v
+	}
+}
+
 // Difference - returns a key set contains difference of two keys.
 // Example:
 //     keySet1 := ["one", "two", "three"]
--- a/pkg/iam/policy/action.go
+++ b/pkg/iam/policy/action.go
@ -266,23 +266,45 @@ var supportedObjectActions = map[Action]struct{}{

 // isObjectAction - returns whether action is object type or not.
 func (action Action) isObjectAction() bool {
-	_, ok := supportedObjectActions[action]
-	return ok
+	for supAction := range supportedObjectActions {
+		if action.Match(supAction) {
+			return true
+		}
+	}
+	return false
 }

-// Match - matches object name with resource pattern.
+// Match - matches action name with action patter.
 func (action Action) Match(a Action) bool {
 	return wildcard.Match(string(action), string(a))
 }

 // IsValid - checks if action is valid or not.
 func (action Action) IsValid() bool {
-	_, ok := supportedActions[action]
-	return ok
+	for supAction := range supportedActions {
+		if action.Match(supAction) {
+			return true
+		}
+	}
+	return false
 }

-// actionConditionKeyMap - holds mapping of supported condition key for an action.
-var actionConditionKeyMap = map[Action]condition.KeySet{
+type actionConditionKeyMap map[Action]condition.KeySet
+
+func (a actionConditionKeyMap) Lookup(action Action) (condition.KeySet, bool) {
+	var ckeysMerged = condition.KeySet{}
+	var found bool
+	for act, ckey := range a {
+		if action.Match(act) {
+			ckeysMerged.Merge(ckey)
+			found = true
+		}
+	}
+	return ckeysMerged, found
+}
+
+// iamActionConditionKeyMap - holds mapping of supported condition key for an action.
+var iamActionConditionKeyMap = actionConditionKeyMap{
 	AllActions: condition.NewKeySet(condition.AllSupportedKeys...),

 	AbortMultipartUploadAction: condition.NewKeySet(condition.CommonKeys...),
@ -291,8 +313,6 @@ var actionConditionKeyMap = map[Action]condition.KeySet{

 	DeleteBucketPolicyAction: condition.NewKeySet(condition.CommonKeys...),

-	DeleteObjectAction: condition.NewKeySet(condition.CommonKeys...),
-
 	GetBucketLocationAction: condition.NewKeySet(condition.CommonKeys...),

 	GetBucketNotificationAction: condition.NewKeySet(condition.CommonKeys...),
@ -303,6 +323,7 @@ var actionConditionKeyMap = map[Action]condition.KeySet{
 		append([]condition.Key{
 			condition.S3XAmzServerSideEncryption,
 			condition.S3XAmzServerSideEncryptionCustomerAlgorithm,
+			condition.S3VersionID,
 		}, condition.CommonKeys...)...),

 	HeadBucketAction: condition.NewKeySet(condition.CommonKeys...),
@ -335,6 +356,11 @@ var actionConditionKeyMap = map[Action]condition.KeySet{

 	PutBucketPolicyAction: condition.NewKeySet(condition.CommonKeys...),

+	DeleteObjectAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+
 	PutObjectAction: condition.NewKeySet(
 		append([]condition.Key{
 			condition.S3XAmzCopySource,
@ -342,6 +368,7 @@ var actionConditionKeyMap = map[Action]condition.KeySet{
 			condition.S3XAmzServerSideEncryptionCustomerAlgorithm,
 			condition.S3XAmzMetadataDirective,
 			condition.S3XAmzStorageClass,
+			condition.S3VersionID,
 			condition.S3ObjectLockRetainUntilDate,
 			condition.S3ObjectLockMode,
 			condition.S3ObjectLockLegalHold,
@ -351,21 +378,32 @@ var actionConditionKeyMap = map[Action]condition.KeySet{
 	// LockLegalHold is not supported with PutObjectRetentionAction
 	PutObjectRetentionAction: condition.NewKeySet(
 		append([]condition.Key{
+			condition.S3XAmzServerSideEncryption,
+			condition.S3XAmzServerSideEncryptionCustomerAlgorithm,
 			condition.S3ObjectLockRemainingRetentionDays,
 			condition.S3ObjectLockRetainUntilDate,
 			condition.S3ObjectLockMode,
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	GetObjectRetentionAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3XAmzServerSideEncryption,
+			condition.S3XAmzServerSideEncryptionCustomerAlgorithm,
+			condition.S3VersionID,
 		}, condition.CommonKeys...)...),
-
-	GetObjectRetentionAction: condition.NewKeySet(condition.CommonKeys...),
 	PutObjectLegalHoldAction: condition.NewKeySet(
 		append([]condition.Key{
+			condition.S3XAmzServerSideEncryption,
+			condition.S3XAmzServerSideEncryptionCustomerAlgorithm,
 			condition.S3ObjectLockLegalHold,
+			condition.S3VersionID,
 		}, condition.CommonKeys...)...),
 	GetObjectLegalHoldAction: condition.NewKeySet(condition.CommonKeys...),

 	// https://docs.aws.amazon.com/AmazonS3/latest/dev/list_amazons3.html
 	BypassGovernanceRetentionAction: condition.NewKeySet(
 		append([]condition.Key{
+			condition.S3VersionID,
 			condition.S3ObjectLockRemainingRetentionDays,
 			condition.S3ObjectLockRetainUntilDate,
 			condition.S3ObjectLockMode,
@ -376,11 +414,24 @@ var actionConditionKeyMap = map[Action]condition.KeySet{
 	PutBucketObjectLockConfigurationAction: condition.NewKeySet(condition.CommonKeys...),
 	GetBucketTaggingAction:                 condition.NewKeySet(condition.CommonKeys...),
 	PutBucketTaggingAction:                 condition.NewKeySet(condition.CommonKeys...),
-	PutObjectTaggingAction:                 condition.NewKeySet(condition.CommonKeys...),
-	GetObjectTaggingAction:                 condition.NewKeySet(condition.CommonKeys...),
-	DeleteObjectTaggingAction:              condition.NewKeySet(condition.CommonKeys...),

-	PutObjectVersionTaggingAction: condition.NewKeySet(condition.CommonKeys...),
+	PutObjectTaggingAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	GetObjectTaggingAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	DeleteObjectTaggingAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+
+	PutObjectVersionTaggingAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
 	GetObjectVersionAction: condition.NewKeySet(
 		append([]condition.Key{
 			condition.S3VersionID,
@ -397,10 +448,22 @@ var actionConditionKeyMap = map[Action]condition.KeySet{
 		append([]condition.Key{
 			condition.S3VersionID,
 		}, condition.CommonKeys...)...),
-	GetReplicationConfigurationAction:    condition.NewKeySet(condition.CommonKeys...),
-	PutReplicationConfigurationAction:    condition.NewKeySet(condition.CommonKeys...),
-	ReplicateObjectAction:                condition.NewKeySet(condition.CommonKeys...),
-	ReplicateDeleteAction:                condition.NewKeySet(condition.CommonKeys...),
-	ReplicateTagsAction:                  condition.NewKeySet(condition.CommonKeys...),
-	GetObjectVersionForReplicationAction: condition.NewKeySet(condition.CommonKeys...),
+	GetReplicationConfigurationAction: condition.NewKeySet(condition.CommonKeys...),
+	PutReplicationConfigurationAction: condition.NewKeySet(condition.CommonKeys...),
+	ReplicateObjectAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	ReplicateDeleteAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	ReplicateTagsAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
+	GetObjectVersionForReplicationAction: condition.NewKeySet(
+		append([]condition.Key{
+			condition.S3VersionID,
+		}, condition.CommonKeys...)...),
 }
--- a/pkg/iam/policy/actionset.go
+++ b/pkg/iam/policy/actionset.go
@ -43,6 +43,15 @@ func (actionSet ActionSet) Match(action Action) bool {
 		if r.Match(action) {
 			return true
 		}
+
+		// This is a special case where GetObjectVersion
+		// means GetObject is enabled implicitly.
+		switch r {
+		case GetObjectVersionAction:
+			if action == GetObjectAction {
+				return true
+			}
+		}
 	}

 	return false
--- a/pkg/iam/policy/policy_test.go
+++ b/pkg/iam/policy/policy_test.go
@ -20,7 +20,9 @@ import (
 	"encoding/json"
 	"net"
 	"reflect"
+	"strings"
 	"testing"
+	"time"

 	"github.com/minio/minio-go/v7/pkg/set"
 	"github.com/minio/minio/pkg/bucket/policy"
@ -432,6 +434,259 @@ func TestPolicyIsValid(t *testing.T) {
 	}
 }

+// Parse config with location constraints
+func TestPolicyParseConfig(t *testing.T) {
+	policy1LocationConstraint := `{
+   "Version":"2012-10-17",
+   "Statement":[
+      {
+         "Sid":"statement1",
+         "Effect":"Allow",
+         "Action": "s3:CreateBucket",
+         "Resource": "arn:aws:s3:::*",
+         "Condition": {
+             "StringLike": {
+                 "s3:LocationConstraint": "us-east-1"
+             }
+         }
+       },
+      {
+         "Sid":"statement2",
+         "Effect":"Deny",
+         "Action": "s3:CreateBucket",
+         "Resource": "arn:aws:s3:::*",
+         "Condition": {
+             "StringNotLike": {
+                 "s3:LocationConstraint": "us-east-1"
+             }
+         }
+       }
+    ]
+}`
+	policy2Condition := `{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "statement1",
+            "Effect": "Allow",
+            "Action": "s3:GetObjectVersion",
+            "Resource": "arn:aws:s3:::test/HappyFace.jpg"
+        },
+        {
+            "Sid": "statement2",
+            "Effect": "Deny",
+            "Action": "s3:GetObjectVersion",
+            "Resource": "arn:aws:s3:::test/HappyFace.jpg",
+            "Condition": {
+                "StringNotEquals": {
+                    "s3:versionid": "AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"
+                }
+            }
+        }
+    ]
+}`
+
+	policy3ConditionActionRegex := `{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "statement2",
+            "Effect": "Allow",
+            "Action": "s3:Get*",
+            "Resource": "arn:aws:s3:::test/HappyFace.jpg",
+            "Condition": {
+                "StringEquals": {
+                    "s3:versionid": "AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"
+                }
+            }
+        }
+    ]
+}`
+
+	policy4ConditionAction := `{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "statement2",
+            "Effect": "Allow",
+            "Action": "s3:GetObject",
+            "Resource": "arn:aws:s3:::test/HappyFace.jpg",
+            "Condition": {
+                "StringEquals": {
+                    "s3:versionid": "AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"
+                }
+            }
+        }
+    ]
+}`
+
+	policy5ConditionCurrenTime := `{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:Get*",
+    "s3:Put*"
+   ],
+   "Resource": [
+    "arn:aws:s3:::test/*"
+   ],
+   "Condition": {
+    "DateGreaterThan": {
+     "aws:CurrentTime": [
+      "2017-02-28T00:00:00Z"
+     ]
+    }
+   }
+  }
+ ]
+}`
+
+	policy5ConditionCurrenTimeLesser := `{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:Get*",
+    "s3:Put*"
+   ],
+   "Resource": [
+    "arn:aws:s3:::test/*"
+   ],
+   "Condition": {
+    "DateLessThan": {
+     "aws:CurrentTime": [
+      "2017-02-28T00:00:00Z"
+     ]
+    }
+   }
+  }
+ ]
+}`
+
+	tests := []struct {
+		p       string
+		args    Args
+		allowed bool
+	}{
+		{
+			p:       policy1LocationConstraint,
+			allowed: true,
+			args: Args{
+				AccountName:     "allowed",
+				Action:          CreateBucketAction,
+				BucketName:      "test",
+				ConditionValues: map[string][]string{"LocationConstraint": {"us-east-1"}},
+			},
+		},
+		{
+			p:       policy1LocationConstraint,
+			allowed: false,
+			args: Args{
+				AccountName:     "disallowed",
+				Action:          CreateBucketAction,
+				BucketName:      "test",
+				ConditionValues: map[string][]string{"LocationConstraint": {"us-east-2"}},
+			},
+		},
+		{
+			p:       policy2Condition,
+			allowed: true,
+			args: Args{
+				AccountName:     "allowed",
+				Action:          GetObjectAction,
+				BucketName:      "test",
+				ObjectName:      "HappyFace.jpg",
+				ConditionValues: map[string][]string{"versionid": {"AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"}},
+			},
+		},
+		{
+			p:       policy2Condition,
+			allowed: false,
+			args: Args{
+				AccountName:     "disallowed",
+				Action:          GetObjectAction,
+				BucketName:      "test",
+				ObjectName:      "HappyFace.jpg",
+				ConditionValues: map[string][]string{"versionid": {"AaaHbAQitwiL_h47_44lRO2DDfLlBO5f"}},
+			},
+		},
+		{
+			p:       policy3ConditionActionRegex,
+			allowed: true,
+			args: Args{
+				AccountName:     "allowed",
+				Action:          GetObjectAction,
+				BucketName:      "test",
+				ObjectName:      "HappyFace.jpg",
+				ConditionValues: map[string][]string{"versionid": {"AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"}},
+			},
+		},
+		{
+			p:       policy3ConditionActionRegex,
+			allowed: false,
+			args: Args{
+				AccountName:     "disallowed",
+				Action:          GetObjectAction,
+				BucketName:      "test",
+				ObjectName:      "HappyFace.jpg",
+				ConditionValues: map[string][]string{"versionid": {"AaaHbAQitwiL_h47_44lRO2DDfLlBO5f"}},
+			},
+		},
+		{
+			p:       policy4ConditionAction,
+			allowed: true,
+			args: Args{
+				AccountName:     "allowed",
+				Action:          GetObjectAction,
+				BucketName:      "test",
+				ObjectName:      "HappyFace.jpg",
+				ConditionValues: map[string][]string{"versionid": {"AaaHbAQitwiL_h47_44lRO2DDfLlBO5e"}},
+			},
+		},
+		{
+			p:       policy5ConditionCurrenTime,
+			allowed: true,
+			args: Args{
+				AccountName: "allowed",
+				Action:      GetObjectAction,
+				BucketName:  "test",
+				ObjectName:  "HappyFace.jpg",
+				ConditionValues: map[string][]string{
+					"CurrentTime": {time.Now().Format(time.RFC3339)},
+				},
+			},
+		},
+		{
+			p:       policy5ConditionCurrenTimeLesser,
+			allowed: false,
+			args: Args{
+				AccountName: "disallowed",
+				Action:      GetObjectAction,
+				BucketName:  "test",
+				ObjectName:  "HappyFace.jpg",
+				ConditionValues: map[string][]string{
+					"CurrentTime": {time.Now().Format(time.RFC3339)},
+				},
+			},
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.args.AccountName, func(t *testing.T) {
+			ip, err := ParseConfig(strings.NewReader(test.p))
+			if err != nil {
+				t.Error(err)
+			}
+			if got := ip.IsAllowed(test.args); got != test.allowed {
+				t.Errorf("Expected %t, got %t", test.allowed, got)
+			}
+		})
+	}
+}
+
 func TestPolicyUnmarshalJSONAndValidate(t *testing.T) {
 	case1Data := []byte(`{
    "ID": "MyPolicyForMyBucket1",
--- a/pkg/iam/policy/statement.go
+++ b/pkg/iam/policy/statement.go
@ -114,8 +114,13 @@ func (statement Statement) isValid() error {
 			return Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
 		}

+		condKeys, ok := iamActionConditionKeyMap.Lookup(action)
+		if !ok {
+			return Errorf("conditions are not supported for action %v", action)
+		}
+
 		keys := statement.Conditions.Keys()
-		keyDiff := keys.Difference(actionConditionKeyMap[action])
+		keyDiff := keys.Difference(condKeys)
 		if !keyDiff.IsEmpty() {
 			return Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
 		}