Remove internal usage of consoleAdmin

"consoleAdmin" was used as the policy for root derived accounts, but this lead to unexpected bugs when an administrator modified the consoleAdmin policy This change avoids evaluating a policy for root derived accounts as by default no policy is mapped to the root user. If a session policy is attached to a root derived account, it will be evaluated as expected.
minio · Jul 26, 2022 · 23c1888 · 23c1888
1 parent 0a3b1ad
commit 23c1888
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 64 deletions.
diff --git a/cmd/iam-store.go b/cmd/iam-store.go
@@ -337,33 +337,16 @@ func (c *iamCache) policyDBGet(mode UsersSysType, name string, isGroup bool) ([]
 		return c.iamGroupPolicyMap[name].toSlice(), c.iamGroupPolicyMap[name].UpdatedAt, nil
 	}
 
-	if name == globalActiveCred.AccessKey {
-		return []string{"consoleAdmin"}, time.Time{}, nil
-	}
-
 	// When looking for a user's policies, we also check if the user
 	// and the groups they are member of are enabled.
-	var parentName string
 	u, ok := c.iamUsersMap[name]
 	if ok {
 		if !u.Credentials.IsValid() {
 			return nil, time.Time{}, nil
 		}
-		parentName = u.Credentials.ParentUser
 	}
 
-	mp, ok := c.iamUserPolicyMap[name]
-	if !ok {
-		// Service accounts with root credentials, inherit parent permissions
-		if parentName == globalActiveCred.AccessKey && u.Credentials.IsServiceAccount() {
-			// even if this is set, the claims present in the service
-			// accounts apply the final permissions if any.
-			return []string{"consoleAdmin"}, mp.UpdatedAt, nil
-		}
-		if parentName != "" {
-			mp = c.iamUserPolicyMap[parentName]
-		}
-	}
+	mp := c.iamUserPolicyMap[name]
 
 	// returned policy could be empty
 	policies := mp.toSlice()

