Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11): Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11):elliottt requested wasmtime-core-reviewers for a review on PR #7538.
Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11):elliottt opened PR #7538 from elliottt:trevor/filter-out-forbidden-headrs to bytecodealliance:main:
Make sure that forbidden headers don't show up in either
incoming-requestor
outgoing-responsevalues.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11):elliottt requested alexcrichton for a review on PR #7538.
Wasmtime GitHub notifications bot (Nov 14 2023 at 21:19):elliottt edited PR #7538:
Make sure that forbidden headers don't show up in either
incoming-requestoroutgoing-responsevalues. One place that this PR doesn't currently address is incoming trailers. Thefuture-trailers.getmethod returns a newfieldsresource every time it's called once it has resolved, and it's not clear where the best place to filter forbidden headers is in that case. I think the right fix for that would be to makefuture-trailers.getmethod return a result exactly once, like our otherfuture-*.getmethods, enabling us to apply the filter when we create the new ownedfieldsresource.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 14 2023 at 21:19):elliottt requested pchickey for a review on PR #7538.
Wasmtime GitHub notifications bot (Nov 14 2023 at 21:38):alexcrichton submitted PR review:
Should this be backported to Wasmtime 15 as well?
Wasmtime GitHub notifications bot (Nov 14 2023 at 22:51):elliottt merged PR #7538.
Wasmtime GitHub notifications bot (Dec 21 2023 at 22:31):sporkmonger commented on PR #7538:
I'm wondering if this is the really the right place for this logic, particularly on incoming requests. I'm using the incoming request type in the context of detections (rather than in request handlers) where it's preferred that these headers _aren't_ stripped because they may carry information indicating malicious behavior by a client. If they're stripped in a constructor before they can be inspected, detections will be potentially working with missing crucial information. I want to be able to compose detection logic with request handlers, which is why I'm using the same type.
Wasmtime GitHub notifications bot (Dec 21 2023 at 22:34):sporkmonger edited a comment on PR #7538:
I'm wondering if this is the really the right place for this logic, particularly on incoming requests (as opposed to outgoing requests). I'm using the incoming request type in the context of detections (rather than in request handlers) where it's preferred that these headers _aren't_ stripped because they may carry information indicating malicious behavior by a client. If they're stripped in a constructor before they can be inspected, detections will be potentially working with missing crucial information. I want to be able to compose detection logic with request handlers, which is why I'm using the same type.
Wasmtime GitHub notifications bot (Aug 27 2024 at 21:13):@elliottt Sorry for chiming in super late here, but @bacongobbler and I tripped over this code today and have some questions about it.
For context: we're currently working on porting an ASP.NET Core app to WASI. This app happens to use Dapr, which in turn uses gRPC, which sets a
TE: trailersheader on outgoing requests, which is currently rejected byis_forbidden_headerby way ofHostFields::from_list.Our questions:
- In your PR summary, you said "Make sure that forbidden headers don't show up in either incoming-request or outgoing-response values," but this code also affects outgoing requests as well since
is_forbidden_headeris used when constructing headers for any purpose. Was that intentional? I'm assuming so, but wanted to verify.- I can see that the
WasiHttpViewtrait _also_ has anis_forbidden_headerfunction, presumably so embedders can override it, but AFAICT theWasiHttpViewversion is never used -- only the top-level function is used. Am I missing something? Or, if not, was that also intentional?- Where did the
FORBIDDEN_HEADERSlist come from? I found https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name and see some overlap, but it's not an exact match.I have other questions about how to use gRPC via wasi-http, but they're probably off topic here, so I'll ask those elsewhere.
Wasmtime GitHub notifications bot (Aug 27 2024 at 21:35):elliottt commented on PR #7538:
- In your PR summary, you said "Make sure that forbidden headers don't show up in either incoming-request or outgoing-response values," but this code also affects outgoing requests as well since
is_forbidden_headeris used when constructing headers for any purpose. Was that intentional? I'm assuming so, but wanted to verify.That was intentional. The list seemed pretty small, and the overlap between requests and responses seemed acceptable.
- I can see that the
WasiHttpViewtrait _also_ has anis_forbidden_headerfunction, presumably so embedders can override it, but AFAICT theWasiHttpViewversion is never used -- only the top-level function is used. Am I missing something? Or, if not, was that also intentional?It's called in
types_impl.rs, but called asis_forbidden_header(self, ...)rather than as a method ofself.There's a test that ensures it's working by adding a custom forbidden header.
- Where did the
FORBIDDEN_HEADERSlist come from? I found https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name and see some overlap, but it's not an exact match.It grew slightly over time, and was informed by some folks that @lukewagner coordinated with in Fastly, he might remember more context about where the full list came from.
Wasmtime GitHub notifications bot (Aug 27 2024 at 21:38):elliottt edited a comment on PR #7538:
- In your PR summary, you said "Make sure that forbidden headers don't show up in either incoming-request or outgoing-response values," but this code also affects outgoing requests as well since
is_forbidden_headeris used when constructing headers for any purpose. Was that intentional? I'm assuming so, but wanted to verify.That was intentional. The list seemed pretty small, and the overlap between requests and responses seemed acceptable.
- I can see that the
WasiHttpViewtrait _also_ has anis_forbidden_headerfunction, presumably so embedders can override it, but AFAICT theWasiHttpViewversion is never used -- only the top-level function is used. Am I missing something? Or, if not, was that also intentional?It's called in
types_impl.rs, but called asis_forbidden_header(self, ...)rather than as a method ofself.That function ultimately bottoms out in the
view's function here:There's a test that ensures it's working by adding a custom forbidden header.
- Where did the
FORBIDDEN_HEADERSlist come from? I found https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name and see some overlap, but it's not an exact match.It grew slightly over time, and was informed by some folks that @lukewagner coordinated with in Fastly, he might remember more context about where the full list came from.
(edited to add the link to the actual call to
view.is_forbidden_header)
Wasmtime GitHub notifications bot (Aug 27 2024 at 21:49):Thanks, @elliottt! As I learn more about gRPC, I'm realizing that gRPC-on-wasi-http isn't possible given the current state of wasi-http, so we'll pursue the wasi-sockets route instead.
Wasmtime GitHub notifications bot (Aug 27 2024 at 21:50):lukewagner commented on PR #7538:
It grew slightly over time, and was informed by some folks that @lukewagner coordinated with in Fastly, he might remember more context about where the full list came from.
FWIW, the list is in this doc, this section. Ideally I think this list would go into the wasi-http spec itself, but what's in the doc was the result of asking various HTTP folks (inside and outside Fastly).
Wasmtime GitHub notifications bot (Aug 27 2024 at 22:04):pchickey commented on PR #7538:
It's unfortunate that wasi-http can't support gRPC as currently specified! Concretely, it seems to me that setting a header
TE: trailers(directives list here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/TE) and other values should be "ok" if the underlying http implementation is actually able to accept those (e.g. whatwg fetch cannot supportTE: trailers, but hyper can). Should we refine the spec to allow this behavior?
Wasmtime GitHub notifications bot (Aug 27 2024 at 22:14):We'll also need some way for the guest to _require_ HTTP/2.
And yeah, I'd definitely be in favor of making wasi-http gRPC-compatible!
Wasmtime GitHub notifications bot (Aug 27 2024 at 22:50):lukewagner commented on PR #7538:
Yes, agreed on the goal of making wasi-http gRPC-compatible; that has been a goal since the beginning (and the reason we have
trailersin the first place).The goal of putting
TEin the definitely-forbidden list is so that the host is empowered to set this as a transport-/implementation-detail (similar toTransport-Encoding). Could we perhaps have some "expect-trailers" flag onoutgoing-requestthat clients can set that tells the host impl to both setTE: trailersand also require H2 (or, reading RFC 9110 6.5.1, maybe it even works for HTTP/1.1?).
Wasmtime GitHub notifications bot (Aug 27 2024 at 22:59):I think the trailers issue and the "require H2" issue are somewhat orthogonal. The latter is because gRPC is defined to use HTTP/2 specifically.
Wasmtime GitHub notifications bot (Aug 28 2024 at 06:54):pavelsavara commented on PR #7538:
This will be impossible or interesting in JCO because of browser
fetchlimitations.
Wasmtime GitHub notifications bot (Aug 28 2024 at 12:11):This will be impossible or interesting in JCO because of browser
fetchlimitations.That's true, but a wasi-aockets-based solution would also be difficult or impossible. The grpc-web protocol exists specifically to handle the browser case, so presumably an app intended to be maximally portable would be prepared to fall back to that.
Last updated: Nov 22 2024 at 16:03 UTC