-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
net/http: Allow obtaining original header capitalization #37834
Comments
Hey @JohnRusk! I understand that one of your concerns is with Azure/azure-storage-azcopy#113, in which the API is expecting capitalized headers, despite RFC2616 specifying that header fields are to be treated as case-insensitive. Issue #5022 was resolved by preserving case on outbound requests: I don't think it's reasonable to keep a mapping of every header field transformation around for every request. As @bradfitz mentioned, http/2 explicitly lowercases all fields. I feel like making changes here for broken servers (which I understand are out of your control) doesn't seem reasonable. Is it possible to keep a map of these special header fields around in your application and re-write them on your outbound request? I assume there is a subset that are sensitive, and it is not all HTTP headers. Their casing is preserved on outbound requests as I understand #5022. |
Not really, because the set of header fields that exists is customer-determined, and may vary from customer to customer and even from file to file.
What about some kind of hook then, some kind of function that get's called as they are pulled of the wire, and before they are canonlicalized. If the hook is null, it doesn't get called. e.g.(roughly)
Then customers like us could provide request.headerNameNotifier and everyone else could leave it nil. |
There are a few dup bugs in this space with a bunch of conversation. I can't find them at the moment (GitHub search is failing me) but they're somewhere. |
I had trouble searching too. I have managed to find a few though. Are any of these the ones you were thinking of?
|
@ACECEO yes, it's a workaround in the sense that it works. No in the sense that it's a totally different product, with a different emphasis and style of usage. For customers who use the tool I work on, AzCopy, this Go issue remains a blocker. |
@bradfitz Any update re thoughts on this? As noted above, the key issue is that we currently can't write case-preserving code that reads headers and forwards them on. In our tool, AzCopy, this is a big deal for certain customers who need to move their data around in Azure. |
I've read the RFC2616, it just mentions that the header is case-insensitive. Nothing says that you should modify the the headers as you forward them. |
I feel like I've replied to this a number of places, but I'll summarize here as well.
As such, any fix here must have super minimal cost, both in terms of API surface (cogntitive load cost, reading more godoc) and runtime cost (we shouldn't allocate or populate a new data structure with this info unless the calling code opted in to wanting that). Notably, we can't change the representation of It probably needs to be opt-in at the Maybe we just add a new If somebody has an alternate API proposal or implementation I'll take a look, assuming it meets these general requirements. |
Hi Brad. That suggestion about Will it work client side too? (Just checking because you mention |
My reservation with a method is that they render pretty prominently in godoc HTML compared to, say, a struct field.
It would be cleaner, but OTOH this is definitely in the realm of "you should know what you're doing" territory so making a super comfortable & safe API isn't the highest priority. BTW, the other constraint that would need to be documented on the lifetime of this slice is that it's only valid before the Request.Body is read from. Because that can also advance the buffer. But now that I think about it more, I'm not even sure that this API works, as I think we only use a 2KB or 4KB bufio.Reader to read from the connection (so that's all we have in memory at a time) and we permit by default up to 1MB of headers (https://golang.org/pkg/net/http/#DefaultMaxHeaderBytes). So So we probably do need some sort of opt-in mechanism, and then it can't work in HTTP middleware packages that don't have control over the http.Server setup.
Yeah, it probably should. I don't think we'd want to accept a change that only did one, lest it turn out that whatever API we pick isn't sufficient for the other. Seeing it work with both would be a good sign that the API was sufficient. |
Good point re whether it should be documented prominently or not. (I agree with you that "not prominent" is preferable). The question of whether it only works on small requests seems key. I think folks who have meaningful info in headers (such as some of our users) could easily have more than 4KB. Do you have any other suggestions? Shall we (our team here) see if we can come up with any alternative that meets the general requirements that you've outlined? |
As I understand this problem, this prevents many users to just use traefik as a plugin-proxy, because it ALTERS the headers. There is a ton of software which is using case-sensitive headers and a transparent proxy should not change them... |
For additional context - There are currently multiple intercepting HTTP proxies which are affected by this issue, including goproxy and martian, plus all the various tools built on these libraries. When building intercepting proxies for security testing or logging, For example, attempting to use glorp proxy (based on Martian) to test a mobile application recently lead to issues as the client required specific capitalisation. Fixing the third-party mobile application to become RFC compliant was not feasible - and instead the testing had to be completed using a non-Go based intercept proxy. |
For my use-case I was able to work around this issue with some semi-sketchy binary patching. I don't suggest anyone use this approach, at least not without significant testing, but it worked to sort out my specific issue: |
Brad, Regarding HTTP/1:
Four years since you posted this reply, browsers still use HTTP/1.1 for websockets. Given the slow trickle of issues referencing this one, HTTP/1 vs HTTP/2 is starting to smell a bit like IPv4 vs IPv6. While we'd prefer to focus on the relatively modern version of a protocol, there are still compelling reasons to continue supporting the old one. I don't disagree with your point that this should be something low-touch and low-impact, particularly when not opting in. What would it take to get something actually moving towards a solution here? Those of us who have hit this issue are somewhat stuck. I can think of a whole bunch of options to solve this, with various trade-offs. For example, for my purposes I would be fine if I could look up the original case of the every header key so that I can decide what to send when proxying a request by just mapping the keys to the old case post-processing. I can probably live with the corner case of there being repeated headers pre-normalisation. But, I imagine that wouldn't suffice for other use-cases. For what it's worth, I ran into this because a websocket server we're proxying to is doing a case-sensitive lookup of Sec-WebSocket-Key, which ultimately breaks the websocket handshake. We do not have any control over the server, so we can't influence it to do the right thing. We're left with figuring out a way to maintain the case sent by the browser, but http.Server stands in our way. We could certainly just patch up this particular key, but I'm sure we'll find other instances of this problem, so a general solution feels appropriate. |
### Motivation After [pip-279](apache/pulsar#20627), all properties keys and values use json string save to header: `X-Pulsar-Property` This PR to compatible with this change when using subscription admin API. Also, Using `pip-279` also avoids the issue where the Go HTTP client automatically formats HTTP headers: golang/go#37834, This will impact the peek command, the previous method might retrieve `properties` with inconsistent casing compared to the user-defined. ### Modifications - Compatible the HTTP header properties with PIP-279
What version of Go are you using (
go version
)?1.13
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?Windows, x64
What did you do?
Make a HTTP request, read it's headers, send those headers somewhere else.
What did you expect to see?
Headers are preserved exactly
What did you see instead?
Header capitialization gets canonicalized
I realize this has been discussed before (about 7 years ago).
However, in the time since then many users have been adversely affected by this. E.g.
traefik/traefik#466
Azure/azure-storage-azcopy#113
So I'd like to find out, would the Go team consider not a change to the current behaviour, but simply a way for an HTTPResponse to provide an additional map, that maps canonicalizedName -> originalName. If we could just get that map, then those of us who really need header case preservation could use the information it contains to achieve what we need. (Find out the original capitalization when we read a response, and then directly manipulate the outbound request map when we forward that data on).
I'd be happy to contribute the code for the above, if we expected it to be accepted.
The text was updated successfully, but these errors were encountered: