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

BaggageSpanProcessor opens up security holes #167

Open
martinjt opened this issue Jan 9, 2022 · 1 comment
Open

BaggageSpanProcessor opens up security holes #167

martinjt opened this issue Jan 9, 2022 · 1 comment
Labels
type: discussion Requests for comments, discussions about possible enhancements.

Comments

@martinjt
Copy link
Member

martinjt commented Jan 9, 2022

I've been giving Baggage a lot of thought recently and it's really worrying me that we've implemented the span processor in the way we have.

Number 1 is that because baggage is just a header on the inbound, a consumer of an API can pass their own baggage over and have persisted in that storage. Meaning that, potentially, they could store malicious payloads. As they're headers, I'm pretty sure they'll be encoded already, but I'm not clear on whether they're decoded before they're then added to the spans.

Further to that, they could provide an excessive amount of baggage parameters that would
a) cause slow downs into the application and;
b) exceed the max columns in the dataset when using honeycomb.

I'm thinking 3 things at this stage.

  1. encode the parameters before they get added to spans.
  2. allow a max amount of baggage parameters to be passed to spans with a sensibly low default
  3. allow a prefix to be used on the baggage to propagate.

I may be completely blowing this out of proportion here, but it's worrying me that this could produce a very bad practice and help people fall into the pit failure rather than success.

Thoughts welcomed, and once we have a plan, I'll happily submit the PR.

@martinjt martinjt added the type: discussion Requests for comments, discussions about possible enhancements. label Jan 9, 2022
@robbkidd
Copy link
Member

robbkidd commented Jan 11, 2022

Thanks, @martinjt! These are definitely legitimate concerns. We've got some work scheduled this week to investigate our options around mitigating propagation issues with the Honeycomb OTel distros' "multi-span attributes." (Currently implemented via Baggage and the BaggageSpanProcessor.)

Followup: I don't believe you are blowing anything out of proportion. Thanks for thinking ahead to what shenanigans people could get up to with the feature. 👍🏻

@JamieDanielson JamieDanielson added this to the GA Release milestone Jan 19, 2022
@MikeGoldsmith MikeGoldsmith modified the milestone: GA Release Aug 24, 2022
@MikeGoldsmith MikeGoldsmith removed this from the GA Release milestone Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: discussion Requests for comments, discussions about possible enhancements.
Projects
None yet
Development

No branches or pull requests

4 participants