Fix access cookie expiration, remove related legacy code

Author: p53

docs/content/configuration/_index.md

@@ -80,7 +80,6 @@ weight: 2
 |	 \--opa-authz-uri                         | OPA endpoint address with path                        |       | PROXY_OPA_AUTHZ_URI
 |    \--pat-retry-count                       | number of retries to get PAT                          |    5  | PROXY_PAT_RETRY_COUNT
 |    \--pat-retry-interval                    | interval between retries to get PAT                   |    2s | PROXY_PAT_RETRY_INTERVAL
-|    \--access-token-duration value           | fallback cookie duration for the access token when using refresh tokens | 720h0m0s | PROXY_ACCESS_TOKEN_DURATION
 |    \--cookie-domain value                   | domain the access cookie is available to, defaults host header | | PROXY_COOKIE_DOMAIN
 |    \--cookie-path value                     | path to which cookie is available | | PROXY_COOKIE_PATH
 |    \--cookie-access-name value              | name of the cookie use to hold the access token | kc-access | PROXY_COOKIE_ACCESS_NAME

e2e/e2e_test.go

@@ -981,6 +981,175 @@ var _ = Describe("Code Flow login/logout", func() {
 	})
 })
 
+var _ = Describe("Code Flow login/logout", func() {
+	var (
+		portNum      string
+		proxyAddress string
+		server       *http.Server
+	)
+
+	errGroup, _ := errgroup.WithContext(context.Background())
+
+	AfterEach(func() {
+		if server != nil {
+			err := server.Shutdown(context.Background())
+			Expect(err).NotTo(HaveOccurred())
+		}
+
+		if errGroup != nil {
+			err := errGroup.Wait()
+			Expect(err).NotTo(HaveOccurred())
+		}
+	})
+
+	BeforeEach(func() {
+		var (
+			err             error
+			upstreamSvcPort string
+		)
+
+		server, upstreamSvcPort = startAndWaitTestUpstream(errGroup, false, false, false)
+		portNum, err = generateRandomPort()
+		Expect(err).NotTo(HaveOccurred())
+
+		proxyAddress = localURI + portNum
+
+		proxyArgs := []string{
+			"--discovery-url=" + idpRealmURI,
+			"--openid-provider-timeout=300s",
+			"--tls-openid-provider-ca-certificate=" + tlsCaCertificate,
+			"--tls-openid-provider-client-certificate=" + tlsCertificate,
+			"--tls-openid-provider-client-private-key=" + tlsPrivateKey,
+			"--listen=" + allInterfaces + portNum,
+			"--client-id=" + testClient,
+			"--client-secret=" + testClientSecret,
+			"--upstream-url=" + localURI + upstreamSvcPort,
+			"--no-redirects=false",
+			"--skip-access-token-clientid-check=true",
+			"--skip-access-token-issuer-check=true",
+			"--enable-idp-session-check=false",
+			"--enable-default-deny=false",
+			"--resources=uri=/*|roles=uma_authorization,offline_access",
+			"--openid-provider-retry-count=30",
+			"--enable-refresh-tokens=true",
+			"--encryption-key=" + testKey,
+			"--secure-cookie=false",
+			"--post-login-redirect-path=" + postLoginRedirectPath,
+			"--enable-register-handler=true",
+			"--enable-encrypted-token=false",
+			"--enable-id-token-claims=true",
+			"--enable-id-token-cookie=true",
+			"--enable-user-info-claims=true",
+			"--add-claims=email_verified",
+			"--add-claims=email",
+			"--enable-pkce=false",
+			"--tls-cert=" + tlsCertificate,
+			"--tls-private-key=" + tlsPrivateKey,
+			"--upstream-ca=" + tlsCaCertificate,
+			"--enable-session-cookies=false",
+		}
+
+		osArgs := make([]string, 0, 1+len(proxyArgs))
+		osArgs = append(osArgs, os.Args[0])
+		osArgs = append(osArgs, proxyArgs...)
+		startAndWait(portNum, osArgs)
+	})
+
+	When("Performing standard login with session cookies disabled", func() {
+		It("should login with user/password and logout successfully",
+			Label("code_flow"),
+			Label("session_cookies_disabled"),
+			func(_ context.Context) {
+				var err error
+
+				rClient := resty.New()
+				rClient.SetTLSClientConfig(&tls.Config{RootCAs: caPool, MinVersion: tls.VersionTLS13})
+				resp := codeFlowLogin(rClient, proxyAddress, http.StatusOK, testUser, testPass)
+				Expect(resp.Header().Get("Proxy-Accepted")).To(Equal("true"))
+				body := resp.Body()
+				Expect(strings.Contains(string(body), postLoginRedirectPath)).To(BeTrue())
+
+				jarURI, err := url.Parse(proxyAddress)
+				Expect(err).NotTo(HaveOccurred())
+
+				cookiesLogin := rClient.GetClient().Jar.Cookies(jarURI)
+
+				var accessCookieLogin string
+
+				for _, cook := range cookiesLogin {
+					if cook.Name == constant.AccessCookie {
+						accessCookieLogin = cook.Value
+					}
+				}
+
+				time.Sleep(testAccessTokenExp)
+
+				resp, err = rClient.R().Get(proxyAddress + anyURI)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(resp.Header().Get("Proxy-Accepted")).To(Equal("true"))
+				body = resp.Body()
+				Expect(strings.Contains(string(body), anyURI)).To(BeTrue())
+				Expect(resp.StatusCode()).To(Equal(http.StatusOK))
+				Expect(err).NotTo(HaveOccurred())
+
+				cookiesAfterRefresh := resp.Cookies()
+
+				var (
+					accessCookieAfterRefresh  *http.Cookie
+					refreshCookieAfterRefresh *http.Cookie
+					testCookie                string
+				)
+
+				for _, cook := range cookiesAfterRefresh {
+					if cook.Name == constant.AccessCookie {
+						accessCookieAfterRefresh = cook
+					}
+
+					if cook.Name == constant.RefreshCookie {
+						refreshCookieAfterRefresh = cook
+					}
+
+					if cook.Name == testCookieValue {
+						testCookie = cook.Value
+					}
+				}
+
+				Expect(testCookie).To(Equal("test_value"))
+
+				_, err = jwt.ParseSigned(accessCookieAfterRefresh.Value, constant.SignatureAlgs[:])
+				Expect(err).NotTo(HaveOccurred())
+				Expect(accessCookieLogin).NotTo(Equal(accessCookieAfterRefresh.Value))
+
+				refresh, err := encryption.DecodeText(refreshCookieAfterRefresh.Value, testKey)
+				Expect(err).NotTo(HaveOccurred())
+
+				_, err = jwt.ParseSigned(refresh, constant.SignatureAlgs[:])
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(accessCookieAfterRefresh.Expires).To(Equal(refreshCookieAfterRefresh.Expires))
+
+				resp, err = rClient.R().Get(proxyAddress + anyURI)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(resp.Header().Get("Proxy-Accepted")).To(Equal("true"))
+				body = resp.Body()
+				By(string(body))
+				Expect(resp.StatusCode()).To(Equal(http.StatusOK))
+				Expect(strings.Contains(string(body), anyURI)).To(BeTrue())
+				Expect(body).To(ContainSubstring(`"X-Auth-Email-Verified":["true"]`))
+				Expect(body).To(ContainSubstring(`"X-Auth-Email":["somebody@somewhere.com"]`))
+
+				resp, err = rClient.R().Get(proxyAddress + logoutURI)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(resp.StatusCode()).To(Equal(http.StatusOK))
+
+				rClient.SetRedirectPolicy(resty.NoRedirectPolicy())
+				resp, _ = rClient.R().Get(proxyAddress)
+				Expect(resp.StatusCode()).To(Equal(http.StatusSeeOther))
+			},
+		)
+	})
+})
+
 var _ = Describe("Code Flow login/logout mTLS", func() {
 	var (
 		portNum      string

pkg/keycloak/config/config.go

@@ -125,7 +125,6 @@ type Config struct {
 	Scopes                             []string                  `json:"scopes,omitempty" usage:"list of scopes requested when authenticating the user" yaml:"scopes"`
 	OpaTimeout                         time.Duration             `env:"OPA_TIMEOUT"              json:"opa-timeout,omitempty"              usage:"timeout for connection to OPA" yaml:"opa-timeout"`
 	PatRetryInterval                   time.Duration             `env:"PAT_RETRY_INTERVAL" json:"pat-retry-interval,omitempty" usage:"interval between retries to get PAT" yaml:"pat-retry-interval"`
-	AccessTokenDuration                time.Duration             `env:"ACCESS_TOKEN_DURATION" json:"access-token-duration,omitempty" usage:"fallback cookie duration for the access token when using refresh tokens" yaml:"access-token-duration"`
 	PatRetryCount                      int                       `env:"PAT_RETRY_COUNT"    json:"pat-retry-count,omitempty"    usage:"number of retries to get PAT"        yaml:"pat-retry-count"`
 	CorsMaxAge                         time.Duration             `env:"CORS_MAX_AGE" json:"cors-max-age,omitempty" usage:"max age applied to cors headers (Access-Control-Max-Age)" yaml:"cors-max-age"`
 	UpstreamTimeout                    time.Duration             `env:"UPSTREAM_TIMEOUT" json:"upstream-timeout,omitempty" usage:"maximum amount of time a dial will wait for a connect to complete" yaml:"upstream-timeout"`
@@ -219,7 +218,6 @@ func NewDefaultConfig() *Config {
 	hostnames = append(hostnames, []string{"localhost", "127.0.0.1", "::1"}...)
 
 	return &Config{
-		AccessTokenDuration:              time.Duration(constant.FallbackAccessTokenDuration) * time.Hour,
 		CookieAccessName:                 constant.AccessCookie,
 		CookieIDTokenName:                constant.IDTokenCookie,
 		CookieRefreshName:                constant.RefreshCookie,

pkg/keycloak/proxy/handlers.go

@@ -477,7 +477,6 @@ func loginHandler(
 	enableCompressToken bool,
 	compressTokenOnlyAuthScheme string,
 	cookManager *cookie.Manager,
-	accessTokenDuration time.Duration,
 	store storage.Storage,
 	compressTokenPool *utils.LimitedBufferPool,
 ) func(wrt http.ResponseWriter, req *http.Request) {
@@ -649,8 +648,15 @@ func loginHandler(
 					}
 				}
 
-				refreshExpiry := session.GetAccessCookieExpiration(scope.Logger, accessTokenDuration, token.RefreshToken)
-				// drop in the access token - cookie expiration = access token
+				stdRefreshClaims, err := utils.ParseRefreshToken(token.RefreshToken)
+				if err != nil {
+					scope.Logger.Error(apperrors.ErrEncryptRefreshToken.Error(), zap.Error(err))
+
+					return http.StatusInternalServerError,
+						errors.Join(apperrors.ErrParseRefreshToken, err)
+				}
+
+				refreshExpiry := time.Until(stdRefreshClaims.Expiry.Time())
 				cookManager.DropAccessTokenCookie(
 					req,
 					writer,
@@ -667,38 +673,20 @@ func loginHandler(
 					)
 				}
 
-				var expiration time.Duration
-				// notes: not all idp refresh tokens are readable, google for example, so we attempt to decode into
-				// a jwt and if possible extract the expiration, else we default to 10 days
-				refreshTokenObj, errRef := jwt.ParseSigned(token.RefreshToken, constant.SignatureAlgs[:])
-				if errRef != nil {
-					return http.StatusInternalServerError,
-						errors.Join(apperrors.ErrParseRefreshToken, errRef)
-				}
-
-				stdRefreshClaims := &jwt.Claims{}
-
-				err = refreshTokenObj.UnsafeClaimsWithoutVerification(stdRefreshClaims)
-				if err != nil {
-					expiration = 0
-				} else {
-					expiration = time.Until(stdRefreshClaims.Expiry.Time())
-				}
-
 				switch store != nil {
 				case true:
 					rCtx, rCancel := context.WithTimeout(ctx, constant.RedisTimeout)
 					defer rCancel()
 
-					err = store.Set(rCtx, identity.ID, refreshToken, expiration)
+					err = store.Set(rCtx, identity.ID, refreshToken, refreshExpiry)
 					if err != nil {
 						scope.Logger.Error(
 							apperrors.ErrSaveTokToStore.Error(),
 							zap.Error(err),
 						)
 					}
 				default:
-					cookManager.DropRefreshTokenCookie(req, writer, refreshToken, expiration)
+					cookManager.DropRefreshTokenCookie(req, writer, refreshToken, refreshExpiry)
 				}
 			} else {
 				cookManager.DropAccessTokenCookie(

pkg/keycloak/proxy/server.go

@@ -562,7 +562,6 @@ func (r *OauthProxy) CreateReverseProxy() error {
 		r.Config.EncryptionKey,
 		newOAuth2Config,
 		r.Store,
-		r.Config.AccessTokenDuration,
 		r.Config.EnableOptionalEncryption,
 		r.Config.EnableCompressToken,
 		r.Config.CompressTokenOnlyAuthScheme,
@@ -586,7 +585,6 @@ func (r *OauthProxy) CreateReverseProxy() error {
 		r.Config.EnableCompressToken,
 		r.Config.CompressTokenOnlyAuthScheme,
 		r.Cm,
-		r.Config.AccessTokenDuration,
 		r.Store,
 		compressTokenPool,
 	)