Repository: minio/minio (language: go)

Issue:
Can not remove existing "svcacct"
## Expected Behavior
The svcacct should be removed

## Current Behavior
The svcacct is not removed ... `mc` is getting a 500 from the server

## Steps to Reproduce (for bugs)
1. `mc alias set provisioning $MINIO_URL $MINIO_ROOT_USER $MINIO_ROOT_PASSWORD`
2. `mc admin user add provisioning $USER $PASS`
3. `mc admin policy set provisioning readwrite user=$USER`
4. `mc admin user enable provisioning $USER`
5. `mc admin user svcacct add provisioning $USER --access-key $SVCACCT_ACCESS_KEY --secret-key $SVCACCT_SECRET_KEY`
6. `mc --debug admin user svcacct rm provisioning $SVCACCT_ACCESS_KEY`

leads to this output:
```
mc: <DEBUG> DELETE /minio/admin/v3/delete-service-account?accessKey=minio-fmserver HTTP/1.1
Host: localhost:9000
User-Agent: MinIO (linux; amd64) madmin-go/0.0.1 mc/RELEASE.2022-03-09T02-08-36Z
Authorization: AWS4-HMAC-SHA256 Credential=minio/20220311//s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20220311T141307Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 500 Internal Server Error
Content-Length: 243
Accept-Ranges: bytes
Content-Security-Policy: block-all-mixed-content
Content-Type: application/json
Date: Fri, 11 Mar 2022 14:13:07 GMT
Server: MinIO
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Origin
Vary: Accept-Encoding
X-Amz-Request-Id: 16DB5909C9FD575F
X-Content-Type-Options: nosniff
X-Xss-Protection: 1; mode=block

{"Code":"InternalError","Message":"We encountered an internal error, please try again. (signature is invalid)","Resource":"/minio/admin/v3/delete-service-account","RequestId":"16DB5909C9FD575F","HostId":"5642f036-1ddf-4a7c-84b4-9f711b3ff0f2"}
mc: <DEBUG> Response Time:  5.181206ms

mc: <ERROR> Unable to remove a new service account. We encountered an internal error, please try again. (signature is invalid)
 (1) admin-user-svcacct-rm.go:69 cmd.mainAdminUserSvcAcctRemove(..) Tags: [provisioning, minio-fmserver]
 (0) admin-user-svcacct-rm.go:69 cmd.mainAdminUserSvcAcctRemove(..)
 Release-Tag:RELEASE.2022-03-09T02-08-36Z | Commit:bd00af5a71bc | Host:didi-Precision-5550 | OS:linux | Arch:amd64 | Lang:go1.17.7 | Mem:3.4 MB/22 MB | Heap:3.4 MB/12 MB.
```

and on the server side I found something like this:
```
API: DeleteServiceAccount()
Time: 13:32:55 UTC 03/11/2022
DeploymentID: 5642f036-1ddf-4a7c-84b4-9f711b3ff0f2
RequestID: 16DB56D82513BC4A
RemoteHost: 172.17.0.15
Host: minio-1.minio-headless.dev.svc.cluster.local:9000
UserAgent: MinIO (linux; amd64) madmin-go/0.0.1 mc/DEVELOPMENT.2022-03-09T03-10-33Z
Error: signature is invalid (*errors.errorString)
       5: cmd/api-errors.go:2112:cmd.toAPIErrorCode()
       4: cmd/admin-handler-utils.go:217:cmd.toAdminAPIErrCode()
       3: cmd/admin-handler-utils.go:204:cmd.toAdminAPIErr()
       2: cmd/admin-handlers-users.go:987:cmd.adminAPIHandlers.DeleteServiceAccount()
       1: net/http/server.go:2047:http.HandlerFunc.ServeHTTP()
```

## Your Environment
* Minio-Version: 2022-03-08T22:28:51Z
* `mc`-Version: RELEASE.2022-03-09T02-08-36Z



Discussion / hints:
@DirkDittmarMonday I am not to reproduce this using the same MinIO/mc versions. Could you describe your MinIO setup ?
> @DirkDittmarMonday I am not to reproduce this using the same MinIO/mc versions. Could you describe your MinIO setup ?

This looks like the user has modified the internal session token @vadmeste 
```
 harsha in ~ λ kubectl exec -it mc /bin/bash
kubectl exec [POD] [COMMAND] is DEPRECATED and will be removed in a future version. Use kubectl exec [POD] -- [COMMAND] instead.
[root@mc /]# mc admin user add all-nodes/ testuser testuser123
Added user `testuser` successfully.
[root@mc /]# mc admin policy set all-nodes readwrite user=testuser
Policy `readwrite` is set on user `testuser`
[root@mc /]# #mc admin user enable all-nodes testuser
[root@mc /]# mc admin user svcacct add all-nodes testuser --access-key hello --secret-key hello1234
Access Key: hello
Secret Key: hello1234
[root@mc /]# mc admin user svcacct rm all-nodes hello

[root@mc /]# 
```

Seems to work fine @DirkDittmarMonday 

> API: DeleteServiceAccount()
> Time: 13:32:55 UTC 03/11/2022
> DeploymentID: 5642f036-1ddf-4a7c-84b4-9f711b3ff0f2
> RequestID: 16DB56D82513BC4A
> RemoteHost: 172.17.0.15
> Host: minio-1.minio-headless.dev.svc.cluster.local:9000
> UserAgent: MinIO (linux; amd64) madmin-go/0.0.1 mc/DEVELOPMENT.2022-03-09T03-10-33Z
> Error: signature is invalid (*errors.errorString)
>        5: cmd/api-errors.go:2112:cmd.toAPIErrorCode()
>        4: cmd/admin-handler-utils.go:217:cmd.toAdminAPIErrCode()
>        3: cmd/admin-handler-utils.go:204:cmd.toAdminAPIErr()
>        2: cmd/admin-handlers-users.go:987:cmd.adminAPIHandlers.DeleteServiceAccount()
>        1: net/http/server.go:2047:http.HandlerFunc.ServeHTTP()

Looks like you have changed the production root credentials here @DirkDittmarMonday 

@vadmeste it's the change in root credentials that causes this problem. 

@vadmeste ... I've build a simple example project and everything works fine. I have to figure out what is going on in kubenetes that leads to the problem ...
@harshavardhana ... I think you're right about the root credentials change. The project is a helm chart and the secret is created with the chart. I get the problem when I uninstall minio and reinstall it. The PVCs are still there after uninstall. But helm deleted the secret and so helm create a new secret on reinstall. The PVCs however will be reused by minio. If I get you right that could cause the problem? 
> @harshavardhana ... I think you're right about the root credentials change. The project is a helm chart and the secret is created with the chart. I get the problem when I uninstall minio and reinstall it. The PVCs are still there after uninstall. But helm deleted the secret and so helm create a new secret on reinstall. The PVCs however will be reused by minio. If I get you right that could cause the problem?

correct its a root credential change you need to preserve the previous root credential for now. I am sending a fix where this can get de-coupled from "Root" credential change. 

Fix patch:
diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go
index 4e1206a218d7d..5bfb332b32bcf 100644
--- a/cmd/auth-handler.go
+++ b/cmd/auth-handler.go
@@ -197,13 +197,7 @@ func mustGetClaimsFromToken(r *http.Request) map[string]interface{} {
 	return claims
 }
 
