Skip to content
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

Bandit.HTTP2.Stream.StreamError when trying to use Envoy HTTP/2 listeners #159

Closed
benmurden opened this issue Jun 8, 2023 · 3 comments · Fixed by #160
Closed

Bandit.HTTP2.Stream.StreamError when trying to use Envoy HTTP/2 listeners #159

benmurden opened this issue Jun 8, 2023 · 3 comments · Fixed by #160

Comments

@benmurden
Copy link

We have Bandit running fine under HTTP/1 with our Envoy App Mesh setup, but see the following error when trying to switch to HTTP/2:

Bandit.HTTP2.Stream.StreamError request target scheme does not agree with transport

The error is reported as coming from this line, but I'm not sure where to go after this.

{:error, reason} -> raise Bandit.HTTP2.Stream.StreamError, reason

Cowboy seems to permit the connection, though I understand it opts to fail silently in most error cases.

If anyone has any advice about where else to look, I'll do my best to provide more information.

@moogle19
Copy link
Contributor

moogle19 commented Jun 8, 2023

The error is cause by determine_scheme/2 which matches the scheme in the HTTP header with the used Transport.
In you case the header probably indicates https but you Envoy Proxy forwards the requests via http?

@mtrudel
Can we remove the matching between the used transport and the scheme header and just fallback to the transport, if the scheme header is missing or invalid?

@mtrudel
Copy link
Owner

mtrudel commented Jun 8, 2023

Can we remove the matching between the used transport and the scheme header and just fallback to the transport, if the scheme header is missing or invalid?

The danger if we do that is that the URI we construct & pass to the plug may not reflect the actual on-the-wire protocol used (the specific risk is that we indicate the connection to be https when in point of fact it's being delivered over http).

This is really the same distinction as we previously had to make for the port value, only for the scheme value this time around, so I was initially inclined to say that we should solve it the same way we did then....

EXCEPT in digging into the RFCs just now, I realized that RFC9113§8.3.1 is actually normative about schemes, at least for HTTP/2:

The ":scheme" pseudo-header field includes the scheme portion of the request target. The scheme is taken from the target URI (Section 3.1 of [RFC3986]) when generating a request directly, or from the scheme of a translated request (for example, see Section 3.3 of [HTTP/1.1]).

I would read that as suggesting that we should actually be using the scheme as declared in the :scheme pseudo header, without concern for the actual underlying transport (put another way, we should be determining the scheme based on what the user thinks they're connecting to, contrary to the conclusion I've arrived at previously for the port), at least for HTTP/2.

(Other clauses in the RFC9113§8.3.1 also specify a similar approach for ports, so I'm going to have to rationalize my approach taken on that other issue as well). Upon a closer read I don't think this is the case. The other issue is fine as currently dispositioned.

In the meantime, what does that mean here? I think we should accept whatever scheme is declared in the request (for both HTTP/1 and HTTP/2 for consistency). I don't see a whole lot of value to logging on a transport mismatch, since in cases such as the originally reported one, we'll be logging that warning on every request.

@mtrudel
Copy link
Owner

mtrudel commented Jun 8, 2023

Merged & released as 1.0.0-pre.6!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants