From 0ccd573229cb86e4056fdcc7ce9d0f5c2440a273 Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Tue, 2 Mar 2021 18:24:22 -0800 Subject: [PATCH] feat: regenerate active users token if it is expiring soon (#5629) * feat: regenerate active users token if it is expiring soon Signed-off-by: Alexander Matyushentsev * Comment how 'renew-token' header is used Signed-off-by: Alexander Matyushentsev --- server/logout/logout.go | 4 +- server/logout/logout_test.go | 18 ++++---- server/project/project_test.go | 6 +-- server/server.go | 69 +++++++++++++++++++---------- util/session/sessionmanager.go | 64 ++++++++++++++++---------- util/session/sessionmanager_test.go | 42 ++++++++++++++---- 6 files changed, 133 insertions(+), 70 deletions(-) diff --git a/server/logout/logout.go b/server/logout/logout.go index 6b8877ba3ef7c..c4b5d60d3b835 100644 --- a/server/logout/logout.go +++ b/server/logout/logout.go @@ -36,7 +36,7 @@ type Handler struct { appClientset versioned.Interface settingsMgr *settings.SettingsManager rootPath string - verifyToken func(tokenString string) (jwt.Claims, error) + verifyToken func(tokenString string) (jwt.Claims, string, error) revokeToken func(ctx context.Context, id string, expiringAt time.Duration) error } @@ -85,7 +85,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Add("Set-Cookie", argocdCookie.String()) } - claims, err := h.verifyToken(tokenString) + claims, _, err := h.verifyToken(tokenString) if err != nil { http.Redirect(w, r, logoutRedirectURL, http.StatusSeeOther) return diff --git a/server/logout/logout_test.go b/server/logout/logout_test.go index c9478964a35ae..d141b341a1c72 100644 --- a/server/logout/logout_test.go +++ b/server/logout/logout_test.go @@ -180,25 +180,25 @@ func TestHandlerConstructLogoutURL(t *testing.T) { sessionManager := session.NewSessionManager(settingsManagerWithOIDCConfig, test.NewFakeProjLister(), "", session.NewUserStateStorage(nil)) oidcHandler := NewHandler(appclientset.NewSimpleClientset(), settingsManagerWithOIDCConfig, sessionManager, "", "default") - oidcHandler.verifyToken = func(tokenString string) (jwt.Claims, error) { + oidcHandler.verifyToken = func(tokenString string) (jwt.Claims, string, error) { if !validJWTPattern.MatchString(tokenString) { - return nil, errors.New("invalid jwt") + return nil, "", errors.New("invalid jwt") } - return &jwt.StandardClaims{Issuer: "okta"}, nil + return &jwt.StandardClaims{Issuer: "okta"}, "", nil } nonoidcHandler := NewHandler(appclientset.NewSimpleClientset(), settingsManagerWithoutOIDCConfig, sessionManager, "", "default") - nonoidcHandler.verifyToken = func(tokenString string) (jwt.Claims, error) { + nonoidcHandler.verifyToken = func(tokenString string) (jwt.Claims, string, error) { if !validJWTPattern.MatchString(tokenString) { - return nil, errors.New("invalid jwt") + return nil, "", errors.New("invalid jwt") } - return &jwt.StandardClaims{Issuer: session.SessionManagerClaimsIssuer}, nil + return &jwt.StandardClaims{Issuer: session.SessionManagerClaimsIssuer}, "", nil } oidcHandlerWithoutLogoutURL := NewHandler(appclientset.NewSimpleClientset(), settingsManagerWithOIDCConfigButNoLogoutURL, sessionManager, "", "default") - oidcHandlerWithoutLogoutURL.verifyToken = func(tokenString string) (jwt.Claims, error) { + oidcHandlerWithoutLogoutURL.verifyToken = func(tokenString string) (jwt.Claims, string, error) { if !validJWTPattern.MatchString(tokenString) { - return nil, errors.New("invalid jwt") + return nil, "", errors.New("invalid jwt") } - return &jwt.StandardClaims{Issuer: "okta"}, nil + return &jwt.StandardClaims{Issuer: "okta"}, "", nil } oidcTokenHeader := make(map[string][]string) diff --git a/server/project/project_test.go b/server/project/project_test.go index 577547d295b63..0a6d69d444d44 100644 --- a/server/project/project_test.go +++ b/server/project/project_test.go @@ -347,7 +347,7 @@ func TestProjectServer(t *testing.T) { projectServer := NewServer("default", fake.NewSimpleClientset(), clientset, enforcer, sync.NewKeyLock(), sessionMgr, policyEnf, projInformer, settingsMgr) tokenResponse, err := projectServer.CreateToken(context.Background(), &project.ProjectTokenCreateRequest{Project: projectWithRole.Name, Role: tokenName, ExpiresIn: 100}) assert.NoError(t, err) - claims, err := sessionMgr.Parse(tokenResponse.Token) + claims, _, err := sessionMgr.Parse(tokenResponse.Token) assert.NoError(t, err) mapClaims, err := jwtutil.MapClaims(claims) @@ -367,7 +367,7 @@ func TestProjectServer(t *testing.T) { projectServer := NewServer("default", fake.NewSimpleClientset(), clientset, enforcer, sync.NewKeyLock(), sessionMgr, policyEnf, projInformer, settingsMgr) tokenResponse, err := projectServer.CreateToken(context.Background(), &project.ProjectTokenCreateRequest{Project: projectWithRole.Name, Role: tokenName, ExpiresIn: 1, Id: id}) assert.NoError(t, err) - claims, err := sessionMgr.Parse(tokenResponse.Token) + claims, _, err := sessionMgr.Parse(tokenResponse.Token) assert.NoError(t, err) mapClaims, err := jwtutil.MapClaims(claims) @@ -388,7 +388,7 @@ func TestProjectServer(t *testing.T) { tokenResponse, err := projectServer.CreateToken(context.Background(), &project.ProjectTokenCreateRequest{Project: projectWithRole.Name, Role: tokenName, ExpiresIn: 1, Id: id}) assert.NoError(t, err) - claims, err := sessionMgr.Parse(tokenResponse.Token) + claims, _, err := sessionMgr.Parse(tokenResponse.Token) assert.NoError(t, err) mapClaims, err := jwtutil.MapClaims(claims) diff --git a/server/server.go b/server/server.go index 5da5835a34d32..7b46d88276bf8 100644 --- a/server/server.go +++ b/server/server.go @@ -106,6 +106,7 @@ import ( const maxConcurrentLoginRequestsCountEnv = "ARGOCD_MAX_CONCURRENT_LOGIN_REQUESTS_COUNT" const replicasCountEnv = "ARGOCD_API_SERVER_REPLICAS" +const renewTokenKey = "renew-token" // ErrNoSession indicates no auth token was supplied as part of a request var ErrNoSession = status.Errorf(codes.Unauthenticated, "no session information") @@ -595,30 +596,44 @@ func (a *ArgoCDServer) newGRPCServer() *grpc.Server { return grpcS } -// TranslateGrpcCookieHeader conditionally sets a cookie on the response. +// translateGrpcCookieHeader conditionally sets a cookie on the response. func (a *ArgoCDServer) translateGrpcCookieHeader(ctx context.Context, w http.ResponseWriter, resp golang_proto.Message) error { if sessionResp, ok := resp.(*sessionpkg.SessionResponse); ok { - cookiePath := fmt.Sprintf("path=/%s", strings.TrimRight(strings.TrimLeft(a.ArgoCDServerOpts.RootPath, "/"), "/")) - flags := []string{cookiePath, "SameSite=lax", "httpOnly"} - if !a.Insecure { - flags = append(flags, "Secure") - } token := sessionResp.Token - if token != "" { - var err error - token, err = zjwt.ZJWT(token) - if err != nil { - return err - } - } - cookies, err := httputil.MakeCookieMetadata(common.AuthCookieName, token, flags...) + err := a.setTokenCookie(token, w) if err != nil { return err } - for _, cookie := range cookies { - w.Header().Add("Set-Cookie", cookie) + } else if md, ok := runtime.ServerMetadataFromContext(ctx); ok { + renewToken := md.HeaderMD[renewTokenKey] + if len(renewToken) > 0 { + return a.setTokenCookie(renewToken[0], w) } } + + return nil +} + +func (a *ArgoCDServer) setTokenCookie(token string, w http.ResponseWriter) error { + cookiePath := fmt.Sprintf("path=/%s", strings.TrimRight(strings.TrimLeft(a.ArgoCDServerOpts.RootPath, "/"), "/")) + flags := []string{cookiePath, "SameSite=lax", "httpOnly"} + if !a.Insecure { + flags = append(flags, "Secure") + } + if token != "" { + var err error + token, err = zjwt.ZJWT(token) + if err != nil { + return err + } + } + cookies, err := httputil.MakeCookieMetadata(common.AuthCookieName, token, flags...) + if err != nil { + return err + } + for _, cookie := range cookies { + w.Header().Add("Set-Cookie", cookie) + } return nil } @@ -895,11 +910,19 @@ func (a *ArgoCDServer) Authenticate(ctx context.Context) (context.Context, error if a.DisableAuth { return ctx, nil } - claims, claimsErr := a.getClaims(ctx) + claims, newToken, claimsErr := a.getClaims(ctx) if claims != nil { // Add claims to the context to inspect for RBAC // nolint:staticcheck ctx = context.WithValue(ctx, "claims", claims) + if newToken != "" { + // Session tokens that are expiring soon should be regenerated if user stays active. + // The renewed token is stored in outgoing ServerMetadata. Metadata is available to grpc-gateway + // response forwarder that will translate it into Set-Cookie header. + if err := grpc.SendHeader(ctx, metadata.New(map[string]string{renewTokenKey: newToken})); err != nil { + log.Warnf("Failed to set %s header", renewTokenKey) + } + } } if claimsErr != nil { @@ -915,20 +938,20 @@ func (a *ArgoCDServer) Authenticate(ctx context.Context) (context.Context, error return ctx, nil } -func (a *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, error) { +func (a *ArgoCDServer) getClaims(ctx context.Context) (jwt.Claims, string, error) { md, ok := metadata.FromIncomingContext(ctx) if !ok { - return nil, ErrNoSession + return nil, "", ErrNoSession } tokenString := getToken(md) if tokenString == "" { - return nil, ErrNoSession + return nil, "", ErrNoSession } - claims, err := a.sessionMgr.VerifyToken(tokenString) + claims, newToken, err := a.sessionMgr.VerifyToken(tokenString) if err != nil { - return claims, status.Errorf(codes.Unauthenticated, "invalid session: %v", err) + return claims, "", status.Errorf(codes.Unauthenticated, "invalid session: %v", err) } - return claims, nil + return claims, newToken, nil } // getToken extracts the token from gRPC metadata or cookie headers diff --git a/util/session/sessionmanager.go b/util/session/sessionmanager.go index 7d354f73395c8..a84aedb6a5a04 100644 --- a/util/session/sessionmanager.go +++ b/util/session/sessionmanager.go @@ -15,6 +15,7 @@ import ( oidc "github.com/coreos/go-oidc" "github.com/dgrijalva/jwt-go/v4" + "github.com/google/uuid" log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -56,11 +57,12 @@ const ( SessionManagerClaimsIssuer = "argocd" // invalidLoginError, for security purposes, doesn't say whether the username or password was invalid. This does not mitigate the potential for timing attacks to determine which is which. - invalidLoginError = "Invalid username or password" - blankPasswordError = "Blank passwords are not allowed" - accountDisabled = "Account %s is disabled" - usernameTooLongError = "Username is too long (%d bytes max)" - userDoesNotHaveCapability = "Account %s does not have %s capability" + invalidLoginError = "Invalid username or password" + blankPasswordError = "Blank passwords are not allowed" + accountDisabled = "Account %s is disabled" + usernameTooLongError = "Username is too long (%d bytes max)" + userDoesNotHaveCapability = "Account %s does not have %s capability" + autoRegenerateTokenDuration = time.Minute * 5 ) const ( @@ -230,7 +232,7 @@ func GetSubjectAccountAndCapability(subject string) (string, settings.AccountCap } // Parse tries to parse the provided string and returns the token claims for local login. -func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, error) { +func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, string, error) { // Parse takes the token string and a function for looking up the key. The latter is especially // useful if you use multiple keys for your application. The standard is to use 'kid' in the // head of the token to identify which key to use, but the parsed token (head and claims) is provided @@ -238,7 +240,7 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, error) { var claims jwt.MapClaims argoCDSettings, err := mgr.settingsMgr.GetSettings() if err != nil { - return nil, err + return nil, "", err } token, err := jwt.ParseWithClaims(tokenString, &claims, func(token *jwt.Token) (interface{}, error) { // Don't forget to validate the alg is what you expect: @@ -248,12 +250,12 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, error) { return argoCDSettings.ServerSignature, nil }) if err != nil { - return nil, err + return nil, "", err } issuedAt, err := jwtutil.IssuedAtTime(claims) if err != nil { - return nil, err + return nil, "", err } subject := jwtutil.StringField(claims, "sub") @@ -262,14 +264,14 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, error) { if projName, role, ok := rbacpolicy.GetProjectRoleFromSubject(subject); ok { proj, err := mgr.projectsLister.Get(projName) if err != nil { - return nil, err + return nil, "", err } _, _, err = proj.GetJWTToken(role, issuedAt.Unix(), id) if err != nil { - return nil, err + return nil, "", err } - return token.Claims, nil + return token.Claims, "", nil } subject, capability := GetSubjectAccountAndCapability(subject) @@ -277,27 +279,41 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, error) { account, err := mgr.settingsMgr.GetAccount(subject) if err != nil { - return nil, err + return nil, "", err } if !account.Enabled { - return nil, fmt.Errorf("account %s is disabled", subject) + return nil, "", fmt.Errorf("account %s is disabled", subject) } if !account.HasCapability(capability) { - return nil, fmt.Errorf("account %s does not have '%s' capability", subject, capability) + return nil, "", fmt.Errorf("account %s does not have '%s' capability", subject, capability) } if id == "" || mgr.storage.IsTokenRevoked(id) { - return nil, errors.New("token is revoked, please re-login") + return nil, "", errors.New("token is revoked, please re-login") } else if capability == settings.AccountCapabilityApiKey && account.TokenIndex(id) == -1 { - return nil, fmt.Errorf("account %s does not have token with id %s", subject, id) + return nil, "", fmt.Errorf("account %s does not have token with id %s", subject, id) } if account.PasswordMtime != nil && issuedAt.Before(*account.PasswordMtime) { - return nil, fmt.Errorf("Account password has changed since token issued") + return nil, "", fmt.Errorf("Account password has changed since token issued") + } + + newToken := "" + if exp, err := jwtutil.ExpirationTime(claims); err == nil { + tokenExpDuration := exp.Sub(issuedAt) + remainingDuration := time.Until(exp) + + if remainingDuration < autoRegenerateTokenDuration && capability == settings.AccountCapabilityLogin { + if uniqueId, err := uuid.NewRandom(); err == nil { + if val, err := mgr.Create(fmt.Sprintf("%s:%s", subject, settings.AccountCapabilityLogin), int64(tokenExpDuration.Seconds()), uniqueId.String()); err == nil { + newToken = val + } + } + } } - return token.Claims, nil + return token.Claims, newToken, nil } // GetLoginFailures retrieves the login failure information from the cache @@ -481,14 +497,14 @@ func (mgr *SessionManager) VerifyUsernamePassword(username string, password stri // VerifyToken verifies if a token is correct. Tokens can be issued either from us or by an IDP. // We choose how to verify based on the issuer. -func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, error) { +func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, string, error) { parser := &jwt.Parser{ ValidationHelper: jwt.NewValidationHelper(jwt.WithoutClaimsValidation(), jwt.WithoutAudienceValidation()), } var claims jwt.StandardClaims _, _, err := parser.ParseUnverified(tokenString, &claims) if err != nil { - return nil, err + return nil, "", err } switch claims.Issuer { case SessionManagerClaimsIssuer: @@ -498,7 +514,7 @@ func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, error) { // IDP signed token prov, err := mgr.provider() if err != nil { - return claims, err + return claims, "", err } // Token must be verified for at least one audience @@ -511,11 +527,11 @@ func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, error) { } } if err != nil { - return claims, err + return claims, "", err } var claims jwt.MapClaims err = idToken.Claims(&claims) - return claims, err + return claims, "", err } } diff --git a/util/session/sessionmanager_test.go b/util/session/sessionmanager_test.go index 4f2fe2ab583ea..1ebda7cb3fa73 100644 --- a/util/session/sessionmanager_test.go +++ b/util/session/sessionmanager_test.go @@ -89,10 +89,9 @@ func TestSessionManager_AdminToken(t *testing.T) { t.Errorf("Could not create token: %v", err) } - claims, err := mgr.Parse(token) - if err != nil { - t.Errorf("Could not parse token: %v", err) - } + claims, newToken, err := mgr.Parse(token) + assert.NoError(t, err) + assert.Empty(t, newToken) mapClaims := *(claims.(*jwt.MapClaims)) subject := mapClaims["sub"].(string) @@ -101,6 +100,31 @@ func TestSessionManager_AdminToken(t *testing.T) { } } +func TestSessionManager_AdminToken_ExpiringSoon(t *testing.T) { + redisClient, closer := test.NewInMemoryRedis() + defer closer() + + settingsMgr := settings.NewSettingsManager(context.Background(), getKubeClient("pass", true), "argocd") + mgr := newSessionManager(settingsMgr, getProjLister(), NewUserStateStorage(redisClient)) + + token, err := mgr.Create("admin:login", int64(autoRegenerateTokenDuration.Seconds()-1), "123") + if err != nil { + t.Errorf("Could not create token: %v", err) + } + + // verify new token is generated is login token is expiring soon + _, newToken, err := mgr.Parse(token) + assert.NoError(t, err) + assert.NotEmpty(t, newToken) + + // verify that new token is valid and for the same user + claims, _, err := mgr.Parse(newToken) + assert.NoError(t, err) + mapClaims := *(claims.(*jwt.MapClaims)) + subject := mapClaims["sub"].(string) + assert.Equal(t, "admin", subject) +} + func TestSessionManager_AdminToken_Revoked(t *testing.T) { redisClient, closer := test.NewInMemoryRedis() defer closer() @@ -116,7 +140,7 @@ func TestSessionManager_AdminToken_Revoked(t *testing.T) { err = storage.RevokeToken(context.Background(), "123", time.Hour) require.NoError(t, err) - _, err = mgr.Parse(token) + _, _, err = mgr.Parse(token) require.Error(t, err) assert.Equal(t, "token is revoked, please re-login", err.Error()) } @@ -130,7 +154,7 @@ func TestSessionManager_AdminToken_Deactivated(t *testing.T) { t.Errorf("Could not create token: %v", err) } - _, err = mgr.Parse(token) + _, _, err = mgr.Parse(token) require.Error(t, err) assert.Contains(t, err.Error(), "account admin is disabled") } @@ -144,7 +168,7 @@ func TestSessionManager_AdminToken_LoginCapabilityDisabled(t *testing.T) { t.Errorf("Could not create token: %v", err) } - _, err = mgr.Parse(token) + _, _, err = mgr.Parse(token) require.Error(t, err) assert.Contains(t, err.Error(), "account admin does not have 'apiKey' capability") } @@ -170,7 +194,7 @@ func TestSessionManager_ProjectToken(t *testing.T) { jwtToken, err := mgr.Create("proj:default:test", 100, "abc") require.NoError(t, err) - _, err = mgr.Parse(jwtToken) + _, _, err = mgr.Parse(jwtToken) assert.NoError(t, err) }) @@ -188,7 +212,7 @@ func TestSessionManager_ProjectToken(t *testing.T) { jwtToken, err := mgr.Create("proj:default:test", 10, "") require.NoError(t, err) - _, err = mgr.Parse(jwtToken) + _, _, err = mgr.Parse(jwtToken) require.Error(t, err) assert.Contains(t, err.Error(), "does not exist in project 'default'")