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

Fix hostname resolution for proxied connections #913

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bartosz822
Copy link

Proxied connections resolve the hostname to an IP address while tunneling even when noResolveHostname option is set.
This behavior is unwanted in some scenarios, especially when a proxy does whitelisting based on hostnames.
This pr should fix this.

@scottf
Copy link
Contributor

scottf commented May 15, 2023

@bartosz822 I'm inclined to approve this, but first...

  1. Should we differentiate between proxied and isNoResolveHostnames.
  2. Can you provide a small code snippet / output that shows the difference from different input?
  3. How does this work with discovered servers, which usually come in the form of an ip address? Probably no effect, but I just want to think about it.

@bartosz822
Copy link
Author

@scottf

  1. I don't think so, I mentioned proxied connections because that's where I discovered the issue and also that is where it can cause most problems, but I believe that in not proxied scenario it also makes sense to perform the initial connection without hostname resolution if someone wants it (by setting the isNoResolveHostnames).
  2. Sure, here you go
        InetSocketAddress address = new java.net.InetSocketAddress("www.example.com", 80);
        System.out.println(address.getAddress()); // www.example.com/93.184.216.34
        System.out.println(address.getHostName()); // www.example.com
        System.out.println(address.isUnresolved()); // false

        InetSocketAddress addressUnresolved = java.net.InetSocketAddress.createUnresolved("www.example.com", 80);
        System.out.println(addressUnresolved.getAddress()); // null
        System.out.println(addressUnresolved.getHostName()); // www.example.com
        System.out.println(addressUnresolved.isUnresolved()); // true

Note:
the java.net.Socket#connect(java.net.SocketAddress, int) method checks if the address is already resolved, example:

            if (epoint.isUnresolved())
                impl.connect(addr.getHostName(), port);
            else
                impl.connect(addr, port);
  1. That's actually a good point, I'll add one more check if the uri is already resolved.

@scottf
Copy link
Contributor

scottf commented May 16, 2023

@bartosz822 Just waiting for another pair of eyes on this.

@scottf
Copy link
Contributor

scottf commented May 23, 2023

@bartosz822 I'm closing this, it's redundant. If you trace back, you can see it's already done before it gets here.

io.nats.client.impl.SocketDataPort.connect(SocketDataPort.java:62)
io.nats.client.impl.SocketDataPort.connect(SocketDataPort.java:52)
io.nats.client.impl.NatsConnection.tryToConnect(NatsConnection.java:425)
io.nats.client.impl.NatsConnection.connect(NatsConnection.java:207)
io.nats.client.impl.NatsImpl.createConnection(NatsImpl.java:29)
io.nats.client.Nats.createConnection(Nats.java:303)
io.nats.client.Nats.connect(Nats.java:210)

Look at the code around NatsConnection, line 207.
NatsConnection line 194 calls resolveHost (NatsConnection, line 1759)
NatsConnection, line 1762 checks for an ip address
NatsConnection, line 1763 is serverPool.resolveHostToIps (NatsServerPool, line 188)
NatsServerPool resolveHostToIps checks the flag isNoResolveHostnames

@scottf scottf closed this May 23, 2023
@bartosz822
Copy link
Author

bartosz822 commented May 24, 2023

@scottf I've seen the code you've mentioned and this pull request is not redundant, could you please reopen it? I've observed in proxy logs using this library that the connection was made using the IP address even though isNoResolveHostnames flag was set. The NatsConnection that is passed to the connect method is indeed unresolved, but the constructor of InetSocketAddress does the resolution under the hood, look at the code from java.net:

public InetSocketAddress(String hostname, int port) {
        checkHost(hostname);
        InetAddress addr = null;
        String host = null;
        try {
            addr = InetAddress.getByName(hostname);
        } catch(UnknownHostException e) {
            host = hostname;
        }
        holder = new InetSocketAddressHolder(host, addr, checkPort(port));
    }

Take also a look at the implementation of java.net.Socket#connect(java.net.SocketAddress, int), it checks if the SocketAddress is resolved or not.

@scottf
Copy link
Contributor

scottf commented May 24, 2023

What was the input, meaning the servers specified in the options? You saw the stack trace. Also, if there is any resolving or checking to be done, it needs to be done before SocketDataPort.connect. I suppose it's possible that the SocketDataPort may need to be changed to never resolve as it should take exactly what it was given

@bartosz822
Copy link
Author

