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

Envoy should not alter thrift THeader key #20595

Closed
fishy opened this issue Mar 30, 2022 · 6 comments
Closed

Envoy should not alter thrift THeader key #20595

fishy opened this issue Mar 30, 2022 · 6 comments
Labels
area/thrift bug stale stalebot believes this issue/PR has not been touched recently

Comments

@fishy
Copy link

fishy commented Mar 30, 2022

If you are reporting any crash or any potential security issue, do not
open an issue in this repo. Please report the issue via emailing
envoy-security@googlegroups.com where the issue will be triaged appropriately.

Currently envoy always lowercase thrift THeader key and this is wrong

Description:
Thrift THeader keys are case-sensitive according to the spec, so altering them is a bug.

This is the line that does it:

const Http::LowerCaseString key = Http::LowerCaseString(key_string);

Repro steps:
Make a thrift client and send "Foo: Bar" THeader to a thrift server via envoy. The server receives "foo: Bar" instead.

Note: The Envoy_collect tool
gathers a tarball with debug logs, config and the following admin
endpoints: /stats, /clusters and /server_info. Please note if there are
privacy concerns, sanitize the data prior to sharing the tarball/pasting.

Admin and Stats Output:

Include the admin output for the following endpoints: /stats,
/clusters, /routes, /server_info. For more information, refer to the
admin endpoint documentation.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Config:

Include the config used to configure Envoy.

Logs:

Include the access logs and the Envoy logs.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Call Stack:

If the Envoy binary is crashing, a call stack is required.
Please refer to the Bazel Stack trace documentation.

@marcoferrer
Copy link

Linking thrift issue requesting clarification on key format in the header spec
https://issues.apache.org/jira/browse/THRIFT-5541

@daixiang0 daixiang0 added area/thrift and removed triage Issue requires triage labels Mar 31, 2022
@daixiang0
Copy link
Member

Good catch, we should follow the spec.

fishy added a commit to fishy/envoy that referenced this issue Apr 5, 2022
Fixes envoyproxy#20595.

Signed-off-by: Yuxuan 'fishy' Wang <yuxuan.wang@reddit.com>
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 30, 2022
@fishy
Copy link
Author

fishy commented Apr 30, 2022

not stale

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 30, 2022
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 30, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/thrift bug stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants