From a81cf7105c23c718000b78dc01f9230be9aff1a1 Mon Sep 17 00:00:00 2001 From: Przemek Maciolek Date: Thu, 11 Feb 2021 15:17:35 +0100 Subject: [PATCH] Enable CORS only when origin is defined --- config/confighttp/README.md | 13 +-- config/confighttp/confighttp.go | 23 ++++-- config/confighttp/confighttp_test.go | 93 +++++++++++++++++----- receiver/otlpreceiver/README.md | 4 +- receiver/otlpreceiver/config_test.go | 1 + receiver/otlpreceiver/otlp.go | 1 + receiver/otlpreceiver/testdata/config.yaml | 3 + 7 files changed, 107 insertions(+), 31 deletions(-) diff --git a/config/confighttp/README.md b/config/confighttp/README.md index 570320e72ee..ead126dc4a3 100644 --- a/config/confighttp/README.md +++ b/config/confighttp/README.md @@ -36,12 +36,13 @@ exporter: [Receivers](https://github.com/open-telemetry/opentelemetry-collector/blob/main/receiver/README.md) leverage server configuration. -- [`cors_allowed_origins`](https://github.com/rs/cors): An empty list here and - in `cors_allowed_headers` means that CORS is not enabled at all. - A wildcard can be used to match any origin or one or more characters of an origin. -- [`cors_allowed_headers`](https://github.com/rs/cors): An empty list here and - in `cors_allowed_origins` means that CORS is not enabled at all. - A wildcard can be used to match any header or one or more characters in the header. +- [`cors_allowed_origins`](https://github.com/rs/cors): An empty list means + that CORS is not enabled at all. A wildcard can be used to match any origin + or one or more characters of an origin. +- [`cors_allowed_headers`](https://github.com/rs/cors): When CORS is enabled, + can be used to specify an optional list of allowed headers. By default, it includes `Accept`, + `Content-Type`, `X-Requested-With`. `Origin` is also always + added to the list. A wildcard (`*`) can be used to match any header. - `endpoint`: Valid value syntax available [here](https://github.com/grpc/grpc/blob/master/doc/naming.md) - [`tls_settings`](../configtls/README.md) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 1efd9b17a8f..d2e71d2fd1f 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -21,6 +21,7 @@ import ( "time" "github.com/rs/cors" + "go.uber.org/zap" "go.opentelemetry.io/collector/config/configtls" "go.opentelemetry.io/collector/internal/middleware" @@ -111,14 +112,14 @@ type HTTPServerSettings struct { // CorsOrigins are the allowed CORS origins for HTTP/JSON requests to grpc-gateway adapter // for the OTLP receiver. See github.com/rs/cors - // An empty CorsOrigins and CorsHeaders means that CORS is not enabled at all. - // A wildcard (*) can be used to match any origin or one or more characters of an origin. + // An empty list means that CORS is not enabled at all. A wildcard (*) can be + // used to match any origin or one or more characters of an origin. CorsOrigins []string `mapstructure:"cors_allowed_origins"` // CorsHeaders are the allowed CORS headers for HTTP/JSON requests to grpc-gateway adapter // for the OTLP receiver. See github.com/rs/cors - // An empty CorsOrigins and CorsHeaders means that CORS is not enabled at all. - // A wildcard (*) can be used to match any header or one or more characters of a header. + // CORS needs to be enabled first by providing a non-empty list in CorsOrigins + // A wildcard (*) can be used to match any header. CorsHeaders []string `mapstructure:"cors_allowed_headers"` } @@ -143,10 +144,18 @@ func (hss *HTTPServerSettings) ToListener() (net.Listener, error) { // returned by HTTPServerSettings.ToServer(). type toServerOptions struct { errorHandler middleware.ErrorHandler + logger *zap.Logger } type ToServerOption func(opts *toServerOptions) +// WithLogger allows to specify optional logger used during initialization. +func WithLogger(logger *zap.Logger) ToServerOption { + return func(opts *toServerOptions) { + opts.logger = logger + } +} + // WithErrorHandler overrides the HTTP error handler that gets invoked // when there is a failure inside middleware.HTTPContentDecompressor. func WithErrorHandler(e middleware.ErrorHandler) ToServerOption { @@ -160,10 +169,14 @@ func (hss *HTTPServerSettings) ToServer(handler http.Handler, opts ...ToServerOp for _, o := range opts { o(serverOpts) } - if len(hss.CorsOrigins) > 0 || len(hss.CorsHeaders) > 0 { + if len(hss.CorsOrigins) > 0 { co := cors.Options{AllowedOrigins: hss.CorsOrigins, AllowedHeaders: hss.CorsHeaders} handler = cors.New(co).Handler(handler) + } else if len(hss.CorsHeaders) > 0 && serverOpts.logger != nil { + serverOpts.logger.Warn( + "CORS needs to be enabled via `cors_allowed_origins` for `cors_allowed_headers` to have an effect") } + handler = middleware.HTTPContentDecompressor( handler, middleware.WithErrorHandler(serverOpts.errorHandler), diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 66d98e4925a..652ffcc8bbd 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -27,6 +27,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" "go.opentelemetry.io/collector/config/configtls" ) @@ -316,37 +317,93 @@ func TestHttpReception(t *testing.T) { } func TestHttpCors(t *testing.T) { - hss := &HTTPServerSettings{ - Endpoint: "localhost:0", - CorsOrigins: []string{"allowed-*.com"}, + tests := []struct { + name string + CorsOrigins []string + CorsHeaders []string + allowedWorks bool + disallowedWorks bool + extraHeaderWorks bool + }{ + { + name: "noCORS", + allowedWorks: false, + disallowedWorks: false, + extraHeaderWorks: false, + }, + { + name: "OriginCORS", + CorsOrigins: []string{"allowed-*.com"}, + CorsHeaders: []string{}, + allowedWorks: true, + disallowedWorks: false, + extraHeaderWorks: false, + }, + { + name: "HeaderCORS", + CorsOrigins: []string{"allowed-*.com"}, + CorsHeaders: []string{"ExtraHeader"}, + allowedWorks: true, + disallowedWorks: false, + extraHeaderWorks: true, + }, } - ln, err := hss.ToListener() - assert.NoError(t, err) - s := hss.ToServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - go func() { - _ = s.Serve(ln) - }() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hss := &HTTPServerSettings{ + Endpoint: "localhost:0", + CorsOrigins: tt.CorsOrigins, + CorsHeaders: tt.CorsHeaders, + } - // TODO: make starting server deterministic - // Wait for the servers to start - <-time.After(10 * time.Millisecond) + ln, err := hss.ToListener() + assert.NoError(t, err) + s := hss.ToServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + go func() { + _ = s.Serve(ln) + }() + + // TODO: make starting server deterministic + // Wait for the servers to start + <-time.After(10 * time.Millisecond) + + url := fmt.Sprintf("http://%s", ln.Addr().String()) + + // Verify allowed domain gets responses that allow CORS. + verifyCorsResp(t, url, "allowed-origin.com", false, 200, tt.allowedWorks) + + // Verify allowed domain and extra headers gets responses that allow CORS. + verifyCorsResp(t, url, "allowed-origin.com", true, 200, tt.extraHeaderWorks) - url := fmt.Sprintf("http://%s", ln.Addr().String()) + // Verify disallowed domain gets responses that disallow CORS. + verifyCorsResp(t, url, "disallowed-origin.com", false, 200, tt.disallowedWorks) - // Verify allowed domain gets responses that allow CORS. - verifyCorsResp(t, url, "allowed-origin.com", 200, true) + require.NoError(t, s.Close()) + }) + } +} - // Verify disallowed domain gets responses that disallow CORS. - verifyCorsResp(t, url, "disallowed-origin.com", 200, false) +func TestHttpCorsInvalidSettings(t *testing.T) { + hss := &HTTPServerSettings{ + Endpoint: "localhost:0", + CorsHeaders: []string{"some-header"}, + } + // This effectively does not enable CORS but should also not cause an error + s := hss.ToServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), WithLogger(zap.NewNop())) + require.NotNil(t, s) require.NoError(t, s.Close()) } -func verifyCorsResp(t *testing.T, url string, origin string, wantStatus int, wantAllowed bool) { +func verifyCorsResp(t *testing.T, url string, origin string, extraHeader bool, wantStatus int, wantAllowed bool) { req, err := http.NewRequest("OPTIONS", url, nil) require.NoError(t, err, "Error creating trace OPTIONS request: %v", err) req.Header.Set("Origin", origin) + if extraHeader { + req.Header.Set("ExtraHeader", "foo") + req.Header.Set("Access-Control-Request-Headers", "ExtraHeader") + } req.Header.Set("Access-Control-Request-Method", "POST") resp, err := http.DefaultClient.Do(req) diff --git a/receiver/otlpreceiver/README.md b/receiver/otlpreceiver/README.md index 1e99e302438..5eff4b51c80 100644 --- a/receiver/otlpreceiver/README.md +++ b/receiver/otlpreceiver/README.md @@ -52,8 +52,8 @@ port is `55681`. The HTTP/JSON endpoint can also optionally configure [CORS](https://fetch.spec.whatwg.org/#cors-protocol), which is enabled by -specifying a list of allowed CORS origins in the `cors_allowed_origins` and/or -`cors_allowed_headers` fields: +specifying a list of allowed CORS origins in the `cors_allowed_origins` +and optionally headers in `cors_allowed_headers`: ```yaml receivers: diff --git a/receiver/otlpreceiver/config_test.go b/receiver/otlpreceiver/config_test.go index 21f444631cc..e225a2a2921 100644 --- a/receiver/otlpreceiver/config_test.go +++ b/receiver/otlpreceiver/config_test.go @@ -185,6 +185,7 @@ func TestLoadConfig(t *testing.T) { Protocols: Protocols{ HTTP: &confighttp.HTTPServerSettings{ Endpoint: "0.0.0.0:55681", + CorsOrigins: []string{"https://*.test.com", "https://test.com"}, CorsHeaders: []string{"ExampleHeader"}, }, }, diff --git a/receiver/otlpreceiver/otlp.go b/receiver/otlpreceiver/otlp.go index 875ea52b350..c00a38601a7 100644 --- a/receiver/otlpreceiver/otlp.go +++ b/receiver/otlpreceiver/otlp.go @@ -143,6 +143,7 @@ func (r *otlpReceiver) startProtocolServers(host component.Host) error { r.serverHTTP = r.cfg.HTTP.ToServer( r.gatewayMux, confighttp.WithErrorHandler(errorHandler), + confighttp.WithLogger(r.logger), ) err = r.startHTTPServer(r.cfg.HTTP, host) if err != nil { diff --git a/receiver/otlpreceiver/testdata/config.yaml b/receiver/otlpreceiver/testdata/config.yaml index 9fd3a9fc4b5..1d00ec43cad 100644 --- a/receiver/otlpreceiver/testdata/config.yaml +++ b/receiver/otlpreceiver/testdata/config.yaml @@ -82,6 +82,9 @@ receivers: otlp/corsheader: protocols: http: + cors_allowed_origins: + - https://*.test.com # Wildcard subdomain. Allows domains like https://www.test.com and https://foo.test.com but not https://wwwtest.com. + - https://test.com # Fully qualified domain name. Allows https://test.com only. cors_allowed_headers: - ExampleHeader processors: