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

Revisiting DNS round-robin + TLS hostname verification #826

Closed
ogolberg opened this issue Aug 9, 2022 · 3 comments
Closed

Revisiting DNS round-robin + TLS hostname verification #826

ogolberg opened this issue Aug 9, 2022 · 3 comments

Comments

@ogolberg
Copy link
Contributor

ogolberg commented Aug 9, 2022

Background

DNS round robin (#138) was disabled because it was deemed not compatible with TLS hostname verification (#394).

My usecase requires both: DNS round robin for edge load balancing, which is pretty common, and hostname verification for obvious security reasons.

Problem

It is possible to have DNS round robin play nice with hostname verification. The problem is the Address abstraction which loses the original hostname when it's constructed from a resolved InetAddress by DnsRecordIpAddressResolver. If we pass InetAddress obtained from InetAddress.getAllByName into the Socket.connect method, the HTTPS hostname verification algorithm will work fine. For example, something like this works:

InetAddress[] addrs = InetAddress.getAllByName("..");
  
for (addr: addrs) {  
    SSLSocket socket = (SSLSocket) socketFactory.createSocket();
 
    SSLParameters params = sock.getSslParameters()	
    params.setEndpointIdentificationAlgorithm("HTTPS")
    socket.setSslParameters(params)  
  
    sock.connect(InetSocketAddress(addr, ...))
}

Proposed change

One way to make this work would be to change the AddressResolver API to return a list of InetSocketAddresses and reintegrate DnsRecordIpAddressResolver. This can also be gated by a configuration flag.

Would you be open to such a change? I can take a stab at a PR.

@michaelklishin
Copy link
Member

I don't have any objections

@acogoluegnes
Copy link
Contributor

Sure, go ahead.

Note we should remain backward compatible to be able to include the change into 5.x. This means we cannot change a public interface like AddressResolver. It's still possible to introduce a sub-interface or a sub-class. The core code can then check for these new types (instanceof) where it matters and use them accordingly. This way do not break things.

Provide the PR as-is if you don't feel comfortable with the backward compatibility technics, the point is to get the idea, and we'll see what we can do afterward.

@ogolberg
Copy link
Contributor Author

I ended up subclassing Address which wasn't too bad - see #827

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

No branches or pull requests

3 participants