search

TurboVNC / turbovnc

Main TurboVNC repository
https://TurboVNC.org
GNU General Public License v2.0
747 stars 137 forks source link

Commit db3d30dcad breaks automatic tunnelling. #321

Closed Parakleta closed 2 years ago

Parakleta commented 2 years ago

In db3d30dcad you changed the tunnelling handling with the session manager to not change the provided option, but rather to use a local variable calculation. This then breaks behaviour in the fillCmdPattern() method is Tunnel.java since it is still looking at the manually set option rather than using the session manager behaviour. The documentation in Params.java says that the tunnel options if "effectively set" when the session manager is being used, but this is no longer entirely true.

https://github.com/TurboVNC/turbovnc/blob/f2b115f207739a1dd2bedcb5a246179e943ee1d6/java/com/turbovnc/vncviewer/Tunnel.java#L284-L285

https://github.com/TurboVNC/turbovnc/blob/f2b115f207739a1dd2bedcb5a246179e943ee1d6/java/com/turbovnc/vncviewer/Tunnel.java#L308-L309

dcommander commented 2 years ago

The code prior to that commit did not change opts.tunnel in Tunnel.java, so I don't understand what the issue is. Can you provide an example of the breakage?

Parakleta commented 2 years ago

In CConn.java the opts.tunnel option is no longer forced true when the session manager is used. This is worked around in the createTunnel() method in Tunnel.java by the local tunnel variable, but this variable is not propagated into the fillCmdPattern() method in the same file.

The side effect of this is that the two referenced lines now operate differently to before that change. That is, the %H replacement no longer contains the username, and the %G token must be provided in the tunnel command or else an error is thrown.

Parakleta commented 2 years ago

This is not necessarily a problem, except that it is a breaking change that isn't documented, and in fact the relevant documentation for the tunnel option suggests that it should be automatically set by the session manager. See:

https://github.com/TurboVNC/turbovnc/blob/473e569bcde180f796a9224288c7033a87be1c84/java/com/turbovnc/rfb/Params.java#L830-L835

and

https://github.com/TurboVNC/turbovnc/blob/473e569bcde180f796a9224288c7033a87be1c84/java/com/turbovnc/rfb/Params.java#L874-L884

dcommander commented 2 years ago

I don't understand your point. External SSH clients are not yet supported with the Session Manager. (See #148.) The modes that should work are:

If one of those modes does not work, then please provide exact steps to reproduce the failure.

dcommander commented 2 years ago

Ah. I think I see the issue. When VNC_TUNNEL_CMD is set, the createTunnel() function assumes that an external SSH client will be used, which is the wrong assumption when the session manager is active. Stand by.

Parakleta commented 2 years ago

Please don’t disable that mode of behaviour. I’ve been using it successfully for over a year and rely on it.

dcommander commented 2 years ago

Why is the built-in SSH client unsuitable for your needs? If you're using the Session Manager, then you are already using the built-in SSH client to connect to the host and manage sessions. The external SSH client only comes into play when you connect to a session. The Session Manager expects that the built-in SSH client will be used. Thus, it leaves the SSH tunnel open, the assumption being that the viewer will reuse the tunnel for the actual connection. As currently implemented, using ExtSSH with the Session Manager would leave an extraneous SSH session open throughout the life of the connection.

I am not philosophically opposed to allowing ExtSSH with the Session Manager, but I would need to address the issue of the extraneous SSH session. I first need to understand the use case that necessitates using ExtSSH with the Session Manager, since that was not intended to be a supported workflow in TurboVNC 3.0. I also think it would probably be best to allow that option only if SessMgrAuto is disabled. (The design principle of the Session Manager is to be "secure by default" and prevent users from overriding any security-related settings unless SessMgrAuto is disabled.)

Parakleta commented 2 years ago

I don’t think it can leave the other connection open because I don’t get an error opening the required ports with the external ssh program. I’d have to check that though to confirm.

My use case is that I open additional port forwarding, specifically to socket files on the server which can have access controls applied to them.

ETA: Actually I guess the session could be left open so long as it hasn’t tried to set up a tunnel yet.

dcommander commented 2 years ago

Yes, it is effectively the same as if you had opened a separate SSH session in another window. Port forwarding is only enabled for the viewer SSH session, not the Session Manager SSH session. (When using the built-in SSH client, the two sessions are the same.)

I will go ahead and fix the issue. Just understand that using ExtSSH with the Session Manager isn't officially supported yet, and without public key authentication, that configuration will require that you authenticate twice. (Officially supporting ExtSSH with the Session Manager will require using the OpenSSH ControlMaster option to enable reuse of the SSH session when using an external SSH client.)