From 69ba80562ecc73319cf42d9337aea99f9c2c473c Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 25 Aug 2022 23:29:47 +0200 Subject: [PATCH] add unprotected flag to the proxy routes I added an unprotected flag to the proxy routes which is evaluated by the authentication middleware. This way we won't have to maintain a hardcoded list of unprotected paths and path prefixes and we will hopefully reduce the times we encounter the basic auth prompt by web browsers. --- services/proxy/pkg/config/config.go | 1 + .../pkg/config/defaults/defaultconfig.go | 35 +++-- .../proxy/pkg/middleware/authentication.go | 59 +------ services/proxy/pkg/proxy/proxy.go | 4 +- services/proxy/pkg/router/router.go | 147 ++++++++++-------- 5 files changed, 110 insertions(+), 136 deletions(-) diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index ce98f8ede2..886351589f 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -54,6 +54,7 @@ type Route struct { // Service name to look up in the registry Service string `yaml:"service,omitempty"` ApacheVHost bool `yaml:"apache_vhost,omitempty"` + Unprotected bool `yaml:"unprotected,omitempty"` } // RouteType defines the type of a route diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 1c6f848934..4a89625d54 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -71,20 +71,24 @@ func DefaultPolicies() []config.Policy { Name: "ocis", Routes: []config.Route{ { - Endpoint: "/", - Backend: "http://localhost:9100", + Endpoint: "/", + Backend: "http://localhost:9100", + Unprotected: true, }, { - Endpoint: "/.well-known/", - Backend: "http://localhost:9130", + Endpoint: "/.well-known/", + Backend: "http://localhost:9130", + Unprotected: true, }, { - Endpoint: "/konnect/", - Backend: "http://localhost:9130", + Endpoint: "/konnect/", + Backend: "http://localhost:9130", + Unprotected: true, }, { - Endpoint: "/signin/", - Backend: "http://localhost:9130", + Endpoint: "/signin/", + Backend: "http://localhost:9130", + Unprotected: true, }, { Endpoint: "/archiver", @@ -161,24 +165,27 @@ func DefaultPolicies() []config.Policy { Backend: "http://localhost:9140", }, { - Endpoint: "/app/", // /app or /apps? ocdav only handles /apps - Backend: "http://localhost:9140", + Endpoint: "/app/", // /app or /apps? ocdav only handles /apps + Backend: "http://localhost:9140", + Unprotected: true, // TODO check if this is safe }, { Endpoint: "/graph/", Backend: "http://localhost:9120", }, { - Endpoint: "/graph-explorer", - Backend: "http://localhost:9135", + Endpoint: "/graph-explorer", + Backend: "http://localhost:9135", + Unprotected: true, }, { Endpoint: "/api/v0/settings", Backend: "http://localhost:9190", }, { - Endpoint: "/settings.js", - Backend: "http://localhost:9190", + Endpoint: "/settings.js", + Backend: "http://localhost:9190", + Unprotected: true, }, }, }, diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 40945e324c..3e18e3ab44 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -6,6 +6,7 @@ import ( "regexp" "strings" + "github.com/owncloud/ocis/v2/services/proxy/pkg/router" "github.com/owncloud/ocis/v2/services/proxy/pkg/webdav" "golang.org/x/text/cases" "golang.org/x/text/language" @@ -26,49 +27,6 @@ var ( "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", "/ocs/v1.php/cloud/capabilities", } - // _unprotectedPaths contains paths which don't need to be authenticated. - _unprotectedPaths = map[string]struct{}{ - "/": {}, - "/login": {}, - "/settings": {}, - "/app/list": {}, - "/config.json": {}, - "/manifest.json": {}, - "/index.html": {}, - "/oidc-callback.html": {}, - "/oidc-callback": {}, - "/settings.js": {}, - "/data": {}, - "/konnect/v1/userinfo": {}, - "/status.php": {}, - "/favicon.ico": {}, - "/ocs/v1.php/config": {}, - "/ocs/v2.php/config": {}, - } - // _unprotectedPathPrefixes contains paths which don't need to be authenticated. - _unprotectedPathPrefixes = [...]string{ - "/files/", - "/data/", - "/account/", - "/s/", - "/external", - "/apps/openidconnect/redirect", - "/settings/", - "/user-management/", - "/.well-known/", - "/js/", - "/css/", - "/fonts/", - "/icons/", - "/themes/", - "/signin/", - "/konnect/", - "/text-editor/", - "/preview/", - "/pdf-viewer/", - "/draw-io/", - "/index.html#/", - } ) const ( @@ -91,7 +49,8 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if isOIDCTokenAuth(r) || isUnprotectedPath(r) { + ri := router.ContextRoutingInfo(r.Context()) + if isOIDCTokenAuth(r) || ri.IsRouteUnprotected() { // The authentication for this request is handled by the IdP. next.ServeHTTP(w, r) return @@ -153,18 +112,6 @@ func isOIDCTokenAuth(req *http.Request) bool { return req.URL.Path == "/konnect/v1/token" } -func isUnprotectedPath(r *http.Request) bool { - if _, ok := _unprotectedPaths[r.URL.Path]; ok { - return true - } - for _, p := range _unprotectedPathPrefixes { - if strings.HasPrefix(r.URL.Path, p) { - return true - } - } - return false -} - func isPublicPath(p string) bool { for _, pp := range _publicPaths { if strings.HasPrefix(p, pp) { diff --git a/services/proxy/pkg/proxy/proxy.go b/services/proxy/pkg/proxy/proxy.go index d1791b10ad..12b49cdb66 100644 --- a/services/proxy/pkg/proxy/proxy.go +++ b/services/proxy/pkg/proxy/proxy.go @@ -43,8 +43,8 @@ func NewMultiHostReverseProxy(opts ...Option) *MultiHostReverseProxy { } rp.Director = func(r *http.Request) { - fn := router.DirectorFunc(r.Context()) - fn(r) + ri := router.ContextRoutingInfo(r.Context()) + ri.Director()(r) } // equals http.DefaultTransport except TLSClientConfig diff --git a/services/proxy/pkg/router/router.go b/services/proxy/pkg/router/router.go index 9983d1dcd8..7a483b9a42 100644 --- a/services/proxy/pkg/router/router.go +++ b/services/proxy/pkg/router/router.go @@ -14,23 +14,29 @@ import ( "go-micro.dev/v4/selector" ) -const directorCtxKey string = "director" +const routingInfoCtxKey string = "director" + +var noInfo = RoutingInfo{} func Middleware(policySelector *config.PolicySelector, policies []config.Policy, logger log.Logger) func(http.Handler) http.Handler { router := New(policySelector, policies, logger) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fn := router.Route(r) - next.ServeHTTP(w, r.WithContext(SetDirectorFunc(r.Context(), fn))) + ri, ok := router.Route(r) + if !ok { + w.WriteHeader(http.StatusInternalServerError) + return + } + next.ServeHTTP(w, r.WithContext(SetRoutingInfo(r.Context(), ri))) }) } } -func (rt Router) Route(r *http.Request) func(*http.Request) { +func (rt Router) Route(r *http.Request) (RoutingInfo, bool) { pol, err := rt.policySelector(r) if err != nil { rt.logger.Error().Err(err).Msg("Error while selecting pol") - return nil + return noInfo, false } if _, ok := rt.directors[pol]; !ok { @@ -38,7 +44,7 @@ func (rt Router) Route(r *http.Request) func(*http.Request) { Error(). Str("policy", pol). Msg("policy is not configured") - return nil + return noInfo, false } method := "" @@ -70,19 +76,16 @@ func (rt Router) Route(r *http.Request) func(*http.Request) { Str("routeType", string(rtype)). Msg("director found") - return rt.directors[pol][rtype][method][endpoint] + return rt.directors[pol][rtype][method][endpoint], true } } } // override default director with root. If any - switch { - case rt.directors[pol][config.PrefixRoute][method]["/"] != nil: - // try specific method - return rt.directors[pol][config.PrefixRoute][method]["/"] - case rt.directors[pol][config.PrefixRoute][""]["/"] != nil: - // fallback to unspecific method - return rt.directors[pol][config.PrefixRoute][""]["/"] + if ri, ok := rt.directors[pol][config.PrefixRoute][method]["/"]; ok { // try specific method + return ri, true + } else if ri, ok := rt.directors[pol][config.PrefixRoute][""]["/"]; ok { // fallback to unspecific method + return ri, true } rt.logger. @@ -90,7 +93,7 @@ func (rt Router) Route(r *http.Request) func(*http.Request) { Str("policy", pol). Str("path", r.URL.Path). Msg("no director found") - return nil + return noInfo, false } func New(policySelector *config.PolicySelector, policies []config.Policy, logger log.Logger) Router { @@ -114,7 +117,7 @@ func New(policySelector *config.PolicySelector, policies []config.Policy, logger } r := Router{ - directors: make(map[string]map[config.RouteType]map[string]map[string]func(req *http.Request)), + directors: make(map[string]map[config.RouteType]map[string]map[string]RoutingInfo), policySelector: selector, } for _, pol := range policies { @@ -140,72 +143,88 @@ func New(policySelector *config.PolicySelector, policies []config.Policy, logger return r } +type RoutingInfo struct { + director func(*http.Request) + unprotected bool +} + +func (r RoutingInfo) Director() func(*http.Request) { + return r.director +} + +func (r RoutingInfo) IsRouteUnprotected() bool { + return r.unprotected +} + type Router struct { logger log.Logger - directors map[string]map[config.RouteType]map[string]map[string]func(req *http.Request) + directors map[string]map[config.RouteType]map[string]map[string]RoutingInfo policySelector policy.Selector } func (rt Router) addHost(policy string, target *url.URL, route config.Route) { targetQuery := target.RawQuery if rt.directors[policy] == nil { - rt.directors[policy] = make(map[config.RouteType]map[string]map[string]func(req *http.Request)) + rt.directors[policy] = make(map[config.RouteType]map[string]map[string]RoutingInfo) } routeType := config.DefaultRouteType if route.Type != "" { routeType = route.Type } if rt.directors[policy][routeType] == nil { - rt.directors[policy][routeType] = make(map[string]map[string]func(req *http.Request)) + rt.directors[policy][routeType] = make(map[string]map[string]RoutingInfo) } if rt.directors[policy][routeType][route.Method] == nil { - rt.directors[policy][routeType][route.Method] = make(map[string]func(req *http.Request)) + rt.directors[policy][routeType][route.Method] = make(map[string]RoutingInfo) } reg := registry.GetRegistry() sel := selector.NewSelector(selector.Registry(reg)) - rt.directors[policy][routeType][route.Method][route.Endpoint] = func(req *http.Request) { - if route.Service != "" { - // select next node - next, err := sel.Select(route.Service) - if err != nil { - rt.logger.Error().Err(err). - Str("service", route.Service). - Msg("could not select service from the registry") - return // TODO error? fallback to target.Host & Scheme? + rt.directors[policy][routeType][route.Method][route.Endpoint] = RoutingInfo{ + unprotected: route.Unprotected, + director: func(req *http.Request) { + if route.Service != "" { + // select next node + next, err := sel.Select(route.Service) + if err != nil { + rt.logger.Error().Err(err). + Str("service", route.Service). + Msg("could not select service from the registry") + return // TODO error? fallback to target.Host & Scheme? + } + node, err := next() + if err != nil { + rt.logger.Error().Err(err). + Str("service", route.Service). + Msg("could not select next node") + return // TODO error? fallback to target.Host & Scheme? + } + req.URL.Host = node.Address + req.URL.Scheme = node.Metadata["protocol"] // TODO check property exists? + + } else { + req.URL.Host = target.Host + req.URL.Scheme = target.Scheme } - node, err := next() - if err != nil { - rt.logger.Error().Err(err). - Str("service", route.Service). - Msg("could not select next node") - return // TODO error? fallback to target.Host & Scheme? + + // Apache deployments host addresses need to match on req.Host and req.URL.Host + // see https://stackoverflow.com/questions/34745654/golang-reverseproxy-with-apache2-sni-hostname-error + if route.ApacheVHost { + req.Host = target.Host } - req.URL.Host = node.Address - req.URL.Scheme = node.Metadata["protocol"] // TODO check property exists? - } else { - req.URL.Host = target.Host - req.URL.Scheme = target.Scheme - } - - // Apache deployments host addresses need to match on req.Host and req.URL.Host - // see https://stackoverflow.com/questions/34745654/golang-reverseproxy-with-apache2-sni-hostname-error - if route.ApacheVHost { - req.Host = target.Host - } - - req.URL.Path = singleJoiningSlash(target.Path, req.URL.Path) - if targetQuery == "" || req.URL.RawQuery == "" { - req.URL.RawQuery = targetQuery + req.URL.RawQuery - } else { - req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery - } - if _, ok := req.Header["User-Agent"]; !ok { - // explicitly disable User-Agent so it's not set to default value - req.Header.Set("User-Agent", "") - } + req.URL.Path = singleJoiningSlash(target.Path, req.URL.Path) + if targetQuery == "" || req.URL.RawQuery == "" { + req.URL.RawQuery = targetQuery + req.URL.RawQuery + } else { + req.URL.RawQuery = targetQuery + "&" + req.URL.RawQuery + } + if _, ok := req.Header["User-Agent"]; !ok { + // explicitly disable User-Agent so it's not set to default value + req.Header.Set("User-Agent", "") + } + }, } } func singleJoiningSlash(a, b string) string { @@ -250,12 +269,12 @@ func prefixRouteMatcher(prefix string, target url.URL) bool { return strings.HasPrefix(target.Path, prefix) && prefix != "/" } -func SetDirectorFunc(parent context.Context, fn func(*http.Request)) context.Context { - return context.WithValue(parent, directorCtxKey, fn) +func SetRoutingInfo(parent context.Context, ri RoutingInfo) context.Context { + return context.WithValue(parent, routingInfoCtxKey, ri) } -// DirectorFunc gets the director function from the context. -func DirectorFunc(ctx context.Context) func(*http.Request) { - val := ctx.Value(directorCtxKey) - return val.(func(*http.Request)) +// ContextRoutingInfo gets the routing information from the context. +func ContextRoutingInfo(ctx context.Context) RoutingInfo { + val := ctx.Value(routingInfoCtxKey) + return val.(RoutingInfo) }