diff --git a/cmd/iam.go b/cmd/iam.go index e9571de52..ed437e345 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