diff --git a/cmd/iam.go b/cmd/iam.go
@@ -1458,7 +1458,7 @@ const sessionPolicyNameExtracted = iampolicy.SessionPolicyName + "-extracted"
 // IsAllowedServiceAccount - checks if the given service account is allowed to perform
 // actions. The permission of the parent user is checked first
 func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parentUser string) bool {
-	// Now check if we have a subject claim
+	// Verify if the parent claim matches the parentUser.
 	p, ok := args.Claims[parentClaim]
 	if ok {
 		parentInClaim, ok := p.(string)
@@ -1478,41 +1478,56 @@ func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parentUser strin
 		return false
 	}
 
-	// Check policy for parent user of service account.
-	svcPolicies, err := sys.PolicyDBGet(parentUser, false, args.Groups...)
-	if err != nil {
-		logger.LogIf(GlobalContext, err)
-		return false
-	}
+	isOwnerDerived := parentUser == globalActiveCred.AccessKey
 
-	if len(svcPolicies) == 0 {
-		// If parent user has no policies, check for OpenID
-		// claims/RoleARN in case it exists.
-		roleArn := args.GetRoleArn()
-		if roleArn != "" {
-			arn, err := arn.Parse(roleArn)
-			if err != nil {
-				logger.LogIf(GlobalContext, fmt.Errorf("error parsing role ARN %s: %v", roleArn, err))
-				return false
-			}
-			svcPolicies = newMappedPolicy(sys.rolesMap[arn]).toSlice()
-		} else {
-			// If there is no roleArn claim, check the OpenID
-			// provider's policy claim.
-			policySet, _ := iampolicy.GetPoliciesFromClaims(args.Claims, iamPolicyClaimNameOpenID())
-			svcPolicies = policySet.ToSlice()
+	var err error
+	var svcPolicies []string
+	roleArn := args.GetRoleArn()
+
+	switch {
+	case isOwnerDerived:
+		// All actions are allowed by default and no policy evaluation is
+		// required.
+
+	case roleArn != "":
+		arn, err := arn.Parse(roleArn)
+		if err != nil {
+			logger.LogIf(GlobalContext, fmt.Errorf("error parsing role ARN %s: %v", roleArn, err))
+			return false
 		}
-		if len(svcPolicies) == 0 {
+		svcPolicies = newMappedPolicy(sys.rolesMap[arn]).toSlice()
+
+	default:
+		// Check policy for parent user of service account.
+		svcPolicies, err = sys.PolicyDBGet(parentUser, false, args.Groups...)
+		if err != nil {
+			logger.LogIf(GlobalContext, err)
 			return false
 		}
+
+		// Finally, if there is no parent policy, check if a policy claim is
+		// present.
+		if len(svcPolicies) == 0 {
+			policySet, _ := iampolicy.GetPoliciesFromClaims(args.Claims, iamPolicyClaimNameOpenID())
+			svcPolicies = policySet.ToSlice()
+		}
 	}
 
-	// Policies were found, evaluate all of them.
-	availablePoliciesStr, combinedPolicy := sys.store.FilterPolicies(strings.Join(svcPolicies, ","), "")
-	if availablePoliciesStr == "" {
+	// Defensive code: Do not allow any operation if no policy is found.
+	if !isOwnerDerived && len(svcPolicies) == 0 {
 		return false
 	}
 
+	var combinedPolicy iampolicy.Policy
+	// Policies were found, evaluate all of them.
+	if !isOwnerDerived {
+		availablePoliciesStr, c := sys.store.FilterPolicies(strings.Join(svcPolicies, ","), "")
+		if availablePoliciesStr == "" {
+			return false
+		}
+		combinedPolicy = c
+	}
+
 	parentArgs := args
 	parentArgs.AccountName = parentUser
 	// These are dynamic values set them appropriately.
@@ -1532,7 +1547,7 @@ func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parentUser strin
 	}
 
 	if saPolicyClaimStr == inheritedPolicyType {
-		return combinedPolicy.IsAllowed(parentArgs)
+		return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)
 	}
 
 	// Now check if we have a sessionPolicy.
@@ -1558,37 +1573,51 @@ func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parentUser strin
 
 	// This can only happen if policy was set but with an empty JSON.
 	if subPolicy.Version == "" && len(subPolicy.Statements) == 0 {
-		return combinedPolicy.IsAllowed(parentArgs)
+		return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)
 	}
 
 	if subPolicy.Version == "" {
 		return false
 	}
 
-	return combinedPolicy.IsAllowed(parentArgs) && subPolicy.IsAllowed(parentArgs)
+	return subPolicy.IsAllowed(parentArgs) && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs))
 }
 
 // IsAllowedSTS is meant for STS based temporary credentials,
 // which implements claims validation and verification other than
 // applying policies.
 func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool {
+	// 1. Determine mapped policies
+
+	isOwnerDerived := parentUser == globalActiveCred.AccessKey
 	var policies []string
 	roleArn := args.GetRoleArn()
-	if roleArn != "" {
+
+	switch {
+	case isOwnerDerived:
+		// All actions are allowed by default and no policy evaluation is
+		// required.
+
+	case roleArn != "":
+		// If a roleARN is present, the role policy is applied.
 		arn, err := arn.Parse(roleArn)
 		if err != nil {
 			logger.LogIf(GlobalContext, fmt.Errorf("error parsing role ARN %s: %v", roleArn, err))
 			return false
 		}
 		policies = newMappedPolicy(sys.rolesMap[arn]).toSlice()
-	} else {
-		// Lookup the parent user's mapping if there's no role-ARN.
+
+	default:
+		// Otherwise, inherit parent user's policy
 		var err error
 		policies, err = sys.store.PolicyDBGet(parentUser, false, args.Groups...)
 		if err != nil {
 			logger.LogIf(GlobalContext, fmt.Errorf("error fetching policies on %s: %v", parentUser, err))
 			return false
 		}
+
+		// Finally, if there is no parent policy, check if a policy claim is
+		// present in the session token.
 		if len(policies) == 0 {
 			// If there is no parent policy mapping, we fall back to
 			// using policy claim from JWT.
@@ -1599,40 +1628,50 @@ func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool {
 			}
 			policies = policySet.ToSlice()
 		}
+
 	}
 
 	// Defensive code: Do not allow any operation if no policy is found in the session token
-	if len(policies) == 0 {
+	if !isOwnerDerived && len(policies) == 0 {
 		return false
 	}
 
-	combinedPolicy, err := sys.store.GetPolicy(strings.Join(policies, ","))
-	if err == errNoSuchPolicy {
-		for _, pname := range policies {
-			_, err := sys.store.GetPolicy(pname)
-			if err == errNoSuchPolicy {
-				// all policies presented in the claim should exist
-				logger.LogIf(GlobalContext, fmt.Errorf("expected policy (%s) missing from the JWT claim %s, rejecting the request", pname, iamPolicyClaimNameOpenID()))
-				return false
+	// 2. Combine the mapped policies into a single combined policy.
+
+	var combinedPolicy iampolicy.Policy
+	if !isOwnerDerived {
+		var err error
+		combinedPolicy, err = sys.store.GetPolicy(strings.Join(policies, ","))
+		if err == errNoSuchPolicy {
+			for _, pname := range policies {
+				_, err := sys.store.GetPolicy(pname)
+				if err == errNoSuchPolicy {
+					// all policies presented in the claim should exist
+					logger.LogIf(GlobalContext, fmt.Errorf("expected policy (%s) missing from the JWT claim %s, rejecting the request", pname, iamPolicyClaimNameOpenID()))
+					return false
+				}
 			}
+			logger.LogIf(GlobalContext, fmt.Errorf("all policies were unexpectedly present!"))
+			return false
 		}
-		logger.LogIf(GlobalContext, fmt.Errorf("all policies were unexpectedly present!"))
-		return false
+
 	}
 
+	// 3. If an inline session-policy is present, evaluate it.
+
 	// These are dynamic values set them appropriately.
 	args.ConditionValues["username"] = []string{parentUser}
 	args.ConditionValues["userid"] = []string{parentUser}
 
 	// Now check if we have a sessionPolicy.
 	hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicy(args)
 	if hasSessionPolicy {
-		return isAllowedSP && combinedPolicy.IsAllowed(args)
+		return isAllowedSP && (isOwnerDerived || combinedPolicy.IsAllowed(args))
 	}
 
 	// Sub policy not set, this is most common since subPolicy
 	// is optional, use the inherited policies.
-	return combinedPolicy.IsAllowed(args)
+	return isOwnerDerived || combinedPolicy.IsAllowed(args)
 }
 
 func isAllowedBySessionPolicy(args iampolicy.Args) (hasSessionPolicy bool, isAllowed bool) {