From e1764d42215fd99ee675661bbaf7b22f256b8f8e Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Sun, 30 May 2021 09:34:37 +0100
Subject: [PATCH] Create AppDirector for getting the application redirect URL
---
pkg/app/redirect/director.go | 96 ++++++++++++
pkg/app/redirect/director_test.go | 177 ++++++++++++++++++++++
pkg/app/redirect/getters.go | 73 +++++++++
pkg/app/redirect/pagewriter_suite_test.go | 22 +++
pkg/requests/util/util.go | 5 +
5 files changed, 373 insertions(+)
create mode 100644 pkg/app/redirect/director.go
create mode 100644 pkg/app/redirect/director_test.go
create mode 100644 pkg/app/redirect/getters.go
diff --git a/pkg/app/redirect/director.go b/pkg/app/redirect/director.go
new file mode 100644
index 00000000..0ed8e408
--- /dev/null
+++ b/pkg/app/redirect/director.go
@@ -0,0 +1,96 @@
+package redirect
+
+import (
+ "fmt"
+ "net/http"
+ "strings"
+
+ "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
+)
+
+// AppDirector is responsible for determining where OAuth2 Proxy should redirect
+// a users request to after the user has authenticated with the identity provider.
+type AppDirector interface {
+ GetRedirect(req *http.Request) (string, error)
+}
+
+// AppDirectorOpts are the requirements for constructing a new AppDirector.
+type AppDirectorOpts struct {
+ ProxyPrefix string
+ Validator Validator
+}
+
+// NewAppDirector constructs a new AppDirector for getting the application
+// redirect URL.
+func NewAppDirector(opts AppDirectorOpts) AppDirector {
+ prefix := opts.ProxyPrefix
+ if !strings.HasSuffix(prefix, "/") {
+ prefix = fmt.Sprintf("%s/", prefix)
+ }
+
+ return &appDirector{
+ proxyPrefix: prefix,
+ validator: opts.Validator,
+ }
+}
+
+// appDirector implements the AppDirector interface.
+type appDirector struct {
+ proxyPrefix string
+ validator Validator
+}
+
+// GetRedirect determines the full URL or URI path to redirect clients to once
+// authenticated with the OAuthProxy.
+// Strategy priority (first legal result is used):
+// - `rd` querysting parameter
+// - `X-Auth-Request-Redirect` header
+// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
+// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
+// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
+// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
+// - `/`
+func (a *appDirector) GetRedirect(req *http.Request) (string, error) {
+ err := req.ParseForm()
+ if err != nil {
+ return "", err
+ }
+
+ // These redirect getter functions are strategies ordered by priority
+ // for figuring out the redirect URL.
+ for _, rdGetter := range []redirectGetter{
+ a.getRdQuerystringRedirect,
+ a.getXAuthRequestRedirect,
+ a.getXForwardedHeadersRedirect,
+ a.getURIRedirect,
+ } {
+ redirect := rdGetter(req)
+ // Call `p.IsValidRedirect` again here a final time to be safe
+ if redirect != "" && a.validator.IsValidRedirect(redirect) {
+ return redirect, nil
+ }
+ }
+
+ return "/", nil
+}
+
+// validateRedirect checks that the redirect is valid.
+// When an invalid, non-empty redirect is found, an error will be logged using
+// the provided format.
+func (a *appDirector) validateRedirect(redirect string, errorFormat string) string {
+ if a.validator.IsValidRedirect(redirect) {
+ return redirect
+ }
+ if redirect != "" {
+ logger.Errorf(errorFormat, redirect)
+ }
+ return ""
+}
+
+// hasProxyPrefix determines whether the obtained path would be a request to
+// one of OAuth2 Proxy's own endpoints, eg. th callback URL.
+// Redirects to these endpoints should not be allowed as they will create
+// redirection loops.
+func (a *appDirector) hasProxyPrefix(path string) bool {
+ return strings.HasPrefix(path, a.proxyPrefix)
+}
diff --git a/pkg/app/redirect/director_test.go b/pkg/app/redirect/director_test.go
new file mode 100644
index 00000000..55359f52
--- /dev/null
+++ b/pkg/app/redirect/director_test.go
@@ -0,0 +1,177 @@
+package redirect
+
+import (
+ "net/http"
+
+ "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
+ . "github.com/onsi/ginkgo"
+ . "github.com/onsi/ginkgo/extensions/table"
+ . "github.com/onsi/gomega"
+)
+
+const testProxyPrefix = "/oauth2"
+
+var _ = Describe("Director Suite", func() {
+ type getRedirectTableInput struct {
+ requestURL string
+ headers map[string]string
+ reverseProxy bool
+ validator Validator
+ expectedRedirect string
+ }
+
+ DescribeTable("GetRedirect",
+ func(in getRedirectTableInput) {
+ appDirector := NewAppDirector(AppDirectorOpts{
+ ProxyPrefix: testProxyPrefix,
+ Validator: in.validator,
+ })
+
+ req, _ := http.NewRequest("GET", in.requestURL, nil)
+ for header, value := range in.headers {
+ if value != "" {
+ req.Header.Add(header, value)
+ }
+ }
+ req = middleware.AddRequestScope(req, &middleware.RequestScope{
+ ReverseProxy: in.reverseProxy,
+ })
+
+ redirect, err := appDirector.GetRedirect(req)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(redirect).To(Equal(in.expectedRedirect))
+ },
+ Entry("Request outside of the proxy prefix, redirects to original request", getRedirectTableInput{
+ requestURL: "/foo/bar",
+ headers: nil,
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "/foo/bar",
+ }),
+ Entry("Request with query, preserves the query", getRedirectTableInput{
+ requestURL: "/foo?bar",
+ headers: nil,
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "/foo?bar",
+ }),
+ Entry("Request under the proxy prefix, redirects to root", getRedirectTableInput{
+ requestURL: testProxyPrefix + "/foo/bar",
+ headers: nil,
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "/",
+ }),
+ Entry("Proxied request with headers, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com",
+ "X-Forwarded-Uri": "/foo/bar",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo/bar",
+ }),
+ Entry("Non-proxied request with spoofed headers, wouldn't redirect", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo?bar",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com",
+ "X-Forwarded-Uri": "/foo/bar",
+ },
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "/foo?bar",
+ }),
+ Entry("Proxied request with headers, under ProxyPrefix, redirects to root", getRedirectTableInput{
+ requestURL: "https://oauth.example.com" + testProxyPrefix + "/foo/bar",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com",
+ "X-Forwarded-Uri": testProxyPrefix + "/foo/bar",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/",
+ }),
+ Entry("Proxied request with port, under ProxyPrefix, redirects to root", getRedirectTableInput{
+ requestURL: "https://oauth.example.com" + testProxyPrefix + "/foo/bar",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com:8443",
+ "X-Forwarded-Uri": testProxyPrefix + "/foo/bar",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com:8443/",
+ }),
+ Entry("Proxied request with headers, missing URI header, redirects to the desired redirect URL", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo?bar",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo?bar",
+ }),
+ Entry("Proxied request without headers, with reverse proxy enabled, redirects to the desired URL", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo?bar",
+ headers: nil,
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "/foo?bar",
+ }),
+ Entry("Proxied request with X-Auth-Request-Redirect, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar",
+ headers: map[string]string{
+ "X-Auth-Request-Redirect": "https://a-service.example.com/foo/bar",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo/bar",
+ }),
+ Entry("Proxied request with RD parameter, outside of ProxyPrefix, redirects to proxied URL", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fbar",
+ headers: nil,
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo/bar",
+ }),
+ Entry("Proxied request with RD parameter and all headers set, reverse proxy disabled, redirects to proxied URL based on the RD parameter", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+ headers: map[string]string{
+ "X-Auth-Request-Redirect": "https://a-service.example.com/foo/baz",
+ "X-Forwarded-Proto": "http",
+ "X-Forwarded-Host": "another-service.example.com",
+ "X-Forwarded-Uri": "/seasons/greetings",
+ },
+ reverseProxy: false,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo/jazz",
+ }),
+ Entry("Proxied request with RD parameter and some headers set, reverse proxy enabled, redirects to proxied URL based on the RD parameter", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar?rd=https%3A%2F%2Fa%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "http",
+ "X-Forwarded-Host": "another-service.example.com",
+ "X-Forwarded-Uri": "/seasons/greetings",
+ },
+ reverseProxy: true,
+ validator: testValidator(true),
+ expectedRedirect: "https://a-service.example.com/foo/jazz",
+ }),
+ Entry("Proxied request with invalid RD parameter and some headers set, reverse proxy enabled, redirects to proxied URL based on the headers", getRedirectTableInput{
+ requestURL: "https://oauth.example.com/foo/bar?rd=http%3A%2F%2Fanother%2Dservice%2Eexample%2Ecom%2Ffoo%2Fjazz",
+ headers: map[string]string{
+ "X-Forwarded-Proto": "https",
+ "X-Forwarded-Host": "a-service.example.com",
+ "X-Forwarded-Uri": "/foo/bar",
+ },
+ reverseProxy: true,
+ validator: testValidator(false, "https://a-service.example.com/foo/bar"),
+ expectedRedirect: "https://a-service.example.com/foo/bar",
+ }),
+ )
+})
diff --git a/pkg/app/redirect/getters.go b/pkg/app/redirect/getters.go
new file mode 100644
index 00000000..5240abeb
--- /dev/null
+++ b/pkg/app/redirect/getters.go
@@ -0,0 +1,73 @@
+package redirect
+
+import (
+ "fmt"
+ "net/http"
+
+ requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util"
+)
+
+// redirectGetter represents a method to allow the proxy to determine a redirect
+// based on the original request.
+type redirectGetter func(req *http.Request) string
+
+// getRdQuerystringRedirect handles this getAppRedirect strategy:
+// - `rd` querysting parameter
+func (a *appDirector) getRdQuerystringRedirect(req *http.Request) string {
+ return a.validateRedirect(
+ req.Form.Get("rd"),
+ "Invalid redirect provided in rd querystring parameter: %s",
+ )
+}
+
+// getXAuthRequestRedirect handles this getAppRedirect strategy:
+// - `X-Auth-Request-Redirect` Header
+func (a *appDirector) getXAuthRequestRedirect(req *http.Request) string {
+ return a.validateRedirect(
+ req.Header.Get("X-Auth-Request-Redirect"),
+ "Invalid redirect provided in X-Auth-Request-Redirect header: %s",
+ )
+}
+
+// getXForwardedHeadersRedirect handles these getAppRedirect strategies:
+// - `X-Forwarded-(Proto|Host|Uri)` headers (when ReverseProxy mode is enabled)
+// - `X-Forwarded-(Proto|Host)` if `Uri` has the ProxyPath (i.e. /oauth2/*)
+func (a *appDirector) getXForwardedHeadersRedirect(req *http.Request) string {
+ if !requestutil.IsForwardedRequest(req) {
+ return ""
+ }
+
+ uri := requestutil.GetRequestURI(req)
+ if a.hasProxyPrefix(uri) {
+ uri = "/"
+ }
+
+ redirect := fmt.Sprintf(
+ "%s://%s%s",
+ requestutil.GetRequestProto(req),
+ requestutil.GetRequestHost(req),
+ uri,
+ )
+
+ return a.validateRedirect(redirect,
+ "Invalid redirect generated from X-Forwarded-* headers: %s")
+}
+
+// getURIRedirect handles these getAppRedirect strategies:
+// - `X-Forwarded-Uri` direct URI path (when ReverseProxy mode is enabled)
+// - `req.URL.RequestURI` if not under the ProxyPath (i.e. /oauth2/*)
+// - `/`
+func (a *appDirector) getURIRedirect(req *http.Request) string {
+ redirect := a.validateRedirect(
+ requestutil.GetRequestURI(req),
+ "Invalid redirect generated from X-Forwarded-Uri header: %s",
+ )
+ if redirect == "" {
+ redirect = req.URL.RequestURI()
+ }
+
+ if a.hasProxyPrefix(redirect) {
+ return "/"
+ }
+ return redirect
+}
diff --git a/pkg/app/redirect/pagewriter_suite_test.go b/pkg/app/redirect/pagewriter_suite_test.go
index 45703ff3..71301f58 100644
--- a/pkg/app/redirect/pagewriter_suite_test.go
+++ b/pkg/app/redirect/pagewriter_suite_test.go
@@ -15,3 +15,25 @@ func TestOptionsSuite(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Redirect Suite")
}
+
+// testValidator creates a mock validator that will always return the given result.
+func testValidator(result bool, allowedRedirects ...string) Validator {
+ return &mockValidator{result: result, allowedRedirects: allowedRedirects}
+}
+
+// mockValidator implements the Validator interface for use in testing.
+type mockValidator struct {
+ result bool
+ allowedRedirects []string
+}
+
+// IsValidRedirect implements the Validator interface.
+func (m *mockValidator) IsValidRedirect(redirect string) bool {
+ for _, allowed := range m.allowedRedirects {
+ if redirect == allowed {
+ return true
+ }
+ }
+
+ return m.result
+}
diff --git a/pkg/requests/util/util.go b/pkg/requests/util/util.go
index 9805db61..44e1ab0e 100644
--- a/pkg/requests/util/util.go
+++ b/pkg/requests/util/util.go
@@ -52,3 +52,8 @@ func IsProxied(req *http.Request) bool {
}
return scope.ReverseProxy
}
+
+func IsForwardedRequest(req *http.Request) bool {
+ return IsProxied(req) &&
+ req.Host != GetRequestHost(req)
+}