diff --git a/doc.go b/doc.go index 46c4ea3f..29158bbe 100644 --- a/doc.go +++ b/doc.go @@ -385,6 +385,11 @@ type RequestScope struct { AccessDenied bool // Identity is the user Identity of the request Identity *userContext + // The parsed (unescaped) value of the request path + Path string + // Preserve the original request path: KEYCLOAK-10864, KEYCLOAK-11276, KEYCLOAK-13315 + // The exact path received in the request, if different than Path + RawPath string } // storage is used to hold the offline refresh token, assuming you don't want to use diff --git a/forwarding.go b/forwarding.go index 61c18801..37f23d5f 100644 --- a/forwarding.go +++ b/forwarding.go @@ -32,8 +32,9 @@ func (r *oauthProxy) proxyMiddleware(next http.Handler) http.Handler { // @step: retrieve the request scope scope := req.Context().Value(contextScopeName) + var sc *RequestScope if scope != nil { - sc := scope.(*RequestScope) + sc = scope.(*RequestScope) if sc.AccessDenied { return } @@ -56,6 +57,12 @@ func (r *oauthProxy) proxyMiddleware(next http.Handler) http.Handler { // @note: by default goproxy only provides a forwarding proxy, thus all requests have to be absolute and we must update the host headers req.URL.Host = r.endpoint.Host req.URL.Scheme = r.endpoint.Scheme + // Restore the unprocessed original path, so that we pass upstream exactly what we received + // as the resource request. + if sc != nil { + req.URL.Path = sc.Path + req.URL.RawPath = sc.RawPath + } if v := req.Header.Get("Host"); v != "" { req.Host = v req.Header.Del("Host") diff --git a/middleware.go b/middleware.go index 522fdd88..44647bf1 100644 --- a/middleware.go +++ b/middleware.go @@ -41,30 +41,35 @@ const ( // entrypointMiddleware is custom filtering for incoming requests func entrypointMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - keep := req.URL.Path + // @step: create a context for the request + scope := &RequestScope{} + // Save the exact formatting of the incoming request so we can use it later + scope.Path = req.URL.Path + scope.RawPath = req.URL.RawPath + + // We want to Normalize the URL so that we can more easily and accurately + // parse it to apply resource protection rules. purell.NormalizeURL(req.URL, normalizeFlags) // ensure we have a slash in the url if !strings.HasPrefix(req.URL.Path, "/") { req.URL.Path = "/" + req.URL.Path } - req.RequestURI = req.URL.RawPath - req.URL.RawPath = req.URL.Path + req.URL.RawPath = req.URL.EscapedPath() - // @step: create a context for the request - scope := &RequestScope{} resp := middleware.NewWrapResponseWriter(w, 1) start := time.Now() + // All the processing, including forwarding the request upstream and getting the response, + // happens here in this chain. next.ServeHTTP(resp, req.WithContext(context.WithValue(req.Context(), contextScopeName, scope))) // @metric record the time taken then response code latencyMetric.Observe(time.Since(start).Seconds()) statusMetric.WithLabelValues(fmt.Sprintf("%d", resp.Status()), req.Method).Inc() - // place back the original uri for proxying request - req.URL.Path = keep - req.URL.RawPath = keep - req.RequestURI = keep + // place back the original uri for any later consumers + req.URL.Path = scope.Path + req.URL.RawPath = scope.RawPath }) } @@ -92,13 +97,24 @@ func (r *oauthProxy) loggingMiddleware(next http.Handler) http.Handler { resp := w.(middleware.WrapResponseWriter) next.ServeHTTP(resp, req) addr := req.RemoteAddr - r.log.Info("client request", - zap.Duration("latency", time.Since(start)), - zap.Int("status", resp.Status()), - zap.Int("bytes", resp.BytesWritten()), - zap.String("client_ip", addr), - zap.String("method", req.Method), - zap.String("path", req.URL.Path)) + if req.URL.Path == req.URL.RawPath || req.URL.RawPath == "" { + r.log.Info("client request", + zap.Duration("latency", time.Since(start)), + zap.Int("status", resp.Status()), + zap.Int("bytes", resp.BytesWritten()), + zap.String("client_ip", addr), + zap.String("method", req.Method), + zap.String("path", req.URL.Path)) + } else { + r.log.Info("client request", + zap.Duration("latency", time.Since(start)), + zap.Int("status", resp.Status()), + zap.Int("bytes", resp.BytesWritten()), + zap.String("client_ip", addr), + zap.String("method", req.Method), + zap.String("path", req.URL.Path), + zap.String("raw path", req.URL.RawPath)) + } }) } diff --git a/middleware_test.go b/middleware_test.go index 9ad50888..2899a607 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -333,11 +333,6 @@ func TestMetricsMiddleware(t *testing.T) { cfg.EnableMetrics = true cfg.LocalhostMetrics = true requests := []fakeRequest{ - { - URI: cfg.WithOAuthURI(metricsURL), - ExpectedCode: http.StatusOK, - ExpectedContentContains: "proxy_request_status_total", - }, { URI: cfg.WithOAuthURI(metricsURL), Headers: map[string]string{ @@ -345,6 +340,12 @@ func TestMetricsMiddleware(t *testing.T) { }, ExpectedCode: http.StatusForbidden, }, + // Some request must run before this one to generate request status numbers + { + URI: cfg.WithOAuthURI(metricsURL), + ExpectedCode: http.StatusOK, + ExpectedContentContains: "proxy_request_status_total", + }, } newFakeProxy(cfg).RunTests(t, requests) } @@ -433,6 +434,100 @@ func TestMethodExclusions(t *testing.T) { newFakeProxy(cfg).RunTests(t, requests) } +func TestPreserveURLEncoding(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.EnableLogging = true + cfg.Resources = []*Resource{ + { + URL: "/api/v2/*", + Methods: allHTTPMethods, + Roles: []string{"dev"}, + }, + { + URL: "/api/v1/auth*", + Methods: allHTTPMethods, + Roles: []string{"admin"}, + }, + { + URL: "/api/v1/*", + Methods: allHTTPMethods, + WhiteListed: true, + }, + { + URL: "/*", + Methods: allHTTPMethods, + Roles: []string{"user"}, + }, + } + requests := []fakeRequest{ + { + URI: "/test", + HasToken: true, + Roles: []string{"nothing"}, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/", + ExpectedCode: http.StatusUnauthorized, + }, + { // See KEYCLOAK-10864 + URI: "/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https%3A%2F%2Flocalhost%3A6001%2Fmanage/", + ExpectedContentContains: `"uri":"/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https%3A%2F%2Flocalhost%3A6001%2Fmanage/"`, + HasToken: true, + Roles: []string{"user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { // See KEYCLOAK-11276 + URI: "/iiif/2/edepot_local:ST%2F00001%2FST00005_00001.jpg/full/1000,/0/default.png", + ExpectedContentContains: `"uri":"/iiif/2/edepot_local:ST%2F00001%2FST00005_00001.jpg/full/1000,/0/default.png"`, + HasToken: true, + Roles: []string{"user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { // See KEYCLOAK-13315 + URI: "/rabbitmqui/%2F/replicate-to-central", + ExpectedContentContains: `"uri":"/rabbitmqui/%2F/replicate-to-central"`, + HasToken: true, + Roles: []string{"user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { // should work + URI: "/api/v1/auth", + HasToken: true, + Roles: []string{"admin"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { // should work + URI: "/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth", + ExpectedContentContains: `"uri":"/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth"`, + HasToken: true, + Roles: []string{"admin"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/api/v1/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth", + HasToken: true, + Roles: []string{"user"}, + ExpectedCode: http.StatusForbidden, + }, + { // should work + URI: "/api/v3/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth", + ExpectedContentContains: `"uri":"/api/v3/auth?referer=https%3A%2F%2Fwww.example.com%2Fauth"`, + HasToken: true, + Roles: []string{"user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + } + + newFakeProxy(cfg).RunTests(t, requests) +} + func TestStrangeRoutingError(t *testing.T) { cfg := newFakeKeycloakConfig() cfg.Resources = []*Resource{ @@ -459,12 +554,13 @@ func TestStrangeRoutingError(t *testing.T) { } requests := []fakeRequest{ { // should work - URI: "/api/v1/events/123456789", - HasToken: true, - Redirects: true, - Roles: []string{"user"}, - ExpectedProxy: true, - ExpectedCode: http.StatusOK, + URI: "/api/v1/events/123456789", + HasToken: true, + Redirects: true, + Roles: []string{"user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + ExpectedContentContains: `"uri":"/api/v1/events/123456789"`, }, { // should break with bad role URI: "/api/v1/events/123456789", diff --git a/server.go b/server.go index ebda91a5..2ce85f01 100644 --- a/server.go +++ b/server.go @@ -32,6 +32,8 @@ import ( "strings" "time" + "go.uber.org/zap/zapcore" + "golang.org/x/crypto/acme/autocert" httplog "log" @@ -137,6 +139,8 @@ func createLogger(config *Config) (*zap.Logger, error) { c := zap.NewProductionConfig() c.DisableStacktrace = true c.DisableCaller = true + // Use human-readable timestamps in the logs until KEYCLOAK-12100 is fixed + c.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder // are we enabling json logging? if !config.EnableJSONLogging { c.Encoding = "console" diff --git a/server_test.go b/server_test.go index ff655c01..5a32ffc0 100644 --- a/server_test.go +++ b/server_test.go @@ -86,9 +86,10 @@ func TestReverseProxyHeaders(t *testing.T) { token := newTestToken(p.idp.getLocation()) token.addRealmRoles([]string{fakeAdminRole}) signed, _ := p.idp.signToken(token.claims) + uri := "/auth_all/test" requests := []fakeRequest{ { - URI: "/auth_all/test", + URI: uri, RawToken: signed.Encode(), ExpectedProxy: true, ExpectedProxyHeaders: map[string]string{ @@ -99,7 +100,8 @@ func TestReverseProxyHeaders(t *testing.T) { "X-Auth-Userid": "rjayawardene", "X-Auth-Username": "rjayawardene", }, - ExpectedCode: http.StatusOK, + ExpectedCode: http.StatusOK, + ExpectedContentContains: `"uri":"` + uri + `"`, }, } p.RunTests(t, requests) @@ -553,7 +555,10 @@ func (f *fakeUpstreamService) ServeHTTP(w http.ResponseWriter, r *http.Request) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) content, _ := json.Marshal(&fakeUpstreamResponse{ - URI: r.RequestURI, + // r.RequestURI is what was received by the proxy. + // r.URL.String() is what is actually sent to the upstream service. + // KEYCLOAK-10864, KEYCLOAK-11276, KEYCLOAK-13315 + URI: r.URL.String(), Method: r.Method, Address: r.RemoteAddr, Headers: r.Header,