From e24e4ef8808e0d886d82f265ccf8ec2910eba764 Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Thu, 17 Oct 2019 16:30:18 +0100 Subject: [PATCH 1/9] Add force-https option and flag Signed-off-by: Josh Michielsen --- main.go | 1 + options.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/main.go b/main.go index a4bf378..5baa8cd 100644 --- a/main.go +++ b/main.go @@ -32,6 +32,7 @@ func main() { flagSet.String("http-address", "127.0.0.1:4180", "[http://]: or unix:// to listen on for HTTP clients") flagSet.String("https-address", ":443", ": to listen on for HTTPS clients") + flagSet.Bool("force-https", false, "force HTTPS redirect") flagSet.String("tls-cert-file", "", "path to certificate file") flagSet.String("tls-key-file", "", "path to private key file") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"") diff --git a/options.go b/options.go index 37bbb0b..ddc10cf 100644 --- a/options.go +++ b/options.go @@ -34,6 +34,7 @@ type Options struct { ProxyWebSockets bool `flag:"proxy-websockets" cfg:"proxy_websockets" env:"OAUTH2_PROXY_PROXY_WEBSOCKETS"` HTTPAddress string `flag:"http-address" cfg:"http_address" env:"OAUTH2_PROXY_HTTP_ADDRESS"` HTTPSAddress string `flag:"https-address" cfg:"https_address" env:"OAUTH2_PROXY_HTTPS_ADDRESS"` + ForceHTTPS bool `flag:"force-https" cfg:"force_https" env:"OAUTH2_PROXY_FORCE_HTTPS"` RedirectURL string `flag:"redirect-url" cfg:"redirect_url" env:"OAUTH2_PROXY_REDIRECT_URL"` ClientID string `flag:"client-id" cfg:"client_id" env:"OAUTH2_PROXY_CLIENT_ID"` ClientSecret string `flag:"client-secret" cfg:"client_secret" env:"OAUTH2_PROXY_CLIENT_SECRET"` @@ -145,6 +146,7 @@ func NewOptions() *Options { ProxyWebSockets: true, HTTPAddress: "127.0.0.1:4180", HTTPSAddress: ":443", + ForceHTTPS: false, DisplayHtpasswdForm: true, CookieOptions: options.CookieOptions{ CookieName: "_oauth2_proxy", From aae91b0ad6cddae6d7ea17085f4713567aba248a Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Thu, 17 Oct 2019 16:30:48 +0100 Subject: [PATCH 2/9] Add new handler to redirect to HTTPS if flag is set Signed-off-by: Josh Michielsen --- http.go | 10 ++++++++++ main.go | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/http.go b/http.go index 2cee227..0c84fb4 100644 --- a/http.go +++ b/http.go @@ -152,3 +152,13 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) { tc.SetKeepAlivePeriod(3 * time.Minute) return tc, nil } + +func redirectToHTTPS(opts *Options, h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if opts.ForceHTTPS { + http.Redirect(w, r, opts.HTTPSAddress, http.StatusPermanentRedirect) + } + + h.ServeHTTP(w, r) + }) +} diff --git a/main.go b/main.go index 5baa8cd..fb85906 100644 --- a/main.go +++ b/main.go @@ -186,9 +186,9 @@ func main() { var handler http.Handler if opts.GCPHealthChecks { - handler = gcpHealthcheck(LoggingHandler(oauthproxy)) + handler = redirectToHTTPS(opts, gcpHealthcheck(LoggingHandler(oauthproxy))) } else { - handler = LoggingHandler(oauthproxy) + handler = redirectToHTTPS(opts, LoggingHandler(oauthproxy)) } s := &Server{ Handler: handler, From 271efe776e3b01b5526f1b284e48732e1131b5c0 Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Thu, 17 Oct 2019 16:37:36 +0100 Subject: [PATCH 3/9] Added tests Signed-off-by: Josh Michielsen --- http_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/http_test.go b/http_test.go index f5ee142..bfb9bb2 100644 --- a/http_test.go +++ b/http_test.go @@ -106,3 +106,32 @@ func TestGCPHealthcheckNotIngressPut(t *testing.T) { assert.Equal(t, "test", rw.Body.String()) } + +func TestRedirectToHTTPSTrue(t *testing.T) { + opts := NewOptions() + opts.ForceHTTPS = true + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := redirectToHTTPS(opts, http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/", nil) + h.ServeHTTP(rw, r) + + assert.Equal(t, http.StatusPermanentRedirect, rw.Code, "status code should be %d, got: %d", http.StatusPermanentRedirect, rw.Code) +} + +func TestRedirectToHTTPSFalse(t *testing.T) { + opts := NewOptions() + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := redirectToHTTPS(opts, http.HandlerFunc(handler)) + rw := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "/", nil) + h.ServeHTTP(rw, r) + + assert.Equal(t, http.StatusOK, rw.Code, "status code should be %d, got: %d", http.StatusOK, rw.Code) +} From bed0336608ca139cd5617be1949ae05aa6f6dc0e Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Thu, 17 Oct 2019 22:04:24 +0100 Subject: [PATCH 4/9] Add SSL check and test no redirect when HTTPS Signed-off-by: Josh Michielsen --- http.go | 2 +- http_test.go | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/http.go b/http.go index 0c84fb4..9ef0499 100644 --- a/http.go +++ b/http.go @@ -155,7 +155,7 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) { func redirectToHTTPS(opts *Options, h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if opts.ForceHTTPS { + if opts.ForceHTTPS && r.TLS == nil { http.Redirect(w, r, opts.HTTPSAddress, http.StatusPermanentRedirect) } diff --git a/http_test.go b/http_test.go index bfb9bb2..400213a 100644 --- a/http_test.go +++ b/http_test.go @@ -135,3 +135,24 @@ func TestRedirectToHTTPSFalse(t *testing.T) { assert.Equal(t, http.StatusOK, rw.Code, "status code should be %d, got: %d", http.StatusOK, rw.Code) } + +func TestRedirectNotWhenHTTPS(t *testing.T) { + opts := NewOptions() + opts.ForceHTTPS = true + handler := func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("test")) + } + + h := redirectToHTTPS(opts, http.HandlerFunc(handler)) + s := httptest.NewTLSServer(h) + defer s.Close() + + opts.HTTPSAddress = s.URL + client := s.Client() + res, err := client.Get(s.URL) + if err != nil { + t.Fatalf("request to test server failed with error: %v", err) + } + + assert.Equal(t, http.StatusOK, res.StatusCode, "status code should be %d, got: %d", http.StatusOK, res.StatusCode) +} From 56d195a433d1c15d860e22fd47b619ac52765be5 Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Thu, 17 Oct 2019 22:20:15 +0100 Subject: [PATCH 5/9] Docs and changelog Signed-off-by: Josh Michielsen --- CHANGELOG.md | 1 + docs/configuration/configuration.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d76e895..d3c1e4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Changes since v4.0.0 - [#227](https://github.com/pusher/oauth2_proxy/pull/227) Add Keycloak provider (@Ofinka) +- [#259](https://github.com/pusher/oauth2_proxy/pull/259) Redirect to HTTPS (@jmickey) - [#273](https://github.com/pusher/oauth2_proxy/pull/273) Support Go 1.13 (@dio) - [#275](https://github.com/pusher/oauth2_proxy/pull/275) docker: build from debian buster (@syscll) diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md index c076dc8..332c223 100644 --- a/docs/configuration/configuration.md +++ b/docs/configuration/configuration.md @@ -44,6 +44,7 @@ An example [oauth2_proxy.cfg]({{ site.gitweb }}/contrib/oauth2_proxy.cfg.example | `-extra-jwt-issuers` | string | if `-skip-jwt-bearer-tokens` is set, a list of extra JWT `issuer=audience` pairs (where the issuer URL has a `.well-known/openid-configuration` or a `.well-known/jwks.json`) | | | `-exclude-logging-paths` | string | comma separated list of paths to exclude from logging, eg: `"/ping,/path2"` |`""` (no paths excluded) | | `-flush-interval` | duration | period between flushing response buffers when streaming responses | `"1s"` | +| `-force-https` | bool | enforce https redirect | `false` | | `-banner` | string | custom banner string. Use `"-"` to disable default banner. | | | `-footer` | string | custom footer string. Use `"-"` to disable default footer. | | | `-gcp-healthchecks` | bool | 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 | false | From dcc430f6f141f2dc4071952ae2f6c3e7a1e15979 Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Mon, 21 Oct 2019 23:21:35 +0100 Subject: [PATCH 6/9] Check `X-Forwared-Proto` for https (via another reverse proxy) Signed-off-by: Josh Michielsen --- http.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http.go b/http.go index 9ef0499..b7ee959 100644 --- a/http.go +++ b/http.go @@ -155,7 +155,8 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) { func redirectToHTTPS(opts *Options, h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if opts.ForceHTTPS && r.TLS == nil { + proto := r.Header.Get("X-Forwarded-Proto") + if opts.ForceHTTPS && r.TLS == nil && strings.ToLower(proto) != "https" { http.Redirect(w, r, opts.HTTPSAddress, http.StatusPermanentRedirect) } From fe9efba0c524c4356ef8e8020f20052954e04aa3 Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Tue, 22 Oct 2019 14:19:39 +0100 Subject: [PATCH 7/9] Documentation change Co-Authored-By: Joel Speed --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index fb85906..e84a796 100644 --- a/main.go +++ b/main.go @@ -32,7 +32,7 @@ func main() { flagSet.String("http-address", "127.0.0.1:4180", "[http://]: or unix:// to listen on for HTTP clients") flagSet.String("https-address", ":443", ": to listen on for HTTPS clients") - flagSet.Bool("force-https", false, "force HTTPS redirect") + flagSet.Bool("force-https", false, "force HTTPS redirect for HTTP requests") flagSet.String("tls-cert-file", "", "path to certificate file") flagSet.String("tls-key-file", "", "path to private key file") flagSet.String("redirect-url", "", "the OAuth Redirect URL. ie: \"https://internalapp.yourcompany.com/oauth2/callback\"") From c0bfe0357a06339831f50a997a6708ea18ac745b Mon Sep 17 00:00:00 2001 From: Josh Michielsen Date: Tue, 22 Oct 2019 14:21:06 +0100 Subject: [PATCH 8/9] Confirm that the proto is not empty, and change condition to OR Co-Authored-By: Joel Speed --- http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.go b/http.go index b7ee959..88280c4 100644 --- a/http.go +++ b/http.go @@ -156,7 +156,7 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) { func redirectToHTTPS(opts *Options, h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { proto := r.Header.Get("X-Forwarded-Proto") - if opts.ForceHTTPS && r.TLS == nil && strings.ToLower(proto) != "https" { + if opts.ForceHTTPS && (r.TLS == nil || (proto != "" && strings.ToLower(proto) != "https")) { http.Redirect(w, r, opts.HTTPSAddress, http.StatusPermanentRedirect) } From 35f2ae9a36bc979feb2377b40611fe4dd3d0c75a Mon Sep 17 00:00:00 2001 From: Tom Deadman Date: Wed, 23 Oct 2019 17:55:34 +0100 Subject: [PATCH 9/9] Improved request errors (#286) * worked on wrapping errors in requests.go, added defer statements * removed .idea (generated by goland) * added another require.NoError * Update pkg/requests/requests.go Co-Authored-By: Dan Bond * fixed out-of-order imports * changelog entry added * swapped error definitions to use fmt.Errorf rather than Wrap() * formatting changes, added new defers to requests_test.go * suppot for go1.12 pipeline removed from travis pipeline, .idea/ added to gitignore * Reorder changelog entry --- .gitignore | 1 + .travis.yml | 1 - CHANGELOG.md | 1 + pkg/requests/requests.go | 25 +++++++++++++++++-------- pkg/requests/requests_test.go | 30 ++++++++++++++++++------------ 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index a5f59b4..aff7b5b 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ release # Folders _obj _test +.idea/ # Architecture specific extensions/prefixes *.[568vq] diff --git a/.travis.yml b/.travis.yml index 7fb4bce..ef8aa3e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,5 @@ language: go go: - - 1.12.x - 1.13.x install: # Fetch dependencies diff --git a/CHANGELOG.md b/CHANGELOG.md index 5554885..9a3281f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - [#275](https://github.com/pusher/oauth2_proxy/pull/275) docker: build from debian buster (@syscll) - [#258](https://github.com/pusher/oauth2_proxy/pull/258) Add IDToken for Azure provider - This PR adds the IDToken into the session for the Azure provider allowing requests to a backend to be identified as a specific user. As a consequence, if you are using a cookie to store the session the cookie will now exceed the 4kb size limit and be split into multiple cookies. This can cause problems when using nginx as a proxy, resulting in no cookie being passed at all. Either increase the proxy_buffer_size in nginx or implement the redis session storage (see https://pusher.github.io/oauth2_proxy/configuration#redis-storage) +- [#286](https://github.com/pusher/oauth2_proxy/pull/286) Requests.go updated with useful error messages (@biotom) # v4.0.0 diff --git a/pkg/requests/requests.go b/pkg/requests/requests.go index 82d1176..9083b2d 100644 --- a/pkg/requests/requests.go +++ b/pkg/requests/requests.go @@ -18,17 +18,23 @@ func Request(req *http.Request) (*simplejson.Json, error) { return nil, err } body, err := ioutil.ReadAll(resp.Body) - resp.Body.Close() - logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body) - if err != nil { - return nil, err + if body != nil { + defer resp.Body.Close() } + + logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body) + + if err != nil { + return nil, fmt.Errorf("problem reading http request body: %w", err) + } + if resp.StatusCode != 200 { return nil, fmt.Errorf("got %d %s", resp.StatusCode, body) } + data, err := simplejson.NewJson(body) if err != nil { - return nil, err + return nil, fmt.Errorf("error unmarshalling json: %w", err) } return data, nil } @@ -41,10 +47,13 @@ func RequestJSON(req *http.Request, v interface{}) error { return err } body, err := ioutil.ReadAll(resp.Body) - resp.Body.Close() + if body != nil { + defer resp.Body.Close() + } + logger.Printf("%d %s %s %s", resp.StatusCode, req.Method, req.URL, body) if err != nil { - return err + return fmt.Errorf("error reading body from http response: %w", err) } if resp.StatusCode != 200 { return fmt.Errorf("got %d %s", resp.StatusCode, body) @@ -56,7 +65,7 @@ func RequestJSON(req *http.Request, v interface{}) error { func RequestUnparsedResponse(url string, header http.Header) (resp *http.Response, err error) { req, err := http.NewRequest("GET", url, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("error performing get request: %w", err) } req.Header = header diff --git a/pkg/requests/requests_test.go b/pkg/requests/requests_test.go index 99a4c3b..c9ec4e8 100644 --- a/pkg/requests/requests_test.go +++ b/pkg/requests/requests_test.go @@ -8,20 +8,21 @@ import ( "testing" "github.com/bitly/go-simplejson" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func testBackend(responseCode int, payload string) *httptest.Server { +func testBackend(t *testing.T, responseCode int, payload string) *httptest.Server { return httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(responseCode) - w.Write([]byte(payload)) + _, err := w.Write([]byte(payload)) + require.NoError(t, err) })) } func TestRequest(t *testing.T) { - backend := testBackend(200, "{\"foo\": \"bar\"}") + backend := testBackend(t, 200, "{\"foo\": \"bar\"}") defer backend.Close() req, _ := http.NewRequest("GET", backend.URL, nil) @@ -35,7 +36,7 @@ func TestRequest(t *testing.T) { func TestRequestFailure(t *testing.T) { // Create a backend to generate a test URL, then close it to cause a // connection error. - backend := testBackend(200, "{\"foo\": \"bar\"}") + backend := testBackend(t, 200, "{\"foo\": \"bar\"}") backend.Close() req, err := http.NewRequest("GET", backend.URL, nil) @@ -49,7 +50,7 @@ func TestRequestFailure(t *testing.T) { } func TestHttpErrorCode(t *testing.T) { - backend := testBackend(404, "{\"foo\": \"bar\"}") + backend := testBackend(t, 404, "{\"foo\": \"bar\"}") defer backend.Close() req, err := http.NewRequest("GET", backend.URL, nil) @@ -60,7 +61,7 @@ func TestHttpErrorCode(t *testing.T) { } func TestJsonParsingError(t *testing.T) { - backend := testBackend(200, "not well-formed JSON") + backend := testBackend(t, 200, "not well-formed JSON") defer backend.Close() req, err := http.NewRequest("GET", backend.URL, nil) @@ -77,7 +78,8 @@ func TestRequestUnparsedResponseUsingAccessTokenParameter(t *testing.T) { token := r.FormValue("access_token") if r.URL.Path == "/" && token == "my_token" { w.WriteHeader(200) - w.Write([]byte("some payload")) + _, err := w.Write([]byte("some payload")) + require.NoError(t, err) } else { w.WriteHeader(403) } @@ -86,16 +88,17 @@ func TestRequestUnparsedResponseUsingAccessTokenParameter(t *testing.T) { response, err := RequestUnparsedResponse( backend.URL+"?access_token=my_token", nil) + defer response.Body.Close() + assert.Equal(t, nil, err) assert.Equal(t, 200, response.StatusCode) body, err := ioutil.ReadAll(response.Body) assert.Equal(t, nil, err) - response.Body.Close() assert.Equal(t, "some payload", string(body)) } func TestRequestUnparsedResponseUsingAccessTokenParameterFailedResponse(t *testing.T) { - backend := testBackend(200, "some payload") + backend := testBackend(t, 200, "some payload") // Close the backend now to force a request failure. backend.Close() @@ -110,7 +113,8 @@ func TestRequestUnparsedResponseUsingHeaders(t *testing.T) { func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/" && r.Header["Auth"][0] == "my_token" { w.WriteHeader(200) - w.Write([]byte("some payload")) + _, err := w.Write([]byte("some payload")) + require.NoError(t, err) } else { w.WriteHeader(403) } @@ -120,10 +124,12 @@ func TestRequestUnparsedResponseUsingHeaders(t *testing.T) { headers := make(http.Header) headers.Set("Auth", "my_token") response, err := RequestUnparsedResponse(backend.URL, headers) + defer response.Body.Close() + assert.Equal(t, nil, err) assert.Equal(t, 200, response.StatusCode) body, err := ioutil.ReadAll(response.Body) assert.Equal(t, nil, err) - response.Body.Close() + assert.Equal(t, "some payload", string(body)) }