git.iko.soy/typeable/oauth2-proxy
on master help sign in
Merge pull request #619 from oauth2-proxy/https-redirect-middleware

Improve Redirect to HTTPs behaviour

commitc4cf15f3e1aaa8f8ae700e1c844f08380421315b authorJoel Speed <Joel.speed@hotmail.co.uk> dateFri 3 Jul 2020, 17:25 (6 years ago) parent39c01d51c11067
7 files changed+222−70
CHANGELOG.md+1 http.go−18 http_test.go−51 main.go+6−1 pkg/middleware/middleware_suite_test.go+7 pkg/middleware/redirect_to_https.go+50 pkg/middleware/redirect_to_https_test.go+158
CHANGELOG.md+1
@@ -8,6 +8,7 @@
88
99## Changes since v6.0.0
1010
11+- [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
1112- [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
1213- [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)
1314- [#577](https://github.com/oauth2-proxy/oauth2-proxy/pull/577) Move Cipher and Session Store initialisation out of Validation (@JoelSpeed)
http.go−18
@@ -9,7 +9,6 @@ import (
99 "strings"
1010 "time"
1111
12− "github.com/justinas/alice"
1312 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
1413 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
1514)
@@ -129,20 +128,3 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
129128 tc.SetKeepAlivePeriod(3 * time.Minute)
130129 return tc, nil
131130}
132−
133−func newRedirectToHTTPS(opts *options.Options) alice.Constructor {
134− return func(next http.Handler) http.Handler {
135− return redirectToHTTPS(opts, next)
136− }
137−}
138−
139−func redirectToHTTPS(opts *options.Options, h http.Handler) http.Handler {
140− return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
141− proto := r.Header.Get("X-Forwarded-Proto")
142− if opts.ForceHTTPS && (r.TLS == nil || (proto != "" && strings.ToLower(proto) != "https")) {
143− http.Redirect(w, r, opts.HTTPSAddress, http.StatusPermanentRedirect)
144− }
145−
146− h.ServeHTTP(w, r)
147− })
148−}
http_test.go−51
@@ -2,7 +2,6 @@ package main
22
33import (
44 "net/http"
5− "net/http/httptest"
65 "sync"
76 "testing"
87 "time"
@@ -11,56 +10,6 @@ import (
1110 "github.com/stretchr/testify/assert"
1211)
1312
14−func TestRedirectToHTTPSTrue(t *testing.T) {
15− opts := options.NewOptions()
16− opts.ForceHTTPS = true
17− handler := func(w http.ResponseWriter, req *http.Request) {
18− w.Write([]byte("test"))
19− }
20−
21− h := redirectToHTTPS(opts, http.HandlerFunc(handler))
22− rw := httptest.NewRecorder()
23− r, _ := http.NewRequest("GET", "/", nil)
24− h.ServeHTTP(rw, r)
25−
26− assert.Equal(t, http.StatusPermanentRedirect, rw.Code, "status code should be %d, got: %d", http.StatusPermanentRedirect, rw.Code)
27−}
28−
29−func TestRedirectToHTTPSFalse(t *testing.T) {
30− opts := options.NewOptions()
31− handler := func(w http.ResponseWriter, req *http.Request) {
32− w.Write([]byte("test"))
33− }
34−
35− h := redirectToHTTPS(opts, http.HandlerFunc(handler))
36− rw := httptest.NewRecorder()
37− r, _ := http.NewRequest("GET", "/", nil)
38− h.ServeHTTP(rw, r)
39−
40− assert.Equal(t, http.StatusOK, rw.Code, "status code should be %d, got: %d", http.StatusOK, rw.Code)
41−}
42−
43−func TestRedirectNotWhenHTTPS(t *testing.T) {
44− opts := options.NewOptions()
45− opts.ForceHTTPS = true
46− handler := func(w http.ResponseWriter, req *http.Request) {
47− w.Write([]byte("test"))
48− }
49−
50− h := redirectToHTTPS(opts, http.HandlerFunc(handler))
51− s := httptest.NewTLSServer(h)
52− defer s.Close()
53−
54− opts.HTTPSAddress = s.URL
55− client := s.Client()
56− res, err := client.Get(s.URL)
57− if err != nil {
58− t.Fatalf("request to test server failed with error: %v", err)
59− }
60−
61− assert.Equal(t, http.StatusOK, res.StatusCode, "status code should be %d, got: %d", http.StatusOK, res.StatusCode)
62−}
63−
6413func TestGracefulShutdown(t *testing.T) {
6514 opts := options.NewOptions()
6615 stop := make(chan struct{}, 1)
main.go+6−1
@@ -3,6 +3,7 @@ package main
33import (
44 "fmt"
55 "math/rand"
6+ "net"
67 "os"
78 "os/signal"
89 "runtime"
@@ -79,7 +80,11 @@ func main() {
7980 chain := alice.New()
8081
8182 if opts.ForceHTTPS {
82− chain = chain.Append(newRedirectToHTTPS(opts))
83+ _, httpsPort, err := net.SplitHostPort(opts.HTTPSAddress)
84+ if err != nil {
85+ logger.Fatalf("FATAL: invalid HTTPS address %q: %v", opts.HTTPAddress, err)
86+ }
87+ chain = chain.Append(middleware.NewRedirectToHTTPS(httpsPort))
8388 }
8489
8590 healthCheckPaths := []string{opts.PingPath}
pkg/middleware/middleware_suite_test.go+7
@@ -1,6 +1,7 @@
11package middleware
22
33import (
4+ "net/http"
45 "testing"
56
67 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
@@ -14,3 +15,9 @@ func TestMiddlewareSuite(t *testing.T) {
1415 RegisterFailHandler(Fail)
1516 RunSpecs(t, "Middleware")
1617}
18+
19+func testHandler() http.Handler {
20+ return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
21+ rw.Write([]byte("test"))
22+ })
23+}
pkg/middleware/redirect_to_https.gonew file+50
@@ -0,0 +1,50 @@
1+package middleware
2+
3+import (
4+ "net"
5+ "net/http"
6+ "net/url"
7+ "strings"
8+
9+ "github.com/justinas/alice"
10+)
11+
12+const httpsScheme = "https"
13+
14+// NewRedirectToHTTPS creates a new redirectToHTTPS middleware that will redirect
15+// HTTP requests to HTTPS
16+func NewRedirectToHTTPS(httpsPort string) alice.Constructor {
17+ return func(next http.Handler) http.Handler {
18+ return redirectToHTTPS(httpsPort, next)
19+ }
20+}
21+
22+// redirectToHTTPS is an HTTP middleware the will redirect a request to HTTPS
23+// if it is not already HTTPS.
24+// If the request is to a non standard port, the redirection request will be
25+// to the port from the httpsAddress given.
26+func redirectToHTTPS(httpsPort string, next http.Handler) http.Handler {
27+ return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
28+ proto := req.Header.Get("X-Forwarded-Proto")
29+ if strings.EqualFold(proto, httpsScheme) || (req.TLS != nil && proto == "") {
30+ // Only care about the connection to us being HTTPS if the proto is empty,
31+ // otherwise the proto is source of truth
32+ next.ServeHTTP(rw, req)
33+ return
34+ }
35+
36+ // Copy the request URL
37+ targetURL, _ := url.Parse(req.URL.String())
38+ // Set the scheme to HTTPS
39+ targetURL.Scheme = httpsScheme
40+
41+ // Overwrite the port if the original request was to a non-standard port
42+ if targetURL.Port() != "" {
43+ // If Port was not empty, this should be fine to ignore the error
44+ host, _, _ := net.SplitHostPort(targetURL.Host)
45+ targetURL.Host = net.JoinHostPort(host, httpsPort)
46+ }
47+
48+ http.Redirect(rw, req, targetURL.String(), http.StatusPermanentRedirect)
49+ })
50+}
pkg/middleware/redirect_to_https_test.gonew file+158
@@ -0,0 +1,158 @@
1+package middleware
2+
3+import (
4+ "crypto/tls"
5+ "fmt"
6+ "net/http/httptest"
7+
8+ . "github.com/onsi/ginkgo"
9+ . "github.com/onsi/ginkgo/extensions/table"
10+ . "github.com/onsi/gomega"
11+)
12+
13+var _ = Describe("RedirectToHTTPS suite", func() {
14+ const httpsPort = "8443"
15+
16+ var permanentRedirectBody = func(address string) string {
17+ return fmt.Sprintf("<a href=\"%s\">Permanent Redirect</a>.\n\n", address)
18+ }
19+
20+ type requestTableInput struct {
21+ requestString string
22+ useTLS bool
23+ headers map[string]string
24+ expectedStatus int
25+ expectedBody string
26+ expectedLocation string
27+ }
28+
29+ DescribeTable("when serving a request",
30+ func(in *requestTableInput) {
31+ req := httptest.NewRequest("", in.requestString, nil)
32+ for k, v := range in.headers {
33+ req.Header.Add(k, v)
34+ }
35+ if in.useTLS {
36+ req.TLS = &tls.ConnectionState{}
37+ }
38+
39+ rw := httptest.NewRecorder()
40+
41+ handler := NewRedirectToHTTPS(httpsPort)(testHandler())
42+ handler.ServeHTTP(rw, req)
43+
44+ Expect(rw.Code).To(Equal(in.expectedStatus))
45+ Expect(rw.Body.String()).To(Equal(in.expectedBody))
46+
47+ if in.expectedLocation != "" {
48+ Expect(rw.Header().Values("Location")).To(ConsistOf(in.expectedLocation))
49+ }
50+ },
51+ Entry("without TLS", &requestTableInput{
52+ requestString: "http://example.com",
53+ useTLS: false,
54+ headers: map[string]string{},
55+ expectedStatus: 308,
56+ expectedBody: permanentRedirectBody("https://example.com"),
57+ expectedLocation: "https://example.com",
58+ }),
59+ Entry("with TLS", &requestTableInput{
60+ requestString: "https://example.com",
61+ useTLS: true,
62+ headers: map[string]string{},
63+ expectedStatus: 200,
64+ expectedBody: "test",
65+ }),
66+ Entry("without TLS and X-Forwarded-Proto=HTTPS", &requestTableInput{
67+ requestString: "http://example.com",
68+ useTLS: false,
69+ headers: map[string]string{
70+ "X-Forwarded-Proto": "HTTPS",
71+ },
72+ expectedStatus: 200,
73+ expectedBody: "test",
74+ }),
75+ Entry("with TLS and X-Forwarded-Proto=HTTPS", &requestTableInput{
76+ requestString: "https://example.com",
77+ useTLS: true,
78+ headers: map[string]string{
79+ "X-Forwarded-Proto": "HTTPS",
80+ },
81+ expectedStatus: 200,
82+ expectedBody: "test",
83+ }),
84+ Entry("without TLS and X-Forwarded-Proto=https", &requestTableInput{
85+ requestString: "http://example.com",
86+ useTLS: false,
87+ headers: map[string]string{
88+ "X-Forwarded-Proto": "https",
89+ },
90+ expectedStatus: 200,
91+ expectedBody: "test",
92+ }),
93+ Entry("with TLS and X-Forwarded-Proto=https", &requestTableInput{
94+ requestString: "https://example.com",
95+ useTLS: true,
96+ headers: map[string]string{
97+ "X-Forwarded-Proto": "https",
98+ },
99+ expectedStatus: 200,
100+ expectedBody: "test",
101+ }),
102+ Entry("without TLS and X-Forwarded-Proto=HTTP", &requestTableInput{
103+ requestString: "http://example.com",
104+ useTLS: false,
105+ headers: map[string]string{
106+ "X-Forwarded-Proto": "HTTP",
107+ },
108+ expectedStatus: 308,
109+ expectedBody: permanentRedirectBody("https://example.com"),
110+ expectedLocation: "https://example.com",
111+ }),
112+ Entry("with TLS and X-Forwarded-Proto=HTTP", &requestTableInput{
113+ requestString: "https://example.com",
114+ useTLS: true,
115+ headers: map[string]string{
116+ "X-Forwarded-Proto": "HTTP",
117+ },
118+ expectedStatus: 308,
119+ expectedBody: permanentRedirectBody("https://example.com"),
120+ expectedLocation: "https://example.com",
121+ }),
122+ Entry("without TLS and X-Forwarded-Proto=http", &requestTableInput{
123+ requestString: "https://example.com",
124+ useTLS: false,
125+ headers: map[string]string{
126+ "X-Forwarded-Proto": "http",
127+ },
128+ expectedStatus: 308,
129+ expectedBody: permanentRedirectBody("https://example.com"),
130+ expectedLocation: "https://example.com",
131+ }),
132+ Entry("with TLS and X-Forwarded-Proto=http", &requestTableInput{
133+ requestString: "https://example.com",
134+ useTLS: true,
135+ headers: map[string]string{
136+ "X-Forwarded-Proto": "http",
137+ },
138+ expectedStatus: 308,
139+ expectedBody: permanentRedirectBody("https://example.com"),
140+ expectedLocation: "https://example.com",
141+ }),
142+ Entry("without TLS on a non-standard port", &requestTableInput{
143+ requestString: "http://example.com:8080",
144+ useTLS: false,
145+ headers: map[string]string{},
146+ expectedStatus: 308,
147+ expectedBody: permanentRedirectBody("https://example.com:8443"),
148+ expectedLocation: "https://example.com:8443",
149+ }),
150+ Entry("with TLS on a non-standard port", &requestTableInput{
151+ requestString: "https://example.com:8443",
152+ useTLS: true,
153+ headers: map[string]string{},
154+ expectedStatus: 200,
155+ expectedBody: "test",
156+ }),
157+ )
158+})