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

oauth2: ID Token JWT expiry may not be synchronised with IdToken cookie expiry #32485

Open
michaelsauter opened this issue Feb 20, 2024 · 6 comments · May be fixed by #34117
Open

oauth2: ID Token JWT expiry may not be synchronised with IdToken cookie expiry #32485

michaelsauter opened this issue Feb 20, 2024 · 6 comments · May be fixed by #34117
Labels
bug stale stalebot believes this issue/PR has not been touched recently

Comments

@michaelsauter
Copy link

Title: The expiry of the IdToken cookie may be longer than the expiry of the JWT stored inside the cookie

Description:
This issue is related to the oauth2 and jwt filters. The authn server sets the expires_in field of the token response in sync with the expiry of the access token. The oauth2 filter however uses this value to set the expiry of all cookies, including the IdToken cookie. However, authn providers are not obliged to use the same lifetime for access tokens and ID tokens. For example, Microsoft Entra does not: https://learn.microsoft.com/en-us/entra/identity-platform/configurable-token-lifetimes. By default, Entra ID tokens have a lifetime of 60 minutes, whereas Entra access tokens have a lifetime of somewhere in between 60 and 90 minutes. The effect of this is that the IdToken cookie expires sometime after the JWT contained in it. As a result, after 60 minutes pass, the JWT filter errors with Jwt is expired and the user must delete the cookies or wait until the cookie expires as well.

This is related to #30053, because the root cause of both issues is the same: the expires_in field of the token response must only be used to determine expiry of the access token, not of any other tokens. The fix should also roughly be the same for both issues: if the IdToken cookie expiry is set from the JWT expiry field, the problem is solved.

Repro steps:
Configure your OAuth2 authorization server to hand out access tokens and ID tokens with different lifetime. In envoy, make use of the ID token in the jwt filter, e.g. like this:

from_cookies:
  - IdToken
forward_payload_header: X-Id-Token-Payload

After the ID token expires, envoy responds with Jwt is expired until the cookies / access token expire as well.

Other comments:
Another way to solve the issue would be to introduce a separate configuration field, named e.g. max_expiry. Say this is set to 60 minutes, then all cookies would not have an expiry of more than 60 minutes. This would fix the issue as well, although it sounds a bit like a hack to me. In addition, max_expiry would have to exclude the refresh token, as otherwise #30053 would happen again.

@michaelsauter michaelsauter added bug triage Issue requires triage labels Feb 20, 2024
@lizan lizan removed the triage Issue requires triage label Feb 22, 2024
@michaelsauter
Copy link
Author

FYI as a workaround, one can add a Go filter between the OAuth2 and JWT filters in the chain, which adjusts the IdToken cookie expiry. Here's the (shortened) code in filter.go:

package main

import (
	"errors"
	"fmt"
	"regexp"
	"strings"
	"time"

	"github.com/envoyproxy/envoy/contrib/golang/common/go/api"
	"github.com/golang-jwt/jwt/v5"
)

type filter struct {
	api.PassThroughStreamFilter
	callbacks api.FilterCallbackHandler
	config    *Config
}

// *filter.DecodeHeaders, *filter.DecodeData, *filter.DecodeTrailers, *filter.EncodeData and *filter.EncodeTrailers
// are omitted for brevity, they all just return api.Continue.

// *filter.OnLog, *filter.OnLogDownstreamStart, *filter.OnLogDownstreamPeriodic and *filter.OnDestroy
// are omitted for brevity, they are all just empty.

func (f *filter) EncodeHeaders(header api.ResponseHeaderMap, endStream bool) api.StatusType {
	setCookies := header.Values("set-cookie")
	if len(setCookies) == 0 {
		return api.Continue
	}

	header.Del("set-cookie")
	for key, value := range setCookies {
		if !strings.HasPrefix(value, "IdToken=") {
			continue
		}
		parts := strings.Split(value, ";")
		_, cookieValue, _ := strings.Cut(parts[0], "=")
		expiry, err := readIDTokenExpiry(cookieValue)
		if err != nil {
			api.LogErrorf("read IdToken expiry: %s", err)
			break
		}
		newMaxAge := int(time.Until(expiry.Time).Seconds())
		api.LogDebugf("fixing max-age - new lifetime: %d", newMaxAge)
		setCookies[key] = replaceMaxAgeInSetCookieValue(value, newMaxAge)
		break
	}
	for _, v := range setCookies {
		header.Add("set-cookie", v)
	}

	return api.Continue
}

func readIDTokenExpiry(idToken string) (*jwt.NumericDate, error) {
	unverifiedToken, _, err := new(jwt.Parser).ParseUnverified(idToken, jwt.MapClaims{})
	if err != nil {
		return nil, fmt.Errorf("parse the JWT: %s", err)
	}
	if unverifiedClaims, ok := unverifiedToken.Claims.(jwt.MapClaims); ok {
		expiry, err := unverifiedClaims.GetExpirationTime()
		if err != nil {
			return nil, fmt.Errorf("get expiry: %s", err)
		}
		return expiry, nil
	}
	return nil, errors.New("invalid JWT token")
}

func replaceMaxAgeInSetCookieValue(value string, maxAge int) string {
	return regexp.MustCompile(`Max-Age=([0-9]+)`).ReplaceAllString(value, fmt.Sprintf("Max-Age=%d", maxAge))
}

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 27, 2024
@michaelsauter
Copy link
Author

I'd appreciate if this would be kept open. Without applying the workaround, which is quite cumbersome, the bug directly affects users as they can be logged out for some time without an easy way to login again until the timeout is reached.

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 28, 2024
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 27, 2024
@michaelsauter
Copy link
Author

not stale

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2024
michaelsauter added a commit to michaelsauter/envoy that referenced this issue May 13, 2024
The expiry of the id token cookie should match the exp claim in the id
token JWT, not the expiry of the access token. Some identity providers
(definitely Microsoft Entra ID, probably others) issue id tokens with
different expiry than the access tokens.

This change is largely based on the work done in envoyproxy#32278 which supports expiry
of the refresh token based on the exp claim of the refresh token if it
is a JWT.

Closes envoyproxy#32485.
michaelsauter added a commit to michaelsauter/envoy that referenced this issue May 14, 2024
The expiry of the id token cookie should match the exp claim in the id
token JWT, not the expiry of the access token. Some identity providers
(definitely Microsoft Entra ID, probably others) issue id tokens with
different expiry than the access tokens.

This change is largely based on the work done in envoyproxy#32278 which supports expiry
of the refresh token based on the exp claim of the refresh token if it
is a JWT.

Closes envoyproxy#32485.

Signed-off-by: Michael Sauter <mail@michaelsauter.net>
michaelsauter added a commit to michaelsauter/envoy that referenced this issue May 15, 2024
The expiry of the id token cookie should match the exp claim in the id
token JWT, not the expiry of the access token. Some identity providers
(definitely Microsoft Entra ID, probably others) issue id tokens with
different expiry than the access tokens.

This change is largely based on the work done in envoyproxy#32278 which supports expiry
of the refresh token based on the exp claim of the refresh token if it
is a JWT.

Closes envoyproxy#32485.

Signed-off-by: Michael Sauter <mail@michaelsauter.net>
michaelsauter added a commit to michaelsauter/envoy that referenced this issue May 15, 2024
The expiry of the id token cookie should match the exp claim in the id
token JWT, not the expiry of the access token. Some identity providers
(definitely Microsoft Entra ID, probably others) issue id tokens with
different expiry than the access tokens.

This change is largely based on the work done in envoyproxy#32278 which supports expiry
of the refresh token based on the exp claim of the refresh token if it
is a JWT.

Closes envoyproxy#32485.

Signed-off-by: Michael Sauter <mail@michaelsauter.net>
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants