From d914ec2c8b298d0d2dd2967b0b9028e91674fa60 Mon Sep 17 00:00:00 2001 From: p53 Date: Sat, 27 Jan 2024 00:04:37 +0100 Subject: [PATCH] Refactor authz middleware (#407) --- pkg/keycloak/proxy/handlers.go | 10 ++- pkg/keycloak/proxy/middleware.go | 100 +++++++++++++++++++-------- pkg/keycloak/proxy/misc.go | 114 +++++++++++++++---------------- pkg/keycloak/proxy/server.go | 26 ++++++- 4 files changed, 162 insertions(+), 88 deletions(-) diff --git a/pkg/keycloak/proxy/handlers.go b/pkg/keycloak/proxy/handlers.go index 3b29744c..1694da56 100644 --- a/pkg/keycloak/proxy/handlers.go +++ b/pkg/keycloak/proxy/handlers.go @@ -295,7 +295,15 @@ func (r *OauthProxy) oauthCallbackHandler(writer http.ResponseWriter, req *http. // could try to get new uma token/cookie, e.g in case he tried first to access // resource to which he doesn't have access - token, erru := r.getRPT(req, redirectURI, accessToken, methodScope) + token, erru := getRPT( + req.Context(), + r.pat, + r.IdpClient, + r.Config.Realm, + redirectURI, + accessToken, + methodScope, + ) umaError = erru if token != nil { umaToken = token.AccessToken diff --git a/pkg/keycloak/proxy/middleware.go b/pkg/keycloak/proxy/middleware.go index b4e7368f..73176622 100644 --- a/pkg/keycloak/proxy/middleware.go +++ b/pkg/keycloak/proxy/middleware.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "regexp" "strings" "time" @@ -477,12 +478,36 @@ func authenticationMiddleware( authorizationMiddleware is responsible for verifying permissions in access_token/uma_token */ //nolint:cyclop -func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { +func authorizationMiddleware( + logger *zap.Logger, + enableUma bool, + enableUmaMethodScope bool, + cookieUMAName string, + noProxy bool, + pat *PAT, + oidcProvider *oidc3.Provider, + idpClient *gocloak.GoCloak, + openIDProviderTimeout time.Duration, + realm string, + enableEncryptedToken bool, + forceEncryptedCookie bool, + encryptionKey string, + cookManager *cookie.Manager, + enableOpa bool, + opaTimeout time.Duration, + opaAuthzURL *url.URL, + discoveryURI *url.URL, + clientID string, + skipClientIDCheck bool, + skipIssuerCheck bool, + getIdentity func(req *http.Request, tokenCookie string, tokenHeader string) (*UserContext, error), + accessForbidden func(wrt http.ResponseWriter, req *http.Request) context.Context, +) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(wrt http.ResponseWriter, req *http.Request) { scope, assertOk := req.Context().Value(constant.ContextScopeName).(*RequestScope) if !assertOk { - r.Log.Error(apperrors.ErrAssertionFailed.Error()) + logger.Error(apperrors.ErrAssertionFailed.Error()) return } @@ -500,16 +525,16 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { scope.Logger.Debug("query external authz provider for authz") - if r.Config.EnableUma { + if enableUma { var methodScope *string - if r.Config.EnableUmaMethodScope { + if enableUmaMethodScope { methSc := "method:" + req.Method - if r.Config.NoProxy { + if noProxy { xForwardedMethod := req.Header.Get("X-Forwarded-Method") if xForwardedMethod == "" { scope.Logger.Error(apperrors.ErrForwardAuthMissingHeaders.Error()) //nolint:contextcheck - next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) + next.ServeHTTP(wrt, req.WithContext(accessForbidden(wrt, req))) return } methSc = "method:" + xForwardedMethod @@ -518,12 +543,12 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { } authzPath := req.URL.Path - if r.Config.NoProxy { + if noProxy { authzPath = req.Header.Get("X-Forwarded-URI") if authzPath == "" { scope.Logger.Error(apperrors.ErrForwardAuthMissingHeaders.Error()) //nolint:contextcheck - next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) + next.ServeHTTP(wrt, req.WithContext(accessForbidden(wrt, req))) return } } @@ -532,49 +557,68 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { targetPath string, userPerms authorization.Permissions, ) (authorization.AuthzDecision, error) { - r.pat.m.RLock() - token := r.pat.Token.AccessToken - r.pat.m.RUnlock() + pat.m.RLock() + token := pat.Token.AccessToken + pat.m.RUnlock() provider = authorization.NewKeycloakAuthorizationProvider( userPerms, targetPath, - r.IdpClient, - r.Config.OpenIDProviderTimeout, + idpClient, + openIDProviderTimeout, token, - r.Config.Realm, + realm, methodScope, ) return provider.Authorize() } - decision, err = r.WithUMAIdentity(req, authzPath, user, authzFunc) + decision, err = WithUMAIdentity( + req, + authzPath, + user, + cookieUMAName, + oidcProvider, + clientID, + skipClientIDCheck, + skipIssuerCheck, + getIdentity, + authzFunc, + ) if err != nil { var umaUser *UserContext scope.Logger.Error(err.Error()) scope.Logger.Info("trying to get new uma token") - umaUser, err = r.refreshUmaToken(req, authzPath, user, methodScope) + umaUser, err = refreshUmaToken( + req.Context(), + pat, + idpClient, + realm, + authzPath, + user, + methodScope, + ) if err == nil { umaToken := umaUser.RawToken - if r.Config.EnableEncryptedToken || r.Config.ForceEncryptedCookie { - if umaToken, err = encryption.EncodeText(umaToken, r.Config.EncryptionKey); err != nil { + if enableEncryptedToken || forceEncryptedCookie { + if umaToken, err = encryption.EncodeText(umaToken, encryptionKey); err != nil { scope.Logger.Error(err.Error()) //nolint:contextcheck - next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) + next.ServeHTTP(wrt, req.WithContext(accessForbidden(wrt, req))) return } } - r.Cm.DropUMATokenCookie(req, wrt, umaToken, time.Until(umaUser.ExpiresAt)) + cookManager.DropUMATokenCookie(req, wrt, umaToken, time.Until(umaUser.ExpiresAt)) wrt.Header().Set(constant.UMAHeader, umaToken) scope.Logger.Debug("got uma token") decision, err = authzFunc(authzPath, umaUser.Permissions) } } - } else if r.Config.EnableOpa { + } else if enableOpa { provider = authorization.NewOpaAuthorizationProvider( - r.Config.OpaTimeout, - *r.Config.OpaAuthzURL, + opaTimeout, + *opaAuthzURL, req, ) decision, err = provider.Authorize() @@ -601,12 +645,12 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { scope.Logger.Info("authz decision", zap.String("decision", decision.String())) if decision == authorization.DeniedAuthz { - if r.Config.EnableUma { + if enableUma { prv, ok := provider.(*authorization.KeycloakAuthorizationProvider) if !ok { scope.Logger.Error(apperrors.ErrAssertionFailed.Error()) //nolint:contextcheck - next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) + next.ServeHTTP(wrt, req.WithContext(accessForbidden(wrt, req))) return } @@ -617,15 +661,15 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { } else { permHeader := fmt.Sprintf( `realm="%s", as_uri="%s", ticket="%s"`, - r.Config.Realm, - r.Config.DiscoveryURI.Host, + realm, + discoveryURI.Host, ticket, ) wrt.Header().Add(constant.UMATicketHeader, permHeader) } } //nolint:contextcheck - next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) + next.ServeHTTP(wrt, req.WithContext(accessForbidden(wrt, req))) return } diff --git a/pkg/keycloak/proxy/misc.go b/pkg/keycloak/proxy/misc.go index 771e89a9..4d1e1406 100644 --- a/pkg/keycloak/proxy/misc.go +++ b/pkg/keycloak/proxy/misc.go @@ -385,19 +385,41 @@ func (r *OauthProxy) getPAT(done chan bool) { } } -func (r *OauthProxy) WithUMAIdentity( +func WithUMAIdentity( req *http.Request, targetPath string, user *UserContext, + cookieUMAName string, + provider *oidc3.Provider, + clientID string, + skipClientIDCheck bool, + skipIssuerCheck bool, + getIdentity func(req *http.Request, tokenCookie string, tokenHeader string) (*UserContext, error), authzFunc func(targetPath string, userPerms authorization.Permissions) (authorization.AuthzDecision, error), ) (authorization.AuthzDecision, error) { - umaUser, err := r.GetIdentity(req, r.Config.CookieUMAName, constant.UMAHeader) + umaUser, err := getIdentity(req, cookieUMAName, constant.UMAHeader) if err != nil { return authorization.DeniedAuthz, err } - err = r.verifyUmaToken(user, umaUser, req) + // make sure somebody doesn't sent one user access token + // and others user valid uma token in one request + if umaUser.ID != user.ID { + return authorization.DeniedAuthz, apperrors.ErrAccessMismatchUmaToken + } + + _, err = verifyToken( + req.Context(), + provider, + umaUser.RawToken, + clientID, + skipClientIDCheck, + skipIssuerCheck, + ) if err != nil { + if strings.Contains(err.Error(), "token is expired") { + return authorization.DeniedAuthz, apperrors.ErrUMATokenExpired + } return authorization.DeniedAuthz, err } @@ -405,18 +427,15 @@ func (r *OauthProxy) WithUMAIdentity( } // getRPT retrieves relaying party token -func (r *OauthProxy) getRPT( - req *http.Request, +func getRPT( + ctx context.Context, + pat *PAT, + idpClient *gocloak.GoCloak, + realm string, targetPath string, userToken string, methodScope *string, ) (*gocloak.JWT, error) { - ctx, cancel := context.WithTimeout( - req.Context(), - r.Config.OpenIDProviderTimeout, - ) - defer cancel() - matchingURI := true resourceParam := gocloak.GetResourceParams{ URI: &targetPath, @@ -424,14 +443,14 @@ func (r *OauthProxy) getRPT( Scope: methodScope, } - r.pat.m.RLock() - pat := r.pat.Token.AccessToken - r.pat.m.RUnlock() + pat.m.RLock() + patTok := pat.Token.AccessToken + pat.m.RUnlock() - resources, err := r.IdpClient.GetResourcesClient( + resources, err := idpClient.GetResourcesClient( ctx, - pat, - r.Config.Realm, + patTok, + realm, resourceParam, ) if err != nil { @@ -474,10 +493,10 @@ func (r *OauthProxy) getRPT( }, } - permTicket, err := r.IdpClient.CreatePermissionTicket( + permTicket, err := idpClient.CreatePermissionTicket( ctx, - pat, - r.Config.Realm, + patTok, + realm, permissions, ) if err != nil { @@ -497,10 +516,10 @@ func (r *OauthProxy) getRPT( } if userToken == "" { - userToken = pat + userToken = patTok } - rpt, err := r.IdpClient.GetRequestingPartyToken(ctx, userToken, r.Config.Realm, rptOptions) + rpt, err := idpClient.GetRequestingPartyToken(ctx, userToken, realm, rptOptions) if err != nil { return nil, fmt.Errorf( "%s resource: %s %w", @@ -563,39 +582,7 @@ func (r *OauthProxy) getCodeFlowTokens( return resp.AccessToken, idToken, resp.RefreshToken, nil } -func (r *OauthProxy) verifyUmaToken( - accessUserCtx *UserContext, - umaUserCtx *UserContext, - req *http.Request, -) error { - // make sure somebody doesn't sent one user access token - // and others user valid uma token in one request - if umaUserCtx.ID != accessUserCtx.ID { - return apperrors.ErrAccessMismatchUmaToken - } - - verifier := r.Provider.Verifier( - &oidc3.Config{ - ClientID: r.Config.ClientID, - SkipClientIDCheck: r.Config.SkipAccessTokenClientIDCheck, - SkipIssuerCheck: r.Config.SkipAccessTokenIssuerCheck, - }, - ) - - ctx, cancel := context.WithTimeout(req.Context(), r.Config.OpenIDProviderTimeout) - defer cancel() - - _, err := verifier.Verify(ctx, umaUserCtx.RawToken) - if err != nil { - if strings.Contains(err.Error(), "token is expired") { - return apperrors.ErrUMATokenExpired - } - return fmt.Errorf("%s %w", apperrors.ErrTokenVerificationFailure.Error(), err) - } - - return nil -} - +// verifyOIDCTokens func verifyOIDCTokens( ctx context.Context, provider *oidc3.Provider, @@ -730,13 +717,24 @@ func (r *OauthProxy) getRequestURIFromCookie( return string(decoded) } -func (r *OauthProxy) refreshUmaToken( - req *http.Request, +func refreshUmaToken( + ctx context.Context, + pat *PAT, + idpClient *gocloak.GoCloak, + realm string, targetPath string, user *UserContext, methodScope *string, ) (*UserContext, error) { - tok, err := r.getRPT(req, targetPath, user.RawToken, methodScope) + tok, err := getRPT( + ctx, + pat, + idpClient, + realm, + targetPath, + user.RawToken, + methodScope, + ) if err != nil { return nil, err } diff --git a/pkg/keycloak/proxy/server.go b/pkg/keycloak/proxy/server.go index 39f26b77..c37a734e 100644 --- a/pkg/keycloak/proxy/server.go +++ b/pkg/keycloak/proxy/server.go @@ -568,7 +568,31 @@ func (r *OauthProxy) CreateReverseProxy() error { if r.Config.EnableUma || r.Config.EnableOpa { middlewares = []func(http.Handler) http.Handler{ authMid, - r.authorizationMiddleware(), + authorizationMiddleware( + r.Log, + r.Config.EnableUma, + r.Config.EnableUmaMethodScope, + r.Config.CookieUMAName, + r.Config.NoProxy, + r.pat, + r.Provider, + r.IdpClient, + r.Config.OpenIDProviderTimeout, + r.Config.Realm, + r.Config.EnableEncryptedToken, + r.Config.ForceEncryptedCookie, + r.Config.EncryptionKey, + r.Cm, + r.Config.EnableOpa, + r.Config.OpaTimeout, + r.Config.OpaAuthzURL, + r.Config.DiscoveryURI, + r.Config.ClientID, + r.Config.SkipAccessTokenClientIDCheck, + r.Config.SkipAccessTokenIssuerCheck, + r.GetIdentity, + r.accessForbidden, + ), admissionMiddleware( r.Log, res,