From dd3d25a5aef23a1d7639947cdead3f599ac9f3f8 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Thu, 19 May 2022 21:07:59 +0200 Subject: [PATCH 1/2] Deprecate/remove AUTH_JWT_EXPIRY JWT expiration is now controled via AUTH_OAUTH2_ACCESS_TOKEN_LIFETIME --- .env.example | 6 ------ app/boot_levels.go | 7 ++++++- app/options/auth.cue | 8 -------- auth/commands/commands.go | 2 +- pkg/options/options.gen.go | 2 -- tests/helpers/app.go | 2 -- 6 files changed, 7 insertions(+), 20 deletions(-) diff --git a/.env.example b/.env.example index f020d18d9..34d079b11 100644 --- a/.env.example +++ b/.env.example @@ -452,12 +452,6 @@ # Default: 72h # AUTH_OAUTH2_REFRESH_TOKEN_LIFETIME=72h -############################################################################### -# Expiration time for the auth JWT tokens. -# Type: time.Duration -# Default: 720h -# AUTH_JWT_EXPIRY=720h - ############################################################################### # Redirect URL to be sent with OAuth2 authentication request to provider # diff --git a/app/boot_levels.go b/app/boot_levels.go index 6e6e43b17..b10c7fddb 100644 --- a/app/boot_levels.go +++ b/app/boot_levels.go @@ -98,6 +98,11 @@ func (app *CortezaApp) Setup() (err error) { "so instead use environment variable MINIO_BUCKET, " + "we have extended it to have more flexibility over minio bucket name") } + + if _, is := os.LookupEnv("AUTH_JWT_EXPIRY"); is { + log.Warn("AUTH_JWT_EXPIRY is removed. " + + "JWT expiration value is set from AUTH_OAUTH2_ACCESS_TOKEN_LIFETIME") + } } hcd := healthcheck.Defaults() @@ -344,7 +349,7 @@ func (app *CortezaApp) InitServices(ctx context.Context) (err error) { auth.WithSecretSigner(app.Opt.Auth.Secret), // @todo implement configurable issuer claim //auth.WithDefaultIssuer(app.Opt.Auth.TokenClaimIssuer), - auth.WithDefaultExpiration(app.Opt.Auth.Expiry), + auth.WithDefaultExpiration(app.Opt.Auth.AccessTokenLifetime), auth.WithDefaultClientID(app.DefaultAuthClient.ID), auth.WithLookup(func(ctx context.Context, accessToken string) (err error) { _, err = store.LookupAuthOa2tokenByAccess(ctx, app.Store, accessToken) diff --git a/app/options/auth.cue b/app/options/auth.cue index a861679fb..a1924657e 100644 --- a/app/options/auth.cue +++ b/app/options/auth.cue @@ -58,14 +58,6 @@ auth: schema.#optionsGroup & { defaultGoExpr: "time.Hour * 24 * 3" defaultValue: "72h" } - expiry: { - type: "time.Duration" - description: "Expiration time for the auth JWT tokens." - env: "AUTH_JWT_EXPIRY" - - defaultGoExpr: "time.Hour * 24 * 30" - defaultValue: "720h" - } external_redirect_URL: { description: """ Redirect URL to be sent with OAuth2 authentication request to provider diff --git a/auth/commands/commands.go b/auth/commands/commands.go index 7fb6497ab..9e8a493b1 100644 --- a/auth/commands/commands.go +++ b/auth/commands/commands.go @@ -144,7 +144,7 @@ func Command(ctx context.Context, app serviceInitializer, storeInit func(ctx con jwtCmd.Flags().DurationVar( &tokenDuration, "duration", - app.Options().Auth.Expiry, + app.Options().Auth.AccessTokenLifetime, "Token expiry duration") testEmails := &cobra.Command{ diff --git a/pkg/options/options.gen.go b/pkg/options/options.gen.go index ed61d8544..665e48315 100644 --- a/pkg/options/options.gen.go +++ b/pkg/options/options.gen.go @@ -99,7 +99,6 @@ type ( Secret string `env:"AUTH_JWT_SECRET"` AccessTokenLifetime time.Duration `env:"AUTH_OAUTH2_ACCESS_TOKEN_LIFETIME"` RefreshTokenLifetime time.Duration `env:"AUTH_OAUTH2_REFRESH_TOKEN_LIFETIME"` - Expiry time.Duration `env:"AUTH_JWT_EXPIRY"` ExternalRedirectURL string `env:"AUTH_EXTERNAL_REDIRECT_URL"` ExternalCookieSecret string `env:"AUTH_EXTERNAL_COOKIE_SECRET"` BaseURL string `env:"AUTH_BASE_URL"` @@ -515,7 +514,6 @@ func Auth() (o *AuthOpt) { Secret: getSecretFromEnv("jwt secret"), AccessTokenLifetime: time.Hour * 2, RefreshTokenLifetime: time.Hour * 24 * 3, - Expiry: time.Hour * 24 * 30, ExternalRedirectURL: fullURL("/auth/external/{provider}/callback"), ExternalCookieSecret: getSecretFromEnv("external cookie secret"), BaseURL: fullURL("/auth"), diff --git a/tests/helpers/app.go b/tests/helpers/app.go index 776f40297..4f0e934f4 100644 --- a/tests/helpers/app.go +++ b/tests/helpers/app.go @@ -2,7 +2,6 @@ package helpers import ( "context" - "time" "github.com/cortezaproject/corteza-server/app" "github.com/cortezaproject/corteza-server/pkg/cli" @@ -30,7 +29,6 @@ func NewIntegrationTestApp(ctx context.Context, initTestServices func(*app.Corte // Create a new JWT secret (to prevent any security weirdness) a.Opt.Auth.Secret = string(rand.Bytes(32)) - a.Opt.Auth.Expiry = time.Minute a.Opt.Auth.DefaultClient = "" a.Log = logger.Default() From 691e3e2900b4645391460f90679f5c689dfd870e Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Thu, 19 May 2022 21:11:59 +0200 Subject: [PATCH 2/2] Fix session & cookie exp. setting logic Ensures that cookies on non-permanent login are set without max-age to ensure removal when browser/tab is closed. Sessions stored in the database are now updated and expiration value slides forward in time with every activity from the user. --- .env.example | 14 +++- app/boot_levels.go | 11 ++++ app/options/auth.cue | 23 +++++-- auth/assets/templates/login.html.tpl | 2 + auth/handlers/handle_create-password.go | 2 +- auth/handlers/handle_external.go | 3 - auth/handlers/handle_login.go | 3 +- auth/handlers/handle_password-reset.go | 2 +- auth/handlers/handle_signup.go | 4 +- auth/handlers/mock_test.go | 3 +- auth/request/auth_user.go | 24 +++---- auth/request/corteza_session_store.go | 85 ++++++++++++++++--------- auth/request/session.go | 11 +++- 13 files changed, 127 insertions(+), 60 deletions(-) diff --git a/.env.example b/.env.example index 34d079b11..5a6831bbb 100644 --- a/.env.example +++ b/.env.example @@ -504,13 +504,23 @@ # AUTH_SESSION_COOKIE_SECURE= ############################################################################### -# How long do we keep the temporary session +# Maximum time user is allowed to stay idle when logged in without "remember-me" option and before session is expired. +# +# Recomended value is between an hour and a day. +# +# [IMPORTANT] +# ==== +# This affects only profile (/auth) pages. Using applications (admin, compose, ...) does not prolong the session. +# ==== +# # Type: time.Duration # Default: 24h # AUTH_SESSION_LIFETIME=24h ############################################################################### -# How long do we keep the permanent session +# Duration of the session in /auth lasts when user logs-in with "remember-me" option. +# +# If set to 0, "remember-me" option is removed. # Type: time.Duration # Default: 8640h # AUTH_SESSION_PERM_LIFETIME=8640h diff --git a/app/boot_levels.go b/app/boot_levels.go index b10c7fddb..530043598 100644 --- a/app/boot_levels.go +++ b/app/boot_levels.go @@ -103,6 +103,17 @@ func (app *CortezaApp) Setup() (err error) { log.Warn("AUTH_JWT_EXPIRY is removed. " + "JWT expiration value is set from AUTH_OAUTH2_ACCESS_TOKEN_LIFETIME") } + + if app.Opt.Auth.SessionLifetime < time.Hour { + log.Warn("AUTH_SESSION_LIFETIME is set to less then an hour, this might not be what you want." + + "When user logs-in without 'remember-me', AUTH_SESSION_LIFETIME is used to set a maximum time before session is expired if user does not interacts with Corteza. " + + "Recommended session lifetime value is between one hour (default) and a day") + } + + if app.Opt.Auth.SessionPermLifetime < time.Hour { + log.Warn("AUTH_SESSION_PERM_LIFETIME is set to less then an hour, this might not be what you want. " + + "Recommended permanent session lifetime values are between a day and a year (default)") + } } hcd := healthcheck.Defaults() diff --git a/app/options/auth.cue b/app/options/auth.cue index a1924657e..adbcb3fd5 100644 --- a/app/options/auth.cue +++ b/app/options/auth.cue @@ -105,14 +105,29 @@ auth: schema.#optionsGroup & { description: "Defaults to true when HTTPS is used. Corteza will try to guess the this setting by" } session_lifetime: { - type: "time.Duration" - description: "How long do we keep the temporary session" + type: "time.Duration" + description: """ + Maximum time user is allowed to stay idle when logged in without \"remember-me\" option and before session is expired. + + Recomended value is between an hour and a day. + + [IMPORTANT] + ==== + This affects only profile (/auth) pages. Using applications (admin, compose, ...) does not prolong the session. + ==== + + """ defaultGoExpr: "24 * time.Hour" defaultValue: "24h" } session_perm_lifetime: { - type: "time.Duration" - description: "How long do we keep the permanent session" + type: "time.Duration" + description: """ + Duration of the session in /auth lasts when user logs-in with \"remember-me\" option. + + If set to 0, \"remember-me\" option is removed. + """ + defaultGoExpr: "360 * 24 * time.Hour" defaultValue: "8640h" } diff --git a/auth/assets/templates/login.html.tpl b/auth/assets/templates/login.html.tpl index 3793deb63..32f889e98 100644 --- a/auth/assets/templates/login.html.tpl +++ b/auth/assets/templates/login.html.tpl @@ -43,6 +43,7 @@ autocomplete="current-password" aria-label="{{ tr "login.template.form.password.label" }}"> + {{ if .enableRememberMe }}
+ {{ end }} {{ else }}
diff --git a/auth/handlers/handle_create-password.go b/auth/handlers/handle_create-password.go index e3fd12acd..7358178b6 100644 --- a/auth/handlers/handle_create-password.go +++ b/auth/handlers/handle_create-password.go @@ -32,7 +32,7 @@ func (h *AuthHandlers) createPasswordForm(req *request.AuthReq) (err error) { passwordSet = h.AuthService.PasswordSet(req.Context(), user.Email) if !passwordSet { // login user - req.AuthUser = request.NewAuthUser(h.Settings, user, false, h.Opt.SessionLifetime) + req.AuthUser = request.NewAuthUser(h.Settings, user, false) // redirect back to self (but without token and with user in session) h.Log.Debug("valid password create token found, refreshing page with stored user") diff --git a/auth/handlers/handle_external.go b/auth/handlers/handle_external.go index d3e655414..291155d85 100644 --- a/auth/handlers/handle_external.go +++ b/auth/handlers/handle_external.go @@ -62,9 +62,6 @@ func (h AuthHandlers) handleSuccessfulExternalAuth(w http.ResponseWriter, r *htt // external logins are never permanent! false, - - // set def. lifetime for this session - h.Opt.SessionLifetime, ) // auto-complete EmailOTP and TOTP when authenticating via external identity provider diff --git a/auth/handlers/handle_login.go b/auth/handlers/handle_login.go index 90eb0233a..0b5d4dcad 100644 --- a/auth/handlers/handle_login.go +++ b/auth/handlers/handle_login.go @@ -32,6 +32,7 @@ func (h *AuthHandlers) loginForm(req *request.AuthReq) error { } req.Data["form"] = kv + req.Data["enableRememberMe"] = h.Opt.SessionPermLifetime > 0 return nil } @@ -88,7 +89,7 @@ func (h *AuthHandlers) loginProc(req *request.AuthReq) (err error) { lifetime = h.Opt.SessionPermLifetime } - req.AuthUser = request.NewAuthUser(h.Settings, user, isPerm, lifetime) + req.AuthUser = request.NewAuthUser(h.Settings, user, isPerm) req.AuthUser.Save(req.Session) diff --git a/auth/handlers/handle_password-reset.go b/auth/handlers/handle_password-reset.go index 8c631affa..1bc85f0c2 100644 --- a/auth/handlers/handle_password-reset.go +++ b/auth/handlers/handle_password-reset.go @@ -54,7 +54,7 @@ func (h *AuthHandlers) resetPasswordForm(req *request.AuthReq) (err error) { user, err = h.AuthService.ValidatePasswordResetToken(req.Context(), token) if err == nil { // login user - req.AuthUser = request.NewAuthUser(h.Settings, user, false, h.Opt.SessionLifetime) + req.AuthUser = request.NewAuthUser(h.Settings, user, false) if req.AuthUser.PendingEmailOTP() { // Email OTP enabled & pending diff --git a/auth/handlers/handle_signup.go b/auth/handlers/handle_signup.go index 54925a79a..cccad79f8 100644 --- a/auth/handlers/handle_signup.go +++ b/auth/handlers/handle_signup.go @@ -44,7 +44,7 @@ func (h *AuthHandlers) signupProc(req *request.AuthReq) error { ) req.RedirectTo = GetLinks().Profile - req.AuthUser = request.NewAuthUser(h.Settings, newUser, false, h.Opt.SessionLifetime) + req.AuthUser = request.NewAuthUser(h.Settings, newUser, false) // auto-complete EmailOTP req.AuthUser.CompleteEmailOTP() @@ -110,7 +110,7 @@ func (h *AuthHandlers) confirmEmail(req *request.AuthReq) (err error) { req.RedirectTo = GetLinks().Profile - req.AuthUser = request.NewAuthUser(h.Settings, user, false, h.Opt.SessionLifetime) + req.AuthUser = request.NewAuthUser(h.Settings, user, false) // auto-complete EmailOTP req.AuthUser.CompleteEmailOTP() diff --git a/auth/handlers/mock_test.go b/auth/handlers/mock_test.go index 6e20df775..b38a035c8 100644 --- a/auth/handlers/mock_test.go +++ b/auth/handlers/mock_test.go @@ -4,7 +4,6 @@ import ( "context" "net/http" "net/http/httptest" - "time" "github.com/cortezaproject/corteza-server/auth/request" "github.com/cortezaproject/corteza-server/auth/settings" @@ -360,7 +359,7 @@ func prepareClientAuthReq(h *AuthHandlers, req *http.Request, user *types.User) } if user != nil { - authReq.AuthUser = request.NewAuthUser(s, user, true, time.Duration(time.Hour)) + authReq.AuthUser = request.NewAuthUser(s, user, true) } return authReq diff --git a/auth/request/auth_user.go b/auth/request/auth_user.go index af1dd1782..ed68441f8 100644 --- a/auth/request/auth_user.go +++ b/auth/request/auth_user.go @@ -2,7 +2,6 @@ package request import ( "encoding/gob" - "time" "github.com/cortezaproject/corteza-server/auth/settings" "github.com/cortezaproject/corteza-server/system/types" @@ -15,8 +14,8 @@ type ( authUser struct { User *types.User - PermSession bool - PermLifetime time.Duration + // is user logged-in with "remember-me"? + PermSession bool MFAStatus map[authType]authStatus } @@ -52,12 +51,11 @@ func init() { gob.Register(&authUser{}) } -func NewAuthUser(s *settings.Settings, u *types.User, perm bool, permLifetime time.Duration) *authUser { +func NewAuthUser(s *settings.Settings, u *types.User, perm bool) *authUser { au := &authUser{ - User: u, - PermSession: perm, - PermLifetime: permLifetime, - MFAStatus: make(map[authType]authStatus), + User: u, + PermSession: perm, + MFAStatus: make(map[authType]authStatus), } au.set(s, u) @@ -154,7 +152,7 @@ func (au *authUser) ResetTOTP() { func (au *authUser) Forget(ses *sessions.Session) { delete(ses.Values, keyAuthUser) - delete(ses.Values, keyPermanent) + delete(ses.Values, keyRememberMe) } func (au *authUser) Save(ses *sessions.Session) { @@ -163,11 +161,9 @@ func (au *authUser) Save(ses *sessions.Session) { // explicitly save roles ses.Values[keyRoles] = au.User.Roles() - if au.PermLifetime > 0 { - ses.Options.MaxAge = int(au.PermLifetime / time.Second) - ses.Values[keyPermanent] = true + if au.PermSession { + ses.Values[keyRememberMe] = true } else { - ses.Options.MaxAge = 0 - delete(ses.Values, keyPermanent) + delete(ses.Values, keyRememberMe) } } diff --git a/auth/request/corteza_session_store.go b/auth/request/corteza_session_store.go index a5c38ba01..f4ad16ccd 100644 --- a/auth/request/corteza_session_store.go +++ b/auth/request/corteza_session_store.go @@ -32,7 +32,7 @@ type ( Codecs []securecookie.Codec - Options *sessions.Options + opt options.AuthOpt } ) @@ -58,13 +58,7 @@ func CortezaSessionStore(store store.AuthSessions, opt options.AuthOpt) *corteza return &cortezaSessionStore{ store: store, Codecs: securecookie.CodecsFromPairs([]byte(opt.Secret)), - Options: &sessions.Options{ - Path: opt.SessionCookiePath, - Domain: domain, - MaxAge: int(opt.SessionLifetime / time.Second), - Secure: opt.SessionCookieSecure, - HttpOnly: true, - }, + opt: opt, } } @@ -72,17 +66,28 @@ func (s cortezaSessionStore) Get(r *http.Request, name string) (*sessions.Sessio return sessions.GetRegistry(r).Get(s, name) } -func (s cortezaSessionStore) New(r *http.Request, name string) (*sessions.Session, error) { - session := sessions.NewSession(s, name) - session.Options = &sessions.Options{ - Path: s.Options.Path, - Domain: s.Options.Domain, - MaxAge: s.Options.MaxAge, - Secure: s.Options.Secure, - HttpOnly: s.Options.HttpOnly, +func (s cortezaSessionStore) New(r *http.Request, name string) (session *sessions.Session, err error) { + var ( + domain = s.opt.SessionCookieDomain + ) + + if strings.Contains(domain, ":") { + // do not set domain on the cookie if it contains colon + // this most likely means it contains port + // + // browsers might misbehave + domain = "" } + + session = sessions.NewSession(s, name) session.IsNew = true - var err error + session.Options = &sessions.Options{ + Path: s.opt.SessionCookiePath, + Domain: domain, + Secure: s.opt.SessionCookieSecure, + HttpOnly: true, + } + if cook, errCookie := r.Cookie(name); errCookie == nil { err = securecookie.DecodeMulti(name, cook.Value, &session.ID, s.Codecs...) if err == nil { @@ -94,7 +99,8 @@ func (s cortezaSessionStore) New(r *http.Request, name string) (*sessions.Sessio } } } - return session, err + + return } func (s cortezaSessionStore) load(ctx context.Context, ses *sessions.Session) error { @@ -103,13 +109,10 @@ func (s cortezaSessionStore) load(ctx context.Context, ses *sessions.Session) er return err } - var maxAge = int(cortezaSession.ExpiresAt.Sub(*now()) / time.Second) - if maxAge < 0 { + if now().After(cortezaSession.ExpiresAt) { return fmt.Errorf("session expired") } - ses.Options.MaxAge = maxAge - if err = gob.NewDecoder(bytes.NewReader(cortezaSession.Data)).Decode(&ses.Values); err != nil { return fmt.Errorf("failed to decode session: %w", err) } @@ -151,23 +154,48 @@ func (s cortezaSessionStore) Save(r *http.Request, w http.ResponseWriter, ses *s ses.ID = string(rand.Bytes(64)) } - if err = s.save(r.Context(), ses); err != nil { + var ( + // make a copy of options for cookie + cOpt = *ses.Options + + // expiration for stored session + exp time.Time + ) + + if IsPermLogin(ses) { + // if permanent login, make sure max-age is recalculated + // so that it slides in the future with every new session-save + cOpt.MaxAge = int(s.opt.SessionPermLifetime / time.Second) + + // recalculate expiration for the stored (db) session + exp = now().Add(s.opt.SessionPermLifetime) + } else { + // Warning! + // when session is not permanent, max-age is NOT set on the cookie + // making cookie last only for the length of the session + + // recalculate expiration for the stored (db) session + // we do that even if the cookie does not have a value + exp = now().Add(s.opt.SessionLifetime) + } + + if err = s.save(r.Context(), ses, exp); err != nil { return err } - // Keep the session ID key in a cookie so it can be looked up in DB later. + // Keep the session ID key in a cookie so that it can be looked up in DB later. encoded, err := securecookie.EncodeMulti(ses.Name(), ses.ID, s.Codecs...) if err != nil { return err } - http.SetCookie(w, sessions.NewCookie(ses.Name(), encoded, ses.Options)) + http.SetCookie(w, sessions.NewCookie(ses.Name(), encoded, &cOpt)) return nil } // save writes encoded session.Values to a database record. // writes to http_sessions table by default. -func (s cortezaSessionStore) save(ctx context.Context, ses *sessions.Session) (err error) { +func (s cortezaSessionStore) save(ctx context.Context, ses *sessions.Session, expires time.Time) (err error) { var ( buf = &bytes.Buffer{} cortezaSession *types.AuthSession @@ -191,11 +219,10 @@ func (s cortezaSessionStore) save(ctx context.Context, ses *sessions.Session) (e extra := auth.GetExtraReqInfoFromContext(ctx) cortezaSession.UserAgent = extra.UserAgent cortezaSession.RemoteAddr = extra.RemoteAddr - - // calculate expiration date from max-age - cortezaSession.ExpiresAt = now().Add(time.Second * time.Duration(ses.Options.MaxAge)) } + cortezaSession.ExpiresAt = expires + delete(ses.Values, keyOriginalSession) if err = gob.NewEncoder(buf).Encode(ses.Values); err != nil { return fmt.Errorf("failed to encode session: %w", err) diff --git a/auth/request/session.go b/auth/request/session.go index de6e65c65..a6929dca2 100644 --- a/auth/request/session.go +++ b/auth/request/session.go @@ -8,7 +8,7 @@ import ( ) const ( - keyPermanent = "permanent" + keyRememberMe = "remember-me" keyOriginalSession = "originalSession" keyAuthUser = "authUser" keyRoles = "roles" @@ -98,3 +98,12 @@ func SetOauth2ClientAuthorized(ses *sessions.Session, val bool) { delete(ses.Values, keyOAuth2ClientAuthorized) } } + +// IsPermLogin decodes remember-me flag from the session and returns true if set +func IsPermLogin(ses *sessions.Session) (p bool) { + if aux, has := ses.Values[keyRememberMe]; has { + p, _ = aux.(bool) + } + + return +}