Java vncviewer sets external SSH remote host incorrectly

[michael-adler]

Set the Java vncviewer to tunnel over ssh and also select "use external SSH client". Keep arguments at default.

Let's assume the remote hostname is "remote-host". Despite this, the resulting ssh command is, e.g.:
/usr/bin/ssh -f -L 58036:localhost:5901 localhost sleep 20

This is wrong. The ssh target should be "remote-host" but is, instead, "localhost".

I suspect this is because of the code at line 62 of https://github.com/TigerVNC/tigervnc/blob/master/java/com/tigervnc/vncviewer/Tunnel.java. Note there that remoteHost is set to "localhost" unless "via" is set, which isn't the case here since no gateway is needed.

I suspect that the right code should set remoteHost = cc.getServerName() unconditionally.

[bphinz]

Apologies for the long delay. I am not able to reproduce this. remoteHost (in the context of line 62 of Tunnel.java) is the machine that we want the viewer to connect to. The "host" part of the SSH forward specification is resolved from remoteHost's perspective, so once the tunnel is established that yields the loopback address on the target specified by remoteHost. The only way that I can see that you would get "/usr/bin/ssh -f -L 58036:localhost:5901 localhost sleep 20" would be if you specified "localhost" in the server dialog.

…

On Fri, Feb 15, 2019 at 6:39 AM Pierre Ossman (Work account) < ***@***.***> wrote: Assigned #796 <#796> to @bphinz <https://github.com/bphinz>. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#796 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHnWbYBgjlTQP6j9urU2gOzakIlueR13ks5vNpxygaJpZM4a8L2l> .

[michael-adler]

There are two main settings in the SSH dialog. The first is "Use SSH gateway" and the second is "Use exteranl SSH client". Let's assume "Tunnel VNC over SSH" is set.

I'm interested in the case where "Use SSH gateway" is OFF and arguments is "default". With only "Use external SSH client" set and the external SSH set to "/usr/bin/ssh", then the command that is issued is always:

/usr/bin/ssh -f -L 58264:localhost:<VNC port> localhost sleep 20

The VNC port in the above is correctly set from the VNC server configuration on the main setting screen. The "localhost" in the -L argument is fine as long as the ssh connection is to the remote host with the VNC server. The problem is that the ssh server in the above command is also "localhost". It should be the VNC server's hostname, taken from the VNC server configuration.

[bphinz]

Gotcha, I can reproduce it. Will try to fix it tonight.

[bphinz]

@michael-adler can you please try the latest nightly build and confirm that the issue is resolved?

[michael-adler]

@bphinz - Good news and bad news. The good news is the change is correct and the ssh connection opens correctly. The bad news is the connection to the VNC server fails:

TigerVNC Java Viewer v1.10.80 (20191103)
Built on 2019-11-03 at 23:44:25
Copyright (C) 1999-2019 TigerVNC Team and many others (see README.rst)
See https://www.tigervnc.org for information on TigerVNC.
DecodeManager: Detected 4 CPU core(s)
DecodeManager: Creating 4 decoder thread(s)
Tunnel: SSH command line: /usr/bin/ssh -f -L 49212:localhost:5921 remotehost sleep 20
Tunnel: Warning: Permanently added 'remotehost,10.1.1.5' (RSA) to the list of known hosts.
Tunnel: *******************************************************
Tunnel: Use of this device and the network are subject to
Tunnel: security monitoring to protect against security
Tunnel: vulnerabilities and to confirm policy compliance.
Tunnel: *******************************************************
com.tigervnc.rfb.Exception: unable to connect:Connection refused
at com.tigervnc.vncviewer.CConn.(CConn.java:143)
at com.tigervnc.vncviewer.VncViewer.run(VncViewer.java:563)
at java.lang.Thread.run(Thread.java:748)

I confirmed the following:

• The ssh command completes correctly.
• The port to the VNC server through ssh works correctly. If I run "nc localhost 49212" after the failure above in the 20 seconds before ssh exits it does connect to the VNC server.
• The exit status of the /usr/bin/ssh command above is 0.

I suspect the new failure is one of two possibilities:

• TigerVNC doesn't wait long enough after running /usr/bin/ssh and the forwarding port isn't open yet.
• The output from the remote host of the VNC connection or the warning message from ssh confuses TigerVNC into thinking there was an error.

[bphinz]

Hmm. It's not the banner message, I've tested that (and just re-tested to confirm). I'm assuming that you have pubkey auth configured? The Java client isn't capable of interactively logging in when using an external SSH client (the internal JSch client can interactively login). The delay is a possibility, can you try increasing the delay on line 217 and see if that works?

[bphinz]

I realized while trying to debug this that the external SSH client arguments field of the dialog was broken and pushed a fix for it. I don't think it's related to the problem that you are seeing, but now if you wanted to try suppressing the banner message you could set the tunneling template to something like:

-q -f -L %L:localhost:%R %H sleep 20

[michael-adler]

I confirmed that increasing the timeout "solves" the problem. I put it in quotes since that method is clearly a hack. Shouldn't it poll/wait for the local port to become available?

[bphinz]

Shouldn't it poll/wait for the local port to become available?

Ick. Yeah, if I can figure out how... How much did you have to increase it by?

[michael-adler]

I just set it to 5 seconds since the goal was to confirm that timing was the problem. There is no guaranteed value that works, so it would be better if you can figure out how to loop until the port shows up.

[michael-adler]

There are a bunch of possibilities you might find useful in https://stackoverflow.com/questions/434718/sockets-discover-port-availability-using-java

[bphinz]

Would you mind trying this patch? I don't have any easy way to simulate whatever latency you are seeing

--- a/java/com/tigervnc/vncviewer/Tunnel.java
+++ b/java/com/tigervnc/vncviewer/Tunnel.java
@@ -29,6 +29,8 @@ import java.io.BufferedReader;
 import java.io.File;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.IOException;
+import java.net.*;
 import java.util.*;
 
 import com.tigervnc.rdr.*;
@@ -211,6 +213,13 @@ public class Tunnel {
       t.start();
       // wait for the ssh process to start
       Thread.sleep(1000);
+      // test the local forwarding socket for up to 5s
+      // to make sure the tunnel is up before connecting
+      SocketAddress sockAddr =
+        new InetSocketAddress("localhost", localPort);
+      java.net.Socket socket = new java.net.Socket();
+      socket.connect(sockAddr, 5000);
+      socket.close();
     } catch (java.lang.Exception e) {
       throw new Exception(e.getMessage());
     }

[michael-adler]

You should be able to test your change by removing the Thread.sleep() right above your patch, since the goal is to detect the port being available instead of picking a wait time.

The proposed patch does not work. I think the timeout argument to Socket.connect() is the connection completion timeout. It appears to raise an exception immediately if it discovers that the port does not have a listener, which is the case here until ssh sets it up.

[bphinz]

Agreed. This seems to work though:

--- a/java/com/tigervnc/vncviewer/Tunnel.java
+++ b/java/com/tigervnc/vncviewer/Tunnel.java
@@ -29,6 +29,8 @@ import java.io.BufferedReader;
 import java.io.File;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.IOException;
+import java.net.*;
 import java.util.*;
 
 import com.tigervnc.rdr.*;
@@ -210,12 +212,30 @@ public class Tunnel {
       Thread t = new Thread(new ExtProcess(cmd, vlog, true));
       t.start();
       // wait for the ssh process to start
-      Thread.sleep(1000);
+      while (!isTunnelReady(localPort)) {
+        Thread.sleep(100);
+      }
     } catch (java.lang.Exception e) {
       throw new Exception(e.getMessage());
     }
   }
 
+  private static boolean isTunnelReady(int localPort)
+  {
+      // test the local forwarding socket for up to 5s
+      // to make sure the tunnel is up before connecting
+      SocketAddress sockAddr =
+        new InetSocketAddress("localhost", localPort);
+      java.net.Socket socket = new java.net.Socket();
+      boolean ready = false;
+      try {
+        socket.connect(sockAddr);
+        ready = socket.isConnected();
+        socket.close();
+      } catch (IOException e) { }
+      return ready;
+  }
+
   private static String fillCmdPattern(String pattern, String gatewayHost,
                                        String remoteHost, int remotePort,
                                        int localPort) {

I do think that it needs a timeout though, so that it doesn't wait indefinitely

[michael-adler]

Yes, that works. You've set yourself up nicely for a timeout by counting iterations of the while (!isTunnelReady(localPort)) loop. You probably also want to check the exception type in isTunnelReady. If the error isn't from the listener missing then failing immediately would be better.

Thank you!

[bphinz]

OK, that's all committed and in the current nightly if you want to give it a try

[michael-adler]

It appears to work. A failing connection timed out after about 5 seconds. It also succeeded when expected.

I have a couple concerns about the new code though:

• You removed the timeout from the Socket.connect() call. Is there an error mode possible in which there will be no timeout there, so isTunnelReady() never returns? The documentation I read wasn't clear, but passing 0 for timeout in the 2 argument variant means infinite timeout.
• Would it be better to track time in createTunnelExt instead of loop trips when calling isTunnelReady? If each isTunnelReady sits for some timeout due to an unforeseen error then 50 trips through the loop might take far longer than you expect. Maybe track the start time of the loop and end after 5 seconds, independent of trips?

[bphinz]

You're absolutely right. The connect timeout is OS dependent if not explicitly set. I've just pushed both of those changes. Thanks.

I'm going to go ahead and close this issue, if anything else comes up please feel free to open a new issue.