Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cors_allowed_headers option to confighttp #2454

Merged
merged 3 commits into from
Feb 22, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/confighttp/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ leverage server configuration.
- [`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)

Expand All @@ -50,6 +54,8 @@ receivers:
cors_allowed_origins:
- https://foo.bar.com
- https://*.test.com
cors_allowed_headers:
- ExampleHeader
endpoint: 0.0.0.0:55690
protocols:
http:
Expand Down
21 changes: 20 additions & 1 deletion config/confighttp/confighttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -114,6 +115,12 @@ type HTTPServerSettings struct {
// 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
// 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"`
Comment on lines 116 to +122
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an ideal world, would you have allowed headers per origin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking because we still have an opportunity to fix this before GA, so I would like to not just add things now and have to cleanup in 2 months :)

Maybe we can get this right now and deprecate the old way.

Copy link
Contributor Author

@pmm-sumo pmm-sumo Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think we are largely relying on rs/cors model here, which does not seem to provide such capability. At the same time, something like this can be also achieved with separate receivers (though they would need different ports, so not ideal, but when used via reverse proxy maybe not so bad either). @keitwb @kkruk-sumo - what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmm-sumo based on your experience what is the more common case:

  • Having same headers per multiple origins?
  • Having special headers per each origin?

Based on this we can optimize for one case or the other :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't have much experience with CORS. My impression is that within a single deployment, origins can be quite similar to each other so they could share the set of headers (so option 1). But would be great to check with someone more experienced, hence the call to @keitwb and @kkruk-sumo :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt many people will want special headers per origin for a single instance of a CORS enabled receiver. That seems overly complicated for the config and you can just make multiple receiver instances with separate ports if you really wanted such a thing.

}

func (hss *HTTPServerSettings) ToListener() (net.Listener, error) {
Expand All @@ -137,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 {
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand All @@ -155,9 +170,13 @@ func (hss *HTTPServerSettings) ToServer(handler http.Handler, opts ...ToServerOp
o(serverOpts)
}
if len(hss.CorsOrigins) > 0 {
co := cors.Options{AllowedOrigins: hss.CorsOrigins}
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),
Expand Down
93 changes: 75 additions & 18 deletions config/confighttp/confighttp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion receiver/otlpreceiver/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +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` field:
specifying a list of allowed CORS origins in the `cors_allowed_origins`
and optionally headers in `cors_allowed_headers`:

```yaml
receivers:
Expand All @@ -64,4 +65,6 @@ receivers:
- http://test.com
# Origins can have wildcards with *, use * by itself to match any origin.
- https://*.example.com
cors_allowed_headers:
- TestHeader
```
17 changes: 16 additions & 1 deletion receiver/otlpreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestLoadConfig(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, cfg)

assert.Equal(t, len(cfg.Receivers), 9)
assert.Equal(t, len(cfg.Receivers), 10)

assert.Equal(t, cfg.Receivers["otlp"], factory.CreateDefaultConfig())

Expand Down Expand Up @@ -176,6 +176,21 @@ func TestLoadConfig(t *testing.T) {
},
})

assert.Equal(t, cfg.Receivers["otlp/corsheader"],
&Config{
ReceiverSettings: configmodels.ReceiverSettings{
TypeVal: typeStr,
NameVal: "otlp/corsheader",
},
Protocols: Protocols{
HTTP: &confighttp.HTTPServerSettings{
Endpoint: "0.0.0.0:55681",
CorsOrigins: []string{"https://*.test.com", "https://test.com"},
CorsHeaders: []string{"ExampleHeader"},
},
},
})

assert.Equal(t, cfg.Receivers["otlp/uds"],
&Config{
ReceiverSettings: configmodels.ReceiverSettings{
Expand Down
1 change: 1 addition & 0 deletions receiver/otlpreceiver/otlp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 9 additions & 0 deletions receiver/otlpreceiver/testdata/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ receivers:
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.
# The following entry demonstrates how to use CORS Header configuration.
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:
exampleprocessor:

Expand Down