git.iko.soy/typeable/oauth2-proxy
on master help sign in
Move RedirectToHTTPS to middleware package

Moves the logic for redirecting to HTTPs to a middleware package and adds tests for this logic. Also makes the functionality more useful, previously it always redirected to the HTTPS address of the proxy, which may not have been intended, now it will redirect based on if a port is provided in the URL (assume public facing 80 to 443 or 4180 to 8443 for example)

commit1c1106721eff8f2414a0e87141b80494a36a9f6c authorJoel Speed <joel.speed@hotmail.co.uk> dateFri 12 Jun 2020, 18:18 (6 years ago) parent39c01d5
7 files changed+222−70
compare with master
unifiedsplit
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+})