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.

Sign up for GitHub

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 helper to set up SSH tunnel for Desktop Viewer #1084

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbc85b0
Add helper functions to determine where the console server is listening
skobyda May 22, 2023
242b8e2
Show helper to set up SSH tunnel for Desktop Viewer
skobyda May 24, 2023
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
43 changes: 43 additions & 0 deletions src/components/vm/consoles/utils.js
View file
Open in desktop
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 •
edited
Loading

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);
}