[go: nahoru, domu]
Skip to content

Commit

Permalink
api/bucketPolicy: Use minio-go/pkg/set and fix bucket policy regressi…
Browse files Browse the repository at this point in the history 
…on. (minio#2506)

Current master has a regression 'mc policy <policy-type> alias/bucket/prefix'
does not work anymore, due to the way new minio-go changes do json marshalling.
This led to a regression on server side when a ``prefix`` is provided
policy is rejected as malformed from th server which is not the case with
AWS S3.

This patch uses the new ``minio-go/pkg/set`` package to address the
unmarshalling problems.

Fixes minio#2503
  • Loading branch information
@harshavardhana
harshavardhana authored Aug 20, 2016
1 parent a3c509f commit 975eb31
Show file tree
Hide file tree
Showing 9 changed files with 553 additions and 186 deletions.
7 changes: 4 additions & 3 deletions cmd/bucket-handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"strings"

mux "github.com/gorilla/mux"
"github.com/minio/minio-go/pkg/set"
)

// http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html
Expand All @@ -43,13 +44,13 @@ func enforceBucketPolicy(bucket string, action string, reqURL *url.URL) (s3Error
resource := AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/")

// Get conditions for policy verification.
conditions := make(map[string]string)
conditionKeyMap := make(map[string]set.StringSet)
for queryParam := range reqURL.Query() {
conditions[queryParam] = reqURL.Query().Get(queryParam)
conditionKeyMap[queryParam] = set.CreateStringSet(reqURL.Query().Get(queryParam))
}

// Validate action, resource and conditions with current policy statements.
if !bucketPolicyEvalStatements(action, resource, conditions, policy.Statements) {
if !bucketPolicyEvalStatements(action, resource, conditionKeyMap, policy.Statements) {
return ErrAccessDenied
}
return ErrNone
Expand Down
39 changes: 15 additions & 24 deletions cmd/bucket-policy-handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"net/http"

mux "github.com/gorilla/mux"
"github.com/minio/minio-go/pkg/set"
"github.com/minio/minio/pkg/wildcard"
)

Expand All @@ -32,7 +33,7 @@ const maxAccessPolicySize = 20 * 1024 // 20KiB.

// Verify if a given action is valid for the url path based on the
// existing bucket access policy.
func bucketPolicyEvalStatements(action string, resource string, conditions map[string]string, statements []policyStatement) bool {
func bucketPolicyEvalStatements(action string, resource string, conditions map[string]set.StringSet, statements []policyStatement) bool {
for _, statement := range statements {
if bucketPolicyMatchStatement(action, resource, conditions, statement) {
if statement.Effect == "Allow" {
Expand All @@ -48,7 +49,7 @@ func bucketPolicyEvalStatements(action string, resource string, conditions map[s
}

// Verify if action, resource and conditions match input policy statement.
func bucketPolicyMatchStatement(action string, resource string, conditions map[string]string, statement policyStatement) bool {
func bucketPolicyMatchStatement(action string, resource string, conditions map[string]set.StringSet, statement policyStatement) bool {
// Verify if action matches.
if bucketPolicyActionMatch(action, statement) {
// Verify if resource matches.
Expand All @@ -64,12 +65,7 @@ func bucketPolicyMatchStatement(action string, resource string, conditions map[s

// Verify if given action matches with policy statement.
func bucketPolicyActionMatch(action string, statement policyStatement) bool {
for _, policyAction := range statement.Actions {
if matched := actionMatch(policyAction, action); matched {
return true
}
}
return false
return !statement.Actions.FuncMatch(actionMatch, action).IsEmpty()
}

// Match function matches wild cards in 'pattern' for resource.
Expand All @@ -84,20 +80,15 @@ func actionMatch(pattern, action string) bool {

// Verify if given resource matches with policy statement.
func bucketPolicyResourceMatch(resource string, statement policyStatement) bool {
for _, resourcep := range statement.Resources {
// the resource rule for object could contain "*" wild card.
// the requested object can be given access based on the already set bucket policy if
// the match is successful.
// More info: http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-arn-format.html .
if matched := resourceMatch(resourcep, resource); matched {
return true
}
}
return false
// the resource rule for object could contain "*" wild card.
// the requested object can be given access based on the already set bucket policy if
// the match is successful.
// More info: http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-arn-format.html.
return !statement.Resources.FuncMatch(resourceMatch, resource).IsEmpty()
}

// Verify if given condition matches with policy statement.
func bucketPolicyConditionMatch(conditions map[string]string, statement policyStatement) bool {
func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement policyStatement) bool {
// Supports following conditions.
// - StringEquals
// - StringNotEquals
Expand All @@ -106,22 +97,22 @@ func bucketPolicyConditionMatch(conditions map[string]string, statement policySt
// - s3:prefix
// - s3:max-keys
var conditionMatches = true
for condition, conditionKeys := range statement.Conditions {
for condition, conditionKeyVal := range statement.Conditions {
if condition == "StringEquals" {
if conditionKeys["s3:prefix"] != conditions["prefix"] {
if !conditionKeyVal["s3:prefix"].Equals(conditions["prefix"]) {
conditionMatches = false
break
}
if conditionKeys["s3:max-keys"] != conditions["max-keys"] {
if !conditionKeyVal["s3:max-keys"].Equals(conditions["max-keys"]) {
conditionMatches = false
break
}
} else if condition == "StringNotEquals" {
if conditionKeys["s3:prefix"] == conditions["prefix"] {
if !conditionKeyVal["s3:prefix"].Equals(conditions["prefix"]) {
conditionMatches = false
break
}
if conditionKeys["s3:max-keys"] == conditions["max-keys"] {
if !conditionKeyVal["s3:max-keys"].Equals(conditions["max-keys"]) {
conditionMatches = false
break
}
Expand Down
6 changes: 4 additions & 2 deletions cmd/bucket-policy-handlers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"net/http"
"net/http/httptest"
"testing"

"github.com/minio/minio-go/pkg/set"
)

// Tests validate Bucket policy resource matcher.
Expand All @@ -31,7 +33,7 @@ func TestBucketPolicyResourceMatch(t *testing.T) {
// generates statement with given resource..
generateStatement := func(resource string) policyStatement {
statement := policyStatement{}
statement.Resources = []string{resource}
statement.Resources = set.CreateStringSet([]string{resource}...)
return statement
}

Expand Down Expand Up @@ -336,7 +338,7 @@ func testGetBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrH
credentials := serverConfig.GetCredential()

// template for constructing HTTP request body for PUT bucket policy.
bucketPolicyTemplate := `{"Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::%s"]},{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/this*"]}]}`
bucketPolicyTemplate := `{"Version":"2012-10-17","Statement":[{"Action":["s3:GetBucketLocation","s3:ListBucket"],"Effect":"Allow","Principal":{"AWS":["*"]},"Resource":["arn:aws:s3:::%s"],"Sid":""},{"Action":["s3:GetObject"],"Effect":"Allow","Principal":{"AWS":["*"]},"Resource":["arn:aws:s3:::%s/this*"],"Sid":""}]}`

// Writing bucket policy before running test on GetBucketPolicy.
putTestPolicies := []struct {
Expand Down
125 changes: 47 additions & 78 deletions cmd/bucket-policy-parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"path"
"sort"
"strings"

"github.com/minio/minio-go/pkg/set"
)

const (
Expand All @@ -34,50 +36,39 @@ const (
)

// supportedActionMap - lists all the actions supported by minio.
var supportedActionMap = map[string]struct{}{
"*": {},
"s3:*": {},
"s3:GetObject": {},
"s3:ListBucket": {},
"s3:PutObject": {},
"s3:GetBucketLocation": {},
"s3:DeleteObject": {},
"s3:AbortMultipartUpload": {},
"s3:ListBucketMultipartUploads": {},
"s3:ListMultipartUploadParts": {},
}
var supportedActionMap = set.CreateStringSet("*", "*", "s3:*", "s3:GetObject",
"s3:ListBucket", "s3:PutObject", "s3:GetBucketLocation", "s3:DeleteObject",
"s3:AbortMultipartUpload", "s3:ListBucketMultipartUploads", "s3:ListMultipartUploadParts")

// supported Conditions type.
var supportedConditionsType = map[string]struct{}{
"StringEquals": {},
"StringNotEquals": {},
}
var supportedConditionsType = set.CreateStringSet("StringEquals", "StringNotEquals")

// Validate s3:prefix, s3:max-keys are present if not
// supported keys for the conditions.
var supportedConditionsKey = map[string]struct{}{
"s3:prefix": {},
"s3:max-keys": {},
}
var supportedConditionsKey = set.CreateStringSet("s3:prefix", "s3:max-keys")

// supportedEffectMap - supported effects.
var supportedEffectMap = set.CreateStringSet("Allow", "Deny")

// User - canonical users list.
// policyUser - canonical users list.
type policyUser struct {
AWS []string
AWS set.StringSet `json:"AWS,omitempty"`
CanonicalUser set.StringSet `json:"CanonicalUser,omitempty"`
}

// Statement - minio policy statement
type policyStatement struct {
Sid string
Actions set.StringSet `json:"Action"`
Conditions map[string]map[string]set.StringSet `json:"Condition,omitempty"`
Effect string
Principal policyUser `json:"Principal"`
Actions []string `json:"Action"`
Resources []string `json:"Resource"`
Conditions map[string]map[string]string `json:"Condition,omitempty"`
Principal policyUser `json:"Principal"`
Resources set.StringSet `json:"Resource"`
Sid string
}

// bucketPolicy - collection of various bucket policy statements.
type bucketPolicy struct {
Version string // date in 0000-00-00 format
Version string // date in YYYY-MM-DD format
Statements []policyStatement `json:"Statement"`
}

Expand All @@ -91,51 +82,42 @@ func (b bucketPolicy) String() string {
return string(bbytes)
}

// supportedEffectMap - supported effects.
var supportedEffectMap = map[string]struct{}{
"Allow": {},
"Deny": {},
}

// isValidActions - are actions valid.
func isValidActions(actions []string) (err error) {
func isValidActions(actions set.StringSet) (err error) {
// Statement actions cannot be empty.
if len(actions) == 0 {
err = errors.New("Action list cannot be empty.")
return err
}
for _, action := range actions {
if _, ok := supportedActionMap[action]; !ok {
err = errors.New("Unsupported action found: ‘" + action + "’, please validate your policy document.")
return err
}
if unsupportedActions := actions.Difference(supportedActionMap); !unsupportedActions.IsEmpty() {
err = fmt.Errorf("Unsupported actions found: ‘%#v’, please validate your policy document.", unsupportedActions)
return err
}
return nil
}

// isValidEffect - is effect valid.
func isValidEffect(effect string) error {
func isValidEffect(effect string) (err error) {
// Statement effect cannot be empty.
if len(effect) == 0 {
err := errors.New("Policy effect cannot be empty.")
if effect == "" {
err = errors.New("Policy effect cannot be empty.")
return err
}
_, ok := supportedEffectMap[effect]
if !ok {
err := errors.New("Unsupported Effect found: ‘" + effect + "’, please validate your policy document.")
if !supportedEffectMap.Contains(effect) {
err = errors.New("Unsupported Effect found: ‘" + effect + "’, please validate your policy document.")
return err
}
return nil
}

// isValidResources - are valid resources.
func isValidResources(resources []string) (err error) {
func isValidResources(resources set.StringSet) (err error) {
// Statement resources cannot be empty.
if len(resources) == 0 {
err = errors.New("Resource list cannot be empty.")
return err
}
for _, resource := range resources {
for resource := range resources {
if !strings.HasPrefix(resource, AWSResourcePrefix) {
err = errors.New("Unsupported resource style found: ‘" + resource + "’, please validate your policy document.")
return err
Expand All @@ -150,63 +132,50 @@ func isValidResources(resources []string) (err error) {
}

// isValidPrincipals - are valid principals.
func isValidPrincipals(principals []string) (err error) {
func isValidPrincipals(principals set.StringSet) (err error) {
// Statement principal should have a value.
if len(principals) == 0 {
err = errors.New("Principal cannot be empty.")
return err
}
for _, principal := range principals {
if unsuppPrincipals := principals.Difference(set.CreateStringSet([]string{"*"}...)); !unsuppPrincipals.IsEmpty() {
// Minio does not support or implement IAM, "*" is the only valid value.
// Amazon s3 doc on principals: http://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements.html#Principal
if principal != "*" {
err = fmt.Errorf("Unsupported principal style found: ‘%s’, please validate your policy document.", principal)
return err
}
err = fmt.Errorf("Unsupported principals found: ‘%#v’, please validate your policy document.", unsuppPrincipals)
return err
}
return nil
}

// isValidConditions - are valid conditions.
func isValidConditions(conditions map[string]map[string]string) (err error) {
// Returns true if string 'a' is found in the list.
findString := func(a string, list []string) bool {
for _, b := range list {
if b == a {
return true
}
}
return false
}
conditionKeyVal := make(map[string][]string)
func isValidConditions(conditions map[string]map[string]set.StringSet) (err error) {
// Verify conditions should be valid.
// Validate if stringEquals, stringNotEquals are present
// if not throw an error.
conditionKeyVal := make(map[string]set.StringSet)
for conditionType := range conditions {
_, validType := supportedConditionsType[conditionType]
if !validType {
if !supportedConditionsType.Contains(conditionType) {
err = fmt.Errorf("Unsupported condition type '%s', please validate your policy document.", conditionType)
return err
}
for key := range conditions[conditionType] {
_, validKey := supportedConditionsKey[key]
if !validKey {
for key, value := range conditions[conditionType] {
if !supportedConditionsKey.Contains(key) {
err = fmt.Errorf("Unsupported condition key '%s', please validate your policy document.", conditionType)
return err
}
conditionArray, ok := conditionKeyVal[key]
if ok && findString(conditions[conditionType][key], conditionArray) {
conditionVal, ok := conditionKeyVal[key]
if ok && !value.Intersection(conditionVal).IsEmpty() {
err = fmt.Errorf("Ambigious condition values for key '%s', please validate your policy document.", key)
return err
}
conditionKeyVal[key] = append(conditionKeyVal[key], conditions[conditionType][key])
conditionKeyVal[key] = value
}
}
return nil
}

// List of actions for which prefixes are not allowed.
var invalidPrefixActions = map[string]struct{}{
var invalidPrefixActions = set.StringSet{
"s3:GetBucketLocation": {},
"s3:ListBucket": {},
"s3:ListBucketMultipartUploads": {},
Expand All @@ -227,10 +196,10 @@ func resourcePrefix(resource string) string {
func checkBucketPolicyResources(bucket string, bucketPolicy *bucketPolicy) APIErrorCode {
// Validate statements for special actions and collect resources
// for others to validate nesting.
var resourceMap = make(map[string]struct{})
var resourceMap = set.NewStringSet()
for _, statement := range bucketPolicy.Statements {
for _, action := range statement.Actions {
for _, resource := range statement.Resources {
for action := range statement.Actions {
for resource := range statement.Resources {
resourcePrefix := strings.SplitAfter(resource, AWSResourcePrefix)[1]
if _, ok := invalidPrefixActions[action]; ok {
// Resource prefix is not equal to bucket for
Expand All @@ -245,7 +214,7 @@ func checkBucketPolicyResources(bucket string, bucketPolicy *bucketPolicy) APIEr
return ErrMalformedPolicy
}
// All valid resources collect them separately to verify nesting.
resourceMap[resourcePrefix] = struct{}{}
resourceMap.Add(resourcePrefix)
}
}
}
Expand Down
Loading

0 comments on commit 975eb31

Please sign in to comment.