Skip to content

Commit

Permalink
shell: Require confirmation before connecting to remote machines
Browse files Browse the repository at this point in the history
This requires calling connect_host() in reaction to the "connect"
event of the ShellState so that the warning dialog will be shown
before attempting any connection.

A consequence of that is that now login attempts are always
interactive, and show the password prompts immediately instead of only
showing the "Troubleshoot" curtain with a button to launch those
dialogs.
  • Loading branch information
mvollmer committed Nov 11, 2024
1 parent 90e8bd1 commit f2d86eb
Show file tree
Hide file tree
Showing 16 changed files with 313 additions and 69 deletions.
1 change: 1 addition & 0 deletions doc/guide/Makefile-guide.am
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ GUIDE_INCLUDES = \
doc/guide/cockpit-session.xml \
doc/guide/cockpit-spawn.xml \
doc/guide/cockpit-util.xml \
doc/guide/multi-host.xml \
doc/guide/authentication.xml \
doc/guide/embedding.xml \
doc/guide/feature-firewall.xml \
Expand Down
1 change: 1 addition & 0 deletions doc/guide/cockpit-guide.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<xi:include href="https.xml"/>
<xi:include href="listen.xml"/>
<xi:include href="startup.xml"/>
<xi:include href="multi-host.xml"/>
<xi:include href="authentication.xml"/>
<xi:include href="sso.xml"/>
<xi:include href="cert-authentication.xml"/>
Expand Down
54 changes: 54 additions & 0 deletions doc/guide/multi-host.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?xml version="1.0"?>
<!DOCTYPE chapter PUBLIC "-//OASIS//DTD DocBook XML V4.3//EN"
"http://www.oasis-open.org/docbook/xml/4.3/docbookx.dtd">
<chapter id="multi-host">
<title>
Managing multiple hosts at the same time
</title>

<para>
Cockpit allows you to access multiple hosts in a single session,
by establishing SSH connections to other hosts. This is quite
similar to logging into these other hosts using the "ssh" command
on the command line, with one very important difference:
</para>
<para>
Code from the local host and all the remote hosts run at the same
time, in the same browser context. They are not sufficiently
isolated from each other in the browser. All code effectively has
the same privileges as the primary session on the local host.
</para>
<para>
Thus, <emphasis>you should only only connect to remote hosts that
you trust</emphasis>. You must be sure that none of the hosts that
you connect to will cause Cockpit to load malicious JavaScript
code into your browser.
</para>
<para>
Going forward, Cockpit will try to provide sufficient isolation to
make it safe to manage multiple hosts in a single Cockpit
session. But until we get there, Cockpit will at least warn you
before connecting to more than one host. It is also possible to
disable multiple hosts entirely, and some operating systems do
this already by default.
</para>
<para>
You can prevent loading of JavaScript, HTML, etc from more than
one host by adding this to <filename>cockpit.conf</filename>:
</para>
<programlisting>
[WebService]
AllowMultiHost=false
</programlisting>
<para>
When you allow multiple hosts in a single Cockpit session by
setting <code>AllowMultiHost</code> to true, then the user will be
warned once per session, before connecting to the second host. If
that is still too much, you can switch it off completely by adding
the following to <filename>cockpit.conf</filename>:
</para>
<programlisting>
[Session]
WarnBeforeConnecting=false
</programlisting>
</chapter>
12 changes: 12 additions & 0 deletions doc/man/cockpit.conf.xml
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,18 @@ IdleTimeout=15
<para>When not specified, there is no idle timeout by default.</para>
</listitem>
</varlistentry>
<varlistentry>
<term><option>WarnBeforeConnecting</option></term>
<listitem>
<para>Whether to warn before connecting to remote hosts from the Shell. Defaults to true</para>
<informalexample>
<programlisting language="ini">
[Session]
WarnBeforeConnecting=false
</programlisting>
</informalexample>
</listitem>
</varlistentry>
</variablelist>
</refsect1>

Expand Down
120 changes: 102 additions & 18 deletions pkg/shell/hosts_dialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@ import { Popover } from "@patternfly/react-core/dist/esm/components/Popover/inde
import { Radio } from "@patternfly/react-core/dist/esm/components/Radio/index.js";
import { Stack } from "@patternfly/react-core/dist/esm/layouts/Stack/index.js";
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js";
import { OutlinedQuestionCircleIcon } from "@patternfly/react-icons";
import { OutlinedQuestionCircleIcon, ExternalLinkAltIcon } from "@patternfly/react-icons";
import { HelperText, HelperTextItem } from "@patternfly/react-core/dist/esm/components/HelperText/index.js";
import { Text, TextContent, TextVariants } from "@patternfly/react-core/dist/esm/components/Text";

import { FormHelper } from "cockpit-components-form-helper";
import { ModalError } from "cockpit-components-inline-notification.jsx";
import { fmt_to_fragments } from "utils.js";

import { build_href, split_connection_string, generate_connection_string } from "./util.jsx";

Expand Down Expand Up @@ -119,6 +122,12 @@ export async function connect_host(state, shell_state, machine) {
address: machine.address,
template: codes[machine.problem],
});
} else if (!window.sessionStorage.getItem("connection-warning-shown")) {
// connect by launching into the "Connection warning" dialog.
connection_string = await state.show_modal({
address: machine.address,
template: "connect"
});
} else {
// Try to connect without any dialog
try {
Expand All @@ -145,6 +154,7 @@ export async function connect_host(state, shell_state, machine) {
}

export const codes = {
danger: "connect",
"no-cockpit": "not-supported",
"not-supported": "not-supported",
"protocol-error": "not-supported",
Expand Down Expand Up @@ -200,6 +210,74 @@ class NotSupported extends React.Component {
}
}

class Connect extends React.Component {
constructor(props) {
super(props);

this.state = {
inProgress: false,
};
}

onConnect() {
window.sessionStorage.setItem("connection-warning-shown", true);
this.setState({ inProgress: true });
this.props.run(try2Connect(this.props.machines_ins, this.props.full_address), ex => {
let keep_message = false;
if (ex.problem === "no-host") {
let host_id_port = this.props.full_address;
let port = "22";
const port_index = host_id_port.lastIndexOf(":");
if (port_index === -1) {
host_id_port = this.props.full_address + ":22";
} else {
port = host_id_port.substr(port_index + 1);
}

ex.message = cockpit.format(_("Unable to contact the given host $0. Make sure it has ssh running on port $1, or specify another port in the address."), host_id_port, port);
ex.problem = "not-found";
keep_message = true;
}
this.setState({ inProgress: false });
this.props.setError(ex, keep_message);
});
}

render() {
return (
<Modal id="hosts_connect_server_dialog" isOpen
position="top" variant="small"
onClose={this.props.onClose}
title={fmt_to_fragments(_("Connect to $0?"), <b>{this.props.host}</b>)}
titleIconVariant="warning"
footer={<>
<HelperText>
<HelperTextItem>{_("You will be reminded once per session.")}</HelperTextItem>
</HelperText>
<Button variant="warning" isLoading={this.state.inProgress}
onClick={() => this.onConnect()}>
{_("Connect")}
</Button>
<Button variant="link" className="btn-cancel" onClick={this.props.onClose}>
{ _("Cancel") }
</Button>
</>}
>
<TextContent>
<Text component={TextVariants.p}>
{_("Remote hosts have the ability to run JavaScript on all connected hosts. Only connect to machines that you trust.")}
</Text>
<Text component={TextVariants.p}>
<a href="https://cockpit-project.org/guide/latest/multi-host.html" target="blank" rel="noopener noreferrer">
<ExternalLinkAltIcon /> {_("Read more")}
</a>
</Text>
</TextContent>
</Modal>
);
}
}

class AddMachine extends React.Component {
constructor(props) {
super(props);
Expand Down Expand Up @@ -323,23 +401,27 @@ class AddMachine extends React.Component {
});
});

this.props.run(try2Connect(this.props.machines_ins, address), ex => {
if (ex.problem === "no-host") {
let host_id_port = address;
let port = "22";
const port_index = host_id_port.lastIndexOf(":");
if (port_index === -1) {
host_id_port = address + ":22";
} else {
port = host_id_port.substr(port_index + 1);
}
if (!window.sessionStorage.getItem("connection-warning-shown")) {
this.props.setError({ problem: "danger", command: "close" });
} else {
this.props.run(try2Connect(this.props.machines_ins, address), ex => {
if (ex.problem === "no-host") {
let host_id_port = address;
let port = "22";
const port_index = host_id_port.lastIndexOf(":");
if (port_index === -1) {
host_id_port = address + ":22";
} else {
port = host_id_port.substr(port_index + 1);
}

ex.message = cockpit.format(_("Unable to contact the given host $0. Make sure it has ssh running on port $1, or specify another port in the address."), host_id_port, port);
ex.problem = "not-found";
}
this.setState({ inProgress: false });
this.props.setError(ex);
});
ex.message = cockpit.format(_("Unable to contact the given host $0. Make sure it has ssh running on port $1, or specify another port in the address."), host_id_port, port);
ex.problem = "not-found";
}
this.setState({ inProgress: false });
this.props.setError(ex);
});
}
}

