Repository: minio/minio (language: go)

Issue:
Docker: Access Keys effective permissions differ from console output
## Desciption

Creating an access key without manually modifying its permissions results in a key which has no permissions.
This breaks existing keys as well as new keys, if there was no policy assigned to the key manually.

## Expected Behavior
The key should inherit the policies assigned to the user unless restricted otherwise, or at least have the permissions shown in the console be accurate.

## Current Behavior
An access key created under a user assigned the default readwrite policy, gets a 403 response on
`mc ls local/`, even though the console shows the following policy as effective for the key:

```
{
 "Version": "2012-10-17",
 "Statement": [
  {
   "Effect": "Allow",
   "Action": [
    "s3:*"
   ],
   "Resource": [
    "arn:aws:s3:::*"
   ]
  }
 ]
}
```

Possible Solution
<!--- Not obligatory, but suggest a fix/reason for the bug, -->
<!--- or ideas how to implement the addition or change -->

## Steps to Reproduce (for bugs)
<!--- Provide a link to a live example, or an unambiguous set of steps to -->
<!--- reproduce this bug. Include code to reproduce, if relevant -->
<!--- and make sure you have followed https://github.com/minio/minio/tree/release/docs/debugging to capture relevant logs -->

1. Setup the docker-compose stack as present in docs/orchestration
2. create an access token (with consoleAdmin or other permissions on your user account)
3. add access token as mc alias 
4. get a 403 on mc ls alias/

## Context
This broke during the bugfix release last night and broke some deployments.

## Regression
<!-- Is this issue a regression? (Yes / No) -->
<!-- If Yes, optionally please include minio version or commit id or PR# that caused this regression, if you have these details. -->
YES. Broke in RELEASE-2024-01-31

## Your Environment
```
minio version RELEASE.2024-01-31T20-20-33Z (commit-id=24ecc44bac566b25a5bfa9772d946a19ce551035)
Runtime: go1.21.6 linux/amd64
License: GNU AGPLv3 <https://www.gnu.org/licenses/agpl-3.0.html>
Copyright: 2015-2024 MinIO, Inc.
```
Default docker-compose setup as present in docs/orchestration during the release specified above.

## Images
(All data is localhost throwaway setup)
![image](https://github.com/minio/minio/assets/12465878/639d5f80-673d-45ea-acd5-84036f8a0ce8)
![image](https://github.com/minio/minio/assets/12465878/c54d0e7c-b5af-453a-a6ce-7c5df12ed179)
![image](https://github.com/minio/minio/assets/12465878/6f487cd8-3c17-447e-ac5e-e279dd0eba36)
![image](https://github.com/minio/minio/assets/12465878/de665383-67dd-4131-ab74-0c6ed4ce9c1c)
![image](https://github.com/minio/minio/assets/12465878/983d90f6-4ed2-4762-89ca-4d4911e2eecd)
![image](https://github.com/minio/minio/assets/12465878/b24a71f4-9e10-41e3-9dad-5882ca8a9982)




Discussion / hints:
Same here. Existing Access Keys stopped working immediately after the **security update**.
@donatello this seems to be happening with service accounts derived from the root access key. Taking a look.
It looks like the added defensive code caused the issue. I see the policy claim is embedded instead of inherited type and I think it is related to a quirk that we did implement long time ago (empty statements). We may need to add a specific code path for owner derived if we cannot get rid of that quirk.  
**There is a temporary hotfix:**
1. Go to the service account and edit the Access key
2. Click update

In case it helps, older keys that are 16 characters long continued to work for me, but keys that are 20 characters long do not. The update fix above works for me.
I am working on a fix for this.

Fix patch:
diff --git a/cmd/iam.go b/cmd/iam.go
index e9571de528acc..ed437e345cf2d 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -1849,7 +1849,7 @@ func (sys *IAMSys) IsAllowedServiceAccount(args policy.Args, parentUser string)
 	}
 
 	// 3. If an inline session-policy is present, evaluate it.
-	hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicy(args)
+	hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicyForServiceAccount(args)
 	if hasSessionPolicy {
 		return isAllowedSP && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs))
 	}
@@ -1945,6 +1945,67 @@ func (sys *IAMSys) IsAllowedSTS(args policy.Args, parentUser string) bool {
 	return isOwnerDerived || combinedPolicy.IsAllowed(args)
 }
 
+func isAllowedBySessionPolicyForServiceAccount(args policy.Args) (hasSessionPolicy bool, isAllowed bool) {
+	hasSessionPolicy = false
+	isAllowed = false
+
+	// Now check if we have a sessionPolicy.
+	spolicy, ok := args.Claims[sessionPolicyNameExtracted]
+	if !ok {
+		return
+	}
+
+	hasSessionPolicy = true
+
+	spolicyStr, ok := spolicy.(string)
+	if !ok {
+		// Sub policy if set, should be a string reject
+		// malformed/malicious requests.
+		return
+	}
+
+	// Check if policy is parseable.
+	subPolicy, err := policy.ParseConfig(bytes.NewReader([]byte(spolicyStr)))
+	if err != nil {
+		// Log any error in input session policy config.
+		logger.LogIf(GlobalContext, err)
+		return
+	}
+
+	// SPECIAL CASE: For service accounts, any valid JSON is allowed as a
+	// policy, regardless of whether the number of statements is 0, this
+	// includes `null`, `{}` and `{"Statement": null}`. In fact, MinIO Console
+	// sends `null` when no policy is set and the intended behavior is that the
+	// service account should inherit parent policy.
+	//
+	// However, for a policy like `{"Statement":[]}`, the intention is to not
+	// provide any permissions via the session policy - i.e. the service account
+	// can do nothing (such a JSON could be generated by an external application
+	// as the policy for the service account). Inheriting the parent policy in
+	// such a case, is a security issue. Ideally, we should not allow such
+	// behavior, but for compatibility with the Console, we currently allow it.
+	//
+	// TODO:
+	//
+	// 1. fix console behavior and allow this inheritance for service accounts
+	// created before a certain (TBD) future date.
+	//
+	// 2. do not allow empty statement policies for service accounts.
+	if subPolicy.Version == "" && subPolicy.Statements == nil && subPolicy.ID == "" {
+		hasSessionPolicy = false
+		return
+	}
+
+	// As the session policy exists, even if the parent is the root account, it
+	// must be restricted by it. So, we set `.IsOwner` to false here
+	// unconditionally.
+	sessionPolicyArgs := args
+	sessionPolicyArgs.IsOwner = false
+
+	// Sub policy is set and valid.
+	return hasSessionPolicy, subPolicy.IsAllowed(sessionPolicyArgs)
+}
+
 func isAllowedBySessionPolicy(args policy.Args) (hasSessionPolicy bool, isAllowed bool) {
 	hasSessionPolicy = false
 	isAllowed = false


Test patch: