From 3bd9636a5bbf5f6017f0b1ad4e876ce6134bdb7e Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 10 Jan 2022 14:26:26 -0800 Subject: [PATCH] do not remove Sid from svcaccount policies (#14064) fixes #13905 --- cmd/admin-handler-utils.go | 12 +++++ cmd/admin-handlers-users.go | 11 ++-- cmd/bucket-policy-handlers_test.go | 80 +++++++++++++++++------------- cmd/gateway/azure/gateway-azure.go | 1 + cmd/gateway/gcs/gateway-gcs.go | 1 + cmd/iam-store.go | 20 +++++--- cmd/iam.go | 25 +++++----- cmd/policy_test.go | 14 +++--- cmd/typed-errors.go | 6 +++ go.mod | 2 +- go.sum | 3 +- 11 files changed, 105 insertions(+), 70 deletions(-) diff --git a/cmd/admin-handler-utils.go b/cmd/admin-handler-utils.go index 2f7938c43..89e7cc098 100644 --- a/cmd/admin-handler-utils.go +++ b/cmd/admin-handler-utils.go @@ -102,6 +102,18 @@ func toAdminAPIErr(ctx context.Context, err error) APIError { Description: err.Error(), HTTPStatusCode: http.StatusForbidden, } + case errors.Is(err, errIAMServiceAccount): + apiErr = APIError{ + Code: "XMinioIAMServiceAccount", + Description: err.Error(), + HTTPStatusCode: http.StatusBadRequest, + } + case errors.Is(err, errIAMServiceAccountUsed): + apiErr = APIError{ + Code: "XMinioIAMServiceAccountUsed", + Description: err.Error(), + HTTPStatusCode: http.StatusBadRequest, + } case errors.Is(err, errIAMNotInitialized): apiErr = APIError{ Code: "XMinioIAMNotInitialized", diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 6ded12e76..97e8aa1c9 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -816,18 +816,15 @@ func (a adminAPIHandlers) InfoServiceAccount(w http.ResponseWriter, r *http.Requ var svcAccountPolicy iampolicy.Policy - impliedPolicy := policy == nil - - // If policy is empty, check for policy of the parent user - if !impliedPolicy { - svcAccountPolicy = svcAccountPolicy.Merge(*policy) + if policy != nil { + svcAccountPolicy = *policy } else { policiesNames, err := globalIAMSys.PolicyDBGet(svcAccount.ParentUser, false) if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } - svcAccountPolicy = svcAccountPolicy.Merge(globalIAMSys.GetCombinedPolicy(policiesNames...)) + svcAccountPolicy = globalIAMSys.GetCombinedPolicy(policiesNames...) } policyJSON, err := json.MarshalIndent(svcAccountPolicy, "", " ") @@ -839,7 +836,7 @@ func (a adminAPIHandlers) InfoServiceAccount(w http.ResponseWriter, r *http.Requ infoResp := madmin.InfoServiceAccountResp{ ParentUser: svcAccount.ParentUser, AccountStatus: svcAccount.Status, - ImpliedPolicy: impliedPolicy, + ImpliedPolicy: policy == nil, Policy: string(policyJSON), } diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go index 85ef66ff6..b9ef9408e 100644 --- a/cmd/bucket-policy-handlers_test.go +++ b/cmd/bucket-policy-handlers_test.go @@ -37,60 +37,72 @@ import ( func getAnonReadOnlyBucketPolicy(bucketName string) *policy.Policy { return &policy.Policy{ Version: policy.DefaultVersion, - Statements: []policy.Statement{policy.NewStatement( - policy.Allow, - policy.NewPrincipal("*"), - policy.NewActionSet(policy.GetBucketLocationAction, policy.ListBucketAction), - policy.NewResourceSet(policy.NewResource(bucketName, "")), - condition.NewFunctions(), - )}, + Statements: []policy.Statement{ + policy.NewStatement( + "", + policy.Allow, + policy.NewPrincipal("*"), + policy.NewActionSet(policy.GetBucketLocationAction, policy.ListBucketAction), + policy.NewResourceSet(policy.NewResource(bucketName, "")), + condition.NewFunctions(), + ), + }, } } func getAnonWriteOnlyBucketPolicy(bucketName string) *policy.Policy { return &policy.Policy{ Version: policy.DefaultVersion, - Statements: []policy.Statement{policy.NewStatement( - policy.Allow, - policy.NewPrincipal("*"), - policy.NewActionSet( - policy.GetBucketLocationAction, - policy.ListBucketMultipartUploadsAction, + Statements: []policy.Statement{ + policy.NewStatement( + "", + policy.Allow, + policy.NewPrincipal("*"), + policy.NewActionSet( + policy.GetBucketLocationAction, + policy.ListBucketMultipartUploadsAction, + ), + policy.NewResourceSet(policy.NewResource(bucketName, "")), + condition.NewFunctions(), ), - policy.NewResourceSet(policy.NewResource(bucketName, "")), - condition.NewFunctions(), - )}, + }, } } func getAnonReadOnlyObjectPolicy(bucketName, prefix string) *policy.Policy { return &policy.Policy{ Version: policy.DefaultVersion, - Statements: []policy.Statement{policy.NewStatement( - policy.Allow, - policy.NewPrincipal("*"), - policy.NewActionSet(policy.GetObjectAction), - policy.NewResourceSet(policy.NewResource(bucketName, prefix)), - condition.NewFunctions(), - )}, + Statements: []policy.Statement{ + policy.NewStatement( + "", + policy.Allow, + policy.NewPrincipal("*"), + policy.NewActionSet(policy.GetObjectAction), + policy.NewResourceSet(policy.NewResource(bucketName, prefix)), + condition.NewFunctions(), + ), + }, } } func getAnonWriteOnlyObjectPolicy(bucketName, prefix string) *policy.Policy { return &policy.Policy{ Version: policy.DefaultVersion, - Statements: []policy.Statement{policy.NewStatement( - policy.Allow, - policy.NewPrincipal("*"), - policy.NewActionSet( - policy.AbortMultipartUploadAction, - policy.DeleteObjectAction, - policy.ListMultipartUploadPartsAction, - policy.PutObjectAction, + Statements: []policy.Statement{ + policy.NewStatement( + "", + policy.Allow, + policy.NewPrincipal("*"), + policy.NewActionSet( + policy.AbortMultipartUploadAction, + policy.DeleteObjectAction, + policy.ListMultipartUploadPartsAction, + policy.PutObjectAction, + ), + policy.NewResourceSet(policy.NewResource(bucketName, prefix)), + condition.NewFunctions(), ), - policy.NewResourceSet(policy.NewResource(bucketName, prefix)), - condition.NewFunctions(), - )}, + }, } } diff --git a/cmd/gateway/azure/gateway-azure.go b/cmd/gateway/azure/gateway-azure.go index 8c61a3da9..5d3f25884 100644 --- a/cmd/gateway/azure/gateway-azure.go +++ b/cmd/gateway/azure/gateway-azure.go @@ -1416,6 +1416,7 @@ func (a *azureObjects) GetBucketPolicy(ctx context.Context, bucket string) (*pol Version: policy.DefaultVersion, Statements: []policy.Statement{ policy.NewStatement( + "", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet( diff --git a/cmd/gateway/gcs/gateway-gcs.go b/cmd/gateway/gcs/gateway-gcs.go index 8838da916..30a42afc1 100644 --- a/cmd/gateway/gcs/gateway-gcs.go +++ b/cmd/gateway/gcs/gateway-gcs.go @@ -1470,6 +1470,7 @@ func (l *gcsGateway) GetBucketPolicy(ctx context.Context, bucket string) (*polic Version: policy.DefaultVersion, Statements: []policy.Statement{ policy.NewStatement( + "", policy.Allow, policy.NewPrincipal("*"), actionSet, diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 01b5fbed2..a12a2dbd6 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -1084,11 +1084,12 @@ func filterPolicies(cache *iamCache, policyName string, bucketName string) (stri continue } p, found := cache.iamPolicyDocsMap[policy] - if found { - if bucketName == "" || p.Policy.MatchResource(bucketName) { - policies = append(policies, policy) - combinedPolicy = combinedPolicy.Merge(p.Policy) - } + if !found { + continue + } + if bucketName == "" || p.Policy.MatchResource(bucketName) { + policies = append(policies, policy) + combinedPolicy = combinedPolicy.Merge(p.Policy) } } return strings.Join(policies, ","), combinedPolicy @@ -1511,13 +1512,16 @@ func (store *IAMStoreSys) AddServiceAccount(ctx context.Context, cred auth.Crede // Found newly requested service account, to be an existing account - // reject such operation (updates to the service account are handled in // a different API). - if _, found := cache.iamUsersMap[accessKey]; found { - return errIAMActionNotAllowed + if scred, found := cache.iamUsersMap[accessKey]; found { + if scred.ParentUser != parentUser { + return errIAMServiceAccountUsed + } + return errIAMServiceAccount } // Parent user must not be a service account. if cr, found := cache.iamUsersMap[parentUser]; found && cr.IsServiceAccount() { - return errIAMActionNotAllowed + return errIAMServiceAccount } u := newUserIdentity(cred) diff --git a/cmd/iam.go b/cmd/iam.go index 80f7e06dd..5172decd9 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -883,18 +883,19 @@ func (sys *IAMSys) getServiceAccount(ctx context.Context, accessKey string) (aut var embeddedPolicy *iampolicy.Policy jwtClaims, err := auth.ExtractClaims(sa.SessionToken, globalActiveCred.SecretKey) - if err == nil { - pt, ptok := jwtClaims.Lookup(iamPolicyClaimNameSA()) - sp, spok := jwtClaims.Lookup(iampolicy.SessionPolicyName) - if ptok && spok && pt == "embedded-policy" { - policyBytes, err := base64.StdEncoding.DecodeString(sp) - if err == nil { - p, err := iampolicy.ParseConfig(bytes.NewReader(policyBytes)) - if err == nil { - policy := iampolicy.Policy{}.Merge(*p) - embeddedPolicy = &policy - } - } + if err != nil { + return auth.Credentials{}, nil, err + } + pt, ptok := jwtClaims.Lookup(iamPolicyClaimNameSA()) + sp, spok := jwtClaims.Lookup(iampolicy.SessionPolicyName) + if ptok && spok && pt == "embedded-policy" { + policyBytes, err := base64.StdEncoding.DecodeString(sp) + if err != nil { + return auth.Credentials{}, nil, err + } + embeddedPolicy, err = iampolicy.ParseConfig(bytes.NewReader(policyBytes)) + if err != nil { + return auth.Credentials{}, nil, err } } diff --git a/cmd/policy_test.go b/cmd/policy_test.go index fae883976..97a24a264 100644 --- a/cmd/policy_test.go +++ b/cmd/policy_test.go @@ -31,14 +31,14 @@ func TestPolicySysIsAllowed(t *testing.T) { p := &policy.Policy{ Version: policy.DefaultVersion, Statements: []policy.Statement{ - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.GetBucketLocationAction), policy.NewResourceSet(policy.NewResource("mybucket", "")), condition.NewFunctions(), ), - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.PutObjectAction), @@ -164,14 +164,14 @@ func TestPolicyToBucketAccessPolicy(t *testing.T) { case1Policy := &policy.Policy{ Version: policy.DefaultVersion, Statements: []policy.Statement{ - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.GetBucketLocationAction, policy.ListBucketAction), policy.NewResourceSet(policy.NewResource("mybucket", "")), condition.NewFunctions(), ), - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.GetObjectAction), @@ -199,7 +199,7 @@ func TestPolicyToBucketAccessPolicy(t *testing.T) { case3Policy := &policy.Policy{ Version: "12-10-2012", Statements: []policy.Statement{ - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.PutObjectAction), @@ -244,14 +244,14 @@ func TestBucketAccessPolicyToPolicy(t *testing.T) { case1Result := &policy.Policy{ Version: policy.DefaultVersion, Statements: []policy.Statement{ - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.GetBucketLocationAction, policy.ListBucketAction), policy.NewResourceSet(policy.NewResource("mybucket", "")), condition.NewFunctions(), ), - policy.NewStatement( + policy.NewStatement("", policy.Allow, policy.NewPrincipal("*"), policy.NewActionSet(policy.GetObjectAction), diff --git a/cmd/typed-errors.go b/cmd/typed-errors.go index 31c5ad978..9b6c0461e 100644 --- a/cmd/typed-errors.go +++ b/cmd/typed-errors.go @@ -91,6 +91,12 @@ var errTooManyPolicies = errors.New("Only a single policy may be specified here. // error returned in IAM subsystem when an external users systems is configured. var errIAMActionNotAllowed = errors.New("Specified IAM action is not allowed") +// error returned in IAM service account +var errIAMServiceAccount = errors.New("Specified service account cannot be updated in this API call") + +// error returned in IAM service account is already used. +var errIAMServiceAccountUsed = errors.New("Specified service account is used by another user") + // error returned in IAM subsystem when IAM sub-system is still being initialized. var errIAMNotInitialized = errors.New("IAM sub-system is being initialized, please try again") diff --git a/go.mod b/go.mod index b4d740481..dacde7f00 100644 --- a/go.mod +++ b/go.mod @@ -52,7 +52,7 @@ require ( github.com/minio/madmin-go v1.2.4 github.com/minio/minio-go/v7 v7.0.20 github.com/minio/parquet-go v1.1.0 - github.com/minio/pkg v1.1.11 + github.com/minio/pkg v1.1.14 github.com/minio/selfupdate v0.4.0 github.com/minio/sha256-simd v1.0.0 github.com/minio/simdjson-go v0.2.1 diff --git a/go.sum b/go.sum index 9d0dbdc77..8b184fddf 100644 --- a/go.sum +++ b/go.sum @@ -1114,8 +1114,9 @@ github.com/minio/pkg v1.0.3/go.mod h1:obU54TZ9QlMv0TRaDgQ/JTzf11ZSXxnSfLrm4tMtBP github.com/minio/pkg v1.0.4/go.mod h1:obU54TZ9QlMv0TRaDgQ/JTzf11ZSXxnSfLrm4tMtBP8= github.com/minio/pkg v1.0.11/go.mod h1:32x/3OmGB0EOi1N+3ggnp+B5VFkSBBB9svPMVfpnf14= github.com/minio/pkg v1.1.3/go.mod h1:32x/3OmGB0EOi1N+3ggnp+B5VFkSBBB9svPMVfpnf14= -github.com/minio/pkg v1.1.11 h1:FQMYrOZzZenQUVwvxM0YjoAZB15Wb+m3TZSu33Lynx0= github.com/minio/pkg v1.1.11/go.mod h1:2WJAxesjzmPK9MnLZKm5n1hVYfBg04f2GQs6N5ImNU8= +github.com/minio/pkg v1.1.14 h1:9tGPH6p0YEsIOHOY3sh2Yl5Dn0JNTkNOdZW/p+MKIi4= +github.com/minio/pkg v1.1.14/go.mod h1:2WJAxesjzmPK9MnLZKm5n1hVYfBg04f2GQs6N5ImNU8= github.com/minio/selfupdate v0.3.1/go.mod h1:b8ThJzzH7u2MkF6PcIra7KaXO9Khf6alWPvMSyTDCFM= github.com/minio/selfupdate v0.4.0 h1:A7t07pN4Ch1tBTIRStW0KhUVyykz+2muCqFsITQeEW8= github.com/minio/selfupdate v0.4.0/go.mod h1:mcDkzMgq8PRcpCRJo/NlPY7U45O5dfYl2Y0Rg7IustY=