opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Review sequence for HTTP and gRPC middleware

Open jmacd opened this issue 7 months ago • 2 comments
trafficstars

Component(s)

No response

Describe the issue you're reporting

I reviewed https://github.com/advisories/GHSA-mh63-6h87-95cp which registered as a CVE for the collector, and it indicated a "high" impact due to the ability of an unauthorized request to trigger a relatively large memory allocation. With this in mind, while working on #7441 as part of #12603, I studied the order in which middleware layers may or may not cause allocations before the authorization extension runs.

For the configgrpc package, we see:

  • otel instrumentation starts before auth because StatsHandler, the gRPC API it uses, is that way.
  • client metadata is copied after auth succeeds by construction

For the confighttp package, the sequence of events appears haphazard. Because the calling convention for building an http.Handler encloses the existing handler, handlers which are registered after the auth handler, in the code, are the ones that run before the auth handler. We see:

  • otel instrumentation runs before auth
  • client metadata is copied before auth succeeds (⚠)
  • response headers are set before auth succeeds (⚠)
  • CORS is checked before auth
  • max-request-size is checked after auth
  • decompression is applied after auth.

It's not clear that the sequence is intentional. Questions:

  1. Should client metadata be copied after auth succeeds?
  2. Should response headers be set after auth succeeds?

I don't see these as security issues, however (1) is O(1) apart from the CVE above and (2) potentially leaks information to unauthorized requests. May we add comments to watch for the reverse-ordering of interceptors in the HTTP setup routine and move client-metadata and response-header interceptors to inside the authorized section?

jmacd avatar Apr 14 '25 20:04 jmacd