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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/components/vm/consoles/desktopConsole.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
*/
import React from "react";
import { CodeBlock, CodeBlockCode } from "@patternfly/react-core/dist/esm/components/CodeBlock";
import { DesktopViewer } from '@patternfly/react-console';

import { getServerAddress, needsTunnel } from "./utils.js";
import cockpit from "cockpit";
import store from './../../../store.js';

const _ = cockpit.gettext;

Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spice?.address || vnc?.address

const serverAddress = getServerAddress();
const loggedUser = store.getState().systemInfo.loggedUser;

return (
<DesktopViewer spice={spice}
vnc={vnc}
Expand All @@ -57,6 +65,12 @@ const DesktopConsoleDownload = ({ vnc, spice, onDesktopConsole }) => {
<p>
{fmt_to_fragments(_("Clicking \"Launch remote viewer\" will download a .vv file and launch $0."), <i>Remote Viewer</i>)}
</p>
{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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

</CodeBlock>
</p>}
<p>
{fmt_to_fragments(_("$0 is available for most operating systems. To install it, search for it in GNOME Software or run the following:"), <i>Remote Viewer</i>)}
</p>
Expand Down
43 changes: 43 additions & 0 deletions src/components/vm/consoles/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* This file is part of Cockpit.
*
* Copyright (C) 2023 Red Hat, Inc.
*
* Cockpit is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation; either version 2.1 of the License, or
* (at your option) any later version.
*
* Cockpit is distributed in the hope that it will be useful, but
* WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
*/
export function getServerAddress() {
return window.location.hostname;
Copy link
Contributor Author

@skobyda skobyda May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁 )

}

export function isLocalhost(address) {
return address === "localhost" || address.startsWith("127");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

export function isAmbigious(address) {
skobyda marked this conversation as resolved.
Show resolved Hide resolved
return isLocalhost(address) || ["0", "0.0.0.0"].includes(address);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!address || isAmbigious(address))
address = getServerAddress();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return address;
}

export function needsTunnel(consoleAddress, serverAddress) {
return isLocalhost(consoleAddress) && !isLocalhost(serverAddress);
}
2 changes: 2 additions & 0 deletions test/check-machines-consoles
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


b.assert_pixels("#vm-subVmTest1-consoles-page", "vm-details-console-external", skip_layouts=["rtl"])

def testInlineConsole(self, urlroot=""):
Expand Down