From c5a80ca5d5a8a8d5cd8d550dceeb6e4ad0c13f89 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Thu, 29 Apr 2021 13:01:42 -0700
Subject: [PATCH] support service accounts for OpenID connect properly (#12178)

OpenID connect generated service accounts do not work
properly after console logout, since the parentUser state
is lost - instead use sub+iss claims for parentUser, according
to OIDC spec both the claims provide the necessary stability
across logins etc.
---
 cmd/admin-handlers-users.go | 13 ++++++++-----
 cmd/iam.go                  | 24 ++++++++++++++++++++++--
 cmd/sts-handlers.go         | 27 +++++++++++++++++++++++----
 3 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go
index 7e37913aa..04e1632e6 100644
--- a/cmd/admin-handlers-users.go
+++ b/cmd/admin-handlers-users.go
@@ -529,11 +529,14 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 		}
 		// targerUser is set to bindDN at this point in time.
 	} else {
-		if targetUser == "" {
-			targetUser = cred.AccessKey
-		}
-		if cred.ParentUser != "" {
+		if cred.IsServiceAccount() || cred.IsTemp() {
+			if cred.ParentUser == "" {
+				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errors.New("service accounts cannot be generated for temporary credentials without parent")), r.URL)
+				return
+			}
 			targetUser = cred.ParentUser
+		} else {
+			targetUser = cred.AccessKey
 		}
 		targetGroups = cred.Groups
 	}
@@ -985,7 +988,7 @@ func (a adminAPIHandlers) AccountInfoHandler(w http.ResponseWriter, r *http.Requ
 		}
 		policies, err = globalIAMSys.PolicyDBGet(parentUser, false, cred.Groups...)
 	default:
-		err = errors.New("should not happen!")
+		err = errors.New("should never happen")
 	}
 	if err != nil {
 		logger.LogIf(ctx, err)
diff --git a/cmd/iam.go b/cmd/iam.go
index 9e8ef575b..817009782 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -872,6 +872,24 @@ func (sys *IAMSys) SetTempUser(accessKey string, cred auth.Credentials, policyNa
 		sys.store.lock()
 		defer sys.store.unlock()
 
+		// We are on purpose not persisting the policy map for parent
+		// user, although this is a hack, it is a good enoug hack
+		// at this point in time - we need to overhaul our OIDC
+		// usage with service accounts with a more cleaner implementation
+		//
+		// This mapping is necessary to ensure that valid credentials
+		// have necessary ParentUser present - this is mainly for only
+		// webIdentity based STS tokens.
+		if cred.IsTemp() && cred.ParentUser != "" {
+			if _, ok := sys.iamUserPolicyMap[cred.ParentUser]; !ok {
+				if err := sys.store.saveMappedPolicy(context.Background(), accessKey, stsUser, false, mp, options{ttl: ttl}); err != nil {
+					sys.store.unlock()
+					return err
+				}
+				sys.iamUserPolicyMap[cred.ParentUser] = mp
+			}
+		}
+
 		if err := sys.store.saveMappedPolicy(context.Background(), accessKey, stsUser, false, mp, options{ttl: ttl}); err != nil {
 			sys.store.unlock()
 			return err
@@ -1458,8 +1476,10 @@ func (sys *IAMSys) GetUser(accessKey string) (cred auth.Credentials, ok bool) {
 	defer sys.store.runlock()
 
 	if ok && cred.IsValid() {
-		if cred.ParentUser != "" && sys.usersSysType == MinIOUsersSysType {
-			_, ok = sys.iamUsersMap[cred.ParentUser]
+		if cred.IsServiceAccount() || cred.IsTemp() {
+			// temporary credentials or service accounts
+			// must have their parent in UsersMap
+			_, ok = sys.iamUserPolicyMap[cred.ParentUser]
 		}
 		// for LDAP service accounts with ParentUser set
 		// we have no way to validate, either because user
diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go
index 519f3c3e5..7e3bbf99a 100644
--- a/cmd/sts-handlers.go
+++ b/cmd/sts-handlers.go
@@ -21,6 +21,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -57,6 +58,7 @@ const (
 	// JWT claim keys
 	expClaim = "exp"
 	subClaim = "sub"
+	issClaim = "iss"
 
 	// JWT claim to check the parent user
 	parentClaim = "parent"
@@ -322,6 +324,21 @@ func (sts *stsAPIHandlers) AssumeRoleWithSSO(w http.ResponseWriter, r *http.Requ
 		return
 	}
 
+	var subFromToken string
+	if v, ok := m[subClaim]; ok {
+		subFromToken, _ = v.(string)
+	}
+
+	if subFromToken == "" {
+		writeSTSErrorResponse(ctx, w, true, ErrSTSInvalidParameterValue, errors.New("STS JWT Token has `sub` claim missing, `sub` claim is mandatory"))
+		return
+	}
+
+	var issFromToken string
+	if v, ok := m[issClaim]; ok {
+		issFromToken, _ = v.(string)
+	}
+
 	// JWT has requested a custom claim with policy value set.
 	// This is a MinIO STS API specific value, this value should
 	// be set and configured on your identity provider as part of
@@ -371,10 +388,12 @@ func (sts *stsAPIHandlers) AssumeRoleWithSSO(w http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	var subFromToken string
-	if v, ok := m[subClaim]; ok {
-		subFromToken, _ = v.(string)
-	}
+	// https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability
+	// claim is only considered stable when subject and iss are used together
+	// this is to ensure that ParentUser doesn't change and we get to use
+	// parentUser as per the requirements for service accounts for OpenID
+	// based logins.
+	cred.ParentUser = "jwt:" + subFromToken + ":" + issFromToken
 
 	// Set the newly generated credentials.
 	if err = globalIAMSys.SetTempUser(cred.AccessKey, cred, policyName); err != nil {