From ae72beb24ef94eef4cf31113c5910c884a368965 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 12:46:26 +0000 Subject: [PATCH 1/7] Enable UseEncodedPath() for frontend mux This allows urls with encoded characters (e.g.: /%2F/) to pass to the upstream mux instead of triggering a HTTP 301 from the frontend. Otherwise a /%2F/test/ will result in a HTTP 301 -> /test/ --- oauthproxy.go | 4 +++- oauthproxy_test.go | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/oauthproxy.go b/oauthproxy.go index e2d20ed6..fb6ef0bc 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -265,7 +265,9 @@ func (p *OAuthProxy) setupServer(opts *options.Options) error { } func (p *OAuthProxy) buildServeMux(proxyPrefix string) { - r := mux.NewRouter() + // Use the encoded path here so we can have the option to pass it on in the upstream mux. + // Otherwise something like /%2F/ would be redirected to / here already. + r := mux.NewRouter().UseEncodedPath() // Everything served by the router must go through the preAuthChain first. r.Use(p.preAuthChain.Then) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index cb1dceed..3a795f18 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -915,6 +915,15 @@ func TestUserInfoEndpointUnauthorizedOnNoCookieSetError(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, test.rw.Code) } +func TestEncodedUrlsStayEncoded(t *testing.T) { + encodeTest, err := NewSignInPageTest(false) + if err != nil { + t.Fatal(err) + } + code, _ := encodeTest.GetEndpoint("/%2F/test1/%2F/test2") + assert.Equal(t, 403, code) +} + func NewAuthOnlyEndpointTest(querystring string, modifiers ...OptionsModifier) (*ProcessCookieTest, error) { pcTest, err := NewProcessCookieTestWithOptionsModifiers(modifiers...) if err != nil { From 12ab4ef52958ab18d1156cd1cee0618ed38efe32 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 13:32:15 +0000 Subject: [PATCH 2/7] Make the Upstreams mux configurable This commit changes Upstreams from []Upstream to a struct{} moving the previous []Upstream into .Configs and adjusts all uses of it. --- main_test.go | 21 ++-- oauthproxy_test.go | 82 ++++++++----- pkg/apis/options/legacy_options.go | 4 +- pkg/apis/options/legacy_options_test.go | 80 +++++++------ pkg/apis/options/load_test.go | 23 ++-- pkg/apis/options/upstreams.go | 6 +- pkg/upstream/proxy.go | 4 +- pkg/upstream/proxy_test.go | 152 ++++++++++++------------ pkg/validation/options_test.go | 2 +- pkg/validation/upstreams.go | 2 +- pkg/validation/upstreams_test.go | 138 ++++++++++++--------- 11 files changed, 283 insertions(+), 231 deletions(-) diff --git a/main_test.go b/main_test.go index e40243ef..b8f7b488 100644 --- a/main_test.go +++ b/main_test.go @@ -25,6 +25,7 @@ client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" const testAlphaConfig = ` upstreams: + configs: - id: / path: / uri: http://httpbin @@ -101,13 +102,15 @@ redirect_url="http://localhost:4180/oauth2/callback" opts.RawRedirectURL = "http://localhost:4180/oauth2/callback" opts.UpstreamServers = options.Upstreams{ - { - ID: "/", - Path: "/", - URI: "http://httpbin", - FlushInterval: durationPtr(options.DefaultUpstreamFlushInterval), - PassHostHeader: boolPtr(true), - ProxyWebSockets: boolPtr(true), + Configs: []options.Upstream{ + { + ID: "/", + Path: "/", + URI: "http://httpbin", + FlushInterval: durationPtr(options.DefaultUpstreamFlushInterval), + PassHostHeader: boolPtr(true), + ProxyWebSockets: boolPtr(true), + }, }, } @@ -130,7 +133,7 @@ redirect_url="http://localhost:4180/oauth2/callback" opts.InjectResponseHeaders = append(opts.InjectResponseHeaders, authHeader) opts.Providers = options.Providers{ - { + options.Provider{ ID: "google=oauth2-proxy", Type: "google", ClientSecret: "b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK", @@ -230,7 +233,7 @@ redirect_url="http://localhost:4180/oauth2/callback" configContent: testCoreConfig, alphaConfigContent: testAlphaConfig + ":", expectedOptions: func() *options.Options { return nil }, - expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 49: did not find expected key"), + expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 50: did not find expected key"), }), Entry("with alpha configuration and bad core configuration", loadConfigurationTableInput{ configContent: testCoreConfig + "unknown_field=\"something\"", diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 3a795f18..d0eaaa4d 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -198,10 +198,12 @@ func TestBasicAuthPassword(t *testing.T) { basicAuthPassword := "This is a secure password" opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ - { - ID: providerServer.URL, - Path: "/", - URI: providerServer.URL, + Configs: []options.Upstream{ + { + ID: providerServer.URL, + Path: "/", + URI: providerServer.URL, + }, }, } @@ -347,14 +349,16 @@ func NewPassAccessTokenTest(opts PassAccessTokenTestOptions) (*PassAccessTokenTe patt.opts = baseTestOptions() patt.opts.UpstreamServers = options.Upstreams{ - { - ID: patt.providerServer.URL, - Path: "/", - URI: patt.providerServer.URL, + Configs: []options.Upstream{ + { + ID: patt.providerServer.URL, + Path: "/", + URI: patt.providerServer.URL, + }, }, } if opts.ProxyUpstream.ID != "" { - patt.opts.UpstreamServers = append(patt.opts.UpstreamServers, opts.ProxyUpstream) + patt.opts.UpstreamServers.Configs = append(patt.opts.UpstreamServers.Configs, opts.ProxyUpstream) } patt.opts.Cookie.Secure = false @@ -1270,10 +1274,12 @@ func TestAuthSkippedForPreflightRequests(t *testing.T) { opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + Configs: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthPreflight = true @@ -1345,10 +1351,12 @@ func NewSignatureTest() (*SignatureTest, error) { return nil, err } opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + Configs: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } @@ -1781,10 +1789,12 @@ func Test_noCacheHeaders(t *testing.T) { opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + Configs: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthRegex = []string{".*"} @@ -2051,10 +2061,12 @@ func TestTrustedIPs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ - { - ID: "static", - Path: "/", - Static: true, + Configs: []options.Upstream{ + { + ID: "static", + Path: "/", + Static: true, + }, }, } opts.TrustedIPs = tt.trustedIPs @@ -2244,10 +2256,12 @@ func TestAllowedRequest(t *testing.T) { opts := baseTestOptions() opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + Configs: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthRegex = []string{ @@ -2359,10 +2373,12 @@ func TestProxyAllowedGroups(t *testing.T) { test, err := NewProcessCookieTestWithOptionsModifiers(func(opts *options.Options) { opts.Providers[0].AllowedGroups = tt.allowedGroups opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + Configs: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } }) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index cf67dedd..fb51feed 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -120,7 +120,7 @@ func (l *LegacyUpstreams) convert() (Upstreams, error) { for _, upstreamString := range l.Upstreams { u, err := url.Parse(upstreamString) if err != nil { - return nil, fmt.Errorf("could not parse upstream %q: %v", upstreamString, err) + return Upstreams{}, fmt.Errorf("could not parse upstream %q: %v", upstreamString, err) } if u.Path == "" { @@ -169,7 +169,7 @@ func (l *LegacyUpstreams) convert() (Upstreams, error) { upstream.FlushInterval = nil } - upstreams = append(upstreams, upstream) + upstreams.Configs = append(upstreams.Configs, upstream) } return upstreams, nil diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go index fb5b816a..88c32817 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -27,34 +27,36 @@ var _ = Describe("Legacy Options", func() { truth := true staticCode := 204 opts.UpstreamServers = Upstreams{ - { - ID: "/baz", - Path: "/baz", - URI: "http://foo.bar/baz", - FlushInterval: &flushInterval, - InsecureSkipTLSVerify: true, - PassHostHeader: &truth, - ProxyWebSockets: &truth, - }, - { - ID: "/bar", - Path: "/bar", - URI: "file:///var/lib/website", - FlushInterval: &flushInterval, - InsecureSkipTLSVerify: true, - PassHostHeader: &truth, - ProxyWebSockets: &truth, - }, - { - ID: "static://204", - Path: "/", - URI: "", - Static: true, - StaticCode: &staticCode, - FlushInterval: nil, - InsecureSkipTLSVerify: false, - PassHostHeader: nil, - ProxyWebSockets: nil, + Configs: []Upstream{ + { + ID: "/baz", + Path: "/baz", + URI: "http://foo.bar/baz", + FlushInterval: &flushInterval, + InsecureSkipTLSVerify: true, + PassHostHeader: &truth, + ProxyWebSockets: &truth, + }, + { + ID: "/bar", + Path: "/bar", + URI: "file:///var/lib/website", + FlushInterval: &flushInterval, + InsecureSkipTLSVerify: true, + PassHostHeader: &truth, + ProxyWebSockets: &truth, + }, + { + ID: "static://204", + Path: "/", + URI: "", + Static: true, + StaticCode: &staticCode, + FlushInterval: nil, + InsecureSkipTLSVerify: false, + PassHostHeader: nil, + ProxyWebSockets: nil, + }, }, } @@ -124,7 +126,7 @@ var _ = Describe("Legacy Options", func() { Context("Legacy Upstreams", func() { type convertUpstreamsTableInput struct { upstreamStrings []string - expectedUpstreams Upstreams + expectedUpstreams []Upstream errMsg string } @@ -219,51 +221,51 @@ var _ = Describe("Legacy Options", func() { Expect(err).ToNot(HaveOccurred()) } - Expect(upstreams).To(ConsistOf(in.expectedUpstreams)) + Expect(upstreams.Configs).To(ConsistOf(in.expectedUpstreams)) }, Entry("with no upstreams", &convertUpstreamsTableInput{ upstreamStrings: []string{}, - expectedUpstreams: Upstreams{}, + expectedUpstreams: []Upstream{}, errMsg: "", }), Entry("with a valid HTTP upstream", &convertUpstreamsTableInput{ upstreamStrings: []string{validHTTP}, - expectedUpstreams: Upstreams{validHTTPUpstream}, + expectedUpstreams: []Upstream{validHTTPUpstream}, errMsg: "", }), Entry("with a HTTP upstream with an empty path", &convertUpstreamsTableInput{ upstreamStrings: []string{emptyPathHTTP}, - expectedUpstreams: Upstreams{emptyPathHTTPUpstream}, + expectedUpstreams: []Upstream{emptyPathHTTPUpstream}, errMsg: "", }), Entry("with a valid File upstream with a fragment", &convertUpstreamsTableInput{ upstreamStrings: []string{validFileWithFragment}, - expectedUpstreams: Upstreams{validFileWithFragmentUpstream}, + expectedUpstreams: []Upstream{validFileWithFragmentUpstream}, errMsg: "", }), Entry("with a valid static upstream", &convertUpstreamsTableInput{ upstreamStrings: []string{validStatic}, - expectedUpstreams: Upstreams{validStaticUpstream}, + expectedUpstreams: []Upstream{validStaticUpstream}, errMsg: "", }), Entry("with an invalid static upstream, code is 200", &convertUpstreamsTableInput{ upstreamStrings: []string{invalidStatic}, - expectedUpstreams: Upstreams{invalidStaticUpstream}, + expectedUpstreams: []Upstream{invalidStaticUpstream}, errMsg: "", }), Entry("with an invalid HTTP upstream", &convertUpstreamsTableInput{ upstreamStrings: []string{invalidHTTP}, - expectedUpstreams: Upstreams{}, + expectedUpstreams: []Upstream{}, errMsg: invalidHTTPErrMsg, }), Entry("with an invalid HTTP upstream and other upstreams", &convertUpstreamsTableInput{ upstreamStrings: []string{validHTTP, invalidHTTP}, - expectedUpstreams: Upstreams{}, + expectedUpstreams: []Upstream{}, errMsg: invalidHTTPErrMsg, }), Entry("with multiple valid upstreams", &convertUpstreamsTableInput{ upstreamStrings: []string{validHTTP, validFileWithFragment, validStatic}, - expectedUpstreams: Upstreams{validHTTPUpstream, validFileWithFragmentUpstream, validStaticUpstream}, + expectedUpstreams: []Upstream{validHTTPUpstream, validFileWithFragmentUpstream, validStaticUpstream}, errMsg: "", }), ) diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index dce5d2d9..ca4918b2 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -470,10 +470,11 @@ sub: It("should load a full example AlphaOptions", func() { config := []byte(` upstreams: -- id: httpbin - path: / - uri: http://httpbin - flushInterval: 500ms + configs: + - id: httpbin + path: / + uri: http://httpbin + flushInterval: 500ms injectRequestHeaders: - name: X-Forwarded-User values: @@ -502,12 +503,14 @@ injectResponseHeaders: flushInterval := Duration(500 * time.Millisecond) Expect(into).To(Equal(&AlphaOptions{ - Upstreams: []Upstream{ - { - ID: "httpbin", - Path: "/", - URI: "http://httpbin", - FlushInterval: &flushInterval, + Upstreams: Upstreams{ + Configs: []Upstream{ + { + ID: "httpbin", + Path: "/", + URI: "http://httpbin", + FlushInterval: &flushInterval, + }, }, }, InjectRequestHeaders: []Header{ diff --git a/pkg/apis/options/upstreams.go b/pkg/apis/options/upstreams.go index 1977c8da..368da3ed 100644 --- a/pkg/apis/options/upstreams.go +++ b/pkg/apis/options/upstreams.go @@ -8,7 +8,11 @@ const ( ) // Upstreams is a collection of definitions for upstream servers. -type Upstreams []Upstream +type Upstreams struct { + // Upstream represents the configuration for an upstream server. + // Requests will be proxied to this upstream if the path matches the request path. + Configs []Upstream `json:"configs,omitempty"` +} // Upstream represents the configuration for an upstream server. // Requests will be proxied to this upstream if the path matches the request path. diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index 594824e2..a7e19b63 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -27,7 +27,7 @@ func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, write serveMux: mux.NewRouter(), } - for _, upstream := range sortByPathLongest(upstreams) { + for _, upstream := range sortByPathLongest(upstreams.Configs) { if upstream.Static { if err := m.registerStaticResponseHandler(upstream, writer); err != nil { return nil, fmt.Errorf("could not register static upstream %q: %v", upstream.ID, err) @@ -153,7 +153,7 @@ func registerTrailingSlashHandler(serveMux *mux.Router) { // precedence (note this is the input to the rewrite logic). // This does not account for when a rewrite would actually make the path shorter. // This should maintain the sorting behaviour of the standard go serve mux. -func sortByPathLongest(in options.Upstreams) options.Upstreams { +func sortByPathLongest(in []options.Upstream) []options.Upstream { sort.Slice(in, func(i, j int) bool { iRW := in[i].RewriteTarget jRW := in[j].RewriteTarget diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index 597f5422..ae73f489 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -33,61 +33,63 @@ var _ = Describe("Proxy Suite", func() { accepted := http.StatusAccepted upstreams := options.Upstreams{ - { - ID: "http-backend", - Path: "/http/", - URI: serverAddr, - }, - { - ID: "file-backend", - Path: "/files/", - URI: fmt.Sprintf("file:///%s", filesDir), - }, - { - ID: "static-backend", - Path: "/static/", - Static: true, - StaticCode: &ok, - }, - { - ID: "static-backend-no-trailing-slash", - Path: "/static", - Static: true, - StaticCode: &accepted, - }, - { - ID: "static-backend-long", - Path: "/static/long", - Static: true, - StaticCode: &accepted, - }, - { - ID: "bad-http-backend", - Path: "/bad-http/", - URI: "http://::1", - }, - { - ID: "single-path-backend", - Path: "/single-path", - Static: true, - StaticCode: &ok, - }, - { - ID: "backend-with-rewrite-prefix", - Path: "^/rewrite-prefix/(.*)", - RewriteTarget: "/different/backend/path/$1", - URI: serverAddr, - }, - { - ID: "double-match-plain", - Path: "/double-match/", - URI: serverAddr, - }, - { - ID: "double-match-rewrite", - Path: "^/double-match/(.*)", - RewriteTarget: "/double-match/rewrite/$1", - URI: serverAddr, + Configs: []options.Upstream{ + { + ID: "http-backend", + Path: "/http/", + URI: serverAddr, + }, + { + ID: "file-backend", + Path: "/files/", + URI: fmt.Sprintf("file:///%s", filesDir), + }, + { + ID: "static-backend", + Path: "/static/", + Static: true, + StaticCode: &ok, + }, + { + ID: "static-backend-no-trailing-slash", + Path: "/static", + Static: true, + StaticCode: &accepted, + }, + { + ID: "static-backend-long", + Path: "/static/long", + Static: true, + StaticCode: &accepted, + }, + { + ID: "bad-http-backend", + Path: "/bad-http/", + URI: "http://::1", + }, + { + ID: "single-path-backend", + Path: "/single-path", + Static: true, + StaticCode: &ok, + }, + { + ID: "backend-with-rewrite-prefix", + Path: "^/rewrite-prefix/(.*)", + RewriteTarget: "/different/backend/path/$1", + URI: serverAddr, + }, + { + ID: "double-match-plain", + Path: "/double-match/", + URI: serverAddr, + }, + { + ID: "double-match-rewrite", + Path: "^/double-match/(.*)", + RewriteTarget: "/double-match/rewrite/$1", + URI: serverAddr, + }, }, } @@ -315,8 +317,8 @@ var _ = Describe("Proxy Suite", func() { Context("sortByPathLongest", func() { type sortByPathLongestTableInput struct { - input options.Upstreams - expectedOutput options.Upstreams + input []options.Upstream + expectedOutput []options.Upstream } var httpPath = options.Upstream{ @@ -346,40 +348,40 @@ var _ = Describe("Proxy Suite", func() { Expect(sortByPathLongest(in.input)).To(Equal(in.expectedOutput)) }, Entry("with a mix of paths registered", sortByPathLongestTableInput{ - input: options.Upstreams{httpPath, httpSubPath, shortSubPathWithRewrite, longerPath, shortPathWithRewrite}, - expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite, longerPath, httpSubPath, httpPath}, + input: []options.Upstream{httpPath, httpSubPath, shortSubPathWithRewrite, longerPath, shortPathWithRewrite}, + expectedOutput: []options.Upstream{shortSubPathWithRewrite, shortPathWithRewrite, longerPath, httpSubPath, httpPath}, }), Entry("when a subpath is registered (in order)", sortByPathLongestTableInput{ - input: options.Upstreams{httpSubPath, httpPath}, - expectedOutput: options.Upstreams{httpSubPath, httpPath}, + input: []options.Upstream{httpSubPath, httpPath}, + expectedOutput: []options.Upstream{httpSubPath, httpPath}, }), Entry("when a subpath is registered (out of order)", sortByPathLongestTableInput{ - input: options.Upstreams{httpPath, httpSubPath}, - expectedOutput: options.Upstreams{httpSubPath, httpPath}, + input: []options.Upstream{httpPath, httpSubPath}, + expectedOutput: []options.Upstream{httpSubPath, httpPath}, }), Entry("when longer paths are registered (in order)", sortByPathLongestTableInput{ - input: options.Upstreams{longerPath, httpPath}, - expectedOutput: options.Upstreams{longerPath, httpPath}, + input: []options.Upstream{longerPath, httpPath}, + expectedOutput: []options.Upstream{longerPath, httpPath}, }), Entry("when longer paths are registered (out of order)", sortByPathLongestTableInput{ - input: options.Upstreams{httpPath, longerPath}, - expectedOutput: options.Upstreams{longerPath, httpPath}, + input: []options.Upstream{httpPath, longerPath}, + expectedOutput: []options.Upstream{longerPath, httpPath}, }), Entry("when a rewrite target is registered (in order)", sortByPathLongestTableInput{ - input: options.Upstreams{shortPathWithRewrite, longerPath}, - expectedOutput: options.Upstreams{shortPathWithRewrite, longerPath}, + input: []options.Upstream{shortPathWithRewrite, longerPath}, + expectedOutput: []options.Upstream{shortPathWithRewrite, longerPath}, }), Entry("when a rewrite target is registered (out of order)", sortByPathLongestTableInput{ - input: options.Upstreams{longerPath, shortPathWithRewrite}, - expectedOutput: options.Upstreams{shortPathWithRewrite, longerPath}, + input: []options.Upstream{longerPath, shortPathWithRewrite}, + expectedOutput: []options.Upstream{shortPathWithRewrite, longerPath}, }), Entry("with multiple rewrite targets registered (in order)", sortByPathLongestTableInput{ - input: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite}, - expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite}, + input: []options.Upstream{shortSubPathWithRewrite, shortPathWithRewrite}, + expectedOutput: []options.Upstream{shortSubPathWithRewrite, shortPathWithRewrite}, }), Entry("with multiple rewrite targets registered (out of order)", sortByPathLongestTableInput{ - input: options.Upstreams{shortPathWithRewrite, shortSubPathWithRewrite}, - expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite}, + input: []options.Upstream{shortPathWithRewrite, shortSubPathWithRewrite}, + expectedOutput: []options.Upstream{shortSubPathWithRewrite, shortPathWithRewrite}, }), ) }) diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 5f9e3716..70387a14 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -22,7 +22,7 @@ const ( func testOptions() *options.Options { o := options.NewOptions() - o.UpstreamServers = append(o.UpstreamServers, options.Upstream{ + o.UpstreamServers.Configs = append(o.UpstreamServers.Configs, options.Upstream{ ID: "upstream", Path: "/", URI: "http://127.0.0.1:8080/", diff --git a/pkg/validation/upstreams.go b/pkg/validation/upstreams.go index 7bd6b2d5..f45f3b94 100644 --- a/pkg/validation/upstreams.go +++ b/pkg/validation/upstreams.go @@ -12,7 +12,7 @@ func validateUpstreams(upstreams options.Upstreams) []string { ids := make(map[string]struct{}) paths := make(map[string]struct{}) - for _, upstream := range upstreams { + for _, upstream := range upstreams.Configs { msgs = append(msgs, validateUpstream(upstream, ids, paths)...) } diff --git a/pkg/validation/upstreams_test.go b/pkg/validation/upstreams_test.go index 122286ad..1c974024 100644 --- a/pkg/validation/upstreams_test.go +++ b/pkg/validation/upstreams_test.go @@ -59,83 +59,99 @@ var _ = Describe("Upstreams", func() { }), Entry("with valid upstreams", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - validHTTPUpstream, - validStaticUpstream, - validFileUpstream, + Configs: []options.Upstream{ + validHTTPUpstream, + validStaticUpstream, + validFileUpstream, + }, }, errStrings: []string{}, }), Entry("with an empty ID", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "", - Path: "/foo", - URI: "http://localhost:8080", + Configs: []options.Upstream{ + { + ID: "", + Path: "/foo", + URI: "http://localhost:8080", + }, }, }, errStrings: []string{emptyIDMsg}, }), Entry("with an empty Path", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "", - URI: "http://localhost:8080", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "", + URI: "http://localhost:8080", + }, }, }, errStrings: []string{emptyPathMsg}, }), Entry("with an empty Path", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "", - URI: "http://localhost:8080", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "", + URI: "http://localhost:8080", + }, }, }, errStrings: []string{emptyPathMsg}, }), Entry("with an empty URI", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - URI: "", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: "", + }, }, }, errStrings: []string{emptyURIMsg}, }), Entry("with an invalid URI", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - URI: ":", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: ":", + }, }, }, errStrings: []string{invalidURIMsg}, }), Entry("with an invalid URI scheme", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - URI: "ftp://foo", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: "ftp://foo", + }, }, }, errStrings: []string{invalidURISchemeMsg}, }), Entry("with a static upstream and invalid optons", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - URI: "ftp://foo", - Static: true, - FlushInterval: &flushInterval, - PassHostHeader: &truth, - ProxyWebSockets: &truth, - InsecureSkipTLSVerify: true, + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: "ftp://foo", + Static: true, + FlushInterval: &flushInterval, + PassHostHeader: &truth, + ProxyWebSockets: &truth, + InsecureSkipTLSVerify: true, + }, }, }, errStrings: []string{ @@ -148,40 +164,46 @@ var _ = Describe("Upstreams", func() { }), Entry("with duplicate IDs", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo1", - URI: "http://foo", - }, - { - ID: "foo", - Path: "/foo2", - URI: "http://foo", + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo1", + URI: "http://foo", + }, + { + ID: "foo", + Path: "/foo2", + URI: "http://foo", + }, }, }, errStrings: []string{multipleIDsMsg}, }), Entry("with duplicate Paths", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo1", - Path: "/foo", - URI: "http://foo", - }, - { - ID: "foo2", - Path: "/foo", - URI: "http://foo", + Configs: []options.Upstream{ + { + ID: "foo1", + Path: "/foo", + URI: "http://foo", + }, + { + ID: "foo2", + Path: "/foo", + URI: "http://foo", + }, }, }, errStrings: []string{multiplePathsMsg}, }), Entry("when a static code is supplied without static", &validateUpstreamTableInput{ upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - StaticCode: &staticCode200, + Configs: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + StaticCode: &staticCode200, + }, }, }, errStrings: []string{emptyURIMsg, staticCodeMsg}, From 733b3fe642c5149b80fb0785240e4ff84ffcb5d2 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 14:01:41 +0000 Subject: [PATCH 3/7] Determine line count for yaml load test dynamically Adding a new option to the yaml alpha config will result in failed tests unless you manually increment the line count. This commit computes this dynamically. --- main_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/main_test.go b/main_test.go index b8f7b488..7f7e1e3b 100644 --- a/main_test.go +++ b/main_test.go @@ -2,8 +2,10 @@ package main import ( "errors" + "fmt" "io/ioutil" "os" + "strings" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" @@ -233,7 +235,7 @@ redirect_url="http://localhost:4180/oauth2/callback" configContent: testCoreConfig, alphaConfigContent: testAlphaConfig + ":", expectedOptions: func() *options.Options { return nil }, - expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 50: did not find expected key"), + expectedErr: fmt.Errorf("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line %d: did not find expected key", strings.Count(testAlphaConfig, "\n")), }), Entry("with alpha configuration and bad core configuration", loadConfigurationTableInput{ configContent: testCoreConfig + "unknown_field=\"something\"", From d51556515e73ed6e3245cabf0e2c5196e62489ee Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 14:57:40 +0000 Subject: [PATCH 4/7] Introduce ProxyRawPath flag Setting this flag will configure the upstream proxy to pass encoded urls as-is. --- CHANGELOG.md | 1 + main_test.go | 1 + pkg/apis/options/upstreams.go | 4 ++++ pkg/upstream/proxy.go | 4 ++++ 4 files changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc1f773a..893f2922 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ - [#1317](https://github.com/oauth2-proxy/oauth2-proxy/pull/1317) Fix incorrect `` tag on the sing_in page when *not* using a custom template (@jord1e) - [#1330](https://github.com/oauth2-proxy/oauth2-proxy/pull/1330) Allow specifying URL as input for custom sign in logo (@MaikuMori) - [#1357](https://github.com/oauth2-proxy/oauth2-proxy/pull/1357) Fix unsafe access to session variable (@harzallah) +- [#997](https://github.com/oauth2-proxy/oauth2-proxy/pull/997) Allow passing the raw url path when proxying upstream requests - e.g. /%2F/ (@FStelzer) # V7.1.3 diff --git a/main_test.go b/main_test.go index 7f7e1e3b..dc7dcb82 100644 --- a/main_test.go +++ b/main_test.go @@ -27,6 +27,7 @@ client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" const testAlphaConfig = ` upstreams: + proxyrawpath: false configs: - id: / path: / diff --git a/pkg/apis/options/upstreams.go b/pkg/apis/options/upstreams.go index 368da3ed..4e1a0547 100644 --- a/pkg/apis/options/upstreams.go +++ b/pkg/apis/options/upstreams.go @@ -9,6 +9,10 @@ const ( // Upstreams is a collection of definitions for upstream servers. type Upstreams struct { + // ProxyRawPath will pass the raw url path to upstream allowing for url's + // like: "/%2F/" which would otherwise be redirected to "/" + ProxyRawPath bool `json:"proxyRawPath,omitempty"` + // Upstream represents the configuration for an upstream server. // Requests will be proxied to this upstream if the path matches the request path. Configs []Upstream `json:"configs,omitempty"` diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index a7e19b63..27bd5229 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -27,6 +27,10 @@ func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, write serveMux: mux.NewRouter(), } + if upstreams.ProxyRawPath { + m.serveMux.UseEncodedPath() + } + for _, upstream := range sortByPathLongest(upstreams.Configs) { if upstream.Static { if err := m.registerStaticResponseHandler(upstream, writer); err != nil { From 662fa72e8c7bd52bad5c1200f9e7a9b07ee3b853 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 14:58:48 +0000 Subject: [PATCH 5/7] Add ProxyRawPath tests Refactor proxy_test to set mux/upstream options for each test individually and add tests for encoded urls with ProxyRawPath set and unset. --- pkg/upstream/proxy_test.go | 215 +++++++++++++++++++++---------------- 1 file changed, 120 insertions(+), 95 deletions(-) diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index ae73f489..2745e680 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -16,96 +16,94 @@ import ( ) var _ = Describe("Proxy Suite", func() { - var upstreamServer http.Handler + type proxyTableInput struct { + target string + response testHTTPResponse + upstream string + upstreams options.Upstreams + } Context("multiUpstreamProxy", func() { - BeforeEach(func() { - sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} - - writer := &pagewriter.WriterFuncs{ - ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) { - rw.WriteHeader(502) - rw.Write([]byte("Proxy Error")) - }, - } - - ok := http.StatusOK - accepted := http.StatusAccepted - - upstreams := options.Upstreams{ - Configs: []options.Upstream{ - { - ID: "http-backend", - Path: "/http/", - URI: serverAddr, - }, - { - ID: "file-backend", - Path: "/files/", - URI: fmt.Sprintf("file:///%s", filesDir), - }, - { - ID: "static-backend", - Path: "/static/", - Static: true, - StaticCode: &ok, - }, - { - ID: "static-backend-no-trailing-slash", - Path: "/static", - Static: true, - StaticCode: &accepted, - }, - { - ID: "static-backend-long", - Path: "/static/long", - Static: true, - StaticCode: &accepted, - }, - { - ID: "bad-http-backend", - Path: "/bad-http/", - URI: "http://::1", - }, - { - ID: "single-path-backend", - Path: "/single-path", - Static: true, - StaticCode: &ok, - }, - { - ID: "backend-with-rewrite-prefix", - Path: "^/rewrite-prefix/(.*)", - RewriteTarget: "/different/backend/path/$1", - URI: serverAddr, - }, - { - ID: "double-match-plain", - Path: "/double-match/", - URI: serverAddr, - }, - { - ID: "double-match-rewrite", - Path: "^/double-match/(.*)", - RewriteTarget: "/double-match/rewrite/$1", - URI: serverAddr, - }, - }, - } - - var err error - upstreamServer, err = NewProxy(upstreams, sigData, writer) - Expect(err).ToNot(HaveOccurred()) - }) - - type proxyTableInput struct { - target string - response testHTTPResponse - upstream string - } - DescribeTable("Proxy ServeHTTP", func(in *proxyTableInput) { + sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} + + writer := &pagewriter.WriterFuncs{ + ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) { + rw.WriteHeader(502) + rw.Write([]byte("Proxy Error")) + }, + } + + ok := http.StatusOK + accepted := http.StatusAccepted + + // Allows for specifying settings and even individual upstreams for specific tests and uses the default upstreams/configs otherwise + upstreams := in.upstreams + if len(in.upstreams.Configs) == 0 { + upstreams.Configs = []options.Upstream{ + { + ID: "http-backend", + Path: "/http/", + URI: serverAddr, + }, + { + ID: "file-backend", + Path: "/files/", + URI: fmt.Sprintf("file:///%s", filesDir), + }, + { + ID: "static-backend", + Path: "/static/", + Static: true, + StaticCode: &ok, + }, + { + ID: "static-backend-no-trailing-slash", + Path: "/static", + Static: true, + StaticCode: &accepted, + }, + { + ID: "static-backend-long", + Path: "/static/long", + Static: true, + StaticCode: &accepted, + }, + { + ID: "bad-http-backend", + Path: "/bad-http/", + URI: "http://::1", + }, + { + ID: "single-path-backend", + Path: "/single-path", + Static: true, + StaticCode: &ok, + }, + { + ID: "backend-with-rewrite-prefix", + Path: "^/rewrite-prefix/(.*)", + RewriteTarget: "/different/backend/path/$1", + URI: serverAddr, + }, + { + ID: "double-match-plain", + Path: "/double-match/", + URI: serverAddr, + }, + { + ID: "double-match-rewrite", + Path: "^/double-match/(.*)", + RewriteTarget: "/double-match/rewrite/$1", + URI: serverAddr, + }, + } + } + + upstreamServer, err := NewProxy(upstreams, sigData, writer) + Expect(err).ToNot(HaveOccurred()) + req := middlewareapi.AddRequestScope( httptest.NewRequest("", in.target, nil), &middlewareapi.RequestScope{}, @@ -133,10 +131,12 @@ var _ = Describe("Proxy Suite", func() { } // Compare the reflected request to the upstream - request := testHTTPRequest{} - Expect(json.Unmarshal(body, &request)).To(Succeed()) - testSanitizeRequestHeader(request.Header) - Expect(request).To(Equal(in.response.request)) + if body != nil { + request := testHTTPRequest{} + Expect(json.Unmarshal(body, &request)).To(Succeed()) + testSanitizeRequestHeader(request.Header) + Expect(request).To(Equal(in.response.request)) + } }, Entry("with a request to the HTTP service", &proxyTableInput{ target: "http://example.localhost/http/1234", @@ -312,6 +312,31 @@ var _ = Describe("Proxy Suite", func() { }, upstream: "double-match-rewrite", }), + Entry("containing an escaped '/' without ProxyRawPath", &proxyTableInput{ + target: "http://example.localhost/%2F/test1/%2F/test2", + response: testHTTPResponse{ + code: 301, + header: map[string][]string{ + "Location": { + "http://example.localhost/test1/test2", + }, + }, + }, + upstream: "", + }), + Entry("containing an escaped '/' with ProxyRawPath", &proxyTableInput{ + upstreams: options.Upstreams{ProxyRawPath: true}, + target: "http://example.localhost/%2F/test1/%2F/test2", + response: testHTTPResponse{ + code: 404, + header: map[string][]string{ + "X-Content-Type-Options": {"nosniff"}, + contentType: {textPlainUTF8}, + }, + raw: "404 page not found\n", + }, + upstream: "", + }), ) }) @@ -321,24 +346,24 @@ var _ = Describe("Proxy Suite", func() { expectedOutput []options.Upstream } - var httpPath = options.Upstream{ + httpPath := options.Upstream{ Path: "/http/", } - var httpSubPath = options.Upstream{ + httpSubPath := options.Upstream{ Path: "/http/subpath/", } - var longerPath = options.Upstream{ + longerPath := options.Upstream{ Path: "/longer-than-http", } - var shortPathWithRewrite = options.Upstream{ + shortPathWithRewrite := options.Upstream{ Path: "^/h/(.*)", RewriteTarget: "/$1", } - var shortSubPathWithRewrite = options.Upstream{ + shortSubPathWithRewrite := options.Upstream{ Path: "^/h/bar/(.*)", RewriteTarget: "/$1", } From fe9159572ca9c434b516a01c6d21210eed3abde9 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Mon, 9 Aug 2021 15:03:52 +0000 Subject: [PATCH 6/7] add docs for new Upstream mux config --- docs/docs/configuration/alpha_config.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 7381e1c0..3cc54037 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -384,9 +384,11 @@ Requests will be proxied to this upstream if the path matches the request path. ### Upstreams -#### ([[]Upstream](#upstream) alias) - (**Appears on:** [AlphaOptions](#alphaoptions)) Upstreams is a collection of definitions for upstream servers. +| Field | Type | Description | +| ----- | ---- | ----------- | +| `proxyRawPath` | _bool_ | ProxyRawPath will pass the raw url path to upstream allowing for url's
like: "/%2F/" which would otherwise be redirected to "/" | +| `configs` | _[[]Upstream](#upstream)_ | Upstream represents the configuration for an upstream server.
Requests will be proxied to this upstream if the path matches the request path. | From 88f32aeaa15dfe3f6596518e0d9970dbff4eff33 Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Fri, 17 Sep 2021 11:08:18 +0000 Subject: [PATCH 7/7] rename Upstreams to UpstreamConfig and its Configs member to Upstreams then --- docs/docs/configuration/alpha_config.md | 10 +++--- main_test.go | 8 ++--- oauthproxy_test.go | 34 +++++++++--------- pkg/apis/options/alpha_options.go | 9 +++-- pkg/apis/options/legacy_options.go | 8 ++--- pkg/apis/options/legacy_options_test.go | 6 ++-- pkg/apis/options/load_test.go | 8 ++--- pkg/apis/options/options.go | 2 +- pkg/apis/options/upstreams.go | 8 ++--- pkg/upstream/proxy.go | 4 +-- pkg/upstream/proxy_test.go | 8 ++--- pkg/validation/options_test.go | 2 +- pkg/validation/upstreams.go | 4 +-- pkg/validation/upstreams_test.go | 48 ++++++++++++------------- 14 files changed, 79 insertions(+), 80 deletions(-) diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 3cc54037..f13ea131 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -124,7 +124,7 @@ They may change between releases without notice. | Field | Type | Description | | ----- | ---- | ----------- | -| `upstreams` | _[Upstreams](#upstreams)_ | Upstreams is used to configure upstream servers.
Once a user is authenticated, requests to the server will be proxied to
these upstream servers based on the path mappings defined in this list. | +| `upstreamConfig` | _[UpstreamConfig](#upstreamconfig)_ | UpstreamConfig is used to configure upstream servers.
Once a user is authenticated, requests to the server will be proxied to
these upstream servers based on the path mappings defined in this list. | | `injectRequestHeaders` | _[[]Header](#header)_ | InjectRequestHeaders is used to configure headers that should be added
to requests to upstream servers.
Headers may source values from either the authenticated user's session
or from a static secret value. | | `injectResponseHeaders` | _[[]Header](#header)_ | InjectResponseHeaders is used to configure headers that should be added
to responses from the proxy.
This is typically used when using the proxy as an external authentication
provider in conjunction with another proxy such as NGINX and its
auth_request module.
Headers may source values from either the authenticated user's session
or from a static secret value. | | `server` | _[Server](#server)_ | Server is used to configure the HTTP(S) server for the proxy application.
You may choose to run both HTTP and HTTPS servers simultaneously.
This can be done by setting the BindAddress and the SecureBindAddress simultaneously.
To use the secure server you must configure a TLS certificate and key. | @@ -364,7 +364,7 @@ TLS contains the information for loading a TLS certifcate and key. ### Upstream -(**Appears on:** [Upstreams](#upstreams)) +(**Appears on:** [UpstreamConfig](#upstreamconfig)) Upstream represents the configuration for an upstream server. Requests will be proxied to this upstream if the path matches the request path. @@ -382,13 +382,13 @@ Requests will be proxied to this upstream if the path matches the request path. | `passHostHeader` | _bool_ | PassHostHeader determines whether the request host header should be proxied
to the upstream server.
Defaults to true. | | `proxyWebSockets` | _bool_ | ProxyWebSockets enables proxying of websockets to upstream servers
Defaults to true. | -### Upstreams +### UpstreamConfig (**Appears on:** [AlphaOptions](#alphaoptions)) -Upstreams is a collection of definitions for upstream servers. +UpstreamConfig is a collection of definitions for upstream servers. | Field | Type | Description | | ----- | ---- | ----------- | | `proxyRawPath` | _bool_ | ProxyRawPath will pass the raw url path to upstream allowing for url's
like: "/%2F/" which would otherwise be redirected to "/" | -| `configs` | _[[]Upstream](#upstream)_ | Upstream represents the configuration for an upstream server.
Requests will be proxied to this upstream if the path matches the request path. | +| `upstreams` | _[[]Upstream](#upstream)_ | Upstreams represents the configuration for the upstream servers.
Requests will be proxied to this upstream if the path matches the request path. | diff --git a/main_test.go b/main_test.go index dc7dcb82..4fa9efbf 100644 --- a/main_test.go +++ b/main_test.go @@ -26,9 +26,9 @@ client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" ` const testAlphaConfig = ` -upstreams: +upstreamConfig: proxyrawpath: false - configs: + upstreams: - id: / path: / uri: http://httpbin @@ -104,8 +104,8 @@ redirect_url="http://localhost:4180/oauth2/callback" opts.Cookie.Secure = false opts.RawRedirectURL = "http://localhost:4180/oauth2/callback" - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "/", Path: "/", diff --git a/oauthproxy_test.go b/oauthproxy_test.go index d0eaaa4d..9add52ed 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -197,8 +197,8 @@ func TestBasicAuthPassword(t *testing.T) { basicAuthPassword := "This is a secure password" opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: providerServer.URL, Path: "/", @@ -348,8 +348,8 @@ func NewPassAccessTokenTest(opts PassAccessTokenTestOptions) (*PassAccessTokenTe })) patt.opts = baseTestOptions() - patt.opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + patt.opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: patt.providerServer.URL, Path: "/", @@ -358,7 +358,7 @@ func NewPassAccessTokenTest(opts PassAccessTokenTestOptions) (*PassAccessTokenTe }, } if opts.ProxyUpstream.ID != "" { - patt.opts.UpstreamServers.Configs = append(patt.opts.UpstreamServers.Configs, opts.ProxyUpstream) + patt.opts.UpstreamServers.Upstreams = append(patt.opts.UpstreamServers.Upstreams, opts.ProxyUpstream) } patt.opts.Cookie.Secure = false @@ -1273,8 +1273,8 @@ func TestAuthSkippedForPreflightRequests(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: upstreamServer.URL, Path: "/", @@ -1350,8 +1350,8 @@ func NewSignatureTest() (*SignatureTest, error) { if err != nil { return nil, err } - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: upstreamServer.URL, Path: "/", @@ -1788,8 +1788,8 @@ func Test_noCacheHeaders(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: upstreamServer.URL, Path: "/", @@ -2060,8 +2060,8 @@ func TestTrustedIPs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "static", Path: "/", @@ -2255,8 +2255,8 @@ func TestAllowedRequest(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: upstreamServer.URL, Path: "/", @@ -2372,8 +2372,8 @@ func TestProxyAllowedGroups(t *testing.T) { test, err := NewProcessCookieTestWithOptionsModifiers(func(opts *options.Options) { opts.Providers[0].AllowedGroups = tt.allowedGroups - opts.UpstreamServers = options.Upstreams{ - Configs: []options.Upstream{ + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: upstreamServer.URL, Path: "/", diff --git a/pkg/apis/options/alpha_options.go b/pkg/apis/options/alpha_options.go index ecfd81c8..04769d7f 100644 --- a/pkg/apis/options/alpha_options.go +++ b/pkg/apis/options/alpha_options.go @@ -9,10 +9,10 @@ package options // They may change between releases without notice. // ::: type AlphaOptions struct { - // Upstreams is used to configure upstream servers. + // UpstreamConfig is used to configure upstream servers. // Once a user is authenticated, requests to the server will be proxied to // these upstream servers based on the path mappings defined in this list. - Upstreams Upstreams `json:"upstreams,omitempty"` + UpstreamConfig UpstreamConfig `json:"upstreamConfig,omitempty"` // InjectRequestHeaders is used to configure headers that should be added // to requests to upstream servers. @@ -48,19 +48,18 @@ type AlphaOptions struct { // MergeInto replaces alpha options in the Options struct with the values // from the AlphaOptions func (a *AlphaOptions) MergeInto(opts *Options) { - opts.UpstreamServers = a.Upstreams + opts.UpstreamServers = a.UpstreamConfig opts.InjectRequestHeaders = a.InjectRequestHeaders opts.InjectResponseHeaders = a.InjectResponseHeaders opts.Server = a.Server opts.MetricsServer = a.MetricsServer opts.Providers = a.Providers - } // ExtractFrom populates the fields in the AlphaOptions with the values from // the Options func (a *AlphaOptions) ExtractFrom(opts *Options) { - a.Upstreams = opts.UpstreamServers + a.UpstreamConfig = opts.UpstreamServers a.InjectRequestHeaders = opts.InjectRequestHeaders a.InjectResponseHeaders = opts.InjectResponseHeaders a.Server = opts.Server diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index fb51feed..3441fd41 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -114,13 +114,13 @@ func legacyUpstreamsFlagSet() *pflag.FlagSet { return flagSet } -func (l *LegacyUpstreams) convert() (Upstreams, error) { - upstreams := Upstreams{} +func (l *LegacyUpstreams) convert() (UpstreamConfig, error) { + upstreams := UpstreamConfig{} for _, upstreamString := range l.Upstreams { u, err := url.Parse(upstreamString) if err != nil { - return Upstreams{}, fmt.Errorf("could not parse upstream %q: %v", upstreamString, err) + return UpstreamConfig{}, fmt.Errorf("could not parse upstream %q: %v", upstreamString, err) } if u.Path == "" { @@ -169,7 +169,7 @@ func (l *LegacyUpstreams) convert() (Upstreams, error) { upstream.FlushInterval = nil } - upstreams.Configs = append(upstreams.Configs, upstream) + upstreams.Upstreams = append(upstreams.Upstreams, upstream) } return upstreams, nil diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go index 88c32817..ecf48494 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -26,8 +26,8 @@ var _ = Describe("Legacy Options", func() { truth := true staticCode := 204 - opts.UpstreamServers = Upstreams{ - Configs: []Upstream{ + opts.UpstreamServers = UpstreamConfig{ + Upstreams: []Upstream{ { ID: "/baz", Path: "/baz", @@ -221,7 +221,7 @@ var _ = Describe("Legacy Options", func() { Expect(err).ToNot(HaveOccurred()) } - Expect(upstreams.Configs).To(ConsistOf(in.expectedUpstreams)) + Expect(upstreams.Upstreams).To(ConsistOf(in.expectedUpstreams)) }, Entry("with no upstreams", &convertUpstreamsTableInput{ upstreamStrings: []string{}, diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index ca4918b2..335facf4 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -469,8 +469,8 @@ sub: It("should load a full example AlphaOptions", func() { config := []byte(` -upstreams: - configs: +upstreamConfig: + upstreams: - id: httpbin path: / uri: http://httpbin @@ -503,8 +503,8 @@ injectResponseHeaders: flushInterval := Duration(500 * time.Millisecond) Expect(into).To(Equal(&AlphaOptions{ - Upstreams: Upstreams{ - Configs: []Upstream{ + UpstreamConfig: UpstreamConfig{ + Upstreams: []Upstream{ { ID: "httpbin", Path: "/", diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 0fd2ece1..d4bb4312 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -41,7 +41,7 @@ type Options struct { // Not used in the legacy config, name not allowed to match an external key (upstreams) // TODO(JoelSpeed): Rename when legacy config is removed - UpstreamServers Upstreams `cfg:",internal"` + UpstreamServers UpstreamConfig `cfg:",internal"` InjectRequestHeaders []Header `cfg:",internal"` InjectResponseHeaders []Header `cfg:",internal"` diff --git a/pkg/apis/options/upstreams.go b/pkg/apis/options/upstreams.go index 4e1a0547..9d735f13 100644 --- a/pkg/apis/options/upstreams.go +++ b/pkg/apis/options/upstreams.go @@ -7,15 +7,15 @@ const ( DefaultUpstreamFlushInterval = 1 * time.Second ) -// Upstreams is a collection of definitions for upstream servers. -type Upstreams struct { +// UpstreamConfig is a collection of definitions for upstream servers. +type UpstreamConfig struct { // ProxyRawPath will pass the raw url path to upstream allowing for url's // like: "/%2F/" which would otherwise be redirected to "/" ProxyRawPath bool `json:"proxyRawPath,omitempty"` - // Upstream represents the configuration for an upstream server. + // Upstreams represents the configuration for the upstream servers. // Requests will be proxied to this upstream if the path matches the request path. - Configs []Upstream `json:"configs,omitempty"` + Upstreams []Upstream `json:"upstreams,omitempty"` } // Upstream represents the configuration for an upstream server. diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index 27bd5229..eabb47ff 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -22,7 +22,7 @@ type ProxyErrorHandler func(http.ResponseWriter, *http.Request, error) // NewProxy creates a new multiUpstreamProxy that can serve requests directed to // multiple upstreams. -func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, writer pagewriter.Writer) (http.Handler, error) { +func NewProxy(upstreams options.UpstreamConfig, sigData *options.SignatureData, writer pagewriter.Writer) (http.Handler, error) { m := &multiUpstreamProxy{ serveMux: mux.NewRouter(), } @@ -31,7 +31,7 @@ func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, write m.serveMux.UseEncodedPath() } - for _, upstream := range sortByPathLongest(upstreams.Configs) { + for _, upstream := range sortByPathLongest(upstreams.Upstreams) { if upstream.Static { if err := m.registerStaticResponseHandler(upstream, writer); err != nil { return nil, fmt.Errorf("could not register static upstream %q: %v", upstream.ID, err) diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index 2745e680..87c27e4b 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -20,7 +20,7 @@ var _ = Describe("Proxy Suite", func() { target string response testHTTPResponse upstream string - upstreams options.Upstreams + upstreams options.UpstreamConfig } Context("multiUpstreamProxy", func() { @@ -40,8 +40,8 @@ var _ = Describe("Proxy Suite", func() { // Allows for specifying settings and even individual upstreams for specific tests and uses the default upstreams/configs otherwise upstreams := in.upstreams - if len(in.upstreams.Configs) == 0 { - upstreams.Configs = []options.Upstream{ + if len(in.upstreams.Upstreams) == 0 { + upstreams.Upstreams = []options.Upstream{ { ID: "http-backend", Path: "/http/", @@ -325,7 +325,7 @@ var _ = Describe("Proxy Suite", func() { upstream: "", }), Entry("containing an escaped '/' with ProxyRawPath", &proxyTableInput{ - upstreams: options.Upstreams{ProxyRawPath: true}, + upstreams: options.UpstreamConfig{ProxyRawPath: true}, target: "http://example.localhost/%2F/test1/%2F/test2", response: testHTTPResponse{ code: 404, diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 70387a14..03afb22a 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -22,7 +22,7 @@ const ( func testOptions() *options.Options { o := options.NewOptions() - o.UpstreamServers.Configs = append(o.UpstreamServers.Configs, options.Upstream{ + o.UpstreamServers.Upstreams = append(o.UpstreamServers.Upstreams, options.Upstream{ ID: "upstream", Path: "/", URI: "http://127.0.0.1:8080/", diff --git a/pkg/validation/upstreams.go b/pkg/validation/upstreams.go index f45f3b94..ba1e216a 100644 --- a/pkg/validation/upstreams.go +++ b/pkg/validation/upstreams.go @@ -7,12 +7,12 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" ) -func validateUpstreams(upstreams options.Upstreams) []string { +func validateUpstreams(upstreams options.UpstreamConfig) []string { msgs := []string{} ids := make(map[string]struct{}) paths := make(map[string]struct{}) - for _, upstream := range upstreams.Configs { + for _, upstream := range upstreams.Upstreams { msgs = append(msgs, validateUpstream(upstream, ids, paths)...) } diff --git a/pkg/validation/upstreams_test.go b/pkg/validation/upstreams_test.go index 1c974024..586c6d85 100644 --- a/pkg/validation/upstreams_test.go +++ b/pkg/validation/upstreams_test.go @@ -11,7 +11,7 @@ import ( var _ = Describe("Upstreams", func() { type validateUpstreamTableInput struct { - upstreams options.Upstreams + upstreams options.UpstreamConfig errStrings []string } @@ -54,12 +54,12 @@ var _ = Describe("Upstreams", func() { Expect(validateUpstreams(o.upstreams)).To(ConsistOf(o.errStrings)) }, Entry("with no upstreams", &validateUpstreamTableInput{ - upstreams: options.Upstreams{}, + upstreams: options.UpstreamConfig{}, errStrings: []string{}, }), Entry("with valid upstreams", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ validHTTPUpstream, validStaticUpstream, validFileUpstream, @@ -68,8 +68,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{}, }), Entry("with an empty ID", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "", Path: "/foo", @@ -80,8 +80,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{emptyIDMsg}, }), Entry("with an empty Path", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "", @@ -92,8 +92,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{emptyPathMsg}, }), Entry("with an empty Path", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "", @@ -104,8 +104,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{emptyPathMsg}, }), Entry("with an empty URI", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo", @@ -116,8 +116,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{emptyURIMsg}, }), Entry("with an invalid URI", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo", @@ -128,8 +128,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{invalidURIMsg}, }), Entry("with an invalid URI scheme", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo", @@ -140,8 +140,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{invalidURISchemeMsg}, }), Entry("with a static upstream and invalid optons", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo", @@ -163,8 +163,8 @@ var _ = Describe("Upstreams", func() { }, }), Entry("with duplicate IDs", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo1", @@ -180,8 +180,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{multipleIDsMsg}, }), Entry("with duplicate Paths", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo1", Path: "/foo", @@ -197,8 +197,8 @@ var _ = Describe("Upstreams", func() { errStrings: []string{multiplePathsMsg}, }), Entry("when a static code is supplied without static", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - Configs: []options.Upstream{ + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ { ID: "foo", Path: "/foo",