What was the input, meaning the servers specified in the options?

Only one server was specified, in the format wss://example.com:443/nats.

Also, if there is any resolving or checking to be done, it needs to be done before SocketDataPort.connect. I suppose it's possible that the SocketDataPort may need to be changed to never resolve as it should take exactly what it was given

It's not SocketDataPort that does the resolution, it happens inside InetSocketAddress constructor. It does the resolution by default, that's why there is createUnresolved method that should be used if it that behavior is not wanted. If that decision is supposed to happen before SocketDataPort.connect it would need to get InetSocketAddress instead of NatsUri.

@scottf
Copy link
Contributor

scottf commented May 24, 2023

Reopening... this conversation is good, getting us to where we need to be. Thanks for the input.

@scottf scottf reopened this May 24, 2023
@bartosz822
Copy link
Author

@scottf thanks for reopening, what are the next steps now?

@bartosz822
Copy link
Author

@scottf I haven't received any new feedback on this PR since quite a while. Is it ok? Is there something else that needs to be done?

@scottf
Copy link
Contributor

scottf commented Jun 13, 2023

@bartosz822 Sorry you are just going to have to be patient, I'm juggling several priorities right now and the behavior is under discussion.

@GrafBlutwurst
Copy link
Contributor

Hi, I was curious how we're doing on this MR? The reason that I'm asking is that we currently have to maintain some firewall rules using IP addresses over domain names which the colleagues from security don't like much.

@scottf
Copy link
Contributor

scottf commented Sep 5, 2023

Hi, I was curious how we're doing on this MR? The reason that I'm asking is that we currently have to maintain some firewall rules using IP addresses over domain names which the colleagues from security don't like much.

It's been quite a while since I reviewed this but I remember it was incomplete. Resolve hostnames is on by default, but you can turn it off in options via the options builder by calling noResolveHostnames()

@GrafBlutwurst
Copy link
Contributor

Looking at the description it seems that the noResolveHostnames doesn't work for proxied connections. We observe the same still or was this fixed in another MR and I just missed it?

@scottf
Copy link
Contributor

scottf commented Sep 6, 2023

@GrafBlutwurst I think the problem is legitimate, just not the fix in this PR. Also, why are you proxying connections?

@scottf
Copy link
Contributor

scottf commented Sep 6, 2023

@GrafBlutwurst I really wanted to close this but the user insisted I was not understanding the problem. Which may be true. Is it possible that you can open a fresh defect issue in the repo and we can try to start this process over? I'm certain, I was not happy with the proposed code change, but frankly it's been a while and I can't remember why.

@GrafBlutwurst
Copy link
Contributor

@GrafBlutwurst I think the problem is legitimate, just not the fix in this PR. Also, why are you proxying connections?

So the reason for proxying is sadly out of our hands. The connection lives at customer site and they require proxied connections for compliance reasons. We actually specifically chose NATS for the websocket & proxying capabilities for this reason.

@GrafBlutwurst
Copy link
Contributor

@scottf Here's the defect issue. #967

Let me know if I can help in any way. Looking at the code it's also how I would have solved the issue I think :/ , but maybe you have a better idea.

@scottf
Copy link
Contributor

scottf commented Sep 7, 2023

@scottf Here's the defect issue. #967

Let me know if I can help in any way. Looking at the code it's also how I would have solved the issue I think :/ , but maybe you have a better idea.

If I remember correctly, by the time it got to that point, it should have already been resolved. So it needs to be traced back and determined where it should happen.

@GrafBlutwurst
Copy link
Contributor

I think the point here is that in a proxied case with firewalls we never want the hostname to be resolved to an IP on the client. Connections should always be established by hostname so that we don't need 2 firewall rules.

In particular the constructor call new InetSocketAddress(host, port) does resolution underneath thus on the firewall this connection attempt is by IP. Thus necessitating the use of InetSocketAddress.createUnresolved(host, port).

At least that was my understanding when looking at the code and would address our issue with hostname resolution.

@scottf
Copy link
Contributor

scottf commented Jul 17, 2024

@GrafBlutwurst Is this still an issue? Recent versions of the client support proxies much better, addressing the fact that the client to proxy is secure, but commonly proxy to server is insecure. Also there is the Options.noResolveHostnames option.
To setup the client for proxy, see this: https://github.com/nats-io/nats.java?tab=readme-ov-file#reverse-proxy

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.

3 participants