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

Allow http/2 requests to fallback to host header if authority pseudoheader is not provided #37531

Open
shawkins opened this issue Dec 5, 2023 · 18 comments
Labels
area/vertx kind/enhancement New feature or request

Comments

@shawkins
Copy link
Contributor

shawkins commented Dec 5, 2023

Description

See keycloak/keycloak#20959 - the user is reporting that AWS ALB is not appropriately including :authority nor x-forwarded- for http/2 requests. The resulting behavior for keycloak is to resolve 0.0.0.0 as the host.

It would appear this behavior by the ALB is not compliant with https://www.rfc-editor.org/rfc/rfc9113.html#section-8.3.1-1 so this is being captured as an enhancement, rather than a bug.

Implementation ideas

No response

@geoand
Copy link
Contributor

geoand commented Dec 6, 2023

cc @vietj

@vietj
Copy link

vietj commented Dec 6, 2023

pseudo header must be provided, otherwise it is rejected by the server an not valid HTTP/2 request, can you elaborate what is happening ?

@shawkins
Copy link
Contributor Author

shawkins commented Dec 6, 2023

@vietj see keycloak/keycloak#20959 for a more detailed description and reproducer that simulates a http/2 request without authority.

@vietj
Copy link

vietj commented Dec 6, 2023

I see, so the load balancer sends an HTTP/2 request that does not have an authority pseudo header but does have a host header as far as I understand ?

currently the vertx server differs from that (it ignores the host header) but it it does provides an authority made of the socket remote host and port values when no pseudo header is provided, which I am not certain is actually valid as it seems that providing a request without an authority has actually a specific semantic and is allowed.

maybe this is something we could actually do in vertx-web ForwardedParser instead of Vert.x HTTP request implementation because it already handles the x-forwarded for behavior.

@shawkins
Copy link
Contributor Author

shawkins commented Dec 6, 2023

I see, so the load balancer sends an HTTP/2 request that does not have an authority pseudo header but does have a host header as far as I understand ?

That is my understanding as well. cc @wadahiro

@hytgbn
Copy link

hytgbn commented Oct 8, 2024

Hi all, is there any plan to fix this or support :authority not existing but host existing case?

H2 doesn't REQUIRE :authority pseudo header.

https://www.rfc-editor.org/rfc/rfc9113.html#section-8.3.1-1
RFC mentions that :authority should be prioritized to Host, if it is present.

The recipient of an HTTP/2 request MUST NOT use the Host header field to determine the target URI if ":authority" is present.

Also under certain scenarios, :authority is not expected.

unless the original request's target URI does not contain authority information (in which case it MUST NOT generate ":authority").

So I think it's reasonable to support this fallback when authority is not provided. Or would introducing an option feasible approach?

@hytgbn
Copy link

hytgbn commented Oct 8, 2024

The code was changed from delegate.getHeader(HttpHeaders.HOST) to delegate.host() by this commit.

How do you think about making changes like

String host = delegate.host();
if (host == null or empty) {
    host = delegate.getHeader(HttpHeaders.HOST);
}

to provide backward compatibility to previous version?

@j-white
Copy link

j-white commented Dec 15, 2024

We're currently using Quarkus behind an AWS LB and this is preventing us from upgrading to 3.13.x or later.

Confirmed with a packet capture that the AWS LB does not set the :authority pseudo header, and the Host header is present. vertx doesn't seem to like this though.

Is there any way to manually set this header at runtime? I tried using a RouteFilter, but it appears the validation is done before these are invoked.

@cescoffier
Copy link
Member

@vietj we may need to adapt the Vert.x code.

@vietj
Copy link

vietj commented Dec 16, 2024

I confirm it might be a defect of HTTP server.

Is the LB translating from HTTP/1 to HTTP/2 ?

@vietj
Copy link

vietj commented Dec 16, 2024

I'll create an issue for it in Vert.x project

@j-white
Copy link

j-white commented Dec 16, 2024

Is the LB translating from HTTP/1 to HTTP/2 ?

In the case I tested it was HTTP/2 to HTTP/2 - using grpcurl as the client.

@vietj
Copy link

vietj commented Dec 16, 2024

why is the authority header missing in that case ? is there a Host HTTP header instead ?

@luisfelipess
Copy link

thanks @vietj - I can confirm that the ALB does not forward the request with the :authority pseudo header, but it does send the Host header on these requests.

@vietj
Copy link

vietj commented Dec 16, 2024

I think it hsould be tolerated however as HTTP/2 client it is totally misbheaving I think

@j-white
Copy link

j-white commented Dec 17, 2024

Client: grpcurl -vv coffee.internal.hiddenbrew.io:8080 coffee.CoffeeHealth/ListMugs

Here's the request the service receives when we query it directly:

:method: POST
:scheme: http
:path: /coffee.CoffeeHealth/ListMugs
:authority: 10.1.1.1:8080
content-type: application/grpc
user-agent: grpcurl/1.9.1 grpc-go/1.61.0
te: trailers
grpc-accept-encoding: gzip

Here's what it looks like when we go through the AWS ALB:

:method: POST
:scheme: http
:path: /coffee.CoffeeHealth/ListMugs
x-forwarded-for: 10.1.1.1
x-forwarded-proto: https
x-forwarded-port: 8080
host: coffee.internal.hiddenbrew.io:8080
x-amzn-trace-id: Root=2-123ab11a-212d59f010e8d94a73a45fcd
te: trailers
content-type: application/grpc
user-agent: grpcurl/1.9.1 grpc-go/1.61.0
grpc-accept-encoding: gzip

So, host is set and :authority is omitted.

@vietj
Copy link

vietj commented Dec 17, 2024

thanks for the details @j-white

@j-white
Copy link

j-white commented Dec 19, 2024

It looks like this needs to be addressed in vert.x core (vs vertx-web)

I took a go here: eclipse-vertx/vert.x#5426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertx kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants