From a082bfa4fe249af65408e7e3e1abb784d71feb84 Mon Sep 17 00:00:00 2001 From: p53 Date: Thu, 26 Oct 2023 22:40:40 +0200 Subject: [PATCH] Add standardized errors where possible (#364) --- pkg/apperrors/apperrors.go | 28 ++++++++- pkg/keycloak/proxy/handlers.go | 104 ++++++++++++------------------- pkg/keycloak/proxy/middleware.go | 37 +++-------- pkg/keycloak/proxy/misc.go | 32 +++------- 4 files changed, 83 insertions(+), 118 deletions(-) diff --git a/pkg/apperrors/apperrors.go b/pkg/apperrors/apperrors.go index 7dac0a51..1a091956 100644 --- a/pkg/apperrors/apperrors.go +++ b/pkg/apperrors/apperrors.go @@ -40,13 +40,37 @@ var ( ErrSessionExpiredRefreshOff = errors.New("session expired and access token refreshing is disabled") ErrRefreshTokenNotFound = errors.New("unable to find refresh token for user") ErrAccTokenRefreshFailure = errors.New("failed to refresh the access token") - ErrEncryptAccToken = errors.New("unable to encode access token") + ErrEncryptAccToken = errors.New("unable to encrypt access token") ErrEncryptRefreshToken = errors.New("failed to encrypt refresh token") - ErrEncryptIDToken = errors.New("unable to encode idToken token") + ErrEncryptIDToken = errors.New("unable to encrypt idToken token") ErrDelTokFromStore = errors.New("failed to remove old token") ErrSaveTokToStore = errors.New("failed to store refresh token") + ErrLoginWithLoginHandleDisabled = errors.New("attempt to login when login handler is disabled") + ErrMissingLoginCreds = errors.New("request does not have both username and password") + ErrInvalidUserCreds = errors.New("invalid user credentials") + ErrAcquireTokenViaPassCredsGrant = errors.New("unable to request the access token via grant_type 'password'") + ErrExtractIdentityFromAccessToken = errors.New("unable to extract identity from access token") + ErrResponseMissingIDToken = errors.New("token response does not contain an id_token") + ErrResponseMissingExpires = errors.New("token response does not contain expires_in") + ErrParseRefreshToken = errors.New("failed to parse refresh token") + ErrParseIDToken = errors.New("failed to parse id token") + ErrParseAccessToken = errors.New("failed to parse access token") + ErrParseIDTokenClaims = errors.New("faled to parse id token claims") + 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") + + ErrCreateRevocationReq = errors.New("unable to construct the revocation request") + ErrRevocationReqFailure = errors.New("request to revocation endpoint failed") + ErrInvalidRevocationResp = errors.New("invalid response from revocation endpoint") + + ErrMarshallDiscoveryResp = errors.New("problem marshalling discovery response") + ErrDiscoveryResponseWrite = errors.New("problem during discovery response write") + // config errors ErrInvalidPostLoginRedirectPath = errors.New("post login redirect path invalid, should be only path not absolute url (no hostname, scheme)") diff --git a/pkg/keycloak/proxy/handlers.go b/pkg/keycloak/proxy/handlers.go index da869a30..f9d55080 100644 --- a/pkg/keycloak/proxy/handlers.go +++ b/pkg/keycloak/proxy/handlers.go @@ -225,7 +225,7 @@ func (r *OauthProxy) oauthCallbackHandler(writer http.ResponseWriter, req *http. case r.useStore(): if err = r.StoreRefreshToken(req.Context(), rawAccessToken, encrypted, oidcTokensCookiesExp); err != nil { scope.Logger.Error( - "failed to save the refresh token in the store", + apperrors.ErrSaveTokToStore.Error(), zap.Error(err), zap.String("sub", stdClaims.Subject), zap.String("email", customClaims.Email), @@ -311,7 +311,7 @@ func (r *OauthProxy) oauthCallbackHandler(writer http.ResponseWriter, req *http. /* loginHandler provide's a generic endpoint for clients to perform a user_credentials login to the provider */ -//nolint:funlen,cyclop // refactor +//nolint:cyclop // refactor func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) { scope, assertOk := req.Context().Value(constant.ContextScopeName).(*RequestScope) @@ -321,7 +321,7 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) return } - errorMsg, code, err := func() (string, int, error) { + code, err := func() (int, error) { ctx, cancel := context.WithTimeout( context.Background(), r.Config.OpenIDProviderTimeout, @@ -338,33 +338,30 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) defer cancel() if !r.Config.EnableLoginHandler { - return "attempt to login when login handler is disabled", - http.StatusNotImplemented, - errors.New("login handler disabled") + return http.StatusNotImplemented, + apperrors.ErrLoginWithLoginHandleDisabled } username := req.PostFormValue("username") password := req.PostFormValue("password") if username == "" || password == "" { - return "request does not have both username and password", - http.StatusBadRequest, - errors.New("no credentials") + return http.StatusBadRequest, + apperrors.ErrMissingLoginCreds } conf := r.newOAuth2Config(r.getRedirectionURL(writer, req)) start := time.Now() token, err := conf.PasswordCredentialsToken(ctx, username, password) - if err != nil { if !token.Valid() { - return "invalid user credentials provided", http.StatusUnauthorized, err + return http.StatusUnauthorized, + errors.Join(apperrors.ErrInvalidUserCreds, err) } - return "unable to request the access token via grant_type 'password'", - http.StatusInternalServerError, - err + return http.StatusInternalServerError, + errors.Join(apperrors.ErrAcquireTokenViaPassCredsGrant, err) } // @metric observe the time taken for a login request @@ -373,34 +370,28 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) accessToken := token.AccessToken refreshToken := "" accessTokenObj, err := jwt.ParseSigned(token.AccessToken) - if err != nil { - return "unable to decode the access token", http.StatusNotImplemented, err + return http.StatusNotImplemented, + errors.Join(apperrors.ErrParseAccessToken, err) } identity, err := ExtractIdentity(accessTokenObj) - if err != nil { - return "unable to extract identity from access token", - http.StatusNotImplemented, - err + return http.StatusNotImplemented, + errors.Join(apperrors.ErrExtractIdentityFromAccessToken, err) } writer.Header().Set("Content-Type", "application/json") idToken, assertOk := token.Extra("id_token").(string) - if !assertOk { - return "", - http.StatusInternalServerError, - fmt.Errorf("token response does not contain an id_token") + return http.StatusInternalServerError, + apperrors.ErrResponseMissingIDToken } expiresIn, assertOk := token.Extra("expires_in").(float64) - if !assertOk { - return "", - http.StatusInternalServerError, - fmt.Errorf("token response does not contain expires_in") + return http.StatusInternalServerError, + apperrors.ErrResponseMissingExpires } // step: are we encrypting the access token? @@ -409,28 +400,24 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) if r.Config.EnableEncryptedToken || r.Config.ForceEncryptedCookie { if accessToken, err = encryption.EncodeText(accessToken, r.Config.EncryptionKey); err != nil { scope.Logger.Error(apperrors.ErrEncryptAccToken.Error(), zap.Error(err)) - return apperrors.ErrEncryptAccToken.Error(), - http.StatusInternalServerError, - err + return http.StatusInternalServerError, + errors.Join(apperrors.ErrEncryptAccToken, err) } if idToken, err = encryption.EncodeText(idToken, r.Config.EncryptionKey); err != nil { scope.Logger.Error(apperrors.ErrEncryptIDToken.Error(), zap.Error(err)) - return apperrors.ErrEncryptIDToken.Error(), - http.StatusInternalServerError, - err + return http.StatusInternalServerError, + errors.Join(apperrors.ErrEncryptIDToken, err) } } // step: does the response have a refresh token and we do NOT ignore refresh tokens? if r.Config.EnableRefreshTokens && token.RefreshToken != "" { refreshToken, err = encryption.EncodeText(token.RefreshToken, r.Config.EncryptionKey) - if err != nil { scope.Logger.Error(apperrors.ErrEncryptRefreshToken.Error(), zap.Error(err)) - return apperrors.ErrEncryptRefreshToken.Error(), - http.StatusInternalServerError, - err + return http.StatusInternalServerError, + errors.Join(apperrors.ErrEncryptRefreshToken, err) } // drop in the access token - cookie expiration = access token @@ -452,18 +439,14 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) // 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) - if errRef != nil { - scope.Logger.Error("failed to parse refresh token", zap.Error(errRef)) - return "failed to parse refresh token", - http.StatusInternalServerError, - errRef + return http.StatusInternalServerError, + errors.Join(apperrors.ErrParseRefreshToken, err) } stdRefreshClaims := &jwt.Claims{} err = refreshTokenObj.UnsafeClaimsWithoutVerification(stdRefreshClaims) - if err != nil { expiration = 0 } else { @@ -474,7 +457,7 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) case true: if err = r.StoreRefreshToken(req.Context(), token.AccessToken, refreshToken, expiration); err != nil { scope.Logger.Warn( - "failed to save the refresh token in the store", + apperrors.ErrSaveTokToStore.Error(), zap.Error(err), ) } @@ -503,10 +486,8 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) if tokenScope != nil { tScope, assertOk = tokenScope.(string) - if !assertOk { - return "", - http.StatusInternalServerError, + return http.StatusInternalServerError, apperrors.ErrAssertionFailed } } @@ -534,21 +515,19 @@ func (r *OauthProxy) loginHandler(writer http.ResponseWriter, req *http.Request) } err = json.NewEncoder(writer).Encode(resp) - if err != nil { - return "", http.StatusInternalServerError, err + return http.StatusInternalServerError, err } - return "", http.StatusOK, nil + return http.StatusOK, nil }() if err != nil { clientIP := utils.RealIP(req) - scope.Logger.Error(errorMsg, + scope.Logger.Error(err.Error(), zap.String("client_ip", clientIP), zap.String("remote_addr", req.RemoteAddr), - zap.Error(err)) - + ) writer.WriteHeader(code) } } @@ -583,7 +562,6 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request } scope, assertOk := req.Context().Value(constant.ContextScopeName).(*RequestScope) - if !assertOk { r.Log.Error(apperrors.ErrAssertionFailed.Error()) writer.WriteHeader(http.StatusInternalServerError) @@ -592,7 +570,6 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request // @step: drop the access token user, err := r.GetIdentity(req, r.Config.CookieAccessName, "") - if err != nil { r.accessError(writer, req) return @@ -607,7 +584,6 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request } idToken, _, err := r.retrieveIDToken(req) - // we are doing it so that in case with no-redirects=true, we can pass // id token in authorization header if err != nil { @@ -624,7 +600,7 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request go func() { if err = r.DeleteRefreshToken(req.Context(), user.RawToken); err != nil { scope.Logger.Error( - "unable to remove the refresh token from store", + apperrors.ErrDelTokFromStore.Error(), zap.Error(err), ) } @@ -695,9 +671,8 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request fmt.Sprintf("token=%s", identityToken), ), ) - if err != nil { - scope.Logger.Error("unable to construct the revocation request", zap.Error(err)) + scope.Logger.Error(apperrors.ErrCreateRevocationReq.Error(), zap.Error(err)) writer.WriteHeader(http.StatusInternalServerError) return } @@ -708,9 +683,8 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request start := time.Now() response, err := client.Do(request) - if err != nil { - scope.Logger.Error("unable to post to revocation endpoint", zap.Error(err)) + scope.Logger.Error(apperrors.ErrRevocationReqFailure.Error(), zap.Error(err)) writer.WriteHeader(http.StatusInternalServerError) return } @@ -731,7 +705,7 @@ func (r *OauthProxy) logoutHandler(writer http.ResponseWriter, req *http.Request content, _ := io.ReadAll(response.Body) scope.Logger.Error( - "invalid response from revocation endpoint", + apperrors.ErrInvalidRevocationResp.Error(), zap.Int("status", response.StatusCode), zap.String("response", string(content)), ) @@ -865,7 +839,7 @@ func (r *OauthProxy) discoveryHandler(wrt http.ResponseWriter, _ *http.Request) if err != nil { r.Log.Error( - "problem marshalling response", + apperrors.ErrMarshallDiscoveryResp.Error(), zap.String("error", err.Error()), ) @@ -879,7 +853,7 @@ func (r *OauthProxy) discoveryHandler(wrt http.ResponseWriter, _ *http.Request) if err != nil { r.Log.Error( - "problem during response write", + apperrors.ErrDiscoveryResponseWrite.Error(), zap.String("error", err.Error()), ) } diff --git a/pkg/keycloak/proxy/middleware.go b/pkg/keycloak/proxy/middleware.go index 9fd52d4a..4503d605 100644 --- a/pkg/keycloak/proxy/middleware.go +++ b/pkg/keycloak/proxy/middleware.go @@ -511,7 +511,7 @@ func (r *OauthProxy) authorizationMiddleware() func(http.Handler) http.Handler { case apperrors.ErrNoAuthzFound: default: if err != nil { - scope.Logger.Error("Undexpected error during authorization", zap.Error(err)) + scope.Logger.Error(apperrors.ErrFailedAuthzRequest.Error(), zap.Error(err)) //nolint:contextcheck next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) return @@ -550,7 +550,6 @@ func (r *OauthProxy) checkClaim(user *UserContext, claimName string, match *rege switch user.Claims[claimName].(type) { case []interface{}: claims, assertOk := user.Claims[claimName].([]interface{}) - if !assertOk { r.Log.Error(apperrors.ErrAssertionFailed.Error()) return false @@ -558,7 +557,6 @@ func (r *OauthProxy) checkClaim(user *UserContext, claimName string, match *rege for _, v := range claims { value, ok := v.(string) - if !ok { r.Log.Warn( "Problem while asserting claim", @@ -595,12 +593,10 @@ func (r *OauthProxy) checkClaim(user *UserContext, claimName string, match *rege return false case string: claims, assertOk := user.Claims[claimName].(string) - if !assertOk { r.Log.Error(apperrors.ErrAssertionFailed.Error()) return false } - if match.MatchString(claims) { return true } @@ -630,7 +626,6 @@ func (r *OauthProxy) checkClaim(user *UserContext, claimName string, match *rege //nolint:cyclop func (r *OauthProxy) admissionMiddleware(resource *authorization.Resource) func(http.Handler) http.Handler { claimMatches := make(map[string]*regexp.Regexp) - for k, v := range r.Config.MatchClaims { claimMatches[k] = regexp.MustCompile(v) } @@ -639,27 +634,26 @@ func (r *OauthProxy) admissionMiddleware(resource *authorization.Resource) func( return http.HandlerFunc(func(wrt http.ResponseWriter, req *http.Request) { // we don't need to continue is a decision has been made scope, assertOk := req.Context().Value(constant.ContextScopeName).(*RequestScope) - if !assertOk { r.Log.Error(apperrors.ErrAssertionFailed.Error()) return } - if scope.AccessDenied { next.ServeHTTP(wrt, req) return } user := scope.Identity + lLog := scope.Logger.With( + zap.String("access", "denied"), + zap.String("email", user.Email), + zap.String("resource", resource.URL), + ) // @step: we need to check the roles if !utils.HasAccess(resource.Roles, user.Roles, !resource.RequireAnyRole) { - scope.Logger.Warn("access denied, invalid roles", - zap.String("access", "denied"), - zap.String("email", user.Email), - zap.String("resource", resource.URL), + lLog.Warn("access denied, invalid roles", zap.String("roles", resource.GetRoles())) - //nolint:contextcheck next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) return @@ -673,12 +667,8 @@ func (r *OauthProxy) admissionMiddleware(resource *authorization.Resource) func( name := resVals[0] canonName := http.CanonicalHeaderKey(name) values, ok := req.Header[canonName] - if !ok { - scope.Logger.Warn("access denied, invalid headers", - zap.String("access", "denied"), - zap.String("email", user.Email), - zap.String("resource", resource.URL), + lLog.Warn("access denied, invalid headers", zap.String("headers", resource.GetHeaders())) //nolint:contextcheck @@ -698,10 +688,7 @@ func (r *OauthProxy) admissionMiddleware(resource *authorization.Resource) func( // @step: we need to check the headers if !utils.HasAccess(resource.Headers, reqHeaders, true) { - scope.Logger.Warn("access denied, invalid headers", - zap.String("access", "denied"), - zap.String("email", user.Email), - zap.String("resource", resource.URL), + lLog.Warn("access denied, invalid headers", zap.String("headers", resource.GetHeaders())) //nolint:contextcheck @@ -712,12 +699,8 @@ func (r *OauthProxy) admissionMiddleware(resource *authorization.Resource) func( // @step: check if we have any groups, the groups are there if !utils.HasAccess(resource.Groups, user.Groups, false) { - scope.Logger.Warn("access denied, invalid groups", - zap.String("access", "denied"), - zap.String("email", user.Email), - zap.String("resource", resource.URL), + lLog.Warn("access denied, invalid groups", zap.String("groups", strings.Join(resource.Groups, ","))) - //nolint:contextcheck next.ServeHTTP(wrt, req.WithContext(r.accessForbidden(wrt, req))) return diff --git a/pkg/keycloak/proxy/misc.go b/pkg/keycloak/proxy/misc.go index 1166fd8e..42555c1b 100644 --- a/pkg/keycloak/proxy/misc.go +++ b/pkg/keycloak/proxy/misc.go @@ -389,7 +389,6 @@ func (r *OauthProxy) getRPT( r.Config.Realm, resourceParam, ) - if err != nil { return nil, fmt.Errorf( "%s %s", @@ -436,7 +435,6 @@ func (r *OauthProxy) getRPT( r.Config.Realm, permissions, ) - if err != nil { return nil, fmt.Errorf( "%s resource: %s %w", @@ -458,7 +456,6 @@ func (r *OauthProxy) getRPT( } rpt, err := r.IdpClient.GetRequestingPartyToken(ctx, userToken, r.Config.Realm, rptOptions) - if err != nil { return nil, fmt.Errorf( "%s resource: %s %w", @@ -504,7 +501,6 @@ func (r *OauthProxy) getCodeFlowTokens( codeVerifier, r.Config.SkipOpenIDProviderTLSVerify, ) - if err != nil { scope.Logger.Error("unable to exchange code for access token", zap.Error(err)) r.accessForbidden(writer, req) @@ -512,7 +508,6 @@ func (r *OauthProxy) getCodeFlowTokens( } idToken, assertOk := resp.Extra("id_token").(string) - if !assertOk { scope.Logger.Error("unable to obtain id token", zap.Error(err)) r.accessForbidden(writer, req) @@ -575,17 +570,15 @@ func (r *OauthProxy) verifyOIDCTokens( defer cancel() idToken, err = verifier.Verify(ctx, rawIDToken) - if err != nil { - scope.Logger.Error("unable to verify the id token", zap.Error(err)) + scope.Logger.Error(apperrors.ErrVerifyIDToken.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, nil, err } token, err := jwt.ParseSigned(rawIDToken) - if err != nil { - scope.Logger.Error("unable to parse id token", zap.Error(err)) + scope.Logger.Error(apperrors.ErrParseIDToken.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, nil, err } @@ -594,9 +587,8 @@ func (r *OauthProxy) verifyOIDCTokens( customClaims := &custClaims{} err = token.UnsafeClaimsWithoutVerification(stdClaims, customClaims) - if err != nil { - scope.Logger.Error("unable to parse id token for claims", zap.Error(err)) + scope.Logger.Error(apperrors.ErrParseIDTokenClaims.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, nil, err } @@ -608,18 +600,15 @@ func (r *OauthProxy) verifyOIDCTokens( err = idToken.VerifyAccessToken(rawAccessToken) if err != nil { - scope.Logger.Error("unable to verify access token", zap.Error(err)) + scope.Logger.Error(apperrors.ErrVerifyAccessToken.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, nil, err } } accToken, err := jwt.ParseSigned(rawAccessToken) - if err != nil { - scope.Logger.Error( - "unable to parse the access token, using id token only", - ) + scope.Logger.Error(apperrors.ErrParseAccessToken.Error()) r.accessForbidden(writer, req) return nil, nil, err } @@ -629,9 +618,8 @@ func (r *OauthProxy) verifyOIDCTokens( customClaims = &custClaims{} err = token.UnsafeClaimsWithoutVerification(stdClaims, customClaims) - if err != nil { - scope.Logger.Error("unable to parse access token for claims", zap.Error(err)) + scope.Logger.Error(apperrors.ErrParseAccessTokenClaims.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, nil, err } @@ -663,18 +651,16 @@ func (r *OauthProxy) verifyRefreshToken( req *http.Request, ) (*jwt.Claims, error) { refreshToken, err := jwt.ParseSigned(rawRefreshToken) - if err != nil { - scope.Logger.Error("failed to parse refresh token", zap.Error(err)) + scope.Logger.Error(apperrors.ErrParseRefreshToken.Error(), zap.Error(err)) writer.WriteHeader(http.StatusInternalServerError) return nil, err } stdRefreshClaims := &jwt.Claims{} err = refreshToken.UnsafeClaimsWithoutVerification(stdRefreshClaims) - if err != nil { - scope.Logger.Error("unable to parse refresh token for claims", zap.Error(err)) + scope.Logger.Error(apperrors.ErrParseRefreshTokenClaims.Error(), zap.Error(err)) r.accessForbidden(writer, req) return nil, err } @@ -709,7 +695,6 @@ func (r *OauthProxy) getRequestURIFromCookie( ) string { // some clients URL-escape padding characters unescapedValue, err := url.PathUnescape(encodedRequestURI.Value) - if err != nil { scope.Logger.Warn( "app did send a corrupted redirectURI in cookie: invalid url escaping", @@ -721,7 +706,6 @@ func (r *OauthProxy) getRequestURIFromCookie( // This is safe for browsers using atob() but needs to be treated with care for nodeJS clients, // which natively use base64url encoding, and url-escape padding '=' characters. decoded, err := base64.StdEncoding.DecodeString(unescapedValue) - if err != nil { scope.Logger.Warn( "app did send a corrupted redirectURI in cookie: invalid base64url encoding",