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

[processor/remotetapprocessor] don't require Origin header #34925

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Aug 29, 2024

Description:

Currently remotetap runs a server using websocket.Handler
This internally creates a websocket.Server
that requires a Origin header in the incoming request.
While this makes sense for browser usage,
for the collector we may want to use other tools e.g. CLI tools which do not set an Origin header.

This changes the server to use websocket.Server directly, without setting Handshake,
bypassing the origin check

Link to tracking Issue: N/A issue reported in slack

Testing: manual https://cloud-native.slack.com/archives/C01N6P7KR6W/p1724957510485869?thread_ts=1724863668.431979&cid=C01N6P7KR6W

Documentation: none / bug fix

@atoulme
Copy link
Contributor

atoulme commented Aug 29, 2024

Could the user not use the server CORS settings to make sure the server is configured for the proper use case?

https://github.com/open-telemetry/opentelemetry-collector/tree/main/config/confighttp#server-configuration

Can you please add a test?

@seankhliao
Copy link
Contributor Author

no, cors in confighttp returns a header that allows the client to check in a preflight request if it is allowed to make a proper request. This is a server side check that the client includes an Origin header, which cli tools don't set (nor do they make preflight cors checks because cross origin isn't a concept that applies to a cli not run from a web origin).

I'm not sure how to add a test, the go websocket client in use requires an origin to be provided to work (which may not be true of other websocket libraries).

@j-kap-t
Copy link
Contributor

j-kap-t commented Aug 29, 2024

Confirmed this fixes the 403 forbidden issue I was having with the websocat CLI.

@atoulme
Copy link
Contributor

atoulme commented Aug 29, 2024

Please add changelog and let's get it in.

@seankhliao
Copy link
Contributor Author

added a changelog entry

@mwear mwear added the ready to merge Code review completed; ready to merge by maintainers label Aug 30, 2024
component: remotetapprocessor

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Origin header is no longer required for websocket connections
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any tests that help confirm this behaviour or a regression isn't added in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like I mentioned earlier in the pr thread, there's no easy way to test it unless we want to pull in another websocket library?

@mx-psi mx-psi merged commit 4a38719 into open-telemetry:main Sep 11, 2024
163 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/remotetap ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants