wasi-http: Filter forbidden headers

[elliottt]

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.

[alexcrichton]

Should this be backported to Wasmtime 15 as well?

[sporkmonger]

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.

[dicej]

@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:

• 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_header is used when constructing headers for any purpose. Was that intentional? I'm assuming so, but wanted to verify.
• I can see that the WasiHttpView trait also has an is_forbidden_header function, presumably so embedders can override it, but AFAICT the WasiHttpView version 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_HEADERS list 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.

[elliottt]

• 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_header is 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 WasiHttpView trait also has an is_forbidden_header function, presumably so embedders can override it, but AFAICT the WasiHttpView version 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 as is_forbidden_header(self, ...) rather than as a method of self.

wasmtime/crates/wasi-http/src/types_impl.rs

Line 131 in 8abaa75

if is_forbidden_header(self, &header) {

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

wasmtime/crates/wasi-http/src/types.rs

Line 253 in 8abaa75

FORBIDDEN_HEADERS.contains(name) || view.is_forbidden_header(name)

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

wasmtime/crates/wasi-http/tests/all/main.rs

Lines 204 to 208 in 8abaa75

	async fn wasi_http_proxy_tests() -> anyhow::Result<()> {
	let req = hyper::Request::builder()
	.header("custom-forbidden-header", "yes")
	.uri("http://example.com:8080/test-path")
	.method(http::Method::GET);

• Where did the FORBIDDEN_HEADERS list 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)

[dicej]

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.

[lukewagner]

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).

[pchickey]

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?

[dicej]

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!

[lukewagner]

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?).

[dicej]

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.

[pavelsavara]

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

[dicej]

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.

[pchickey]

from @sporkmonger:

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.

I just ran into this exact situation - I don't believe these headers should be stripped out of incoming requests because I have wasi-http guests who need to examine those headers - like the parent comment, to compose detection logic with request handlers.