-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Regression with ForwardedParser setting an empty host header #37045
Comments
@cescoffier @aloubyansky for your awareness. If we release a 3.2.9, we probably need to fix that too. |
It got fixed in Vertx. It is not something we can fix in Quarkus only. |
This is blocking Keycloak from upgrading from 3.2.7. It'd be super nice to get this fixed. :) |
jFTR – this is a regression introduced by 3.2.8. |
Unfortunately, it requires a vertx bump. |
4.4.6 @cescoffier? |
ah, it's already 4.4.6 in 3.2.8, so Vert.X with the fix hasn't even been released yet? |
No 4.4.6 contains the fix. |
I would need a reproducer. We cannot rollback that change as it is a fix for http/2. Note that empty host header is slightly odd. |
I just read the issue again. So it's actually the opposite. You got an empty host. I may have been missing something (I do not touch to the security layer). Note that while odd, it's valid. |
Modern browser, e.g. Chrome, seem to be omitting the I was able to somewhat reproduce it outside of Keycloak as simply as: @GET
@Produces(MediaType.TEXT_PLAIN)
public String get(@Context UriInfo uriInfo) {
return uriInfo.getAbsolutePath().toString();
} which throws an NPE in 3.2.8 when accessed via a real hostname and TLS (e.g. |
Can you give me the instructions to set up this kind of environment? |
@cescoffier No need to set up anything. :) Just use a self-signed TLS cert and access the endpoint via nip.io (it works as is out of the box, |
Ok, got the NPE. |
Unfortunately, we would need a fix in Vert.x.
|
Just opened vert-x3/vertx-web#2511. |
The issue is unrelated to the forward parsed or empty host header. It's because the parsing to the nip URL was considered as an IP (as it starts like an IP). It's fixed by eclipse-vertx/vert.x#4948 (comment). |
See vert-x3/vertx-web#2511 (comment) for further explainations. As you will see, it's is not related to HTTP2 or the forward parser. If it's not the root cause, please provide another standalone reproducer. |
@cescoffier Thanks for the update! As it turned out, while the NPE was a real issue, it was unrelated to this one. Sorry for the confusion. To reproduce the original issue, I forgot to mention you need to set The root cause seems to be still present in the Quarkus 3.2 branch. |
Ok, I will have another look, but not before Friday or next week. |
BTW, I would need a standalone reproducer - not involving keycloak. |
Thanks. Feels quite critical, TBH. Quarkus basically broken proxy headers handling in an LTS version micro release.
Sure, the reproducer I provided in the comment is standalone. |
Looks it worked for the Keycloak use case. Thanks for the quick fix! Can we expect this to be fixed in 3.2.9? |
Yes |
Describe the bug
Keycloak has failing tests in keycloak/keycloak#24639 with the upgrade to quarkus 3.2.8. The ForwardedParser is setting an empty host header eventually results in uris being returned to the client without a host.
Expected behavior
location uris should be fully formed.
Actual behavior
Location uris are missing the host "https:/auth/admin..."
The likely cause is this change d41c78b#diff-26b0d509bb91cfaf3c4160496ea25fc584e85d4e2cc1b708eb72ea3b25b74c4dR124 - if there is no host header it will proceed to set an explicit empty value.
How to Reproduce?
See the pr keycloak/keycloak#24639
Output of
uname -a
orver
No response
Output of
java -version
No response
Quarkus version or git rev
3.2.8
Build tool (ie. output of
mvnw --version
orgradlew --version
)No response
Additional information
No response
The text was updated successfully, but these errors were encountered: