-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
gRPC-Go should drop received headers containing upper ASCII (0x80+) characters #5318
Comments
The change was intended to bring us in line with the HTTP spec, but I think there is a bug in the enforcement. RFC7230 says:
|
@dfawley Sounds reasonable to fix, I think. Is there anything I can help with to have that fixed? |
I did a little more digging into this. It looks like the gRPC spec itself forbids this: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests
Also, gRPC-Java for sure will have trouble with these values (I'm not sure about C++/other implementations). The only reason it worked for go->go servers and clients is that both implementations assumed UTF-8, which would not be the case in other languages. For this reason, I believe it is a bug that we were allowing these to be used before, and that the validation, as added is mostly working as intended. I do apologize for the breakage, though, which was unexpected because we did not realize that the underlying HTTP framer implementation was accepting unicode values. Instead of transmitting the strings directly, it is recommended to percent-encode them first (and decode them on the receiving side). However, we do still have a bug. The spec goes on to say:
This means that on the receiving side, we also need to drop these headers, as we have no way to distinguish between metadata received over the wire and metadata created by the application. |
Any update on this? |
Yeah, I just got burned by this too. Whether the original behavior was a bug or not, introducing a breaking change like this is not a small thing. (If we're going to start silently dropping the headers on the server-side too, I'm concerned that will also lead to more confusion.) |
I'm sorry. Although I implement this validating, I getting burned by this too now. We didn't anticipate unicode is being widely used on header value. |
i have the same problem, does not have a better solution |
any solution found besides downgrading? |
It should be possible to metadata key names ending with |
We ran into situations where public clients were sending HTTP requests with unsupported non-ascii characters in the `x-datadog-origin` headers. These headers were being propagated, via the [`HTTPPropagator.extract`](https://github.com/DataDog/dd-trace-py/blob/fd0820b9d7b561d6b75b75c6865a5b3b5792054d/ddtrace/propagation/http.py#L835) and [gRPC client interceptor](https://github.com/DataDog/dd-trace-py/blob/c5dfcec45ae21483171c0ac148f94e61ade0b4d2/ddtrace/contrib/grpc/client_interceptor.py#L203), to branching gRPC calls. The gRPC calls are then failing on the client side due to gRPC client side validation which [forbids metadata values with non-ascii](grpc/grpc-go#5318) characters. The proposed solution here will prevent these unsupported headers values from being propagated by preventing them from being set within `Context`. This approach may impact HTTP propagation to branching HTTP requests, but my understanding is that non-ascii characters are not supported in these situations either. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) - [x] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. - [x] This PR doesn't touch any of that. --------- Co-authored-by: ZStriker19 <[email protected]> Co-authored-by: Zachary Groves <[email protected]>
Hey @dfawley I'm currently facing some problems due to this "If accepted, care must be taken to make sure that the application is permitted to echo the value back as metadata." Do you know if there are any plans to get a fix for this? On the same topic:
|
We aren't scheduling this work right now, and I'm a bit nervous about such a behavior change, too. The only thing we can really do, given our API in Go, to follow the gRPC spec, is to start removing invalid incoming metadata. It's unclear to me whether this would be better or worse than the current state. Perhaps we need a new metadata API so that we can accept incoming invalid metadata but not permit the addition of new instances? |
NOPE! It was there for Latin-1. Latin-1 was the only RFC-allowed use, but... Anyway, the "obs" part of the HTTP spec stands for "obsolete" and shouldn't be used. From RFC 7230:
And yes, that means if you chose to support obs-text you have to define header values as bytes/bytestring in Java, Go, Python 3, and every other Unicode-enabled language. Basically nobody used RFC2047 for HTTP much; it looked like |
I'll also note
|
A have a question about a new version: v1.46.0 released on Apr 22.
The release adds a client-side validation of HTTP-invalid metadata before attempting to send (PR #4886).
One of the validation rules states that the gRPC request header value must contain one or more characters from the set [%x20-%x7E] (ref code) effectively excluding non-printable characters.
As a result, in some cases, bumping the package from v1.45 to 1.46 may become a breaking change
whenever header values pass on names and localized content (e.g. : Łukasz, Renée, Noël).
Was that an oversight or fully intentional to comply with RFC7230?
The text was updated successfully, but these errors were encountered: