From 06a1b52adf4dfeaa690d425c6eb5ae988bdd67aa Mon Sep 17 00:00:00 2001 From: Livio Amstutz Date: Mon, 2 May 2022 17:26:54 +0200 Subject: [PATCH] fix: improve interceptor handling (#3578) * fix: improve interceptor handling * fix: improve interceptor handling Co-authored-by: Florian Forster --- cmd/admin/start/start.go | 4 +-- .../server/middleware/instance_interceptor.go | 8 ++++-- internal/api/http/cookie.go | 7 +++++ .../http/middleware/instance_interceptor.go | 25 +++++++++++++---- .../api/http/middleware/user_agent_cookie.go | 27 ++++++++++++------- internal/api/oidc/key.go | 2 +- internal/api/ui/console/console.go | 4 +-- internal/api/ui/login/login.go | 23 +++++++++++----- internal/api/ui/login/router.go | 9 +++++++ 9 files changed, 81 insertions(+), 28 deletions(-) diff --git a/cmd/admin/start/start.go b/cmd/admin/start/start.go index 986079ce0b..6ae353617a 100644 --- a/cmd/admin/start/start.go +++ b/cmd/admin/start/start.go @@ -174,10 +174,10 @@ func startAPIs(ctx context.Context, router *mux.Router, commands *command.Comman return err } - instanceInterceptor := middleware.InstanceInterceptor(queries, config.HTTP1HostHeader) + instanceInterceptor := middleware.InstanceInterceptor(queries, config.HTTP1HostHeader, login.IgnoreInstanceEndpoints...) authenticatedAPIs.RegisterHandler(assets.HandlerPrefix, assets.NewHandler(commands, verifier, config.InternalAuthZ, id.SonyFlakeGenerator, store, queries, instanceInterceptor.Handler)) - userAgentInterceptor, err := middleware.NewUserAgentHandler(config.UserAgentCookie, keys.UserAgentCookieKey, id.SonyFlakeGenerator, config.ExternalSecure) + userAgentInterceptor, err := middleware.NewUserAgentHandler(config.UserAgentCookie, keys.UserAgentCookieKey, id.SonyFlakeGenerator, config.ExternalSecure, login.EndpointResources) if err != nil { return err } diff --git a/internal/api/grpc/server/middleware/instance_interceptor.go b/internal/api/grpc/server/middleware/instance_interceptor.go index f497fd103d..707a4533b6 100644 --- a/internal/api/grpc/server/middleware/instance_interceptor.go +++ b/internal/api/grpc/server/middleware/instance_interceptor.go @@ -11,6 +11,7 @@ import ( "google.golang.org/grpc/status" "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/telemetry/tracing" ) type InstanceVerifier interface { @@ -24,20 +25,23 @@ func InstanceInterceptor(verifier authz.InstanceVerifier, headerName string, ign } func setInstance(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler, verifier authz.InstanceVerifier, headerName string, ignoredServices ...string) (_ interface{}, err error) { + interceptorCtx, span := tracing.NewServerInterceptorSpan(ctx) + defer func() { span.EndWithError(err) }() for _, service := range ignoredServices { if strings.HasPrefix(info.FullMethod, service) { return handler(ctx, req) } } - host, err := hostNameFromContext(ctx, headerName) + host, err := hostNameFromContext(interceptorCtx, headerName) if err != nil { return nil, status.Error(codes.PermissionDenied, err.Error()) } - instance, err := verifier.InstanceByHost(ctx, host) + instance, err := verifier.InstanceByHost(interceptorCtx, host) if err != nil { return nil, status.Error(codes.PermissionDenied, err.Error()) } + span.End() return handler(authz.WithInstance(ctx, instance), req) } diff --git a/internal/api/http/cookie.go b/internal/api/http/cookie.go index 4dbadcc24f..68eb50b3bd 100644 --- a/internal/api/http/cookie.go +++ b/internal/api/http/cookie.go @@ -136,4 +136,11 @@ func (c *CookieHandler) httpSet(w http.ResponseWriter, name, domain, value strin Secure: c.secureOnly, SameSite: c.sameSite, }) + varyValues := w.Header().Values("vary") + for _, vary := range varyValues { + if vary == "Cookie" { + return + } + } + w.Header().Add("vary", "Cookie") } diff --git a/internal/api/http/middleware/instance_interceptor.go b/internal/api/http/middleware/instance_interceptor.go index e37560a538..cf0d72d0a5 100644 --- a/internal/api/http/middleware/instance_interceptor.go +++ b/internal/api/http/middleware/instance_interceptor.go @@ -4,25 +4,34 @@ import ( "context" "fmt" "net/http" + "strings" "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/telemetry/tracing" ) type instanceInterceptor struct { - verifier authz.InstanceVerifier - headerName string + verifier authz.InstanceVerifier + headerName string + ignoredPrefixes []string } -func InstanceInterceptor(verifier authz.InstanceVerifier, headerName string) *instanceInterceptor { +func InstanceInterceptor(verifier authz.InstanceVerifier, headerName string, ignoredPrefixes ...string) *instanceInterceptor { return &instanceInterceptor{ - verifier: verifier, - headerName: headerName, + verifier: verifier, + headerName: headerName, + ignoredPrefixes: ignoredPrefixes, } } func (a *instanceInterceptor) Handler(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for _, prefix := range a.ignoredPrefixes { + if strings.HasPrefix(r.URL.Path, prefix) { + next.ServeHTTP(w, r) + return + } + } ctx, err := setInstance(r, a.verifier, a.headerName) if err != nil { http.Error(w, err.Error(), http.StatusUnauthorized) @@ -35,6 +44,12 @@ func (a *instanceInterceptor) Handler(next http.Handler) http.Handler { func (a *instanceInterceptor) HandlerFunc(next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + for _, prefix := range a.ignoredPrefixes { + if strings.HasPrefix(r.URL.Path, prefix) { + next.ServeHTTP(w, r) + return + } + } ctx, err := setInstance(r, a.verifier, a.headerName) if err != nil { http.Error(w, err.Error(), http.StatusForbidden) diff --git a/internal/api/http/middleware/user_agent_cookie.go b/internal/api/http/middleware/user_agent_cookie.go index e471a5b288..99aa482bff 100644 --- a/internal/api/http/middleware/user_agent_cookie.go +++ b/internal/api/http/middleware/user_agent_cookie.go @@ -3,6 +3,7 @@ package middleware import ( "context" "net/http" + "strings" "time" http_utils "github.com/zitadel/zitadel/internal/api/http" @@ -26,10 +27,11 @@ type UserAgent struct { } type userAgentHandler struct { - cookieHandler *http_utils.CookieHandler - cookieName string - idGenerator id.Generator - nextHandler http.Handler + cookieHandler *http_utils.CookieHandler + cookieName string + idGenerator id.Generator + nextHandler http.Handler + ignoredPrefixes []string } type UserAgentCookieConfig struct { @@ -37,7 +39,7 @@ type UserAgentCookieConfig struct { MaxAge time.Duration } -func NewUserAgentHandler(config *UserAgentCookieConfig, cookieKey []byte, idGenerator id.Generator, externalSecure bool) (func(http.Handler) http.Handler, error) { +func NewUserAgentHandler(config *UserAgentCookieConfig, cookieKey []byte, idGenerator id.Generator, externalSecure bool, ignoredPrefixes ...string) (func(http.Handler) http.Handler, error) { opts := []http_utils.CookieHandlerOpt{ http_utils.WithEncryption(cookieKey, cookieKey), http_utils.WithMaxAge(int(config.MaxAge.Seconds())), @@ -47,15 +49,22 @@ func NewUserAgentHandler(config *UserAgentCookieConfig, cookieKey []byte, idGene } return func(handler http.Handler) http.Handler { return &userAgentHandler{ - nextHandler: handler, - cookieName: config.Name, - cookieHandler: http_utils.NewCookieHandler(opts...), - idGenerator: idGenerator, + nextHandler: handler, + cookieName: config.Name, + cookieHandler: http_utils.NewCookieHandler(opts...), + idGenerator: idGenerator, + ignoredPrefixes: ignoredPrefixes, } }, nil } func (ua *userAgentHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + for _, prefix := range ua.ignoredPrefixes { + if strings.HasPrefix(r.URL.Path, prefix) { + ua.nextHandler.ServeHTTP(w, r) + return + } + } agent, err := ua.getUserAgent(r) if err != nil { agent, err = ua.newUserAgent() diff --git a/internal/api/oidc/key.go b/internal/api/oidc/key.go index 220c4e059d..f19b2d80cf 100644 --- a/internal/api/oidc/key.go +++ b/internal/api/oidc/key.go @@ -201,11 +201,11 @@ func setOIDCCtx(ctx context.Context) context.Context { func retry(retryable func() error) (err error) { for i := 0; i < retryCount; i++ { - time.Sleep(retryBackoff) err = retryable() if err == nil { return nil } + time.Sleep(retryBackoff) } return err } diff --git a/internal/api/ui/console/console.go b/internal/api/ui/console/console.go index 8d6729f7cb..1da26feb17 100644 --- a/internal/api/ui/console/console.go +++ b/internal/api/ui/console/console.go @@ -74,7 +74,7 @@ func Start(config Config, externalSecure bool, issuer op.IssuerFromRequest, inst handler := mux.NewRouter() handler.Use(cache, security) - handler.Handle(envRequestPath, instanceHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handler.Handle(envRequestPath, middleware.TelemetryHandler()(instanceHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { instance := authz.GetInstance(r.Context()) if instance.InstanceID() == "" { http.Error(w, "empty instanceID", http.StatusInternalServerError) @@ -88,7 +88,7 @@ func Start(config Config, externalSecure bool, issuer op.IssuerFromRequest, inst } _, err = w.Write(environmentJSON) logging.OnError(err).Error("error serving environment.json") - }))) + })))) handler.SkipClean(true).PathPrefix("").Handler(http.FileServer(&spaHandler{http.FS(fSys)})) return handler, nil } diff --git a/internal/api/ui/login/login.go b/internal/api/ui/login/login.go index d2040055da..43aa6d6f69 100644 --- a/internal/api/ui/login/login.go +++ b/internal/api/ui/login/login.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "strings" "github.com/gorilla/csrf" "github.com/gorilla/mux" @@ -93,7 +94,7 @@ func CreateLogin(config Config, } security := middleware.SecurityHeaders(csp(), login.cspErrorHandler) - login.router = CreateRouter(login, statikFS, instanceHandler, csrfInterceptor, cacheInterceptor, security, userAgentCookie, middleware.TelemetryHandler(EndpointResources), issuerInterceptor) + login.router = CreateRouter(login, statikFS, middleware.TelemetryHandler(IgnoreInstanceEndpoints...), instanceHandler, csrfInterceptor, cacheInterceptor, security, userAgentCookie, issuerInterceptor) login.renderer = CreateRenderer(HandlerPrefix, statikFS, staticStorage, config.LanguageCookieName) login.parser = form.NewParser() return login, nil @@ -109,12 +110,20 @@ func csp() *middleware.CSP { func createCSRFInterceptor(cookieName string, csrfCookieKey []byte, externalSecure bool, errorHandler http.Handler) (func(http.Handler) http.Handler, error) { path := "/" - return csrf.Protect(csrfCookieKey, - csrf.Secure(externalSecure), - csrf.CookieName(http_utils.SetCookiePrefix(cookieName, "", path, externalSecure)), - csrf.Path(path), - csrf.ErrorHandler(errorHandler), - ), nil + return func(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, EndpointResources) { + handler.ServeHTTP(w, r) + return + } + csrf.Protect(csrfCookieKey, + csrf.Secure(externalSecure), + csrf.CookieName(http_utils.SetCookiePrefix(cookieName, "", path, externalSecure)), + csrf.Path(path), + csrf.ErrorHandler(errorHandler), + )(handler).ServeHTTP(w, r) + }) + }, nil } func (l *Login) Handler() http.Handler { diff --git a/internal/api/ui/login/router.go b/internal/api/ui/login/router.go index da24c2acb9..f4c335d5d2 100644 --- a/internal/api/ui/login/router.go +++ b/internal/api/ui/login/router.go @@ -46,6 +46,15 @@ const ( EndpointDynamicResources = "/resources/dynamic" ) +var ( + IgnoreInstanceEndpoints = []string{ + EndpointResources + "/fonts", + EndpointResources + "/images", + EndpointResources + "/scripts", + EndpointResources + "/themes", + } +) + func CreateRouter(login *Login, staticDir http.FileSystem, interceptors ...mux.MiddlewareFunc) *mux.Router { router := mux.NewRouter() router.Use(interceptors...)