New Feature: DNS over SOCKS #347
Conversation
|
I haven't forgotten about this, but as mentioned, it'll take some more time until I can review it properly. |
|
No worries, take your time. Thanks for the update. |
|
I'll review in more detail later, but as a first change, please undo the new |
|
|
||
| c.connect(remote); | ||
| if (proxy != null) { | ||
| c.configureBlocking(true); |
There was a problem hiding this comment.
This is too easy :-)
The whole process of connecting to the proxy must be async here too. If this method starts to block, the entire thread pool can get exhausted and all the timeouts won't work anymore.
There was a problem hiding this comment.
Yes, I probably need to implement it as KeyProcessor. I would like to use the Transaction class from NioTcpClient and make it a separate public class.
| if (local != null) { | ||
| tcpChannel.bind(local); | ||
| } | ||
| tcpChannel.connect(proxy.getProxyAddress()); |
There was a problem hiding this comment.
Here too, this needs to be fully async. Also, this creates a dedicated TCP connection to the proxy for each request. Shouldn't there be just one (or maybe one per remote) and reused?
There was a problem hiding this comment.
You need only one open TCP connection to establish the UDP relay.
It might be optimal to have just one or a pool off connections.
| if (!state.channel.isConnected()) { | ||
| state.channel.register(selector, SelectionKey.OP_CONNECT, state); | ||
| } else { | ||
| if (state.channel.keyFor(selector) == null) { |
There was a problem hiding this comment.
What did you encounter that this is necessary? Also, would it be possible to merge the condition with the outer if?
There was a problem hiding this comment.
I encountered:
Exception in thread "dnsjava NIO selector" java.lang.NullPointerException: Cannot invoke "java.nio.channels.SelectionKey.interestOps(int)" because the return value of "java.nio.channels.SocketChannel.keyFor(java.nio.channels.Selector)" is null
at org.dnsjava/org.xbill.DNS.NioTcpClient.processPendingRegistrations(NioTcpClient.java:47)
at org.dnsjava/org.xbill.DNS.NioClient.runTasks(NioClient.java:168)
at org.dnsjava/org.xbill.DNS.NioClient.runSelector(NioClient.java:133)
at java.base/java.lang.Thread.run(Thread.java:1583)
Exception in thread "main" java.io.IOException: Timed out while trying to resolve google.com./A, id=25831
at org.dnsjava/org.xbill.DNS.Resolver.send(Resolver.java:172)
at org.dnsjava/org.xbill.DNS.tools.dig.main(dig.java:227)
Caused by: java.util.concurrent.TimeoutException
at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1960)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2095)
at org.dnsjava/org.xbill.DNS.Resolver.send(Resolver.java:153)
... 1 more
The merge with the outer condition didn't work out for me.
|
|
||
| private void writeToChannel(SocketChannel c, ByteBuffer buffer) { | ||
| 8000 try { | ||
| c.write(buffer); |
There was a problem hiding this comment.
Return the return value, then handle it everywhere the write might have failed but not thrown.
|
|
||
| private void readFromChannel(SocketChannel c, ByteBuffer buffer) { | ||
| try { | ||
| c.read(buffer); |
There was a problem hiding this comment.
Return the return value and handle it everywhere
| return new InetSocketAddress(this.getProxyAddress().getAddress(), port); | ||
| } | ||
|
|
||
| public byte[] addUdpHeader(byte[] in, InetSocketAddress to) { |
There was a problem hiding this comment.
With this IPv6 worked for me
ByteBuffer buffer;
byte addressType;
byte[] addressBytes;
if (remote.getAddress() instanceof Inet4Address) {
addressType = SOCKS5_ATYP_IPV4;
addressBytes = remote.getAddress().getAddress();
buffer = ByteBuffer.allocate(4 + addressBytes.length + 2 + data.length);
} else if (remote.getAddress() instanceof Inet6Address) {
addressType = SOCKS5_ATYP_IPV6;
addressBytes = remote.getAddress().getAddress();
buffer = ByteBuffer.allocate(4 + addressBytes.length + 2 + data.length);
} else {
addressType = SOCKS5_ATYP_DOMAINNAME;
addressBytes = remote.getHostName().getBytes(StandardCharsets.UTF_8);
buffer = ByteBuffer.allocate(4 + 1 + addressBytes.length + 2 + data.length);
}
buffer.put((byte) 0x00); // RSV
buffer.put((byte) 0x00); // RSV
buffer.put((byte) 0x00); // FRAG
buffer.put(addressType); // ATYP (IPv4)
if (addressType == SOCKS5_ATYP_DOMAINNAME) {
buffer.put((byte) addressBytes.length);
}
buffer.put(addressBytes); // DST.ADDR
buffer.putShort((short) remote.getPort()); // DST.PORT
buffer.put(data); // DATA
| return buffer.array(); | ||
| } | ||
|
|
||
| public byte[] removeUdpHeader(byte[] in) { |
There was a problem hiding this comment.
It keeps track of the headerLength in case of IPv4, IPv6 or domainname and removes it accordingly
| buffer.put(SOCKS5_VERSION); | ||
| buffer.put(SOCKS5_CMD_CONNECT); | ||
| buffer.put(SOCKS5_RESERVED); | ||
| buffer.put(SOCKS5_ATYP_IPV4); |
There was a problem hiding this comment.
Handle IPv6; and remote could also be a hostname. What if the proxy should resolve the hostname because the client isn't even able to?
There was a problem hiding this comment.
I adjusted it to
if (remote.getAddress() instanceof Inet4Address) {
addressType = SOCKS5_ATYP_IPV4;
addressBytes = remote.getAddress().getAddress();
buffer = ByteBuffer.allocate(10);
} else if (remote.getAddress() i
6D40
nstanceof Inet6Address) {
addressType = SOCKS5_ATYP_IPV6;
addressBytes = remote.getAddress().getAddress();
buffer = ByteBuffer.allocate(22);
} else {
addressType = SOCKS5_ATYP_DOMAINNAME;
addressBytes = remote.getHostName().getBytes(StandardCharsets.UTF_8);
buffer = ByteBuffer.allocate(7 + addressBytes.length);
}
buffer.put(SOCKS5_VERSION);
buffer.put(SOCKS5_CMD_CONNECT);
buffer.put(SOCKS5_RESERVED);
buffer.put(addressType);
if (addressType == SOCKS5_ATYP_DOMAINNAME) {
buffer.put((byte) addressBytes.length);
}
buffer.put(addressBytes);
buffer.putShort((short) remote.getPort());
| ByteBuffer buffer = ByteBuffer.allocate(3); | ||
| buffer.put(SOCKS5_VERSION); | ||
| buffer.put((byte) 1); | ||
| buffer.put((this.socks5User != null && this.socks5Password != null) ? SOCKS5_AUTH_USER_PASS : SOCKS5_AUTH_NONE); |
There was a problem hiding this comment.
Java has built-in GSSAPI support. Would be nice to include the support here too, or at least prepare the code flow to be extendable.
There was a problem hiding this comment.
I tried to use org.ietf.jgss, but I'm getting
Field gssCredential: Undefined reference: org.ietf.jgss.GSSCredential
Undefined reference: org.ietf.jgss.GSSCredential
when running mvn clean package
Adding requires java.security.jgss; to the module-info.java didn't help.
Do you have an idea, why this is happening?
| import java.util.Objects; | ||
|
|
||
| @Getter | ||
| public class Socks5Proxy { |
There was a problem hiding this comment.
I'm not sure if there's a need for timeout handling?
There was a problem hiding this comment.
You can set timeouts on the Dante config. It should reply with "TTL expired" then. Don't know, how to do this properly. There might be the need to track and handle this. Or you add a retry mechanism.
|
I undid the changes in I also added a Socks5ProxyIoClientFactory. Now, you can set it up like following: I couldn't figure out how to add the SOCKS5 transactions with the NioClient. I implemented it like an event loop in the Socks5ProxyIoClientFactory and added the read and write operations in the attachment of the channel keys. I tried to implement the timeout handling with a ScheduledExecutorService, but I think it is not ready, yet. Now it's fully async. IPv6 worked with 2001:4860:4860::8888 for me. It also reuses the connection without repeating the SOCKS5 handshake again. |
|
Hi @ibauersachs. I suggested a huge amount of code additions and I also have changed a lot since the requested changes. I think, I should give some more descriptions. Initially, I did not understand the Then later I realized I could not sync up the close operations for the implementation of timeouts into the event loop through the To reuse the connections and avoiding the repetition of the SOCKS handshake, I defined What do you think? |
|
Hi, sorry for not getting back to you yet. I haven't had time to thoroughly review this PR again, and as you probably figured out, the NIO stuff is nasty. At first glance, there's a lot of duplication between the existing IO client classes and what you wrote now. I wonder if there couldn't be more shared code, especially the selector thread and timeout handling. The IO clients are currently package-private, so there can be changes to the API if needed. It would also be good if there were some unit/integration tests. You could use Testcontainers to run a real proxy to test against. |
|
Hi. Thanks for your quick reply. Yes. I now believe that the logic for the SOCKS proxy should be built on top of the NioClients. I can't use the current NioTcpClient because of this part |
|
The current TcpIoClient implementation currently makes a hard assumption that it's talking directly with DNS servers, also with the prefix-length expected in each reply. You can also try introducing an intermediate class if it helps: Going back to the |
|
That might help. I try it out. |
|
Hallo @ibauersachs. Thanks for your input. I appreciate it. I integrated the transactions with the The builds for this pull request are currently failing. Probably, Docker is not set up in the build containers. It works fine on my server. It seems that on Mac, there is an issue with routing UDP traffic into the Docker container. I do not believe this is related to the proper functioning of the SOCKS interactions. It works fine when I try it from a test container in the same Docker network. The introduction of intermediate classes worked well. For the For the For the I plan to add more tests and further improve and simplify the It would be nice to get a short review and your opinion on the pull request. |
|
Thanks! I'll need a while to dig through all of this. As for the integration tests, you could add a tag to only run them on the stage that also runs Sonar (i.e. Ubuntu and currently Java 21). Docker might need to be installed beforehand, you can do that as an actions step with apt-get install. Also, can you please rebase the branch on top of master? |
bb16b23 to
e94d15c
Compare
|
I rebased the branch on top of master. |
…l + better error handling
I added SOCKS5 support to the ioClients.
It does work for TCP and UDP (udp associate).
It also supports user/pwd authentication.
The implementation is based on
I tested it on a Dante SOCKS5 proxy with following config.
You should be able to install the Dante SOCKS5 proxy via package managers like apt, apk or brew.
I can also provide you a Docker setup.
I tried it like
or
I implemented it with method overloading on
sendAndReceiveTcpandsendAndReceiveUDP. In this way it does not affect the tests.