From 7e4e7a66af93c87e7b344b9df93e1194f1c78da5 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Tue, 26 Jul 2022 19:06:55 -0700 Subject: [PATCH] Remove internal usage of consoleAdmin (#15402) "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. --- cmd/iam-store.go | 19 +------ cmd/iam.go | 131 ++++++++++++++++++++++++++++++----------------- 2 files changed, 86 insertions(+), 64 deletions(-) diff --git a/cmd/iam-store.go b/cmd/iam-store.go index f0d8534a8..ea8291ab3 100644 --- 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 index 2e6f0c043..d8720d456 100644 --- 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. + 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 + } + 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() } - if len(svcPolicies) == 0 { - return false - } } - // 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,27 +1628,37 @@ 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} @@ -1627,12 +1666,12 @@ func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool { // 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) {