render() {
Expand Down Expand Up @@ -1157,7 +1239,9 @@ class HostModalInner extends React.Component {
complete: this.complete,
};

if (template === "add-machine")
if (template === "connect")
return <Connect {...props} />;
else if (template === "add-machine")
return <AddMachine {...props} />;
else if (template === "unknown-hostkey" || template === "unknown-host" || template === "invalid-hostkey")
return <HostKey {...props} />;
Expand Down
5 changes: 1 addition & 4 deletions pkg/shell/shell.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,7 @@ const Shell = () => {
useEvent(host_modal_state, "changed");

useEvent(state, "connect", () => {
// We could launch some dialogs here, but the traditional
// behavior is to just connect the loader and open the dialogs
// from the troubleshoot button.
state.loader.connect(state.current_machine.address);
connect_host(host_modal_state, state, state.current_machine);
});

const {
Expand Down
21 changes: 19 additions & 2 deletions pkg/shell/state.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,24 @@ export function ShellState() {
if (meta_multihost instanceof HTMLMetaElement && meta_multihost.content == "yes")
config.host_switcher_enabled = true;

/* Should show warning before connecting? */
let config_ready = false;
cockpit.dbus(null, { bus: "internal" }).call("/config", "cockpit.Config", "GetString",
["Session", "WarnBeforeConnecting"], [])
.then(([result]) => {
if (result == "false" || result == "no") {
window.sessionStorage.setItem("connection-warning-shown", "yes");
}
})
.catch(e => {
if (e.name != "cockpit.Config.KeyError")
console.warn("Error reading WarnBeforeConnecting configuration:", e.message);
})
.finally(() => {
config_ready = true;
on_ready();
});

/* MACHINES DATABASE AND MANIFEST LOADER
*
* These are part of the machinery in the basement that maintains
Expand Down Expand Up @@ -74,7 +92,7 @@ export function ShellState() {
on_ready();

function on_ready() {
if (machines.ready) {
if (machines.ready && config_ready) {
self.ready = true;
window.addEventListener("popstate", ev => {
update();
Expand Down Expand Up @@ -487,7 +505,6 @@ export function ShellState() {

// Methods
jump,
ensure_connection,
remove_frame,
most_recent_path_for_host,

Expand Down
20 changes: 17 additions & 3 deletions test/common/testlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -1141,13 +1141,23 @@ def start_machine_troubleshoot(
known_host: bool = False,
password: str | None = None,
expect_closed_dialog: bool = True,
expect_warning: bool = True,
need_click: bool = True
) -> None:
self.click('#machine-troubleshoot')
if need_click:
self.click('#machine-troubleshoot')

if not new and expect_warning:
self.wait_visible('#hosts_connect_server_dialog')
self.click("#hosts_connect_server_dialog button.pf-m-warning")

self.wait_visible('#hosts_setup_server_dialog')
if new:
self.wait_text("#hosts_setup_server_dialog button.pf-m-primary", "Add")
self.click("#hosts_setup_server_dialog button.pf-m-primary")
if expect_warning:
self.wait_visible('#hosts_connect_server_dialog')
self.click("#hosts_connect_server_dialog button.pf-m-warning")
if not known_host:
self.wait_in_text('#hosts_setup_server_dialog', "You are connecting to")
self.wait_in_text('#hosts_setup_server_dialog', "for the first time.")
Expand All @@ -1161,10 +1171,14 @@ def start_machine_troubleshoot(
if expect_closed_dialog:
self.wait_not_present('#hosts_setup_server_dialog')

def add_machine(self, address: str, known_host: bool = False, password: str = "foobar") -> None:
def add_machine(self, address: str, known_host: bool = False, password: str = "foobar",
expect_warning: bool = True) -> None:
self.switch_to_top()
self.go(f"/@{address}")
self.start_machine_troubleshoot(new=True, known_host=known_host, password=password)
self.start_machine_troubleshoot(new=True,
known_host=known_host,
password=password,
expect_warning=expect_warning)
self.enter_page("/system", host=address)

def grant_permissions(self, *args: str) -> None:
Expand Down
Loading

0 comments on commit f2d86eb

Please sign in to comment.