fix: deprecate requirement of session token for service accounts (#9320)

This PR fixes couple of behaviors with service accounts

- not need to have session token for service accounts
- service accounts can be generated by any user for themselves
  implicitly, with a valid signature.
- policy input for AddNewServiceAccount API is not fully typed
  allowing for validation before it is sent to the server.
- also bring in additional context for admin API errors if any
  when replying back to client.
- deprecate GetServiceAccount API as we do not need to reply
  back session tokens

cmd/admin-handlers-users.go

@@ -379,53 +379,65 @@ func (a adminAPIHandlers) AddUser(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-// AddServiceAccount - PUT /minio/admin/v3/add-service-account?parentUser=<parent_user_accesskey>
+// AddServiceAccount - PUT /minio/admin/v3/add-service-account
 func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Request) {
 	ctx := newContext(r, w, "AddServiceAccount")
 
-	objectAPI, cred := validateAdminUsersReq(ctx, w, r, iampolicy.CreateUserAdminAction)
-	if objectAPI == nil {
+	// Get current object layer instance.
+	objectAPI := newObjectLayerWithoutSafeModeFn()
+	if objectAPI == nil || globalNotificationSys == nil || globalIAMSys == nil {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrServerNotInitialized), r.URL)
 		return
 	}
 
-	// Deny if WORM is enabled
-	if globalWORMEnabled {
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrMethodNotAllowed), r.URL)
-		return
-	}
-
-	if r.ContentLength > maxEConfigJSONSize || r.ContentLength == -1 {
-		// More than maxConfigSize bytes were available
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigTooLarge), r.URL)
+	cred, _, owner, s3Err := validateAdminSignature(ctx, r, "")
+	if s3Err != ErrNone {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL)
 		return
 	}
 
 	password := cred.SecretKey
 	configBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength))
 	if err != nil {
-		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), r.URL)
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErrWithErr(ErrAdminConfigBadJSON, err), r.URL)
 		return
 	}
 
 	var createReq madmin.AddServiceAccountReq
 	if err = json.Unmarshal(configBytes, &createReq); err != nil {
-		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), r.URL)
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErrWithErr(ErrAdminConfigBadJSON, err), r.URL)
 		return
 	}
 
 	if createReq.Parent == "" {
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL)
+		apiErr := APIError{
+			Code:           "XMinioAdminInvalidArgument",
+			Description:    "Service account parent cannot be empty",
+			HTTPStatusCode: http.StatusBadRequest,
+		}
+		writeErrorResponseJSON(ctx, w, apiErr, r.URL)
 		return
 	}
 
-	// Disallow creating service accounts for the root user
+	// Disallow creating service accounts by root user as well.
 	if createReq.Parent == globalActiveCred.AccessKey {
 		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAddServiceAccountInvalidArgument), r.URL)
 		return
 	}
 
+	// Disallow creating service accounts by users who are not the requested parent.
+	// this restriction is not required for Owner account i.e root user.
+	if createReq.Parent != cred.AccessKey && !owner {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAddServiceAccountInvalidParent), r.URL)
+		return
+	}
+
+	// Deny if WORM is enabled
+	if globalWORMEnabled {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrMethodNotAllowed), r.URL)
+		return
+	}
+
 	creds, err := globalIAMSys.NewServiceAccount(ctx, createReq.Parent, createReq.Policy)
 	if err != nil {
 		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
@@ -441,7 +453,10 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 	}
 
 	var createResp = madmin.AddServiceAccountResp{
-		Credentials: creds,
+		Credentials: auth.Credentials{
+			AccessKey: creds.AccessKey,
+			SecretKey: creds.SecretKey,
+		},
 	}
 
 	data, err := json.Marshal(createResp)
@@ -459,41 +474,6 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 	writeSuccessResponseJSON(w, econfigData)
 }
 
-// GetServiceAccount - GET /minio/admin/v3/get-service-account?accessKey=<access_key>
-func (a adminAPIHandlers) GetServiceAccount(w http.ResponseWriter, r *http.Request) {
-	ctx := newContext(r, w, "GetServiceAccount")
-
-	objectAPI, cred := validateAdminUsersReq(ctx, w, r, iampolicy.GetUserAdminAction)
-	if objectAPI == nil {
-		return
-	}
-
-	vars := mux.Vars(r)
-	accessKey := vars["accessKey"]
-
-	password := cred.SecretKey
-
-	creds, err := globalIAMSys.GetServiceAccount(ctx, accessKey)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	data, err := json.Marshal(creds)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	econfigData, err := madmin.EncryptData(password, data)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	writeSuccessResponseJSON(w, econfigData)
-}
-
 // InfoCannedPolicyV2 - GET /minio/admin/v2/info-canned-policy?name={policyName}
 func (a adminAPIHandlers) InfoCannedPolicyV2(w http.ResponseWriter, r *http.Request) {
 	ctx := newContext(r, w, "InfoCannedPolicyV2")

cmd/admin-handlers.go

@@ -942,7 +942,7 @@ func toAdminAPIErr(ctx context.Context, err error) APIError {
 				HTTPStatusCode: http.StatusNotFound,
 			}
 		} else {
-			apiErr = errorCodes.ToAPIErr(toAdminAPIErrCode(ctx, err))
+			apiErr = errorCodes.ToAPIErrWithErr(toAdminAPIErrCode(ctx, err), err)
 		}
 	}
 	return apiErr

cmd/admin-router.go

@@ -115,7 +115,6 @@ func registerAdminRouter(router *mux.Router, enableConfigOps, enableIAMOps bool)
 
 			// Service accounts ops
 			adminRouter.Methods(http.MethodPut).Path(adminVersion + "/add-service-account").HandlerFunc(httpTraceHdrs(adminAPI.AddServiceAccount))
-			adminRouter.Methods(http.MethodGet).Path(adminVersion+"/get-service-account").HandlerFunc(httpTraceHdrs(adminAPI.GetServiceAccount)).Queries("accessKey", "{accessKey:.*}")
 
 			if adminVersion == adminAPIVersionV2Prefix {
 				// Info policy IAM v2

cmd/api-errors.go

@@ -336,19 +336,27 @@ const (
 	ErrInvalidDecompressedSize
 	ErrAddUserInvalidArgument
 	ErrAddServiceAccountInvalidArgument
+	ErrAddServiceAccountInvalidParent
 	ErrPostPolicyConditionInvalidFormat
 )
 
 type errorCodeMap map[APIErrorCode]APIError
 
-func (e errorCodeMap) ToAPIErr(errCode APIErrorCode) APIError {
+func (e errorCodeMap) ToAPIErrWithErr(errCode APIErrorCode, err error) APIError {
 	apiErr, ok := e[errCode]
 	if !ok {
-		return e[ErrInternalError]
+		apiErr = e[ErrInternalError]
+	}
+	if err != nil {
+		apiErr.Description = fmt.Sprintf("%s (%s)", apiErr.Description, err)
 	}
 	return apiErr
 }
 
+func (e errorCodeMap) ToAPIErr(errCode APIErrorCode) APIError {
+	return e.ToAPIErrWithErr(errCode, nil)
+}
+
 // error code to APIError structure, these fields carry respective
 // descriptions for all the error responses.
 var errorCodes = errorCodeMap{
@@ -1593,8 +1601,13 @@ var errorCodes = errorCodeMap{
 		HTTPStatusCode: http.StatusConflict,
 	},
 	ErrAddServiceAccountInvalidArgument: {
-		Code:           "XMinioInvalidArgument",
-		Description:    "New service accounts for admin access key is not allowed",
+		Code:           "XMinioInvalidIAMCredentials",
+		Description:    "Creating service accounts for admin access key is not allowed",
+		HTTPStatusCode: http.StatusConflict,
+	},
+	ErrAddServiceAccountInvalidParent: {
+		Code:           "XMinioInvalidIAMCredentialsParent",
+		Description:    "Creating service accounts for other users is not allowed",
 		HTTPStatusCode: http.StatusConflict,
 	},
 

cmd/auth-handler.go

@@ -121,9 +121,7 @@ func getRequestAuthType(r *http.Request) authType {
 	return authTypeUnknown
 }
 
-// checkAdminRequestAuthType checks whether the request is a valid signature V2 or V4 request.
-// It does not accept presigned or JWT or anonymous requests.
-func checkAdminRequestAuthType(ctx context.Context, r *http.Request, action iampolicy.AdminAction, region string) (auth.Credentials, APIErrorCode) {
+func validateAdminSignature(ctx context.Context, r *http.Request, region string) (auth.Credentials, map[string]interface{}, bool, APIErrorCode) {
 	var cred auth.Credentials
 	var owner bool
 	s3Err := ErrAccessDenied
@@ -132,7 +130,7 @@ func checkAdminRequestAuthType(ctx context.Context, r *http.Request, action iamp
 		// We only support admin credentials to access admin APIs.
 		cred, owner, s3Err = getReqAccessKeyV4(r, region, serviceS3)
 		if s3Err != ErrNone {
-			return cred, s3Err
+			return cred, nil, owner, s3Err
 		}
 
 		// we only support V4 (no presign) with auth body
@@ -144,12 +142,21 @@ func checkAdminRequestAuthType(ctx context.Context, r *http.Request, action iamp
 		logger.LogIf(ctx, errors.New(getAPIError(s3Err).Description), logger.Application)
 	}
 
-	var claims map[string]interface{}
-	claims, s3Err = checkClaimsFromToken(r, cred)
+	claims, s3Err := checkClaimsFromToken(r, cred)
+	if s3Err != ErrNone {
+		return cred, nil, owner, s3Err
+	}
+
+	return cred, claims, owner, ErrNone
+}
+
+// checkAdminRequestAuthType checks whether the request is a valid signature V2 or V4 request.
+// It does not accept presigned or JWT or anonymous requests.
+func checkAdminRequestAuthType(ctx context.Context, r *http.Request, action iampolicy.AdminAction, region string) (auth.Credentials, APIErrorCode) {
+	cred, claims, owner, s3Err := validateAdminSignature(ctx, r, region)
 	if s3Err != ErrNone {
 		return cred, s3Err
 	}
-
 	if globalIAMSys.IsAllowed(iampolicy.Args{
 		AccountName:     cred.AccessKey,
 		Action:          iampolicy.Action(action),
@@ -176,15 +183,13 @@ func getSessionToken(r *http.Request) (token string) {
 // Fetch claims in the security token returned by the client, doesn't return
 // errors - upon errors the returned claims map will be empty.
 func mustGetClaimsFromToken(r *http.Request) map[string]interface{} {
-	claims, _ := getClaimsFromToken(r)
+	claims, _ := getClaimsFromToken(r, getSessionToken(r))
 	return claims
 }
 
 // Fetch claims in the security token returned by the client.
-func getClaimsFromToken(r *http.Request) (map[string]interface{}, error) {
+func getClaimsFromToken(r *http.Request, token string) (map[string]interface{}, error) {
 	claims := xjwt.NewMapClaims()
-
-	token := getSessionToken(r)
 	if token == "" {
 		return claims.Map(), nil
 	}
@@ -245,10 +250,13 @@ func checkClaimsFromToken(r *http.Request, cred auth.Credentials) (map[string]in
 	if token != "" && cred.AccessKey == "" {
 		return nil, ErrNoAccessKey
 	}
+	if cred.IsServiceAccount() && token == "" {
+		token = cred.SessionToken
+	}
 	if subtle.ConstantTimeCompare([]byte(token), []byte(cred.SessionToken)) != 1 {
 		return nil, ErrInvalidToken
 	}
-	claims, err := getClaimsFromToken(r)
+	claims, err := getClaimsFromToken(r, token)
 	if err != nil {
 		return nil, toAPIErrorCode(r.Context(), err)
 	}

cmd/bucket-handlers.go

@@ -303,7 +303,7 @@ func (api objectAPIHandlers) ListBucketsHandler(w http.ResponseWriter, r *http.R
 
 		// err will be nil here as we already called this function
 		// earlier in this request.
-		claims, _ := getClaimsFromToken(r)
+		claims, _ := getClaimsFromToken(r, getSessionToken(r))
 		n := 0
 		// Use the following trick to filter in place
 		// https://github.com/golang/go/wiki/SliceTricks#filter-in-place

cmd/iam.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/base64"
+	"encoding/json"
 	"fmt"
 
 	"github.com/minio/minio-go/v6/pkg/set"
@@ -764,24 +765,24 @@ func (sys *IAMSys) SetUserStatus(accessKey string, status madmin.AccountStatus)
 }
 
 // NewServiceAccount - create a new service account
-func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser, sessionPolicy string) (auth.Credentials, error) {
+func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, sessionPolicy *iampolicy.Policy) (auth.Credentials, error) {
 	objectAPI := newObjectLayerWithoutSafeModeFn()
 	if objectAPI == nil || sys == nil || sys.store == nil {
 		return auth.Credentials{}, errServerNotInitialized
 	}
 
-	if len(sessionPolicy) > 16*1024 {
-		return auth.Credentials{}, fmt.Errorf("Session policy should not exceed 16 KiB characters")
-	}
-
-	if len(sessionPolicy) > 0 {
-		policy, err := iampolicy.ParseConfig(bytes.NewReader([]byte(sessionPolicy)))
+	var policyBuf []byte
+	if sessionPolicy != nil {
+		err := sessionPolicy.Validate()
 		if err != nil {
 			return auth.Credentials{}, err
 		}
-		// Version in policy must not be empty
-		if policy.Version == "" {
-			return auth.Credentials{}, fmt.Errorf("Invalid session policy version")
+		policyBuf, err = json.Marshal(sessionPolicy)
+		if err != nil {
+			return auth.Credentials{}, err
+		}
+		if len(policyBuf) > 16*1024 {
+			return auth.Credentials{}, fmt.Errorf("Session policy should not exceed 16 KiB characters")
 		}
 	}
 
@@ -808,8 +809,8 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser, sessionPol
 	m := make(map[string]interface{})
 	m[parentClaim] = parentUser
 
-	if len(sessionPolicy) > 0 {
-		m[iampolicy.SessionPolicyName] = base64.StdEncoding.EncodeToString([]byte(sessionPolicy))
+	if len(policyBuf) > 0 {
+		m[iampolicy.SessionPolicyName] = base64.StdEncoding.EncodeToString(policyBuf)
 		m[iamPolicyClaimNameSA()] = "embedded-policy"
 	} else {
 		m[iamPolicyClaimNameSA()] = "inherited-policy"
@@ -833,32 +834,6 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser, sessionPol
 	return cred, nil
 }
 
-// GetServiceAccount - returns the credentials of the given service account
-func (sys *IAMSys) GetServiceAccount(ctx context.Context, serviceAccountAccessKey string) (auth.Credentials, error) {
-	objectAPI := newObjectLayerWithoutSafeModeFn()
-	if objectAPI == nil || sys == nil || sys.store == nil {
-		return auth.Credentials{}, errServerNotInitialized
-	}
-
-	sys.store.lock()
-	defer sys.store.unlock()
-
-	if sys.usersSysType != MinIOUsersSysType {
-		return auth.Credentials{}, errIAMActionNotAllowed
-	}
-
-	cr, ok := sys.iamUsersMap[serviceAccountAccessKey]
-	if !ok {
-		return auth.Credentials{}, errNoSuchUser
-	}
-
-	if !cr.IsServiceAccount() {
-		return auth.Credentials{}, errIAMActionNotAllowed
-	}
-
-	return cr, nil
-}
-
 // SetUser - set user credentials and policy.
 func (sys *IAMSys) SetUser(accessKey string, uinfo madmin.UserInfo) error {
 	objectAPI := newObjectLayerWithoutSafeModeFn()
@@ -1472,53 +1447,50 @@ func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parent string) b
 
 	parentArgs := args
 	parentArgs.AccountName = parent
-	if !combinedPolicy.IsAllowed(parentArgs) {
+
+	saPolicyClaim, ok := args.Claims[iamPolicyClaimNameSA()]
+	if !ok {
 		return false
 	}
 
-	saPolicyClaim, ok := args.Claims[iamPolicyClaimNameSA()]
-	if ok {
-		saPolicyClaimStr, ok := saPolicyClaim.(string)
-		if !ok {
-			// Sub policy if set, should be a string reject
-			// malformed/malicious requests.
-			return false
-		}
+	saPolicyClaimStr, ok := saPolicyClaim.(string)
+	if !ok {
+		// Sub policy if set, should be a string reject
+		// malformed/malicious requests.
+		return false
+	}
 
-		if saPolicyClaimStr == "inherited-policy" {
-			// Immediately returns true since at this stage, since
-			// parent user is allowed to do this action.
-			return true
-		}
+	if saPolicyClaimStr == "inherited-policy" {
+		return combinedPolicy.IsAllowed(parentArgs)
 	}
 
 	// Now check if we have a sessionPolicy.
 	spolicy, ok := args.Claims[iampolicy.SessionPolicyName]
-	if ok {
-		spolicyStr, ok := spolicy.(string)
-		if !ok {
-			// Sub policy if set, should be a string reject
-			// malformed/malicious requests.
-			return false
-		}
-
-		// Check if policy is parseable.
-		subPolicy, err := iampolicy.ParseConfig(bytes.NewReader([]byte(spolicyStr)))
-		if err != nil {
-			// Log any error in input session policy config.
-			logger.LogIf(context.Background(), err)
-			return false
-		}
-
-		// Policy without Version string value reject it.
-		if subPolicy.Version == "" {
-			return false
-		}
-
-		return subPolicy.IsAllowed(args)
+	if !ok {
+		return false
 	}
 
-	return false
+	spolicyStr, ok := spolicy.(string)
+	if !ok {
+		// Sub policy if set, should be a string reject
+		// malformed/malicious requests.
+		return false
+	}
+
+	// Check if policy is parseable.
+	subPolicy, err := iampolicy.ParseConfig(bytes.NewReader([]byte(spolicyStr)))
+	if err != nil {
+		// Log any error in input session policy config.
+		logger.LogIf(context.Background(), err)
+		return false
+	}
+
+	// Policy without Version string value reject it.
+	if subPolicy.Version == "" {
+		return false
+	}
+
+	return combinedPolicy.IsAllowed(parentArgs) && subPolicy.IsAllowed(parentArgs)
 }
 
 // IsAllowedSTS is meant for STS based temporary credentials,