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

Bugfix/handling headers for Authorization and Host #65

Merged
merged 1 commit into from
Jun 6, 2016

Conversation

mwitkow
Copy link
Member

@mwitkow mwitkow commented Oct 27, 2015

Upstream gRPC has a bug where the HTTP2 headers aren't case sensitive. As such, if you call the HTTP1.1 endpoint of GRPC-GW with Authorization: Bearer foo, this will be translated into a Authorization field of the gRPC Metadata on the server. It means that code written to understand authorization header (e.g. based on the gRPC credentials/oauth) won't work:

upstream bug: grpc/grpc-go#415

Add a work around by setting the authorization header to lower case. Additionally add propagation of the Host header.

Add tests.

@yugui
Copy link
Member

yugui commented Oct 28, 2015

Thanks. LGTM for Authorization header.
It would be even better if you split this PR into two; one for each header.

For Host header, I am wondering if it should be forwarded as a forwarded metadata according to RFC 7239. What do you think?

@mwitkow
Copy link
Member Author

mwitkow commented Oct 28, 2015

@yugui regarding RFC 7239:
the change is in accordance to Section 5.3 of that RFC. It says that the original Host header of the proxy request is passed into the Host header of the request to the backend.

The forwarded header is meant for forwarding information about the client, e.g. the client IP of the request that the gRPC Gateway got, and wants to pass this information to the backend. Which would be useful to do anyway, but probably in a different PR.

I can split it up into two PRs, but I'm pretty swamped so I won't be able to do that until the end of the week.

@yugui
Copy link
Member

yugui commented Oct 29, 2015

the change is in accordance to Section 5.3 of that RFC. It says that the original Host header of the proxy request is passed into the Host header of the request to the backend.

IIUC, the section is talking about host parameter of forwarded header but not forwarding host header itself.

@mwitkow
Copy link
Member Author

mwitkow commented Nov 10, 2015

@yugui you're correct, apologies for misreading :)

The PR has been updated with use of x-forwarded-host, and additionally added support for x-forwarded-for to indicate the caller's client ID.

I thought about splitting it into another PR, but the tests are slightly coupled and it was easier to do in a single PR. I can separate it out if you strongly prefer that :)

@mwitkow
Copy link
Member Author

mwitkow commented Feb 10, 2016

@yugui, can you help me move this forward? :) We're maintaining that in our own fork, and would really like to have this upstream :)

pairs = append(pairs, "authorization", val[0])
}
}
if req.Header.Get(xForwardedHost) != "" {
Copy link
Member

Choose a reason for hiding this comment

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

    if host := req.Header.Get(xForwardedHost); host != "" {
        pairs = append(pairs, strings.ToLower(xForwardedHost), host)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@yugui
Copy link
Member

yugui commented Feb 16, 2016

Sorry for being late.

Almost LGTM except several things

  • The comment I posted above
  • Merge conflict
  • What do you think about Forwarded header in RFC 7239?

@mwitkow
Copy link
Member Author

mwitkow commented May 12, 2016

Apologies it took me so long to get up to speed on this, we've been running this in prod for a while on our git subtree and had no time to upstream :(

Updated to comply with your comment and also rebased upon master with squashing commits.
Regarding RFC 7239, it'd be great to implement that, however it's significantly more complicated to parse/set, and everyone supports X-Forwarded-For anyway for legacy purposes.

@yugui yugui merged commit 17b6bbc into grpc-ecosystem:master Jun 6, 2016
ithinker1991 pushed a commit to tronprotocol/grpc-gateway that referenced this pull request Jun 20, 2018
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