From 3476daf32226f890fdc9a1dae2ace8d8bed401e3 Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Wed, 20 Mar 2019 14:29:44 -0700 Subject: [PATCH 01/21] 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/21] 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/21] 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 978c0a33e4ceffb57ad0ff15aabeaa864f4a6262 Mon Sep 17 00:00:00 2001 From: gyson Date: Fri, 22 Mar 2019 17:19:38 -0400 Subject: [PATCH 04/21] Improve websocket support --- logging_handler.go | 10 ++++++++++ logging_handler_test.go | 5 +++++ oauthproxy.go | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/logging_handler.go b/logging_handler.go index d47ae58..4502ed3 100644 --- a/logging_handler.go +++ b/logging_handler.go @@ -4,6 +4,8 @@ package main import ( + "bufio" + "errors" "fmt" "io" "net" @@ -32,6 +34,14 @@ func (l *responseLogger) Header() http.Header { return l.w.Header() } +// Support Websocket +func (l *responseLogger) Hijack() (rwc net.Conn, buf *bufio.ReadWriter, err error) { + if hj, ok := l.w.(http.Hijacker); ok { + return hj.Hijack() + } + return nil, nil, errors.New("http.Hijacker is not available on writer") +} + // ExtractGAPMetadata extracts and removes GAP headers from the ResponseWriter's // Header func (l *responseLogger) ExtractGAPMetadata() { diff --git a/logging_handler_test.go b/logging_handler_test.go index de0efcc..ea5d968 100644 --- a/logging_handler_test.go +++ b/logging_handler_test.go @@ -24,6 +24,11 @@ func TestLoggingHandler_ServeHTTP(t *testing.T) { for _, test := range tests { buf := bytes.NewBuffer(nil) handler := func(w http.ResponseWriter, req *http.Request) { + _, ok := w.(http.Hijacker) + if !ok { + t.Error("http.Hijacker is not available") + } + w.Write([]byte("test")) } diff --git a/oauthproxy.go b/oauthproxy.go index 561dce3..24fea21 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -110,7 +110,7 @@ func (u *UpstreamProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { r.Header.Set("GAP-Auth", w.Header().Get("GAP-Auth")) u.auth.SignRequest(r) } - if u.wsHandler != nil && r.Header.Get("Connection") == "Upgrade" && r.Header.Get("Upgrade") == "websocket" { + if u.wsHandler != nil && strings.ToLower(r.Header.Get("Connection")) == "upgrade" && r.Header.Get("Upgrade") == "websocket" { u.wsHandler.ServeHTTP(w, r) } else { u.handler.ServeHTTP(w, r) From b67614c90f178a717864f2b9a0b91c2f7661cbf0 Mon Sep 17 00:00:00 2001 From: gyson Date: Fri, 22 Mar 2019 17:41:55 -0400 Subject: [PATCH 05/21] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9823907..14f0e3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Changes since v3.1.0 +- [#112](https://github.com/pusher/oauth2_proxy/pull/112) Improve websocket support (@gyson) - [#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 06/21] 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 07/21] 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 08/21] 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 09/21] 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 10/21] 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 11/21] 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 96608396677e1440b83763639716af8ee2028bb4 Mon Sep 17 00:00:00 2001 From: daB0bby Date: Tue, 26 Mar 2019 16:49:04 +0100 Subject: [PATCH 12/21] fixes typo in set-authorization-header --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 1bb63be..bd0953e 100644 --- a/README.md +++ b/README.md @@ -505,7 +505,7 @@ server { auth_request_set $auth_cookie $upstream_http_set_cookie; add_header Set-Cookie $auth_cookie; - # When using the --set-authorization flag, some provider's cookies can exceed the 4kb + # When using the --set-authorization-header flag, some provider's cookies can exceed the 4kb # limit and so the OAuth2 Proxy splits these into multiple parts. # Nginx normally only copies the first `Set-Cookie` header from the auth_request to the response, # so if your cookies are larger than 4kb, you will need to extract additional cookies manually. From 6bb32c8059821ef75ff2fe1a42b9624463c0465c Mon Sep 17 00:00:00 2001 From: timothy-spencer Date: Tue, 26 Mar 2019 08:59:03 -0700 Subject: [PATCH 13/21] 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() From 4f7517b2f91cca30a71cb8a8c7f6efa0d83b6008 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Tue, 9 Apr 2019 14:55:33 +0300 Subject: [PATCH 14/21] Encrypting user/email from cookie --- providers/session_state.go | 27 +++++++++++++++++++++++++++ providers/session_state_test.go | 12 ++++++------ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/providers/session_state.go b/providers/session_state.go index 4741b4a..10cbba4 100644 --- a/providers/session_state.go +++ b/providers/session_state.go @@ -62,6 +62,19 @@ func (s *SessionState) EncodeSessionState(c *cookie.Cipher) (string, error) { } else { ss = *s var err error + // Encrypt also Email and User when cipher is provided + if ss.Email != "" { + ss.Email, err = c.Encrypt(ss.Email) + if err != nil { + return "", err + } + } + if ss.User != "" { + ss.User, err = c.Encrypt(ss.User) + if err != nil { + return "", err + } + } if ss.AccessToken != "" { ss.AccessToken, err = c.Encrypt(ss.AccessToken) if err != nil { @@ -172,6 +185,20 @@ func DecodeSessionState(v string, c *cookie.Cipher) (*SessionState, error) { User: ss.User, } } else { + // Backward compatibility with using unecrypted Email + if ss.Email != "" { + decryptedEmail, err := c.Decrypt(ss.Email) + if err == nil { + ss.Email = decryptedEmail + } + } + // Backward compatibility with using unecrypted User + if ss.User != "" { + decryptedUser, err := c.Decrypt(ss.User) + if err == nil { + ss.User = decryptedUser + } + } if ss.AccessToken != "" { ss.AccessToken, err = c.Decrypt(ss.AccessToken) if err != nil { diff --git a/providers/session_state_test.go b/providers/session_state_test.go index 9557eea..dee81bb 100644 --- a/providers/session_state_test.go +++ b/providers/session_state_test.go @@ -41,8 +41,8 @@ func TestSessionStateSerialization(t *testing.T) { ss, err = DecodeSessionState(encoded, c2) t.Logf("%#v", ss) assert.Equal(t, nil, err) - assert.Equal(t, "user", ss.User) - assert.Equal(t, s.Email, ss.Email) + assert.NotEqual(t, "user", ss.User) + assert.NotEqual(t, s.Email, ss.Email) assert.Equal(t, s.ExpiresOn.Unix(), ss.ExpiresOn.Unix()) assert.NotEqual(t, s.AccessToken, ss.AccessToken) assert.NotEqual(t, s.IDToken, ss.IDToken) @@ -77,8 +77,8 @@ func TestSessionStateSerializationWithUser(t *testing.T) { ss, err = DecodeSessionState(encoded, c2) t.Logf("%#v", ss) assert.Equal(t, nil, err) - assert.Equal(t, s.User, ss.User) - assert.Equal(t, s.Email, ss.Email) + assert.NotEqual(t, s.User, ss.User) + assert.NotEqual(t, s.Email, ss.Email) assert.Equal(t, s.ExpiresOn.Unix(), ss.ExpiresOn.Unix()) assert.NotEqual(t, s.AccessToken, ss.AccessToken) assert.NotEqual(t, s.RefreshToken, ss.RefreshToken) @@ -229,7 +229,7 @@ func TestDecodeSessionState(t *testing.T) { ExpiresOn: e, RefreshToken: "refresh4321", }, - Encoded: fmt.Sprintf(`{"Email":"user@domain.com","User":"just-user","AccessToken":"I6s+ml+/MldBMgHIiC35BTKTh57skGX24w==","IDToken":"xojNdyyjB1HgYWh6XMtXY/Ph5eCVxa1cNsklJw==","RefreshToken":"qEX0x6RmASxo4dhlBG6YuRs9Syn/e9sHu/+K","ExpiresOn":%s}`, eString), + Encoded: fmt.Sprintf(`{"Email":"FsKKYrTWZWrxSOAqA/fTNAUZS5QWCqOBjuAbBlbVOw==","User":"rT6JP3dxQhxUhkWrrd7yt6c1mDVyQCVVxw==","AccessToken":"I6s+ml+/MldBMgHIiC35BTKTh57skGX24w==","IDToken":"xojNdyyjB1HgYWh6XMtXY/Ph5eCVxa1cNsklJw==","RefreshToken":"qEX0x6RmASxo4dhlBG6YuRs9Syn/e9sHu/+K","ExpiresOn":%s}`, eString), Cipher: c, }, { @@ -237,7 +237,7 @@ func TestDecodeSessionState(t *testing.T) { Email: "user@domain.com", User: "just-user", }, - Encoded: `{"Email":"user@domain.com","User":"just-user"}`, + Encoded: `{"Email":"EGTllJcOFC16b7LBYzLekaHAC5SMMSPdyUrg8hd25g==","User":"rT6JP3dxQhxUhkWrrd7yt6c1mDVyQCVVxw=="}`, Cipher: c, }, { From 6da6ee7f849d583bcd2019604bb1f3c474dc242f Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Tue, 9 Apr 2019 15:00:17 +0300 Subject: [PATCH 15/21] Encrypting user/email from cookie, add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c36c201..6d3100f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Use JSON to encode session state to be stored in browser cookies - Implement legacy decode function to support existing cookies generated by older versions - Add detailed table driven tests in session_state_test.go +- [#120](https://github.com/pusher/oauth2_proxy/pull/120) Encrypting user/email from cookie (@costelmoraru) - [#55](https://github.com/pusher/oauth2_proxy/pull/55) Added login.gov provider (@timothy-spencer) - [#55](https://github.com/pusher/oauth2_proxy/pull/55) Added environment variables for all config options (@timothy-spencer) - [#70](https://github.com/pusher/oauth2_proxy/pull/70) Fix handling of splitted cookies (@einfachchr) From f5a6609b450e1d0c31473c68967b5842b059ce01 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Tue, 9 Apr 2019 15:17:40 +0300 Subject: [PATCH 16/21] Fixing lint error --- providers/session_state.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/providers/session_state.go b/providers/session_state.go index 10cbba4..5e5a005 100644 --- a/providers/session_state.go +++ b/providers/session_state.go @@ -187,15 +187,15 @@ func DecodeSessionState(v string, c *cookie.Cipher) (*SessionState, error) { } else { // Backward compatibility with using unecrypted Email if ss.Email != "" { - decryptedEmail, err := c.Decrypt(ss.Email) - if err == nil { + decryptedEmail, errEmail := c.Decrypt(ss.Email) + if errEmail == nil { ss.Email = decryptedEmail } } // Backward compatibility with using unecrypted User if ss.User != "" { - decryptedUser, err := c.Decrypt(ss.User) - if err == nil { + decryptedUser, errUser := c.Decrypt(ss.User) + if errUser == nil { ss.User = decryptedUser } } From 071d17b521d3652380f02ecf80849712bec02526 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Wed, 10 Apr 2019 00:36:35 +0300 Subject: [PATCH 17/21] Expose -cookie-path as configuration parameter --- main.go | 1 + oauthproxy.go | 6 ++++-- options.go | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 0989067..ac9f80f 100644 --- a/main.go +++ b/main.go @@ -69,6 +69,7 @@ func main() { flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates") flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)") flagSet.String("cookie-domain", "", "an optional cookie domain to force cookies to (ie: .yourcompany.com)*") + flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*") flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie") flagSet.Duration("cookie-refresh", time.Duration(0), "refresh the cookie after this duration; 0 to disable") flagSet.Bool("cookie-secure", true, "set secure (HTTPS) cookie flag") diff --git a/oauthproxy.go b/oauthproxy.go index 24fea21..17767ad 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -56,6 +56,7 @@ type OAuthProxy struct { CookieName string CSRFCookieName string CookieDomain string + CookiePath string CookieSecure bool CookieHTTPOnly bool CookieExpire time.Duration @@ -214,7 +215,7 @@ func NewOAuthProxy(opts *Options, validator func(string) bool) *OAuthProxy { refresh = fmt.Sprintf("after %s", opts.CookieRefresh) } - log.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domain:%s refresh:%s", opts.CookieName, opts.CookieSecure, opts.CookieHTTPOnly, opts.CookieExpire, opts.CookieDomain, refresh) + log.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domain:%s path:%s refresh:%s", opts.CookieName, opts.CookieSecure, opts.CookieHTTPOnly, opts.CookieExpire, opts.CookieDomain, opts.CookiePath, refresh) var cipher *cookie.Cipher if opts.PassAccessToken || opts.SetAuthorization || opts.PassAuthorization || (opts.CookieRefresh != time.Duration(0)) { @@ -230,6 +231,7 @@ func NewOAuthProxy(opts *Options, validator func(string) bool) *OAuthProxy { CSRFCookieName: fmt.Sprintf("%v_%v", opts.CookieName, "csrf"), CookieSeed: opts.CookieSecret, CookieDomain: opts.CookieDomain, + CookiePath: opts.CookiePath, CookieSecure: opts.CookieSecure, CookieHTTPOnly: opts.CookieHTTPOnly, CookieExpire: opts.CookieExpire, @@ -430,7 +432,7 @@ func (p *OAuthProxy) makeCookie(req *http.Request, name string, value string, ex return &http.Cookie{ Name: name, Value: value, - Path: "/", + Path: p.CookiePath, Domain: p.CookieDomain, HttpOnly: p.CookieHTTPOnly, Secure: p.CookieSecure, diff --git a/options.go b/options.go index b736521..620d626 100644 --- a/options.go +++ b/options.go @@ -49,6 +49,7 @@ type Options struct { CookieName string `flag:"cookie-name" cfg:"cookie_name" env:"OAUTH2_PROXY_COOKIE_NAME"` CookieSecret string `flag:"cookie-secret" cfg:"cookie_secret" env:"OAUTH2_PROXY_COOKIE_SECRET"` CookieDomain string `flag:"cookie-domain" cfg:"cookie_domain" env:"OAUTH2_PROXY_COOKIE_DOMAIN"` + CookiePath string `flag:"cookie-path" cfg:"cookie_path" env:"OAUTH2_PROXY_COOKIE_PATH"` CookieExpire time.Duration `flag:"cookie-expire" cfg:"cookie_expire" env:"OAUTH2_PROXY_COOKIE_EXPIRE"` CookieRefresh time.Duration `flag:"cookie-refresh" cfg:"cookie_refresh" env:"OAUTH2_PROXY_COOKIE_REFRESH"` CookieSecure bool `flag:"cookie-secure" cfg:"cookie_secure" env:"OAUTH2_PROXY_COOKIE_SECURE"` From f5f64e7d6c0c8ba82e2627ad5d70e89b41fa46e3 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Wed, 10 Apr 2019 00:42:17 +0300 Subject: [PATCH 18/21] Update the changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c36c201..e2d38df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - [#41](https://github.com/pusher/oauth2_proxy/pull/41) Added option to manually specify OIDC endpoints instead of relying on discovery - [#83](https://github.com/pusher/oauth2_proxy/pull/83) Add `id_token` refresh to Google provider (@leki75) - [#10](https://github.com/pusher/oauth2_proxy/pull/10) fix redirect url param handling (@dt-rush) +- [#122](https://github.com/pusher/oauth2_proxy/pull/122) Expose -cookie-path as configuration parameter (@costelmoraru) # v3.1.0 From dc8934ca930a09eb2e19a5ee709a8b3642ad74a9 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Wed, 10 Apr 2019 12:52:50 +0300 Subject: [PATCH 19/21] Update documentation, to add the flag to the list of flags --- README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0a9cacb..65df515 100644 --- a/README.md +++ b/README.md @@ -172,12 +172,12 @@ OpenID Connect is a spec for OAUTH 2.0 + identity that is implemented by many ma login.gov is an OIDC provider for the US Government. If you are a US Government agency, you can contact the login.gov team through the contact information that you can find on https://login.gov/developers/ and work with them to understand how to get login.gov -accounts for integration/test and production access. +accounts for integration/test and production access. A developer guide is available here: https://developers.login.gov/, though this proxy handles everything but the data you need to create to register your application in the login.gov dashboard. -As a demo, we will assume that you are running your application that you want to secure locally on +As a demo, we will assume that you are running your application that you want to secure locally on http://localhost:3000/, that you will be starting your proxy up on http://localhost:4180/, and that you have an agency integration account for testing. @@ -261,6 +261,7 @@ Usage of oauth2_proxy: -client-secret string: the OAuth Client Secret -config string: path to config file -cookie-domain string: an optional cookie domain to force cookies to (ie: .yourcompany.com) + -cookie-path string: an optional cookie path to force cookies to (ie: .yourcompany.com/foo) -cookie-expire duration: expire timeframe for cookie (default 168h0m0s) -cookie-httponly: set HttpOnly cookie flag (default true) -cookie-name string: the name of the cookie that the oauth_proxy creates (default "_oauth2_proxy") @@ -336,6 +337,7 @@ The following environment variables can be used in place of the corresponding co - `OAUTH2_PROXY_COOKIE_NAME` - `OAUTH2_PROXY_COOKIE_SECRET` - `OAUTH2_PROXY_COOKIE_DOMAIN` +- `OAUTH2_PROXY_COOKIE_PATH` - `OAUTH2_PROXY_COOKIE_EXPIRE` - `OAUTH2_PROXY_COOKIE_REFRESH` - `OAUTH2_PROXY_SIGNATURE_KEY` @@ -412,7 +414,7 @@ The command line to run `oauth2_proxy` in this configuration would look like thi OAuth2 Proxy responds directly to the following endpoints. All other endpoints will be proxied upstream when authenticated. The `/oauth2` prefix can be changed with the `--proxy-prefix` config variable. - /robots.txt - returns a 200 OK response that disallows all User-agents from all paths; see [robotstxt.org](http://www.robotstxt.org/) for more info -- /ping - returns a 200 OK response, which is intended for use with health checks +- /ping - returns a 200 OK response, which is intended for use with health checks - /oauth2/sign_in - the login page, which also doubles as a sign out page (it clears cookies) - /oauth2/start - a URL that will redirect to start the OAuth cycle - /oauth2/callback - the URL used at the end of the OAuth cycle. The oauth app will be configured with this as the callback url. From 862e75a4e4a77fcbc0e036a835c45ec3c09c8c88 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Wed, 10 Apr 2019 14:50:19 +0300 Subject: [PATCH 20/21] Adjusted the cookie path sample in the documentation --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 65df515..55f7c34 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,7 @@ Usage of oauth2_proxy: -client-secret string: the OAuth Client Secret -config string: path to config file -cookie-domain string: an optional cookie domain to force cookies to (ie: .yourcompany.com) - -cookie-path string: an optional cookie path to force cookies to (ie: .yourcompany.com/foo) + -cookie-path string: an optional cookie path to force cookies to (ie: /foo) -cookie-expire duration: expire timeframe for cookie (default 168h0m0s) -cookie-httponly: set HttpOnly cookie flag (default true) -cookie-name string: the name of the cookie that the oauth_proxy creates (default "_oauth2_proxy") From f7c85a4d16b8c9f09b28448b10f09bd50303f7c3 Mon Sep 17 00:00:00 2001 From: Costel Moraru Date: Wed, 10 Apr 2019 15:28:03 +0300 Subject: [PATCH 21/21] Removing obsolete comment from EncodeSessionState --- providers/session_state.go | 1 - 1 file changed, 1 deletion(-) diff --git a/providers/session_state.go b/providers/session_state.go index 5e5a005..5d4a892 100644 --- a/providers/session_state.go +++ b/providers/session_state.go @@ -62,7 +62,6 @@ func (s *SessionState) EncodeSessionState(c *cookie.Cipher) (string, error) { } else { ss = *s var err error - // Encrypt also Email and User when cipher is provided if ss.Email != "" { ss.Email, err = c.Encrypt(ss.Email) if err != nil {