From 9c846106fa6a4bd3b35ef99994b4790747f28e63 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 14 Mar 2022 09:09:22 -0700 Subject: [PATCH] decouple service accounts from root credentials (#14534) changing root credentials makes service accounts in-operable, this PR changes the way sessionToken is generated for service accounts. It changes service account behavior to generate sessionToken claims from its own secret instead of using global root credential. Existing credentials will be supported by falling back to verify using root credential. fixes #14530 --- cmd/auth-handler.go | 61 ++++++++++++++++++++++++++++++++------------- cmd/iam-store.go | 53 ++++++++++++++++++++++++++++----------- cmd/iam.go | 50 ++++++++++++++++++++++++++++--------- cmd/jwt.go | 2 +- cmd/jwt_test.go | 2 +- cmd/metrics.go | 2 +- 6 files changed, 123 insertions(+), 47 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 4e1206a21..5bfb332b3 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 subtle.ConstantTimeCompare([]byte(token), []byte(cred.SessionToken)) != 1 { + + if token == "" && cred.IsTemp() { + // Temporary credentials should always have x-amz-security-token return nil, ErrInvalidToken } - claims, err := getClaimsFromToken(token) - if err != nil { - return nil, toAPIErrorCode(r.Context(), err) + + if token != "" && !cred.IsTemp() { + // x-amz-security-token should not present for static credentials. + return nil, ErrInvalidToken } - return claims, ErrNone + + if cred.IsTemp() && subtle.ConstantTimeCompare([]byte(token), []byte(cred.SessionToken)) != 1 { + // validate token for temporary credentials only. + return nil, ErrInvalidToken + } + + 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 1e719afca..5c126ff32 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 f8a83dc12..0a14c2059 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) - if err != nil { - // skip this cred - session token seems - // invalid + 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 + 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 f2b18d19f..1df051733 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/jwt_test.go b/cmd/jwt_test.go index 8a6b09be2..0774839f5 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) } diff --git a/cmd/metrics.go b/cmd/metrics.go index e626eddaa..761d59777 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