-// Fetch claims in the security token returned by the client.
-func getClaimsFromToken(token string) (map[string]interface{}, error) {
-	if token == "" {
-		claims := xjwt.NewMapClaims()
-		return claims.Map(), nil
-	}
-
+func getClaimsFromTokenWithSecret(token, secret string) (map[string]interface{}, error) {
 	// JWT token for x-amz-security-token is signed with admin
 	// secret key, temporary credentials become invalid if
 	// server admin credentials change. This is done to ensure
@@ -212,9 +206,15 @@ func getClaimsFromToken(token string) (map[string]interface{}, error) {
 	// hijacking the policies. We need to make sure that this is
 	// based an admin credential such that token cannot be decoded
 	// on the client side and is treated like an opaque value.
-	claims, err := auth.ExtractClaims(token, globalActiveCred.SecretKey)
+	claims, err := auth.ExtractClaims(token, secret)
 	if err != nil {
-		return nil, errAuthentication
+		if subtle.ConstantTimeCompare([]byte(secret), []byte(globalActiveCred.SecretKey)) == 1 {
+			return nil, errAuthentication
+		}
+		claims, err = auth.ExtractClaims(token, globalActiveCred.SecretKey)
+		if err != nil {
+			return nil, errAuthentication
+		}
 	}
 
 	// If OPA is set, return without any further checks.
@@ -241,23 +241,50 @@ func getClaimsFromToken(token string) (map[string]interface{}, error) {
 	return claims.Map(), nil
 }
 
+// Fetch claims in the security token returned by the client.
+func getClaimsFromToken(token string) (map[string]interface{}, error) {
+	return getClaimsFromTokenWithSecret(token, globalActiveCred.SecretKey)
+}
+
 // Fetch claims in the security token returned by the client and validate the token.
 func checkClaimsFromToken(r *http.Request, cred auth.Credentials) (map[string]interface{}, APIErrorCode) {
 	token := getSessionToken(r)
 	if token != "" && cred.AccessKey == "" {
+		// x-amz-security-token is not allowed for anonymous access.
 		return nil, ErrNoAccessKey
 	}
-	if cred.IsServiceAccount() && token == "" {
-		token = cred.SessionToken
+
+	if token == "" && cred.IsTemp() {
+		// Temporary credentials should always have x-amz-security-token
+		return nil, ErrInvalidToken
 	}
-	if subtle.ConstantTimeCompare([]byte(token), []byte(cred.SessionToken)) != 1 {
+
+	if token != "" && !cred.IsTemp() {
+		// x-amz-security-token should not present for static credentials.
 		return nil, ErrInvalidToken
 	}
-	claims, err := getClaimsFromToken(token)
-	if err != nil {
-		return nil, toAPIErrorCode(r.Context(), err)
+
+	if cred.IsTemp() && subtle.ConstantTimeCompare([]byte(token), []byte(cred.SessionToken)) != 1 {
+		// validate token for temporary credentials only.
+		return nil, ErrInvalidToken
 	}
-	return claims, ErrNone
+
+	secret := globalActiveCred.SecretKey
+	if cred.IsServiceAccount() {
+		token = cred.SessionToken
+		secret = cred.SecretKey
+	}
+
+	if token != "" {
+		claims, err := getClaimsFromTokenWithSecret(token, secret)
+		if err != nil {
+			return nil, toAPIErrorCode(r.Context(), err)
+		}
+		return claims, ErrNone
+	}
+
+	claims := xjwt.NewMapClaims()
+	return claims.Map(), ErrNone
 }
 
 // Check request auth type verifies the incoming http request
diff --git a/cmd/iam-store.go b/cmd/iam-store.go
index 1e719afcac4b0..5c126ff321adb 100644
--- a/cmd/iam-store.go
+++ b/cmd/iam-store.go
@@ -1456,13 +1456,28 @@ func (store *IAMStoreSys) GetAllParentUsers() map[string]string {
 
 	res := map[string]string{}
 	for _, cred := range cache.iamUsersMap {
-		if cred.IsServiceAccount() || cred.IsTemp() {
+		if (cred.IsServiceAccount() || cred.IsTemp()) && cred.SessionToken != "" {
 			parentUser := cred.ParentUser
-			if cred.SessionToken != "" {
-				claims, err := getClaimsFromToken(cred.SessionToken)
+
+			var (
+				err    error
+				claims map[string]interface{}
+			)
+
+			if cred.IsServiceAccount() {
+				claims, err = getClaimsFromTokenWithSecret(cred.SessionToken, cred.SecretKey)
 				if err != nil {
-					continue
+					claims, err = getClaimsFromTokenWithSecret(cred.SessionToken, globalActiveCred.SecretKey)
 				}
+			} else if cred.IsTemp() {
+				claims, err = getClaimsFromTokenWithSecret(cred.SessionToken, globalActiveCred.SecretKey)
+			}
+
+			if err != nil {
+				continue
+			}
+
+			if len(claims) > 0 {
 				if v, ok := claims[subClaim]; ok {
 					subFromToken, ok := v.(string)
 					if ok {
@@ -1470,6 +1485,11 @@ func (store *IAMStoreSys) GetAllParentUsers() map[string]string {
 					}
 				}
 			}
+
+			if parentUser == "" {
+				continue
+			}
+
 			if _, ok := res[parentUser]; !ok {
 				res[parentUser] = cred.ParentUser
 			}
@@ -1561,6 +1581,7 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 		return errNoSuchServiceAccount
 	}
 
+	currentSecretKey := cr.SecretKey
 	if opts.secretKey != "" {
 		if !auth.IsSecretKeyValid(opts.secretKey) {
 			return auth.ErrInvalidSecretKeyLength
@@ -1582,20 +1603,21 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 		return errors.New("unknown account status value")
 	}
 
-	if opts.sessionPolicy != nil {
-		m, err := getClaimsFromToken(cr.SessionToken)
-		if err != nil {
-			return fmt.Errorf("unable to get svc acc claims: %v", err)
-		}
+	m, err := getClaimsFromTokenWithSecret(cr.SessionToken, currentSecretKey)
+	if err != nil {
+		return fmt.Errorf("unable to get svc acc claims: %v", err)
+	}
 
-		err = opts.sessionPolicy.Validate()
-		if err != nil {
+	if opts.sessionPolicy != nil {
+		if err := opts.sessionPolicy.Validate(); err != nil {
 			return err
 		}
+
 		policyBuf, err := json.Marshal(opts.sessionPolicy)
 		if err != nil {
 			return err
 		}
+
 		if len(policyBuf) > 16*humanize.KiByte {
 			return fmt.Errorf("Session policy should not exceed 16 KiB characters")
 		}
@@ -1603,10 +1625,11 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st
 		// Overwrite session policy claims.
 		m[iampolicy.SessionPolicyName] = base64.StdEncoding.EncodeToString(policyBuf)
 		m[iamPolicyClaimNameSA()] = "embedded-policy"
-		cr.SessionToken, err = auth.JWTSignWithAccessKey(accessKey, m, globalActiveCred.SecretKey)
-		if err != nil {
-			return err
-		}
+	}
+
+	cr.SessionToken, err = auth.JWTSignWithAccessKey(accessKey, m, cr.SecretKey)
+	if err != nil {
+		return err
 	}
 
 	u := newUserIdentity(cr)
diff --git a/cmd/iam.go b/cmd/iam.go
index f8a83dc12dc7f..0a14c205992f8 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -37,6 +37,7 @@ import (
 	"github.com/minio/minio/internal/arn"
 	"github.com/minio/minio/internal/auth"
 	"github.com/minio/minio/internal/color"
+	"github.com/minio/minio/internal/jwt"
 	"github.com/minio/minio/internal/logger"
 	iampolicy "github.com/minio/pkg/iam/policy"
 	etcd "go.etcd.io/etcd/client/v3"
@@ -799,14 +800,17 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro
 		}
 	}
 
-	var cred auth.Credentials
-
+	var accessKey, secretKey string
 	var err error
 	if len(opts.accessKey) > 0 {
-		cred, err = auth.CreateNewCredentialsWithMetadata(opts.accessKey, opts.secretKey, m, globalActiveCred.SecretKey)
+		accessKey, secretKey = opts.accessKey, opts.secretKey
 	} else {
-		cred, err = auth.GetNewCredentialsWithMetadata(m, globalActiveCred.SecretKey)
+		accessKey, secretKey, err = auth.GenerateCredentials()
+		if err != nil {
+			return auth.Credentials{}, err
+		}
 	}
+	cred, err := auth.CreateNewCredentialsWithMetadata(accessKey, secretKey, m, secretKey)
 	if err != nil {
 		return auth.Credentials{}, err
 	}
@@ -880,9 +884,12 @@ func (sys *IAMSys) getServiceAccount(ctx context.Context, accessKey string) (aut
 
 	var embeddedPolicy *iampolicy.Policy
 
-	jwtClaims, err := auth.ExtractClaims(sa.SessionToken, globalActiveCred.SecretKey)
+	jwtClaims, err := auth.ExtractClaims(sa.SessionToken, sa.SecretKey)
 	if err != nil {
-		return auth.Credentials{}, nil, err
+		jwtClaims, err = auth.ExtractClaims(sa.SessionToken, globalActiveCred.SecretKey)
+		if err != nil {
+			return auth.Credentials{}, nil, err
+		}
 	}
 	pt, ptok := jwtClaims.Lookup(iamPolicyClaimNameSA())
 	sp, spok := jwtClaims.Lookup(iampolicy.SessionPolicyName)
@@ -915,9 +922,12 @@ func (sys *IAMSys) GetClaimsForSvcAcc(ctx context.Context, accessKey string) (ma
 		return nil, errNoSuchServiceAccount
 	}
 
-	jwtClaims, err := auth.ExtractClaims(sa.SessionToken, globalActiveCred.SecretKey)
+	jwtClaims, err := auth.ExtractClaims(sa.SessionToken, sa.SecretKey)
 	if err != nil {
-		return nil, err
+		jwtClaims, err = auth.ExtractClaims(sa.SessionToken, globalActiveCred.SecretKey)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return jwtClaims.Map(), nil
 }
@@ -1063,12 +1073,28 @@ func (sys *IAMSys) updateGroupMembershipsForLDAP(ctx context.Context) {
 		if _, ok := parentUserToCredsMap[cred.ParentUser]; !ok {
 			// Try to find the ldapUsername for this
 			// parentUser by extracting JWT claims
-			jwtClaims, err := auth.ExtractClaims(cred.SessionToken, globalActiveCred.SecretKey)
+			var (
+				jwtClaims *jwt.MapClaims
+				err       error
+			)
+
+			if cred.SessionToken == "" {
+				continue
+			}
+
+			if cred.IsServiceAccount() {
+				jwtClaims, err = auth.ExtractClaims(cred.SessionToken, cred.SecretKey)
+				if err != nil {
+					jwtClaims, err = auth.ExtractClaims(cred.SessionToken, globalActiveCred.SecretKey)
+				}
+			} else {
+				jwtClaims, err = auth.ExtractClaims(cred.SessionToken, globalActiveCred.SecretKey)
+			}
 			if err != nil {
-				// skip this cred - session token seems
-				// invalid
+				// skip this cred - session token seems invalid
 				continue
 			}
+
 			ldapUsername, ok := jwtClaims.Lookup(ldapUserN)
 			if !ok {
 				// skip this cred - we dont have the
diff --git a/cmd/jwt.go b/cmd/jwt.go
index f2b18d19fb384..1df0517333ea1 100644
--- a/cmd/jwt.go
+++ b/cmd/jwt.go
@@ -132,7 +132,7 @@ func authenticateURL(accessKey, secretKey string) (string, error) {
 // Check if the request is authenticated.
 // Returns nil if the request is authenticated. errNoAuthToken if token missing.
 // Returns errAuthentication for all other errors.
-func webRequestAuthenticate(req *http.Request) (*xjwt.MapClaims, []string, bool, error) {
+func metricsRequestAuthenticate(req *http.Request) (*xjwt.MapClaims, []string, bool, error) {
 	token, err := jwtreq.AuthorizationHeaderExtractor.ExtractToken(req)
 	if err != nil {
 		if err == jwtreq.ErrNoTokenInRequest {
diff --git a/cmd/metrics.go b/cmd/metrics.go
index e626eddaa6a9e..761d5977766ee 100644
--- a/cmd/metrics.go
+++ b/cmd/metrics.go
@@ -674,7 +674,7 @@ func metricsHandler() http.Handler {
 // AuthMiddleware checks if the bearer token is valid and authorized.
 func AuthMiddleware(h http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		claims, groups, owner, authErr := webRequestAuthenticate(r)
+		claims, groups, owner, authErr := metricsRequestAuthenticate(r)
 		if authErr != nil || !claims.VerifyIssuer("prometheus", true) {
 			w.WriteHeader(http.StatusForbidden)
 			return


Test patch:
diff --git a/cmd/jwt_test.go b/cmd/jwt_test.go
index 8a6b09be21b56..0774839f55596 100644
--- a/cmd/jwt_test.go
+++ b/cmd/jwt_test.go
@@ -149,7 +149,7 @@ func TestWebRequestAuthenticate(t *testing.T) {
 	}
 
 	for i, testCase := range testCases {
-		_, _, _, gotErr := webRequestAuthenticate(testCase.req)
+		_, _, _, gotErr := metricsRequestAuthenticate(testCase.req)
 		if testCase.expectedErr != gotErr {
 			t.Errorf("Test %d, expected err %s, got %s", i+1, testCase.expectedErr, gotErr)
 		}