From 6346dafc1e9c091cc79f2b6a8432f61c14eb767a Mon Sep 17 00:00:00 2001 From: Jordan Crawford Date: Mon, 6 Jul 2020 13:39:06 +1200 Subject: [PATCH] (#649) Remove blank helthcheck user agents and paths when setting up the healthcheck middleware A blank user agent is considered == to an empty string. When no -ping-user-agent option is specified, this is considered to be an empty string. This reveals two problems: - When no ping-user-agent is specified, main.go sets up a health check user agent of "" - When no user agent is specified, the empty string is still checked against the health check user agents. Now the health check middleware ignores blank user agents and paths in order to sanitise it's input to avoid this issue. Additional tests have been added to verify these situations. --- CHANGELOG.md | 1 + pkg/middleware/healthcheck.go | 8 ++++-- pkg/middleware/healthcheck_test.go | 44 ++++++++++++++++++++++++++---- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17b81383..1e2040a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed) - [#577](https://github.com/oauth2-proxy/oauth2-proxy/pull/577) Move Cipher and Session Store initialisation out of Validation (@JoelSpeed) - [#635](https://github.com/oauth2-proxy/oauth2-proxy/pull/635) Support specifying alternative provider TLS trust source(s) (@k-wall) +- [#649](https://github.com/oauth2-proxy/oauth2-proxy/pull/650) Resolve an issue where an empty healthcheck URL and ping-user-agent returns the healthcheck response (@jordancrawfordnz) # v6.0.0 diff --git a/pkg/middleware/healthcheck.go b/pkg/middleware/healthcheck.go index ea1b533a..2dcfc1d4 100644 --- a/pkg/middleware/healthcheck.go +++ b/pkg/middleware/healthcheck.go @@ -17,13 +17,17 @@ func healthCheck(paths, userAgents []string, next http.Handler) http.Handler { // Use a map as a set to check health check paths pathSet := make(map[string]struct{}) for _, path := range paths { - pathSet[path] = struct{}{} + if len(path) > 0 { + pathSet[path] = struct{}{} + } } // Use a map as a set to check health check paths userAgentSet := make(map[string]struct{}) for _, userAgent := range userAgents { - userAgentSet[userAgent] = struct{}{} + if len(userAgent) > 0 { + userAgentSet[userAgent] = struct{}{} + } } return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { diff --git a/pkg/middleware/healthcheck_test.go b/pkg/middleware/healthcheck_test.go index 8db4d57b..1a6a788d 100644 --- a/pkg/middleware/healthcheck_test.go +++ b/pkg/middleware/healthcheck_test.go @@ -34,6 +34,14 @@ var _ = Describe("HealthCheck suite", func() { Expect(rw.Code).To(Equal(in.expectedStatus)) Expect(rw.Body.String()).To(Equal(in.expectedBody)) }, + Entry("when no health check paths are configured", &requestTableInput{ + healthCheckPaths: []string{}, + healthCheckUserAgents: []string{"hc/1.0"}, + requestString: "http://example.com/ping", + headers: map[string]string{}, + expectedStatus: 404, + expectedBody: "404 page not found\n", + }), Entry("when requesting the healthcheck path", &requestTableInput{ healthCheckPaths: []string{"/ping"}, healthCheckUserAgents: []string{"hc/1.0"}, @@ -50,15 +58,23 @@ var _ = Describe("HealthCheck suite", func() { expectedStatus: 404, expectedBody: "404 page not found\n", }), - Entry("with a request from the health check user agent", &requestTableInput{ - healthCheckPaths: []string{"/ping"}, + Entry("when a blank string is configured as a health check path and the request has no specific path", &requestTableInput{ + healthCheckPaths: []string{""}, healthCheckUserAgents: []string{"hc/1.0"}, + requestString: "http://example.com", + headers: map[string]string{}, + expectedStatus: 404, + expectedBody: "404 page not found\n", + }), + Entry("with no health check user agents configured", &requestTableInput{ + healthCheckPaths: []string{"/ping"}, + healthCheckUserAgents: []string{}, requestString: "http://example.com/abc", headers: map[string]string{ - "User-Agent": "hc/1.0", + "User-Agent": "user", }, - expectedStatus: 200, - expectedBody: "OK", + expectedStatus: 404, + expectedBody: "404 page not found\n", }), Entry("with a request from a different user agent", &requestTableInput{ healthCheckPaths: []string{"/ping"}, @@ -70,6 +86,24 @@ var _ = Describe("HealthCheck suite", func() { expectedStatus: 404, expectedBody: "404 page not found\n", }), + Entry("with a request from the health check user agent", &requestTableInput{ + healthCheckPaths: []string{"/ping"}, + healthCheckUserAgents: []string{"hc/1.0"}, + requestString: "http://example.com/abc", + headers: map[string]string{ + "User-Agent": "hc/1.0", + }, + expectedStatus: 200, + expectedBody: "OK", + }), + Entry("when a blank string is configured as a health check agent and a request has no user agent", &requestTableInput{ + healthCheckPaths: []string{"/ping"}, + healthCheckUserAgents: []string{""}, + requestString: "http://example.com/abc", + headers: map[string]string{}, + expectedStatus: 404, + expectedBody: "404 page not found\n", + }), Entry("with multiple paths, request one of the healthcheck paths", &requestTableInput{ healthCheckPaths: []string{"/ping", "/liveness_check", "/readiness_check"}, healthCheckUserAgents: []string{"hc/1.0"},