Oauth2-proxy: 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 https://github.com/oauth2-proxy/oauth2-proxy/pull/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: https://github.com/oauth2-proxy/oauth2-proxy/pull/946#discussion_r536892999

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: https://github.com/oauth2-proxy/oauth2-proxy/blob/d67d6e3152e2a1c9614906ee9bd9890f8da9a007/pkg/middleware/stored_session.go#L115

AND

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

For this: https://github.com/oauth2-proxy/oauth2-proxy/blob/d67d6e3152e2a1c9614906ee9bd9890f8da9a007/pkg/middleware/stored_session.go#L158