Refactor RefreshSessionIfNeeded to RefreshSession

[NickMeves]

Currently, when we are passed the cookie-refresh TTL, we only refresh the session if the Access or ID Token is expired (depending on which the provider uses for the ExpiresOn field).

This leads to a lot of issues on the border of the expiration - where a session might for a split second be valid and pass at the proxy layer, and pass an Access or ID Token to the upstream that will be expired by the time it gets there.

We should support opportunistic ability to refresh tokens/sessions ahead of expiration (E.g. 80% of token lifespan).

Expected Behavior

Our RefreshSessions provider methods make no judgments on when to refresh. They will refresh if called.

This gives more control to the end user to set the time appropriately with cookie-refresh.

Current Behavior

Sessions only refresh if they are already expired.

Possible Solution

This logic throughout providers ditches the ExpiresOn checks:

if s == nil || (s.ExpiresOn != nil && s.ExpiresOn.After(time.Now())) || s.RefreshToken == "" {

This global manager that handles this at the middleware level will also need to change:

https://github.com/oauth2-proxy/oauth2-proxy/blob/3fa42edb7350219d317c4bd47faf5da6192dc70f/pkg/middleware/stored_session.go

I lean toward if we want any logic about only at 80% of greater of the life of a session, we do that in stored_session.go (as a safety precaution for people setting overeager refresh times that result in refresh requests every request).

[JoelSpeed]

Agreed, we should have the decision and logic on when to refresh centrally and then have the providers implement the refreshing.

I wonder if we can interpret somehow the config so that a provider knows that is has been configured for refreshing? If so, it could expose a method RefreshEnabled that returns a bool that would allow stored_session.go to know whether it should even try to attempt refreshing the session or not 🤔 Maybe overthinking this

[arcivanov]

Actually it looks to me that the cookie is never refreshed when the token expires, or at least is not refreshed via the /auth endpoint.

Case in point - the id_token on Keycloak by default only lives 5 minutes. After five minutes with pretty much eternal cookie the id_token returned by oauth2-proxy is expired and the session does not change.

[arcivanov]

FYI, I'm working on a solution, will submit today, hopefully.

[Jc2k]

Have been bitten by this. Currently trying out #915 and it seems to be working for my needs so far. Cheers @arcivanov !

[NickMeves]

So in looking over #946 I think I caught a very subtle but very dangerous interaction between RefreshSession & ValidateSession that might cause nasty bugs in the future: #946 (comment)

I'm thinking it might be prudent for us to completely decouple the Refresh & Validate into two separate timers that users can controls as they wish (session-refresh & session-validate). This will let users fully control if they want earlier session validation for providers who under the hood would make a network request to a validateURL

[NickMeves]

Related - I think the decoupled manner we manage the underlying session fields (via public fields across disparate middleware & provider implementations) is ripe for confusion and bugs. We've seen many times where contributors add refresh logic to a provider and don't manage the time settings properly.

So I think we should refactor these to being owned by the SessionState so it can fully control timing logic (and potentially add its own LastRefreshed & LastValidated timers).

Something like this:

func (s *SessionState) Refresh(ctx context.Context, p *provider.Provider) error {
  # Do stuff with p.RefreshSession(ctx, s)
  
  # Then forcing timing sync logix now, e.g. something like
  s.CreatedAt = time.Now()
}

To help centralize the disparate logic between individual providers and here:

oauth2-proxy/pkg/middleware/stored_session.go

Line 115 in d67d6e3

func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req *http.Request, session *sessionsapi.SessionState) error {

AND

func (s *SessionState) Validate(ctx context.Context, p *provider.Provider) error

For this:

oauth2-proxy/pkg/middleware/stored_session.go

Line 158 in d67d6e3

func (s *storedSessionLoader) validateSession(ctx context.Context, session *sessionsapi.SessionState) error {

[github-actions]

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

[Bibob7]

This issue is still relevant for me!

[arcivanov]

@Bibob7 Try #915 as it works for most people.

[lathspell]

A quick and dirty but at least minimal invasive hack that also refreshes the OIDC Token 30s before expiration. Forked from 2021-04-03 master:

diff --git a/providers/oidc.go b/providers/oidc.go
index df133f4d..47f5c52e 100644
--- a/providers/oidc.go
+++ b/providers/oidc.go
@@ -107,16 +107,25 @@ func (p *OIDCProvider) enrichFromProfileURL(ctx context.Context, s *sessions.Ses
 	return nil
 }
 
+const RefreshTokenOffset = 30 * time.Second
+
 // ValidateSession checks that the session's IDToken is still valid
 func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool {
 	_, err := p.Verifier.Verify(ctx, s.IDToken)
+
+	if s == nil || (s.ExpiresOn != nil && s.ExpiresOn.Sub(time.Now()) < RefreshTokenOffset) || s.RefreshToken == "" {
+		logger.Printf("session will expire is less than 30 seconds: %s", s)
+		return false
+	}
+
 	return err == nil
 }
 
 // RefreshSessionIfNeeded checks if the session has expired and uses the
 // RefreshToken to fetch a new Access Token (and optional ID token) if required
 func (p *OIDCProvider) RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) {
-	if s == nil || (s.ExpiresOn != nil && s.ExpiresOn.After(time.Now())) || s.RefreshToken == "" {
+
+	if s == nil || (s.ExpiresOn != nil && s.ExpiresOn.Sub(time.Now()) > RefreshTokenOffset) || s.RefreshToken == "" {
 		return false, nil
 	}
 

[pierluigilenoci]

@NickMeves any news about this?

[NickMeves]

I have a draft PR started, but I will be slow. Job commitments at the moment are drastically limiting the time I have to work on OAuth2 Proxy.

[pierluigilenoci]

@NickMeves thanks anyway! ❤️

[krin-wang]

@NickMeves Thank you for your replies. Is this issue still on track?

[NickMeves]

I plan to dive back into this draft and reassess this weekend or next: #1086

We merged a prereq I wanted to get in place for this refactor to manage time throughout our codebase in a more streamlined way, so it shouldn't have any blockers besides me getting back into the flow of OAuth2 Proxy now that I'm onboarded at my new job.

[github-actions]

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

[gysel]

This issue is still relevant to us.

[pierluigilenoci]

@gysel the PR #1086 is merged in master since June.
You need to wait for the next release but it is close to coming! 😄

@NickMeves make sense for you to close this issue?

[NickMeves]

Fixed in #1086