Lookup of trusted proxies by hostname broken due to DNS issues in Vert.x

[ahus1]

Describe the bug

As part of #29888 (cc: @michalvavrik) a check was added for the address of the proxy which the proxy uses to connect to Quarkus.

When using the option quarkus.http.proxy.trusted-proxies, it allows passing in a hostname that is then resolved via a DNS lookup. While this is expected to be slower and not recommended, I found that there is behavior in Vert.x 4.x that make is impossible to work with some DNS servers with the current implementation. I also see obstacles in mixed IPv4/IPv6 environments, CNAMEs and multiple IP addresses assigned to a host.

Expected behavior

For an incoming request the host name should be resolved to an IP address to be checked against the incoming IP address.

The description doesn't provide much how IPv4 and IPv6 are handled here. One could expect that remote IPv4 remote addresses are matched against host's resolved IPv4 addresses, and IPv6 remote addresses are matched against host's resolved IPv6 addresses. Also CNAME handling could be expected.

Actual behavior

With Vert.x 4.x, a one DNS query asking for both A and AAAA records will be sent. Only some DNS servers support it, while other don't. This was first reported here in the lase commend to this Vert.x issue: eclipse-vertx/vert.x#5035 (comment)

Apparently the behavior changed in Vert.x 5.x and will now send two separate queries, which is expected to work with all DNS servers.

In my case, the DNS queries lead always lead to timeouts after several seconds as a reply was never received, so my Quarkus application practically stopped.

Looking at the code, the DnsClient.lookup() will return "The first found". So if a hostname has both an IPv4 and IPv6 address, only one of them will be matched independent if the remote IP address is an IPv4 or an IPv6 address.

While there is no description on how hostnames are handled, there seems to be no handling of multiple A and AAAA entries for a hostname, and no handling of CNAMEs.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

Instead of lookup(), the methods resolveA() and resolveAAAA() could be better suited to resolve all IP addresses of a host depending if the caller is using an IPv4 or IPv6 address. Still this leaves the CNAME handling on the table to handled as well.

In the context of Keycloak where I'm currently evaluating this, I might not expose the functionality of passing in a host name either temporarily (given the open issues I see above), or permanently given the performance impacts the DNS look will have even if it succeeds until a Keycloak user shows a use case where this should be supported.

So from the perspective of Keycloak, I would be ok if the functionality to pass in a hostname would be deprecated and eventually dropped.

There is a discussion related to the same Quarkus option on a different subject here: #42760

[sberyozkin]

@ahus1 But surely, from the user's perspective, passing a hostname is simpler. Would you like to propose a PR which can make a DNS resolution more robust ?

[michalvavrik]

@ahus1 But surely, from the user's perspective, passing a hostname is simpler. Would you like to propose a PR which can make a DNS resolution more robust ?

That would be great.

@ahus1 I don't have time to comment right now. Just FYI: this async DNS client I used here is something we don't do in Quarkus in general and I remember @cescoffier doesn't fancy async DNS for it's complicated and tricky (my loose interpretation). I used it to avoid blocking warnings. Blocking resolution is cached for a while, I don't remember about this DNS client. Not sure what is ETA for Vert.x 5 in Quarkus, but I remember some comment about 5 months plus.

[ahus1]

When go down the route to make DNS resolution more robust, IMHO we are avoiding the currently problematic Vert.x
4.x method and would not need to wait for Vert.x 5.x to arrive in Quarkus.

I could try to prepare a PR on this one.

Still we would need to agree first that passing in a hostname is a good idea that should be supported instead of being deprecated.

[cescoffier]

Vert.x 5 in Quarkus is not going to happen before (optimistic) September 2025. So do not hold your breathe.

[ahus1]

There is now a PR available which doesn't depend on a Vert.x release but uses the existing features: #43188

[cescoffier]

@ahus1 Any progress on #43188? You mention a future commit in on of your comment.

[ahus1]

@cescoffier - see my comment #43188 (comment)
The gist: I added a test with FakeDNS, still it is not testing all code paths.

I'd like to get feedback if the tests are sufficient from your perspective or not. If they are not, I might need more help and probably the code needs a different approach to make the code mode approachable for automated testing.

The "future commit" would only remove some code of the FakeDNS server which are not used.

All in all I'm still on the fence if the functionality to do a DNS lookup for proxy addresses makes sense at all as it adds overhead, and if the functionality should be dropped.

So TL;DR: A comment if this goes into the right direction with the tests I added in September would be good.

[cescoffier]

@ahus1 It should be good to go. No more feedback on my side.