Show helper to set up SSH tunnel for Desktop Viewer #1084

skobyda · 2023-05-22T12:34:37Z

The first one of many upcoming changes, fixes to consoles.

What this fixes:

Desktop viewer for situation when VMs are hosted on a remote server

What this doesn't fix, and still need to be changed/fixed in the greater VNC redesign:

Desktop viewer when VMs are hosted on secondary server which is channelled through primary server
Embedded VNC viewer when VMs are hosted on secondary server which is channelled through primary server
Situations where users want VNC server genuinely to listen on non-localhost addresses. In such cases we need to guide user to update firewall, open ports...
Situation where user specifies multiple addresses where VNC is listening to (and only one of them is localhost)
Situations where user already has some VM running on localhost which takes up th same port as the VM running on a remote server
Solution for full screen VNC
Optional: show user the virt-viewer command (e.g. virt-viewer qemu+ssh://user@remoteaddress/system domainname) which opens a VNC connection to a remote VM. The benefit is that virt-viewer command sets up SSH tunnel for you. Might be a simpler solution than downloading .vv file and setting up SSH tunnel manually (which we do now)

This PR adds a message to the details of Desktop Viewer if VM's is listening on localhost, but VMs are located at a remote server.

Fixes #1078

src/components/vm/consoles/desktopConsole.jsx

skobyda · 2023-05-24T17:11:00Z

src/components/vm/consoles/utils.js

+ * along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
+ */
+export function getServerAddress() {
+    return window.location.hostname;


@martinpitt Is there any way for me to mock window.location.hostname in our tests so I can test a situation where it's value is different from localhost/127.x.x.x ?

Not directly -- if you write to that attribute, you will actually change the page. But you can add a destructive test with provision to actually get a second machine, like you already did in test/check-machines-migrate. That's IMHO a better way to validate that this really works.

As you just said in the chat, this won't help, as the browser runs outside the VMs and the cockpit port always gets tunneled to 127.0.0.2. We can't run a browser inside the VM. So this is actually exactly the unfixable cockpit behind a reverse proxy scenario below.

But there's a workaround: You can restrict the "localhost" check to 127.0.0.1, treat any other 127.* address as "remote", and then have one test set up a socat from 127.0.0.2:cockpit_port to 127.0.0.1:5555 (or any other port which is definitively not used); then that test can connect the browser to that forwarded port and it'll appear as truly "localhost". See e.g. this cockpit test where we do this. The caveat is that we run socat in the VM in these tests. But (1) our tasks container has socat, and (2) you can also use ssh -L to tunnel the port (which is incidentally the very thing that this PR advertises, so perhaps that's better 😁 )

martinpitt

Thanks! This is a helpful guide indeed, and better than trying to set this up automatically.

You also mentioned something like telling the VNC viewer to directly connect to something like a qemu+ssh://server/.. address. This could be shown as an alternative?

martinpitt · 2023-05-25T05:07:03Z

src/components/vm/consoles/utils.js

+}
+
+export function isLocalhost(address) {
+    return address === "localhost" || address.startsWith("127");


Do we need to be concerned about IPv6? ::1 and localhost6 and such?

src/components/vm/consoles/utils.js

martinpitt · 2023-05-25T05:12:10Z

src/components/vm/consoles/utils.js

+
+// Get address where VNC or SPICE server is located
+export function getConsoleAddress(consoleDetails) {
+    let address = consoleDetails.address;


This looks like a server listening address then, judging by the test that you run on it. Would this ever be an actually real address? Even if it's interface specific, it's usually a netmask then, such as 10.1.2.0/24

Yeah that is an address at which a VNC server is listening to.
By default, it's limited to localhost, so if the host is connected to the wider network ad has some IP assigned, nobody should be able to just connect to a VNC of some VM which is running on the server.
But of course, the user might want the VM's VNC to be exposed to connections coming from the outside. Then they might configure this address to the address of the host.

martinpitt · 2023-05-25T05:12:45Z

src/components/vm/consoles/utils.js

+    let address = consoleDetails.address;
+
+    if (!address || isAmbigious(address))
+        address = getServerAddress();


I don't understand why the UI wouldn't just always show window.location.hostname -- in what situations would that be wrong?

It could be if Cockpit is running behind a reverse proxy -- but then you have no chance of figuring out the real server name/IP anyway, and the UI will be wrong. But I don't see much that you can do about it.

Server can have multiple IP addresses, and VNC server can be configured to listen only to one specific IP address.
But I'm overthinking here, in the vast majority of cases console address is either going to be a localhost (in which case user needs to make a tunnel) or it's going to be a host's IP address (in which case VNC should work fine without a tunnel)

So I think I'll drop this logic here, and see if somebody asks for it in the future

martinpitt · 2023-05-25T05:15:10Z

src/components/vm/consoles/desktopConsole.jsx

@@ -37,6 +40,11 @@ function fmt_to_fragments(fmt) {
 }

 const DesktopConsoleDownload = ({ vnc, spice, onDesktopConsole }) => {
+    // DesktopViewer prefers spice over vnc
+    const address = (spice && spice.address) || (vnc && vnc.address);


spice?.address || vnc?.address

martinpitt · 2023-05-25T05:18:09Z

src/components/vm/consoles/desktopConsole.jsx

+                           {needsTunnel(address, serverAddress) && <p>
+                               {_("SSH tunnel needs to be set up on client:")}
+                               <CodeBlock>
+                                   <CodeBlockCode>{`ssh -L 5900:localhost:5900 -N ${loggedUser.name}@${serverAddress}`}</CodeBlockCode>


This is actually really nice! Even if the serverAddress is slightly incorrect (due to reverse proxy and such), it will at least give admins an idea what needs to happen. Same for the local port -- it might be busy, and this doesn't work if you run this for more than one VM (you need to pick a different local port then), but this is meant as a guideline.

However, is it always 5900 on the remote side? Are VNC and spice using the same port? Can we ask libvirt about the port?

martinpitt · 2023-05-25T05:18:41Z

test/check-machines-consoles

@@ -67,6 +67,8 @@ class TestMachinesConsoles(VirtualMachinesCase):
        b.wait_in_text('.pf-c-expandable-section__content',
                       'Clicking "Launch remote viewer" will download')

+        b.wait_not_in_text('.pf-c-expandable-section__content', 'SSH tunnel needs to be set up on client')


Needs a positive test case, too. You already asked about this, just opening a thread to track it.

skobyda force-pushed the fixDesktopConsole branch 5 times, most recently from cf524a7 to ab2b488 Compare May 23, 2023 09:28

skobyda requested review from KKoukiou and martinpitt May 23, 2023 09:48

skobyda changed the title ~~User server hostname/IP for desktop console~~ Use server hostname/IP for desktop console May 23, 2023

skobyda force-pushed the fixDesktopConsole branch from ab2b488 to 2bfcbb1 Compare May 23, 2023 09:49

skobyda removed request for martinpitt and KKoukiou May 23, 2023 11:55

skobyda marked this pull request as draft May 23, 2023 11:59

skobyda force-pushed the fixDesktopConsole branch 2 times, most recently from 4cd662a to b7b7dc7 Compare May 23, 2023 12:47

skobyda marked this pull request as ready for review May 23, 2023 12:51

skobyda changed the title ~~Use server hostname/IP for desktop console~~ VNC/SPICE should listen on all interfaces May 23, 2023

skobyda force-pushed the fixDesktopConsole branch from b7b7dc7 to 0fbf07e Compare May 23, 2023 12:57

skobyda marked this pull request as draft May 23, 2023 13:13

skobyda mentioned this pull request May 24, 2023

desktop console show wrong ip address #1078

Open

skobyda force-pushed the fixDesktopConsole branch from 0fbf07e to 89e939b Compare May 24, 2023 16:25

skobyda marked this pull request as ready for review May 24, 2023 16:25

skobyda changed the title ~~VNC/SPICE should listen on all interfaces~~ Show helper to set up SSH tunnel for Desktop Viewer May 24, 2023

github-advanced-security bot found potential problems May 24, 2023

View reviewed changes

src/components/vm/consoles/desktopConsole.jsx Fixed Show fixed Hide fixed

skobyda force-pushed the fixDesktopConsole branch from 89e939b to 18e972b Compare May 24, 2023 16:31

skobyda added 2 commits May 24, 2023 18:33

Add helper functions to determine where the console server is listening

bbc85b0

Show helper to set up SSH tunnel for Desktop Viewer

242b8e2

skobyda force-pushed the fixDesktopConsole branch from 18e972b to 242b8e2 Compare May 24, 2023 16:33

skobyda requested review from martinpitt, marusak and KKoukiou May 24, 2023 16:40

skobyda commented May 24, 2023

View reviewed changes

martinpitt requested changes May 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show helper to set up SSH tunnel for Desktop Viewer #1084

Show helper to set up SSH tunnel for Desktop Viewer #1084

skobyda commented May 22, 2023 •

edited

skobyda May 24, 2023 •

edited

martinpitt May 25, 2023

martinpitt May 25, 2023

martinpitt left a comment

martinpitt May 25, 2023

martinpitt May 25, 2023

skobyda Jul 13, 2023

martinpitt May 25, 2023

skobyda Jul 14, 2023

martinpitt May 25, 2023

martinpitt May 25, 2023

martinpitt May 25, 2023

Show helper to set up SSH tunnel for Desktop Viewer #1084

Are you sure you want to change the base?

Show helper to set up SSH tunnel for Desktop Viewer #1084

Conversation

skobyda commented May 22, 2023 • edited

skobyda May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skobyda commented May 22, 2023 •

edited

skobyda May 24, 2023 •

edited