Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session duration > 30 days not supported #2485

Closed
romanlytvyn opened this issue Apr 22, 2021 · 1 comment
Closed

Session duration > 30 days not supported #2485

romanlytvyn opened this issue Apr 22, 2021 · 1 comment

Comments

@romanlytvyn
Copy link
Contributor

romanlytvyn commented Apr 22, 2021

Describe the bug

Session duration >30 days (86400 * 30) is not behaving properly.
oauth2_authentication_session cookie is rejected after 30 days regardless of specified expiration time.
The session cookie expiration can be set at 10 years from now, but hydra rejects it after 30 days.

Reproducing the bug

Steps to reproduce the behavior:

  1. Initiate auth flow via /oauth2/auth endpoint.
  2. Accept login request with remember_for: 7776000 (3600 * 24 * 90) // 90 days
    Cookie examination reveals that expiration time is properly set at 90 days in the future.
  3. Wait 30 days (86400 * 30 seconds) or change system time to 30 days in the future.
  4. Check that oauth2_authentication_session cookie is still present and not expired in broswer - looks good.
  5. Try to perform login with prompt=none.
  6. An error is returned Prompt 'none' was requested, but no existing login session was found.

Expected behavior

Login flow performed without any errors since session cookie is still not expired.

Environment

  • Version: v1.10.1

Additional context

A bit of a code digging reveals that cookie is not returned by CookieStore after 30 days:

cookie, err := s.r.CookieStore().Get(r, CookieName(s.c.ServesHTTPS(), CookieAuthenticationName))

A CookieStore is initalized by NewCookieStore:

func (m *RegistryBase) CookieStore() sessions.Store {
if m.cs == nil {
m.cs = sessions.NewCookieStore(m.C.GetCookieSecrets()...)
m.csPrev = m.C.GetCookieSecrets()
}
return m.cs
}

Which sets default MaxAge to exactly 30 days:
https://github.com/gorilla/sessions/blob/61fa50d034f99479a7de0d1c02c5e9dea5ad30cb/store.go#L50-L61

func NewCookieStore(keyPairs ...[]byte) *CookieStore {
	cs := &CookieStore{
		Codecs: securecookie.CodecsFromPairs(keyPairs...),
		Options: &Options{
			Path:   "/",
			MaxAge: 86400 * 30,  //  <--- here it is, default value set to 30 days
		},
	}

	cs.MaxAge(cs.Options.MaxAge)
	return cs
}

Which later on is used inside SecureCookie Decode method on Get and returns errTimestampExpired error if cookie is older than MaxAge:
https://github.com/gorilla/securecookie/blob/f37875ef1fb538320ab97fc6c9927d94c280ed5b/securecookie.go#L339

// t1 - cookie issue time, extracted from the cookie value
// t2 - current timestamp
if s.maxAge != 0 && t1 < t2-s.maxAge {
    return errTimestampExpired
}

Even though CookieStore exposes possibility to override MaxAge via method:
https://github.com/gorilla/sessions/blob/61fa50d034f99479a7de0d1c02c5e9dea5ad30cb/store.go#L116

func (s *CookieStore) MaxAge(age int) {
	s.Options.MaxAge = age
...

Hydra does not override it, therefore leaving the default MaxAge: 86400 * 30, essentially rendering any remember_for greater than 30 days completely useless.

@romanlytvyn
Copy link
Contributor Author

One easy fix could be to disable SecureCookie MaxAge check inside CookieStore by setting MaxAge to 0 after initialization:

m.cs = sessions.NewCookieStore(m.C.GetCookieSecrets()...)

func (m *RegistryBase) CookieStore() sessions.Store {
	if m.cs == nil {
		cs := sessions.NewCookieStore(m.C.GetCookieSecrets()...)
		cs.MaxAge(0) // <--- this should disable the check

		m.cs = cs
		m.csPrev = m.C.GetCookieSecrets()
	}
	return m.cs
}

Alternatively, instead of hardcoding 0, a new hydra config can be exposed to control CookieStore MaxAge value.

romanlytvyn added a commit to romanlytvyn/hydra that referenced this issue Apr 22, 2021
CookieStore MaxAge is set to 86400 * 30 by default.
This prevents secure cookies retrieval with expiration > 30 days.
MaxAge: 0 disables MaxAge check by SecureCookie, thus allowing sessions lasting > 30 days.
mitar pushed a commit to mitar/hydra that referenced this issue May 13, 2021
CookieStore MaxAge is set to 86400 * 30 by default. This prevents secure cookies retrieval with expiration > 30 days. MaxAge: 0 disables MaxAge check by SecureCookie, thus allowing sessions lasting > 30 days.

Closes  ory#2485

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant