From 3476daf32226f890fdc9a1dae2ace8d8bed401e3 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Wed, 20 Mar 2019 14:29:44 -0700 Subject: [PATCH 01/10] added an option to enable GCP healthcheck endpoints --- http.go | 17 +++++++++++++++++ main.go | 10 +++++++++- options.go | 9 +++++---- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/http.go b/http.go index 4456e39..3f9ba57 100644 --- a/http.go +++ b/http.go @@ -24,6 +24,23 @@ func (s *Server) ListenAndServe() { } } +// gcpHealthcheck handles healthcheck queries from GCP +func gcpHealthcheck(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.EscapedPath() == "/liveness_check" { + w.WriteHeader(http.StatusOK) + w.Write([]byte("OK")) + return + } + if r.URL.EscapedPath() == "/readiness_check" { + w.WriteHeader(http.StatusOK) + w.Write([]byte("OK")) + return + } + h.ServeHTTP(w, r) + }) +} + // ServeHTTP constructs a net.Listener and starts handling HTTP requests func (s *Server) ServeHTTP() { HTTPAddress := s.Opts.HTTPAddress diff --git a/main.go b/main.go index 5625259..efe336c 100644 --- a/main.go +++ b/main.go @@ -5,6 +5,7 @@ import ( "fmt" "log" "math/rand" + "net/http" "os" "runtime" "strings" @@ -92,6 +93,7 @@ func main() { flagSet.String("acr-values", "http://idmanagement.gov/ns/assurance/loa/1", "acr values string: optional, used by login.gov") flagSet.String("jwt-key", "", "private key used to sign JWT: required by login.gov") flagSet.String("pubjwk-url", "", "JWK pubkey access endpoint: required by login.gov") + flagSet.Bool("gcp-healthchecks", false, "Enable GCP healthcheck endpoints") flagSet.Parse(os.Args[1:]) @@ -139,8 +141,14 @@ func main() { rand.Seed(time.Now().UnixNano()) + var myhandler http.Handler + if opts.GCPHealthChecks { + myhandler = gcpHealthcheck(LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat)) + } else { + myhandler = LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat) + } s := &Server{ - Handler: LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat), + Handler: myhandler, Opts: opts, } s.ListenAndServe() diff --git a/options.go b/options.go index 90af3d3..b736521 100644 --- a/options.go +++ b/options.go @@ -86,10 +86,11 @@ type Options struct { RequestLogging bool `flag:"request-logging" cfg:"request_logging" env:"OAUTH2_PROXY_REQUEST_LOGGING"` RequestLoggingFormat string `flag:"request-logging-format" cfg:"request_logging_format" env:"OAUTH2_PROXY_REQUEST_LOGGING_FORMAT"` - SignatureKey string `flag:"signature-key" cfg:"signature_key" env:"OAUTH2_PROXY_SIGNATURE_KEY"` - AcrValues string `flag:"acr-values" cfg:"acr_values" env:"OAUTH2_PROXY_ACR_VALUES"` - JWTKey string `flag:"jwt-key" cfg:"jwt_key" env:"OAUTH2_PROXY_JWT_KEY"` - PubJWKURL string `flag:"pubjwk-url" cfg:"pubjwk_url" env:"OAUTH2_PROXY_PUBJWK_URL"` + SignatureKey string `flag:"signature-key" cfg:"signature_key" env:"OAUTH2_PROXY_SIGNATURE_KEY"` + AcrValues string `flag:"acr-values" cfg:"acr_values" env:"OAUTH2_PROXY_ACR_VALUES"` + JWTKey string `flag:"jwt-key" cfg:"jwt_key" env:"OAUTH2_PROXY_JWT_KEY"` + PubJWKURL string `flag:"pubjwk-url" cfg:"pubjwk_url" env:"OAUTH2_PROXY_PUBJWK_URL"` + GCPHealthChecks bool `flag:"gcp-healthchecks" cfg:"gcp_healthchecks" env:"OAUTH2_PROXY_GCP_HEALTHCHECKS"` // internal values that are set after config validation redirectURL *url.URL From 2147ae8cfd9db736ceb4a2bcca7163512c3ddcac Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Wed, 20 Mar 2019 14:38:06 -0700 Subject: [PATCH 02/10] added gcp-healthchecks flag in readme, fixed link to logingov-provider --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1bb63be..6d28de1 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ Valid providers are : - [GitHub](#github-auth-provider) - [GitLab](#gitlab-auth-provider) - [LinkedIn](#linkedin-auth-provider) -- [login.gov](#login.gov-provider) +- [login.gov](#logingov-provider) The provider can be selected using the `provider` configuration value. @@ -272,6 +272,7 @@ Usage of oauth2_proxy: -email-domain value: authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email -flush-interval: period between flushing response buffers when streaming responses (default "1s") -footer string: custom footer string. Use "-" to disable default footer. + -gcp-healthchecks: will enable /liveness_check and /readiness_check endpoints that will make it work well with GCP App Engine (default false) -github-org string: restrict logins to members of this organisation -github-team string: restrict logins to members of any of these teams (slug), separated by a comma -google-admin-email string: the google admin to impersonate for api calls From e9f36fa4b58934a03474d4a6ea530973ac4dc770 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Wed, 20 Mar 2019 14:44:01 -0700 Subject: [PATCH 03/10] added the PR to the changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9823907..b9d0385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Changes since v3.1.0 +- [#110](https://github.com/pusher/oauth2_proxy/pull/110) Added GCP healthcheck option (@timothy-spencer) - [#63](https://github.com/pusher/oauth2_proxy/pull/63) Use encoding/json for SessionState serialization (@yaegashi) - Use JSON to encode session state to be stored in browser cookies - Implement legacy decode function to support existing cookies generated by older versions From 3d22a116589ce2bc8561b477eaaf2ba9441692e5 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 09:56:56 -0700 Subject: [PATCH 04/10] added better tests for gcp healthcheck stuff --- http_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ options_test.go | 6 +++++ 2 files changed, 77 insertions(+) create mode 100644 http_test.go diff --git a/http_test.go b/http_test.go new file mode 100644 index 0000000..49e9791 --- /dev/null +++ b/http_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGCPHealthcheckLiveness(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/liveness_check", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + h.ServeHTTP(rw, r) + + assert.Equal(t, 200, rw.Code) + assert.Equal(t, "OK", rw.Body.String()) +} + +func TestGCPHealthcheckReadiness(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/readiness_check", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + h.ServeHTTP(rw, r) + + assert.Equal(t, 200, rw.Code) + assert.Equal(t, "OK", rw.Body.String()) +} + +func TestGCPHealthcheckNotReadiness(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/NOT_readiness_check", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + h.ServeHTTP(rw, r) + + assert.NotEqual(t, "OK", rw.Body.String()) +} + +func TestGCPHealthcheckNotLiveness(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/NOT_liveness_check", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + h.ServeHTTP(rw, r) + + assert.NotEqual(t, "OK", rw.Body.String()) +} diff --git a/options_test.go b/options_test.go index 7b12a2b..171ff36 100644 --- a/options_test.go +++ b/options_test.go @@ -268,3 +268,9 @@ func TestSkipOIDCDiscovery(t *testing.T) { assert.Equal(t, nil, o.Validate()) } + +func TestGCPHealthcheck(t *testing.T) { + o := testOptions() + o.GCPHealthChecks = true + assert.Equal(t, nil, o.Validate()) +} From e2755624eccc039c2be7287059df4fc2b7348e8b Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 10:03:22 -0700 Subject: [PATCH 05/10] made gcp healthcheck test better --- http_test.go | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/http_test.go b/http_test.go index 49e9791..9b663de 100644 --- a/http_test.go +++ b/http_test.go @@ -40,32 +40,17 @@ func TestGCPHealthcheckReadiness(t *testing.T) { assert.Equal(t, "OK", rw.Body.String()) } -func TestGCPHealthcheckNotReadiness(t *testing.T) { +func TestGCPHealthcheckNotHealthcheck(t *testing.T) { handler := func(w http.ResponseWriter, req *http.Request) { w.Write([]byte("test")) } h := gcpHealthcheck(http.HandlerFunc(handler)) rw := httptest.NewRecorder() - r, _ := http.NewRequest("GET", "/NOT_readiness_check", nil) + r, _ := http.NewRequest("GET", "/NOT_any_check", nil) r.RemoteAddr = "127.0.0.1" r.Host = "test-server" h.ServeHTTP(rw, r) - assert.NotEqual(t, "OK", rw.Body.String()) -} - -func TestGCPHealthcheckNotLiveness(t *testing.T) { - handler := func(w http.ResponseWriter, req *http.Request) { - w.Write([]byte("test")) - } - - h := gcpHealthcheck(http.HandlerFunc(handler)) - rw := httptest.NewRecorder() - r, _ := http.NewRequest("GET", "/NOT_liveness_check", nil) - r.RemoteAddr = "127.0.0.1" - r.Host = "test-server" - h.ServeHTTP(rw, r) - - assert.NotEqual(t, "OK", rw.Body.String()) + assert.Equal(t, "test", rw.Body.String()) } From 1ff17a3fa1643aa7c90a256f83f93e77556fe4df Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 10:10:07 -0700 Subject: [PATCH 06/10] travis ci tests had a temporary failure, so this is to get it to retest --- http_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_test.go b/http_test.go index 9b663de..8495fa8 100644 --- a/http_test.go +++ b/http_test.go @@ -47,7 +47,7 @@ func TestGCPHealthcheckNotHealthcheck(t *testing.T) { h := gcpHealthcheck(http.HandlerFunc(handler)) rw := httptest.NewRecorder() - r, _ := http.NewRequest("GET", "/NOT_any_check", nil) + r, _ := http.NewRequest("GET", "/not_any_check", nil) r.RemoteAddr = "127.0.0.1" r.Host = "test-server" h.ServeHTTP(rw, r) From ff4e5588d87cf92b2df01b0ab16c7ebd8ec02d43 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 10:32:29 -0700 Subject: [PATCH 07/10] incorporate suggestions from @benfdking --- http.go | 24 +++++++++++++++++++++++- http_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/http.go b/http.go index 3f9ba57..b45b3c1 100644 --- a/http.go +++ b/http.go @@ -24,9 +24,15 @@ func (s *Server) ListenAndServe() { } } -// gcpHealthcheck handles healthcheck queries from GCP +// Used with gcpHealthcheck() +const userAgentHeader = "User-Agent" +const googleHealthCheckUserAgent = "GoogleHC/1.0" +const rootPath = "/" + +// gcpHealthcheck handles healthcheck queries from GCP. func gcpHealthcheck(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check for liveness and readiness: used for Google App Engine if r.URL.EscapedPath() == "/liveness_check" { w.WriteHeader(http.StatusOK) w.Write([]byte("OK")) @@ -37,6 +43,22 @@ func gcpHealthcheck(h http.Handler) http.Handler { w.Write([]byte("OK")) return } + + // Check for GKE ingress healthcheck: The ingress requires the root + // path of the target to return a 200 (OK) to indicate the service's good health. This can be quite a challenging demand + // depending on the application's path structure. This middleware filters out the requests from the health check by + // + // 1. checking that the request path is indeed the root path + // 2. ensuring that the User-Agent is "GoogleHC/1.0", the health checker + // 3. ensuring the request method is "GET" + if r.URL.Path == rootPath && + r.Header.Get(userAgentHeader) == googleHealthCheckUserAgent && + r.Method == http.MethodGet { + + w.WriteHeader(http.StatusOK) + return + } + h.ServeHTTP(w, r) }) } diff --git a/http_test.go b/http_test.go index 8495fa8..2bc3d5d 100644 --- a/http_test.go +++ b/http_test.go @@ -54,3 +54,36 @@ func TestGCPHealthcheckNotHealthcheck(t *testing.T) { assert.Equal(t, "test", rw.Body.String()) } + +func TestGCPHealthcheckIngress(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + r.Header.Set(userAgentHeader, googleHealthCheckUserAgent) + h.ServeHTTP(rw, r) + + assert.Equal(t, 200, rw.Code) + assert.Equal(t, "", rw.Body.String()) +} + +func TestGCPHealthcheckNotIngress(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/foo", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + r.Header.Set(userAgentHeader, googleHealthCheckUserAgent) + h.ServeHTTP(rw, r) + + assert.Equal(t, "test", rw.Body.String()) +} From d44f58f0e24b886d9c26c7baab6340c0b4e0867d Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 10:47:30 -0700 Subject: [PATCH 08/10] found another edge case to test --- http_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/http_test.go b/http_test.go index 2bc3d5d..b365d6f 100644 --- a/http_test.go +++ b/http_test.go @@ -87,3 +87,19 @@ func TestGCPHealthcheckNotIngress(t *testing.T) { assert.Equal(t, "test", rw.Body.String()) } + +func TestGCPHealthcheckNotIngressPut(t *testing.T) { + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := gcpHealthcheck(http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("PUT", "/", nil) + r.RemoteAddr = "127.0.0.1" + r.Host = "test-server" + r.Header.Set(userAgentHeader, googleHealthCheckUserAgent) + h.ServeHTTP(rw, r) + + assert.Equal(t, "test", rw.Body.String()) +} From 2679579f44085ce448d06b7e3a2e1ced6b3607a7 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Mon, 25 Mar 2019 11:44:17 -0700 Subject: [PATCH 09/10] updated documentation to reflect GKE ingress support too --- README.md | 2 +- main.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6d28de1..c6bcbac 100644 --- a/README.md +++ b/README.md @@ -272,7 +272,7 @@ Usage of oauth2_proxy: -email-domain value: authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email -flush-interval: period between flushing response buffers when streaming responses (default "1s") -footer string: custom footer string. Use "-" to disable default footer. - -gcp-healthchecks: will enable /liveness_check and /readiness_check endpoints that will make it work well with GCP App Engine (default false) + -gcp-healthchecks: will enable /liveness_check, /readiness_check, and / (with the proper user-agent) endpoints that will make it work well with GCP App Engine and GKE Ingresses (default false) -github-org string: restrict logins to members of this organisation -github-team string: restrict logins to members of any of these teams (slug), separated by a comma -google-admin-email string: the google admin to impersonate for api calls diff --git a/main.go b/main.go index efe336c..fe1b8ce 100644 --- a/main.go +++ b/main.go @@ -93,7 +93,7 @@ func main() { flagSet.String("acr-values", "http://idmanagement.gov/ns/assurance/loa/1", "acr values string: optional, used by login.gov") flagSet.String("jwt-key", "", "private key used to sign JWT: required by login.gov") flagSet.String("pubjwk-url", "", "JWK pubkey access endpoint: required by login.gov") - flagSet.Bool("gcp-healthchecks", false, "Enable GCP healthcheck endpoints") + flagSet.Bool("gcp-healthchecks", false, "Enable GCP/GKE healthcheck endpoints") flagSet.Parse(os.Args[1:]) From 6bb32c8059821ef75ff2fe1a42b9624463c0465c Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Tue, 26 Mar 2019 08:59:03 -0700 Subject: [PATCH 10/10] It's not really mine --- main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/main.go b/main.go index fe1b8ce..0989067 100644 --- a/main.go +++ b/main.go @@ -141,14 +141,14 @@ func main() { rand.Seed(time.Now().UnixNano()) - var myhandler http.Handler + var handler http.Handler if opts.GCPHealthChecks { - myhandler = gcpHealthcheck(LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat)) + handler = gcpHealthcheck(LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat)) } else { - myhandler = LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat) + handler = LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat) } s := &Server{ - Handler: myhandler, + Handler: handler, Opts: opts, } s.ListenAndServe()