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

Normalize IDN hostnames before DNS resolution to prevent UnknownHostException #608

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

arturobernalg
Copy link
Member

This PR addresses an issue where Unicode hostnames cause UnknownHostException during DNS resolution. The change normalizes hostnames to punycode using IDN.toASCII() before passing them to the DNS resolver.

@arturobernalg arturobernalg requested a review from ok2c January 5, 2025 19:13
@ok2c
Copy link
Member

ok2c commented Jan 6, 2025

@arturobernalg Just a question. Why not having this normalization logic in SystemDefaultDnsResolver? It already has some extra logic for resolution of IPv6 names in it.

@arturobernalg
Copy link
Member Author

@arturobernalg Just a question. Why not having this normalization logic in SystemDefaultDnsResolver? It already has some extra logic for resolution of IPv6 names in it.

You right. And we avoid duplicated code.

@@ -44,8 +45,9 @@ public class SystemDefaultDnsResolver implements DnsResolver {
@Override
public InetAddress[] resolve(final String host) throws UnknownHostException {
try {
final String normalizedHost = IDN.toASCII(host);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg I would do IDN.toASCII only if the hostname fails TextUtils#isAllASCII for the sake of optimization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Catching IllegalArgumentException and recovering from it (for instance, by keeping the original hostmame) would be nice here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ok2c Please do another pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Do you have any objections to doing the test TextUtils#isAllASCII prior to doing IDN.toASCII?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ok2c done

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Looks good to me

@arturobernalg arturobernalg merged commit 3c82807 into apache:master Jan 6, 2025
10 checks passed
@ok2c
Copy link
Member

ok2c commented Jan 6, 2025

@arturobernalg Please cherry-pick to 5.4.x

arturobernalg added a commit that referenced this pull request Jan 6, 2025
@arturobernalg
Copy link
Member Author

@arturobernalg Please cherry-pick to 5.4.x

Done

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 this pull request may close these issues.

2 participants