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/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 7381e1c0..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,11 +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 - -#### ([[]Upstream](#upstream) alias) +### 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 "/" | +| `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 e40243ef..4fa9efbf 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" @@ -24,7 +26,9 @@ client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" ` const testAlphaConfig = ` -upstreams: +upstreamConfig: + proxyrawpath: false + upstreams: - id: / path: / uri: http://httpbin @@ -100,14 +104,16 @@ redirect_url="http://localhost:4180/oauth2/callback" opts.Cookie.Secure = false 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), + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: "/", + Path: "/", + URI: "http://httpbin", + FlushInterval: durationPtr(options.DefaultUpstreamFlushInterval), + PassHostHeader: boolPtr(true), + ProxyWebSockets: boolPtr(true), + }, }, } @@ -130,7 +136,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 +236,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: 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\"", 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..9add52ed 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -197,11 +197,13 @@ func TestBasicAuthPassword(t *testing.T) { basicAuthPassword := "This is a secure password" opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - { - ID: providerServer.URL, - Path: "/", - URI: providerServer.URL, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: providerServer.URL, + Path: "/", + URI: providerServer.URL, + }, }, } @@ -346,15 +348,17 @@ func NewPassAccessTokenTest(opts PassAccessTokenTestOptions) (*PassAccessTokenTe })) patt.opts = baseTestOptions() - patt.opts.UpstreamServers = options.Upstreams{ - { - ID: patt.providerServer.URL, - Path: "/", - URI: patt.providerServer.URL, + patt.opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []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.Upstreams = append(patt.opts.UpstreamServers.Upstreams, opts.ProxyUpstream) } patt.opts.Cookie.Secure = false @@ -915,6 +919,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 { @@ -1260,11 +1273,13 @@ func TestAuthSkippedForPreflightRequests(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthPreflight = true @@ -1335,11 +1350,13 @@ func NewSignatureTest() (*SignatureTest, error) { if err != nil { return nil, err } - opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } @@ -1771,11 +1788,13 @@ func Test_noCacheHeaders(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthRegex = []string{".*"} @@ -2041,11 +2060,13 @@ func TestTrustedIPs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - { - ID: "static", - Path: "/", - Static: true, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: "static", + Path: "/", + Static: true, + }, }, } opts.TrustedIPs = tt.trustedIPs @@ -2234,11 +2255,13 @@ func TestAllowedRequest(t *testing.T) { t.Cleanup(upstreamServer.Close) opts := baseTestOptions() - opts.UpstreamServers = options.Upstreams{ - { - ID: upstreamServer.URL, - Path: "/", - URI: upstreamServer.URL, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } opts.SkipAuthRegex = []string{ @@ -2349,11 +2372,13 @@ 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, + opts.UpstreamServers = options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: upstreamServer.URL, + Path: "/", + URI: upstreamServer.URL, + }, }, } }) 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 cf67dedd..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 nil, 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 = append(upstreams, 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 fb5b816a..ecf48494 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -26,35 +26,37 @@ 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, + opts.UpstreamServers = UpstreamConfig{ + Upstreams: []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.Upstreams).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..335facf4 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -469,11 +469,12 @@ sub: It("should load a full example AlphaOptions", func() { config := []byte(` -upstreams: -- id: httpbin - path: / - uri: http://httpbin - flushInterval: 500ms +upstreamConfig: + upstreams: + - 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, + UpstreamConfig: UpstreamConfig{ + Upstreams: []Upstream{ + { + ID: "httpbin", + Path: "/", + URI: "http://httpbin", + FlushInterval: &flushInterval, + }, }, }, InjectRequestHeaders: []Header{ 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 1977c8da..9d735f13 100644 --- a/pkg/apis/options/upstreams.go +++ b/pkg/apis/options/upstreams.go @@ -7,8 +7,16 @@ const ( DefaultUpstreamFlushInterval = 1 * time.Second ) -// Upstreams is a collection of definitions for upstream servers. -type Upstreams []Upstream +// 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"` + + // Upstreams represents the configuration for the upstream servers. + // Requests will be proxied to this upstream if the path matches the request path. + Upstreams []Upstream `json:"upstreams,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..eabb47ff 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -22,12 +22,16 @@ 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(), } - for _, upstream := range sortByPathLongest(upstreams) { + if upstreams.ProxyRawPath { + m.serveMux.UseEncodedPath() + } + + 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) @@ -153,7 +157,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..87c27e4b 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -16,94 +16,94 @@ import ( ) var _ = Describe("Proxy Suite", func() { - var upstreamServer http.Handler + type proxyTableInput struct { + target string + response testHTTPResponse + upstream string + upstreams options.UpstreamConfig + } 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{ - { - 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.Upstreams) == 0 { + upstreams.Upstreams = []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{}, @@ -131,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", @@ -310,33 +312,58 @@ 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.UpstreamConfig{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: "", + }), ) }) Context("sortByPathLongest", func() { type sortByPathLongestTableInput struct { - input options.Upstreams - expectedOutput options.Upstreams + input []options.Upstream + 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", } @@ -346,40 +373,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..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 = append(o.UpstreamServers, 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 7bd6b2d5..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 { + 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 122286ad..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,88 +54,104 @@ 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{ - validHTTPUpstream, - validStaticUpstream, - validFileUpstream, + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ + validHTTPUpstream, + validStaticUpstream, + validFileUpstream, + }, }, errStrings: []string{}, }), Entry("with an empty ID", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - { - ID: "", - Path: "/foo", - URI: "http://localhost:8080", + upstreams: options.UpstreamConfig{ + Upstreams: []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", + upstreams: options.UpstreamConfig{ + Upstreams: []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", + upstreams: options.UpstreamConfig{ + Upstreams: []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: "", + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: "", + }, }, }, errStrings: []string{emptyURIMsg}, }), Entry("with an invalid URI", &validateUpstreamTableInput{ - upstreams: options.Upstreams{ - { - ID: "foo", - Path: "/foo", - URI: ":", + upstreams: options.UpstreamConfig{ + Upstreams: []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", + upstreams: options.UpstreamConfig{ + Upstreams: []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, + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + URI: "ftp://foo", + Static: true, + FlushInterval: &flushInterval, + PassHostHeader: &truth, + ProxyWebSockets: &truth, + InsecureSkipTLSVerify: true, + }, }, }, errStrings: []string{ @@ -147,41 +163,47 @@ 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", + upstreams: options.UpstreamConfig{ + Upstreams: []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", + upstreams: options.UpstreamConfig{ + Upstreams: []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, + upstreams: options.UpstreamConfig{ + Upstreams: []options.Upstream{ + { + ID: "foo", + Path: "/foo", + StaticCode: &staticCode200, + }, }, }, errStrings: []string{emptyURIMsg, staticCodeMsg},