Refactor middlewares (#392)

Co-authored By: Pierre Bogossian <[email protected]>

docs/content/configuration/_index.md

@@ -62,7 +62,7 @@ weight: 2
 |    --localhost-metrics                     | enforces the metrics page can only been requested from 127.0.0.1 | false | PROXY_LOCALHOST_METRICS
 |    --enable-compression                    | enable gzip compression for response | false | PROXY_ENABLE_COMPRESSION
 |    --enable-pkce                           | enable pkce for auth code flow, only S256 code challenge supported | false | PROXY_ENABLE_PKCE
-|    --enable-idp-session-check              | during token validation it also checks if user session is still present, useful for multi app logout | false | PROXY_ENABLE_IDP_SESSION_CHECK
+|    --enable-idp-session-check              | during token validation it also checks if user session is still present, useful for multi app logout | true | PROXY_ENABLE_IDP_SESSION_CHECK
 |    --enable-uma                            | enable UMA authorization, please don't use in production as it is new feature, we would like to receive feedback first             | false | PROXY_ENABLE_UMA
 |	 --enable-opa                            | enable authorization with external Open policy agent  | false | PROXY_ENABLE_OPA
 |	 --opa-timeout                           | timeout for connection to OPA                         |   10s | PROXY_OPA_TIMEOUT

pkg/apperrors/apperrors.go

@@ -61,8 +61,11 @@ var (
 	ErrParseAccessTokenClaims         = errors.New("faled to parse access token claims")
 	ErrParseRefreshTokenClaims        = errors.New("faled to parse refresh token claims")
 
-	ErrVerifyIDToken     = errors.New("unable to verify the ID token")
-	ErrVerifyAccessToken = errors.New("unable to verify the access token")
+	ErrTokenSignature          = errors.New("invalid token signature")
+	ErrVerifyIDToken           = errors.New("unable to verify ID token")
+	ErrVerifyAccessToken       = errors.New("unable to verify access token")
+	ErrVerifyRefreshToken      = errors.New("refresh token failed verification")
+	ErrAccRefreshTokenMismatch = errors.New("seems that access token and refresh token doesn't match")
 
 	ErrCreateRevocationReq   = errors.New("unable to construct the revocation request")
 	ErrRevocationReqFailure  = errors.New("request to revocation endpoint failed")
@@ -76,13 +79,14 @@ var (
 
 	// config errors
 
-	ErrInvalidPostLoginRedirectPath            = errors.New("post login redirect path invalid, should be only path not absolute url (no hostname, scheme)")
-	ErrPostLoginRedirectPathNoRedirectsInvalid = errors.New("post login redirect path can be enabled only with no-redirect=false")
-	ErrMissingListenInterface                  = errors.New("you have not specified the listening interface")
-	ErrAdminListenerScheme                     = errors.New("scheme for admin listener must be one of [http, https]")
-	ErrInvalidIdpProviderProxyURI              = errors.New("invalid proxy address for IDP provider proxy")
-	ErrInvalidMaxIdleConnections               = errors.New("max-idle-connections must be a number > 0")
-	ErrInvalidMaxIdleConnsPerHost              = errors.New(
+	ErrNoRedirectsWithEnableRefreshTokensInvalid = errors.New("no-redirects true cannot be enabled with refresh tokens")
+	ErrInvalidPostLoginRedirectPath              = errors.New("post login redirect path invalid, should be only path not absolute url (no hostname, scheme)")
+	ErrPostLoginRedirectPathNoRedirectsInvalid   = errors.New("post login redirect path can be enabled only with no-redirect=false")
+	ErrMissingListenInterface                    = errors.New("you have not specified the listening interface")
+	ErrAdminListenerScheme                       = errors.New("scheme for admin listener must be one of [http, https]")
+	ErrInvalidIdpProviderProxyURI                = errors.New("invalid proxy address for IDP provider proxy")
+	ErrInvalidMaxIdleConnections                 = errors.New("max-idle-connections must be a number > 0")
+	ErrInvalidMaxIdleConnsPerHost                = errors.New(
 		"maxi-idle-connections-per-host must be a " +
 			"number > 0 and <= max-idle-connections",
 	)

pkg/keycloak/config/config.go

@@ -341,6 +341,7 @@ func NewDefaultConfig() *Config {
 		EnableDefaultDeny:             true,
 		EnableSessionCookies:          true,
 		EnableTokenHeader:             true,
+		EnableIDPSessionCheck:         true,
 		HTTPOnlyCookie:                true,
 		Headers:                       make(map[string]string),
 		LetsEncryptCacheDir:           "./cache/",

pkg/keycloak/proxy/handlers.go

@@ -29,6 +29,7 @@ import (
 	"strings"
 	"time"
 
+	oidc3 "github.com/coreos/go-oidc/v3/oidc"
 	"github.com/go-jose/go-jose/v3/jwt"
 	"github.com/gogatekeeper/gatekeeper/pkg/apperrors"
 	"github.com/gogatekeeper/gatekeeper/pkg/constant"
@@ -193,25 +194,60 @@ func (r *OauthProxy) oauthCallbackHandler(writer http.ResponseWriter, req *http.
 	}
 
 	rawAccessToken := accessToken
-	stdClaims, customClaims, err := r.verifyOIDCTokens(scope, accessToken, identityToken, writer, req)
+	oAccToken, _, err := verifyOIDCTokens(
+		req.Context(),
+		r.Provider,
+		r.Config.ClientID,
+		accessToken,
+		identityToken,
+		r.Config.SkipAccessTokenClientIDCheck,
+		r.Config.SkipAccessTokenIssuerCheck,
+	)
 	if err != nil {
+		scope.Logger.Error(err.Error())
+		r.accessForbidden(writer, req)
 		return
 	}
 
+	scope.Logger.Debug(
+		"issuing access token for user",
+		zap.String("access token", rawAccessToken),
+		zap.String("sub", oAccToken.Subject),
+		zap.String("expires", oAccToken.Expiry.Format(time.RFC3339)),
+		zap.String("duration", time.Until(oAccToken.Expiry).String()),
+	)
+
+	scope.Logger.Info(
+		"issuing access token for user",
+		zap.String("sub", oAccToken.Subject),
+		zap.String("expires", oAccToken.Expiry.Format(time.RFC3339)),
+		zap.String("duration", time.Until(oAccToken.Expiry).String()),
+	)
+
 	// @metric a token has been issued
 	metrics.OauthTokensMetric.WithLabelValues("issued").Inc()
 
-	oidcTokensCookiesExp := time.Until(stdClaims.Expiry.Time())
+	oidcTokensCookiesExp := time.Until(oAccToken.Expiry)
 	// step: does the response have a refresh token and we do NOT ignore refresh tokens?
 	if r.Config.EnableRefreshTokens && refreshToken != "" {
 		var encrypted string
-		var stdRefreshClaims *jwt.Claims
-		stdRefreshClaims, err = r.verifyRefreshToken(scope, refreshToken, writer, req)
+		var oRefresh *oidc3.IDToken
+		oRefresh, err = verifyToken(
+			req.Context(),
+			r.Provider,
+			refreshToken,
+			r.Config.ClientID,
+			r.Config.SkipAccessTokenClientIDCheck,
+			r.Config.SkipAccessTokenIssuerCheck,
+		)
+
 		if err != nil {
+			scope.Logger.Error(apperrors.ErrVerifyRefreshToken.Error(), zap.Error(err))
+			r.accessForbidden(writer, req)
 			return
 		}
 
-		oidcTokensCookiesExp = time.Until(stdRefreshClaims.Expiry.Time())
+		oidcTokensCookiesExp = time.Until(oRefresh.Expiry)
 		encrypted, err = r.encryptToken(scope, refreshToken, r.Config.EncryptionKey, "refresh", writer)
 		if err != nil {
 			return
@@ -223,8 +259,7 @@ func (r *OauthProxy) oauthCallbackHandler(writer http.ResponseWriter, req *http.
 				scope.Logger.Error(
 					apperrors.ErrSaveTokToStore.Error(),
 					zap.Error(err),
-					zap.String("sub", stdClaims.Subject),
-					zap.String("email", customClaims.Email),
+					zap.String("sub", oAccToken.Subject),
 				)
 				r.accessForbidden(writer, req)
 				return

pkg/keycloak/proxy/middleware.go

@@ -17,6 +17,7 @@ package proxy
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -32,7 +33,6 @@ import (
 	"golang.org/x/oauth2"
 
 	"github.com/PuerkitoBio/purell"
-	oidc3 "github.com/coreos/go-oidc/v3/oidc"
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/gogatekeeper/gatekeeper/pkg/apperrors"
 	"github.com/unrolled/secure"
@@ -219,17 +219,25 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 					return
 				}
 			} else { //nolint:gocritic
-				verifier := r.Provider.Verifier(
-					&oidc3.Config{
-						ClientID:          r.Config.ClientID,
-						SkipClientIDCheck: r.Config.SkipAccessTokenClientIDCheck,
-						SkipIssuerCheck:   r.Config.SkipAccessTokenIssuerCheck,
-					},
+				_, err := verifyToken(
+					ctx,
+					r.Provider,
+					user.RawToken,
+					r.Config.ClientID,
+					r.Config.SkipAccessTokenClientIDCheck,
+					r.Config.SkipAccessTokenIssuerCheck,
 				)
-
-				//nolint:contextcheck
-				_, err := verifier.Verify(context.Background(), user.RawToken)
 				if err != nil {
+					if errors.Is(err, apperrors.ErrTokenSignature) {
+						lLog.Error(
+							apperrors.ErrAccTokenVerifyFailure.Error(),
+							zap.Error(err),
+						)
+						//nolint:contextcheck
+						next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req)))
+						return
+					}
+
 					if !strings.Contains(err.Error(), "token is expired") {
 						lLog.Error(
 							apperrors.ErrAccTokenVerifyFailure.Error(),
@@ -261,6 +269,33 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 						return
 					}
 
+					oRefresh, err := verifyToken(
+						ctx,
+						r.Provider,
+						refresh,
+						r.Config.ClientID,
+						r.Config.SkipAccessTokenClientIDCheck,
+						r.Config.SkipAccessTokenIssuerCheck,
+					)
+					if err != nil {
+						lLog.Error(
+							apperrors.ErrVerifyRefreshToken.Error(),
+							zap.Error(err),
+						)
+						//nolint:contextcheck
+						next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req)))
+						return
+					}
+					if user.ID != oRefresh.Subject {
+						lLog.Error(
+							apperrors.ErrAccRefreshTokenMismatch.Error(),
+							zap.Error(err),
+						)
+						//nolint:contextcheck
+						next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req)))
+						return
+					}
+
 					// attempt to refresh the access token, possibly with a renewed refresh token
 					//
 					// NOTE: atm, this does not retrieve explicit refresh token expiry from oauth2,
@@ -278,7 +313,7 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 					)
 
 					httpClient := r.IdpClient.RestyClient().GetClient()
-					_, newRawAccToken, newRefreshToken, accessExpiresAt, refreshExpiresIn, err := getRefreshedToken(ctx, conf, httpClient, refresh)
+					newAccToken, newRawAccToken, newRefreshToken, accessExpiresAt, refreshExpiresIn, err := getRefreshedToken(ctx, conf, httpClient, refresh)
 					if err != nil {
 						switch err {
 						case apperrors.ErrRefreshTokenExpired:
@@ -331,22 +366,32 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 								apperrors.ErrEncryptAccToken.Error(),
 								zap.Error(err),
 							)
-							wrt.WriteHeader(http.StatusInternalServerError)
+							//nolint:contextcheck
+							next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req)))
 							return
 						}
 					}
 
 					// step: inject the refreshed access token
 					r.Cm.DropAccessTokenCookie(req.WithContext(ctx), wrt, accessToken, accessExpiresIn)
 
+					// update the with the new access token and inject into the context
+					newUser, err := ExtractIdentity(&newAccToken)
+					if err != nil {
+						lLog.Error(err.Error())
+						//nolint:contextcheck
+						next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req)))
+						return
+					}
+
 					// step: inject the renewed refresh token
 					if newRefreshToken != "" {
 						lLog.Debug(
 							"renew refresh cookie with new refresh token",
 							zap.Duration("refresh_expires_in", refreshExpiresIn),
 						)
-
-						encryptedRefreshToken, err := encryption.EncodeText(newRefreshToken, r.Config.EncryptionKey)
+						var encryptedRefreshToken string
+						encryptedRefreshToken, err = encryption.EncodeText(newRefreshToken, r.Config.EncryptionKey)
 						if err != nil {
 							lLog.Error(
 								apperrors.ErrEncryptRefreshToken.Error(),
@@ -358,14 +403,14 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 
 						if r.useStore() {
 							go func(old, newToken string, encrypted string) {
-								if err := r.DeleteRefreshToken(req.Context(), old); err != nil {
+								if err = r.DeleteRefreshToken(req.Context(), old); err != nil {
 									lLog.Error(
 										apperrors.ErrDelTokFromStore.Error(),
 										zap.Error(err),
 									)
 								}
 
-								if err := r.StoreRefreshToken(req.Context(), newToken, encrypted, refreshExpiresIn); err != nil {
+								if err = r.StoreRefreshToken(req.Context(), newToken, encrypted, refreshExpiresIn); err != nil {
 									lLog.Error(
 										apperrors.ErrSaveTokToStore.Error(),
 										zap.Error(err),
@@ -378,8 +423,10 @@ func (r *OauthProxy) authenticationMiddleware() func(http.Handler) http.Handler
 						}
 					}
 
-					// update the with the new access token and inject into the context
-					user.RawToken = newRawAccToken
+					// IMPORTANT: on this rely other middlewares, must be refreshed
+					// with new identity!
+					newUser.RawToken = newRawAccToken
+					scope.Identity = newUser
 					ctx = context.WithValue(req.Context(), constant.ContextScopeName, scope)
 				}
 			}