Repository: minio/minio (language: go)

Issue:
IAM Policies do not keep Sid
According to the documentation "MinIO policy documents use the same schema as AWS IAM Policy documents."

But **IAM Policies** and **Service Account Policies** remove Sid field from the given json and don't store it. **(Note that Bucket policies keep Sid field)**

Sid is really useful when it comes to controlling policies programmatically. otherwise, you have to loop through all statements and have some special checks to find your desired statement to update.

## Expected Behavior
Keep Sid field in **IAM Policies** and **Service Account policies**

## Current Behavior
Sid field gets removed.

## Steps to Reproduce (for bugs)
- Suppose you have this policy.json
```
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "AddSomeRestrictions",
            "Effect": "Deny",
            "Action": [
                "s3:GetObject"
            ],
            "Resource": [
                "arn:aws:s3:::public-bucket/private.txt*",
                "arn:aws:s3:::public-bucket/private2.txt*"
            ]
        }
    ]
}
```
- Run `mc admin policy add local newpolicy policy.json`
- Run `mc admin policy info local newpolicy`
- You should see that Sid is removed
- The same thing happens for Service Account policies

## Your Environment
* Version used (`minio --version`): RELEASE.2021-12-10T23-03-39Z



Discussion / hints:
@bluemmb Thanks for reporting this. I am able to reproduce this. We will fix this soon.
@bluemmb this looks like already fixed by this commit, 44fefe5b9

The fix will be in the next release @bluemmb .
@vadmeste Thanks. I tested the latest version `2021-12-20T22-07-16Z` which includes #13796

It now accepts Sid for **IAM Policies** but still **Service Account Policies** do not keep Sid

Steps to reproduce:
- Suppose you have this policy.json
```
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "AddSomeRestrictions",
            "Effect": "Deny",
            "Action": [
                "s3:GetObject"
            ],
            "Resource": [
                "arn:aws:s3:::public-bucket/private.txt*",
                "arn:aws:s3:::public-bucket/private2.txt*"
            ]
        }
    ]
}
```
- Run `mc admin user svcacct add local admin --access-key abc --secret-key 12345678 --policy policy.json`
- Run `mc admin user svcacct info local abc --policy`
- You should see that Sid is removed
> * Run `mc admin user svcacct add local admin --access-key abc --secret-key 12345678 --policy policy.json`
> * Run `mc admin user svcacct info local abc --policy`
> * You should see that Sid is removed

This a server bug, that needs to be fixed // cc @vadmeste - I am able to reproduce it.
@harshavardhana Thanks. Would you reopen this issue so it wouldn't be missed out?

Fix patch:
diff --git a/cmd/admin-handler-utils.go b/cmd/admin-handler-utils.go
index 2f7938c43c904..89e7cc0983d07 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 6ded12e760d54..97e8aa1c9a55f 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/gateway/azure/gateway-azure.go b/cmd/gateway/azure/gateway-azure.go
index 8c61a3da90846..5d3f25884a960 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 8838da916d7e4..30a42afc1ee22 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 01b5fbed2a089..a12a2dbd65076 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 80f7e06dd07b9..5172decd9aae2 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/typed-errors.go b/cmd/typed-errors.go
index 31c5ad9784299..9b6c0461e25f1 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 b4d740481cb42..dacde7f00d34f 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 9d0dbdc773c6f..8b184fddf8a92 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=


Test patch:
diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go
index 85ef66ff68285..b9ef9408e4b79 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/policy_test.go b/cmd/policy_test.go
index fae883976a87c..97a24a2643532 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),