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

mux: forward X- headers #1264

Merged
merged 5 commits into from
Apr 9, 2021
Merged

mux: forward X- headers #1264

merged 5 commits into from
Apr 9, 2021

Conversation

scarlettperry
Copy link
Contributor

@scarlettperry scarlettperry commented Apr 8, 2021

Description

Background:
By default, http request headers are passed to gRPC context via the DefaultHeaderMatcher rule. If the request headers don't meet certain preconditions, they are dropped.

Solution:
PR adds a custom header rule so that X- request headers are forwarded (and prefixed with grpcgateway). All other headers will be checked by the DefaultHeaderMatcher rule.

Use case for this change:
For the slackbot work, we need to verify a request came from Slack by certain X- request headers sent in the request.

Testing Performed

Unit test and locally with #1250

@scarlettperry scarlettperry requested a review from a team as a code owner April 8, 2021 14:53
@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 8, 2021

RE comment - want to double check that we should append the grpcgateway prefex to the -X headers?

If so, I noticed something interesting locally in which the context metadata list had both grpcgateway-x-forwarded-for and x-forwarded-for. Same situation with x-forwarded-host. This is because if those headers are not found ( which is the case since we're appending a prefix to the header), then it gets added by default here https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/context.go#L121-L134.

Might not be an issue but wanted to raise.

@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 8, 2021

side note: curious about

if tokens := md.Get("authorization"); len(tokens) > 0 {

authorization is part of the isPermanentHTTPHeader group, which gets the grpcgateway prefix, so why isn't the lookup grpcgateway-authorization? I ask b/c just want to make sure I'm not missing context that would be important to testing this PR.

  • the only thing I can think of is that perhaps the incoming http request is Grpc-Metadata-Authorization which then gets changed to Authorization but didn't find code / docs that mentioned this.

@scarlettperry scarlettperry mentioned this pull request Apr 8, 2021
2 tasks
@danielhochman
Copy link
Collaborator

re: authorization handling it's right above the other code you linked
https://github.com/grpc-ecosystem/grpc-gateway/blob/0a9acf6a7e6399acd4da6a2d24880bab0390f233/runtime/context.go#L102-L105

for X-Forwarded-For and X-Forwarded-Host is the result just that it will get appended twice, once with once without prefix if passed in by caller? alternatively i guess you could exclude those to avoid confusion.

backend/gateway/mux/mux.go Outdated Show resolved Hide resolved
backend/gateway/mux/mux.go Show resolved Hide resolved
@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 8, 2021

re: authorization handling it's right above the other code you linked
https://github.com/grpc-ecosystem/grpc-gateway/blob/0a9acf6a7e6399acd4da6a2d24880bab0390f233/runtime/context.go#L102-L105

for X-Forwarded-For and X-Forwarded-Host is the result just that it will get appended twice, once with once without prefix if passed in by caller? alternatively i guess you could exclude those to avoid confusion.

ah got it, thanks!

yep will get appended 2x, so list will have prefix-x-forwarded-for, x-forwarded-for, prefix-x-forwarded-host, x-forwarded-host. Okay will exclude these 2 from getting the prefix to avoid confusion.

@scarlettperry scarlettperry changed the title mux: forward all X- headers mux: forward X- headers Apr 8, 2021
Copy link
Collaborator

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

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

lgtm! 🚢

@scarlettperry scarlettperry merged commit f3693b6 into main Apr 9, 2021
@scarlettperry scarlettperry deleted the sperry-custom-header-matcher branch April 9, 2021 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants