open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.39k stars 1.45k forks source link

Setting otlp/http CORS `allowed_headers` replaces the default safelist #6513

Open ahayworth opened 1 year ago

ahayworth commented 1 year ago

Describe the bug The documentation around CORS allowed_headers for the otlpreceiver and confighttp are unintentionally misleading. The language implies that headers listed here will be allowed in addition to the default safelist; but the actual behavior is to replace the default safelist (except for Origin).

You can see this behavior in the upstream CORS code that we rely on.

Steps to reproduce

  1. Configure OTLP/HTTP+JSON, and run a test application that uses opentelemetry-js to send metrics to it.
  2. Try to set the allowed_headers to anything you'd like.
  3. Observe that CORS preflight requests (and consequently, metrics export requests) will fail.

What did you expect to see? I expected to see requests succeeding. πŸ˜„

What did you see instead? I saw all requests failing. πŸ˜“

What version did you use? Version: v0.61.0, but this is not necessarily version-dependent

What config did you use? Config:

(NB: the astute observer will note that we didn't need to set this option. We ... are now also aware of this fact πŸ˜„ )

  receivers:
    otlp:
      protocols:
        grpc:
          include_metadata: true
        http:
          endpoint:
          include_metadata: true
          cors:
            allowed_origins:
              - http://*
            allowed_headers:
              - X-Geo-Country-ISO-Code

  processors:
    batch:
    memory_limiter:
      limit_mib: 3072
      spike_limit_mib: 512
      check_interval: 5s
    resource/geo:
      attributes:
        - key: geo.country_iso_code
          from_context: metadata.X-Geo-Country-ISO-Code
          action: insert

  exporters:
    logging:
      loglevel: debug
    unreleased_internal_thing_sorry:
      endpoint: 0.0.0.0:9000
      tls:
        insecure: true

  service:
    pipelines:
      metrics:
        receivers: [otlp]
        processors: [memory_limiter, resource/geo, batch]
        exporters: [logging, unreleased_internal_thing_sorry]

Environment OS: 🀫 I don't think I am allowed to say 🀫 Compiler(if manually compiled): go 1.19

Additional context

I believe this can be addressed in one of two ways:

  1. A simple documentation PR could be enough to clarify the actual behavior of setting the CORS allowed_headers option.
  2. Alternately, a slightly more involved PR could be opened that would always merge in the default "safe" CORS headers, as the documentation currently implies.

I'm happy to make either fix: please just let me know which one the maintainers would prefer.

codeboten commented 1 year ago

@ahayworth if you'd like to open a PR to fix the behaviour, I believe #2 is the correct way to proceed

ahayworth commented 1 year ago

Fantastic, I'll take that approach. I still intend to fix it (likely tonight or tomorrow night), I just haven't been able to squeeze in a few minutes to write any code in the past few days. πŸ˜†