Stream: git-wasmtime

Topic: wasmtime / PR #7538 wasi-http: Filter forbidden headers


view this post on Zulip Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11):

elliottt requested wasmtime-core-reviewers for a review on PR #7538.

view this post on Zulip 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-request or
outgoing-response values.
<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 14 2023 at 21:11):

elliottt requested alexcrichton for a review on PR #7538.

view this post on Zulip 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-request or outgoing-response values. One place that this PR doesn't currently address is incoming trailers. The future-trailers.get method returns a new fields resource 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 make future-trailers.get method return a result exactly once, like our other future-*.get methods, enabling us to apply the filter when we create the new owned fields resource.
<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 14 2023 at 21:19):

elliottt requested pchickey for a review on PR #7538.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 14 2023 at 21:38):

alexcrichton submitted PR review:

Should this be backported to Wasmtime 15 as well?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 14 2023 at 22:51):

elliottt merged PR #7538.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 21:13):

dicej commented on PR #7538:

@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: trailers header on outgoing requests, which is currently rejected by is_forbidden_header by way of HostFields::from_list.

Our questions:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 21:35):

elliottt commented on PR #7538:

That was intentional. The list seemed pretty small, and the overlap between requests and responses seemed acceptable.

It's called in types_impl.rs, but called as is_forbidden_header(self, ...) rather than as a method of self.

https://github.com/bytecodealliance/wasmtime/blob/8abaa75f55dbaddf6e4c3a5e5d26bf82fa53bc1e/crates/wasi-http/src/types_impl.rs#L131

There's a test that ensures it's working by adding a custom forbidden header.

https://github.com/bytecodealliance/wasmtime/blob/8abaa75f55dbaddf6e4c3a5e5d26bf82fa53bc1e/crates/wasi-http/tests/all/main.rs#L204-L208

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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 21:38):

elliottt edited a comment on PR #7538:

That was intentional. The list seemed pretty small, and the overlap between requests and responses seemed acceptable.

It's called in types_impl.rs, but called as is_forbidden_header(self, ...) rather than as a method of self.

https://github.com/bytecodealliance/wasmtime/blob/8abaa75f55dbaddf6e4c3a5e5d26bf82fa53bc1e/crates/wasi-http/src/types_impl.rs#L131

That function ultimately bottoms out in the view's function here:

https://github.com/bytecodealliance/wasmtime/blob/8abaa75f55dbaddf6e4c3a5e5d26bf82fa53bc1e/crates/wasi-http/src/types.rs#L253

There's a test that ensures it's working by adding a custom forbidden header.

https://github.com/bytecodealliance/wasmtime/blob/8abaa75f55dbaddf6e4c3a5e5d26bf82fa53bc1e/crates/wasi-http/tests/all/main.rs#L204-L208

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)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 21:49):

dicej commented on PR #7538:

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.

view this post on Zulip 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).

view this post on Zulip 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 support TE: trailers, but hyper can). Should we refine the spec to allow this behavior?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 22:14):

dicej commented on PR #7538:

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!

view this post on Zulip 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 trailers in the first place).

The goal of putting TE in the definitely-forbidden list is so that the host is empowered to set this as a transport-/implementation-detail (similar to Transport-Encoding). Could we perhaps have some "expect-trailers" flag on outgoing-request that clients can set that tells the host impl to both set TE: trailers and also require H2 (or, reading RFC 9110 6.5.1, maybe it even works for HTTP/1.1?).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 27 2024 at 22:59):

dicej commented on PR #7538:

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.

view this post on Zulip 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 fetch limitations.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2024 at 12:11):

dicej commented on PR #7538:

This will be impossible or interesting in JCO because of browser fetch limitations.

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: Dec 23 2024 at 12:05 UTC