From 5db533c0243428c216418be366cc605682767289 Mon Sep 17 00:00:00 2001 From: Aaron Walker Date: Sat, 5 Aug 2017 04:00:05 -0400 Subject: [PATCH] bucket-policy: Add IPAddress/NotIPAddress conditions support (#4736) --- cmd/auth-handler.go | 3 +- cmd/bucket-handlers.go | 4 +- cmd/bucket-policy-handlers.go | 47 ++++++++++++++++++ cmd/bucket-policy-handlers_test.go | 30 +++++++++++- cmd/bucket-policy-parser.go | 4 +- cmd/bucket-policy-parser_test.go | 78 +++++++++++++++++++++++++++++- cmd/object-handlers.go | 27 +++++++++-- cmd/object-handlers_test.go | 53 ++++++++++++++++++++ docs/bucket/policy/README.md | 3 ++ 9 files changed, 239 insertions(+), 10 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 85fce51a6..ae4a27975 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -126,8 +126,9 @@ func checkRequestAuthType(r *http.Request, bucket, policyAction, region string) if reqAuthType == authTypeAnonymous && policyAction != "" { // http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html + sourceIP := getSourceIPAddress(r) return enforceBucketPolicy(bucket, policyAction, r.URL.Path, - r.Referer(), r.URL.Query()) + r.Referer(), sourceIP, r.URL.Query()) } // By default return ErrAccessDenied diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index e6945331c..4400f4ce6 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -33,7 +33,7 @@ import ( // http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html // Enforces bucket policies for a bucket for a given tatusaction. -func enforceBucketPolicy(bucket, action, resource, referer string, queryParams url.Values) (s3Error APIErrorCode) { +func enforceBucketPolicy(bucket, action, resource, referer, sourceIP string, queryParams url.Values) (s3Error APIErrorCode) { // Verify if bucket actually exists if err := checkBucketExist(bucket, newObjectLayerFn()); err != nil { err = errorCause(err) @@ -73,6 +73,8 @@ func enforceBucketPolicy(bucket, action, resource, referer string, queryParams u if referer != "" { conditionKeyMap["referer"] = set.CreateStringSet(referer) } + // Add request source Ip to conditionKeyMap. + conditionKeyMap["ip"] = set.CreateStringSet(sourceIP) // Validate action, resource and conditions with current policy statements. if !bucketPolicyEvalStatements(action, arn, conditionKeyMap, policy.Statements) { diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index d68e2537f..66593e55f 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -20,6 +20,7 @@ import ( "fmt" "io" "io/ioutil" + "net" "net/http" "runtime" "strings" @@ -81,6 +82,18 @@ func refererMatch(pattern, referer string) bool { return wildcard.MatchSimple(pattern, referer) } +// isIPInCIDR - checks if a given a IP address is a member of the given subnet. +func isIPInCIDR(cidr, ip string) bool { + // AWS S3 spec says IPs must use standard CIDR notation. + // http://docs.aws.amazon.com/AmazonS3/latest/dev/example-bucket-policies.html#example-bucket-policies-use-case-3. + _, cidrNet, err := net.ParseCIDR(cidr) + if err != nil { + return false // If provided CIDR can't be parsed no IP will be in the subnet. + } + addr := net.ParseIP(ip) + return cidrNet.Contains(addr) +} + // Verify if given resource matches with policy statement. func bucketPolicyResourceMatch(resource string, statement policyStatement) bool { // the resource rule for object could contain "*" wild card. @@ -97,11 +110,14 @@ func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement p // - StringNotEquals // - StringLike // - StringNotLike + // - IpAddress + // - NotIpAddress // // Supported applicable condition keys for each conditions. // - s3:prefix // - s3:max-keys // - s3:aws-Referer + // - s3:aws-SourceIp // The following loop evaluates the logical AND of all the // conditions in the statement. Note: we can break out of the @@ -159,6 +175,37 @@ func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement p return false } } + } else if condition == "IpAddress" { + awsIps := conditionKeyVal["aws:SourceIp"] + // Skip empty condition, it is trivially satisfied. + if awsIps.IsEmpty() { + continue + } + // wildcard match of ip if statement was not empty. + // Find a valid ip. + ipFound := false + for ip := range conditions["ip"] { + if !awsIps.FuncMatch(isIPInCIDR, ip).IsEmpty() { + ipFound = true + break + } + } + if !ipFound { + return false + } + } else if condition == "NotIpAddress" { + awsIps := conditionKeyVal["aws:SourceIp"] + // Skip empty condition, it is trivially satisfied. + if awsIps.IsEmpty() { + continue + } + // wildcard match of ip if statement was not empty. + // Find if nothing matches. + for ip := range conditions["ip"] { + if !awsIps.FuncMatch(isIPInCIDR, ip).IsEmpty() { + return false + } + } } } diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go index 5a625c2f4..6a607818d 100644 --- a/cmd/bucket-policy-handlers_test.go +++ b/cmd/bucket-policy-handlers_test.go @@ -843,7 +843,7 @@ func testDeleteBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName str // TestBucketPolicyConditionMatch - Tests to validate whether bucket policy conditions match. func TestBucketPolicyConditionMatch(t *testing.T) { - // obtain the inner map[string]set.StringSet for policyStatement.Conditions . + // obtain the inner map[string]set.StringSet for policyStatement.Conditions. getInnerMap := func(key2, value string) map[string]set.StringSet { innerMap := make(map[string]set.StringSet) innerMap[key2] = set.CreateStringSet(value) @@ -970,6 +970,34 @@ func TestBucketPolicyConditionMatch(t *testing.T) { condition: getInnerMap("referer", "http://somethingelse.com/"), expectedMatch: true, }, + // Test case 13. + // IpAddress condition evaluates to true. + { + statementCondition: getStatementWithCondition("IpAddress", "aws:SourceIp", "54.240.143.0/24"), + condition: getInnerMap("ip", "54.240.143.2"), + expectedMatch: true, + }, + // Test case 14. + // IpAddress condition evaluates to false. + { + statementCondition: getStatementWithCondition("IpAddress", "aws:SourceIp", "54.240.143.0/24"), + condition: getInnerMap("ip", "127.240.143.224"), + expectedMatch: false, + }, + // Test case 15. + // NotIpAddress condition evaluates to true. + { + statementCondition: getStatementWithCondition("NotIpAddress", "aws:SourceIp", "54.240.143.0/24"), + condition: getInnerMap("ip", "54.240.144.188"), + expectedMatch: true, + }, + // Test case 16. + // NotIpAddress condition evaluates to false. + { + statementCondition: getStatementWithCondition("NotIpAddress", "aws:SourceIp", "54.240.143.0/24"), + condition: getInnerMap("ip", "54.240.143.243"), + expectedMatch: false, + }, } for i, tc := range testCases { diff --git a/cmd/bucket-policy-parser.go b/cmd/bucket-policy-parser.go index 5d98b8690..084e9aeae 100644 --- a/cmd/bucket-policy-parser.go +++ b/cmd/bucket-policy-parser.go @@ -40,11 +40,11 @@ var supportedActionMap = set.CreateStringSet("*", "s3:*", "s3:GetObject", "s3:AbortMultipartUpload", "s3:ListBucketMultipartUploads", "s3:ListMultipartUploadParts") // supported Conditions type. -var supportedConditionsType = set.CreateStringSet("StringEquals", "StringNotEquals", "StringLike", "StringNotLike") +var supportedConditionsType = set.CreateStringSet("StringEquals", "StringNotEquals", "StringLike", "StringNotLike", "IpAddress", "NotIpAddress") // Validate s3:prefix, s3:max-keys are present if not // supported keys for the conditions. -var supportedConditionsKey = set.CreateStringSet("s3:prefix", "s3:max-keys", "aws:Referer") +var supportedConditionsKey = set.CreateStringSet("s3:prefix", "s3:max-keys", "aws:Referer", "aws:SourceIp") // supportedEffectMap - supported effects. var supportedEffectMap = set.CreateStringSet("Allow", "Deny") diff --git a/cmd/bucket-policy-parser_test.go b/cmd/bucket-policy-parser_test.go index a712a30c4..d89479c64 100644 --- a/cmd/bucket-policy-parser_test.go +++ b/cmd/bucket-policy-parser_test.go @@ -374,6 +374,12 @@ func TestIsValidConditions(t *testing.T) { // returns map with the "StringNotLike" set to empty map. setEmptyStringNotLike := getEmptyConditionKeyMap("StringNotLike") + // returns map with the "IpAddress" set to empty map. + setEmptyIPAddress := getEmptyConditionKeyMap("IpAddress") + + // returns map with "NotIpAddress" set to empty map. + setEmptyNotIPAddress := getEmptyConditionKeyMap("NotIpAddress") + // Generate conditions. generateConditions := func(key1, key2, value string) map[string]map[string]set.StringSet { innerMap := make(map[string]set.StringSet) @@ -427,6 +433,8 @@ func TestIsValidConditions(t *testing.T) { setEmptyStringNotEquals(), setEmptyStringLike(), setEmptyStringNotLike(), + setEmptyIPAddress(), + setEmptyNotIPAddress(), generateConditions("StringEquals", "s3:prefix", "Asia/"), generateConditions("StringEquals", "s3:max-keys", "100"), generateConditions("StringNotEquals", "s3:prefix", "Asia/"), @@ -482,7 +490,13 @@ func TestIsValidConditions(t *testing.T) { // Test case - 12. {roBucketActionSet, testConditions[11], nil, true}, // Test case - 13. - {getObjectActionSet, testConditions[11], maxKeysConditionErr, false}, + {roBucketActionSet, testConditions[12], nil, true}, + // Test case - 11. + {roBucketActionSet, testConditions[13], nil, true}, + // Test case - 12. + {roBucketActionSet, testConditions[14], nil, true}, + // Test case - 13. + {getObjectActionSet, testConditions[15], maxKeysConditionErr, false}, } for i, testCase := range testCases { actualErr := isValidConditions(testCase.inputActions, testCase.inputCondition) @@ -787,3 +801,65 @@ func TestAWSRefererCondition(t *testing.T) { } } } + +func TestAWSSourceIPCondition(t *testing.T) { + resource := set.CreateStringSet([]string{ + fmt.Sprintf("%s%s", bucketARNPrefix, "minio-bucket"+"/"+"Asia"+"*"), + }...) + + conditionsKeyMap := make(policy.ConditionKeyMap) + // Test both IPv4 and IPv6 addresses. + conditionsKeyMap.Add("aws:SourceIp", + set.CreateStringSet("54.240.143.0/24", + "2001:DB8:1234:5678::/64")) + + requestConditionKeyMap := make(map[string]set.StringSet) + requestConditionKeyMap["ip"] = set.CreateStringSet("54.240.143.2") + + testCases := []struct { + effect string + conditionKey string + match bool + }{ + { + effect: "Allow", + conditionKey: "IpAddress", + match: true, + }, + { + effect: "Allow", + conditionKey: "NotIpAddress", + match: false, + }, + { + effect: "Deny", + conditionKey: "IpAddress", + match: true, + }, + { + effect: "Deny", + conditionKey: "NotIpAddress", + match: false, + }, + } + + for i, test := range testCases { + conditions := make(map[string]map[string]set.StringSet) + conditions[test.conditionKey] = conditionsKeyMap + + allowStatement := policyStatement{ + Sid: "Testing AWS referer condition", + Effect: test.effect, + Principal: map[string]interface{}{ + "AWS": "*", + }, + Resources: resource, + Conditions: conditions, + } + + if result := bucketPolicyConditionMatch(requestConditionKeyMap, allowStatement); result != test.match { + t.Errorf("Test %d - Expected conditons to evaluate to %v but got %v", + i+1, test.match, result) + } + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 1da5b0dd7..c5130ed8b 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -48,16 +48,33 @@ func setGetRespHeaders(w http.ResponseWriter, reqParams url.Values) { } } +// getSourceIPAddress - get the source ip address of the request. +func getSourceIPAddress(r *http.Request) string { + var ip string + // Attempt to get ip from standard headers. + // Do not support X-Forwarded-For because it is easy to spoof. + ip = r.Header.Get("X-Real-Ip") + parsedIP := net.ParseIP(ip) + // Skip non valid IP address. + if parsedIP != nil { + return ip + } + // Default to remote address if headers not set. + ip, _, _ = net.SplitHostPort(r.RemoteAddr) + return ip +} + // errAllowableNotFound - For an anon user, return 404 if have ListBucket, 403 otherwise // this is in keeping with the permissions sections of the docs of both: // HEAD Object: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectHEAD.html // GET Object: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectGET.html func errAllowableObjectNotFound(bucket string, r *http.Request) APIErrorCode { if getRequestAuthType(r) == authTypeAnonymous { - //we care about the bucket as a whole, not a particular resource + // We care about the bucket as a whole, not a particular resource. resource := "/" + bucket + sourceIP := getSourceIPAddress(r) if s3Error := enforceBucketPolicy(bucket, "s3:ListBucket", resource, - r.Referer(), r.URL.Query()); s3Error != ErrNone { + r.Referer(), sourceIP, r.URL.Query()); s3Error != ErrNone { return ErrAccessDenied } } @@ -505,8 +522,9 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req return case authTypeAnonymous: // http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html + sourceIP := getSourceIPAddress(r) if s3Error := enforceBucketPolicy(bucket, "s3:PutObject", r.URL.Path, - r.Referer(), r.URL.Query()); s3Error != ErrNone { + r.Referer(), sourceIP, r.URL.Query()); s3Error != ErrNone { writeErrorResponse(w, s3Error, r.URL) return } @@ -791,8 +809,9 @@ func (api objectAPIHandlers) PutObjectPartHandler(w http.ResponseWriter, r *http return case authTypeAnonymous: // http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuAndPermissions.html + sourceIP := getSourceIPAddress(r) if s3Error := enforceBucketPolicy(bucket, "s3:PutObject", r.URL.Path, - r.Referer(), r.URL.Query()); s3Error != ErrNone { + r.Referer(), sourceIP, r.URL.Query()); s3Error != ErrNone { writeErrorResponse(w, s3Error, r.URL) return } diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index 624329a00..6be58a585 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -3552,3 +3552,56 @@ func testAPIListObjectPartsHandler(obj ObjectLayer, instanceType, bucketName str // `ExecObjectLayerAPINilTest` sets the Object Layer to `nil` and calls the handler. ExecObjectLayerAPINilTest(t, nilBucket, nilObject, instanceType, apiRouter, nilReq) } + +// TestGetSourceIPAddress - check the source ip of a request is parsed correctly. +func TestGetSourceIPAddress(t *testing.T) { + testCases := []struct { + request *http.Request + expectedIP string + }{ + { + // Test Case 1. Use only RemoteAddr as host and port. + request: &http.Request{ + RemoteAddr: "127.0.0.1:9000", + }, + expectedIP: "127.0.0.1", + }, + { + // Test Case 2. Use both RemoteAddr and single header. + request: &http.Request{ + RemoteAddr: "127.0.0.1:9000", + Header: map[string][]string{ + "X-Real-Ip": {"54.240.143.0"}, + }, + }, + expectedIP: "54.240.143.0", // Use headers before RemoteAddr. + }, + { + // Test Case 3. Use both RemoteAddr and several header vals. + // Check that first val in header is used. + request: &http.Request{ + RemoteAddr: "127.0.0.1:9000", + Header: map[string][]string{ + "X-Real-Ip": {"54.240.143.0", "54.240.143.188"}, + }, + }, + expectedIP: "54.240.143.0", + }, + { + // Test Case 4. Use header and corrupt header value. + request: &http.Request{ + RemoteAddr: "127.0.0.1:9000", + Header: map[string][]string{ + "X-Real-Ip": {"54.240.143.188", "corrupt"}, + }, + }, + expectedIP: "54.240.143.188", + }, + } + for i, test := range testCases { + receivedIP := getSourceIPAddress(test.request) + if test.expectedIP != receivedIP { + t.Fatalf("Case %d: Expected the IP to be `%s`, but instead got `%s`", i+1, test.expectedIP, receivedIP) + } + } +} diff --git a/docs/bucket/policy/README.md b/docs/bucket/policy/README.md index a738ff2c9..abb1c98bf 100644 --- a/docs/bucket/policy/README.md +++ b/docs/bucket/policy/README.md @@ -24,12 +24,15 @@ This package implements parsing and validating bucket access policies based on A StringNotEquals StringLike StringNotLike + IpAddress + NotIpAddress Supported applicable condition keys for each conditions. s3:prefix s3:max-keys aws:Referer + aws:SourceIp ### Nested policy support.