From 22729f8ce13e7e748eb9effa133fc051a30c172c Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 29 Sep 2023 13:44:32 +0200 Subject: [PATCH 01/10] flatpak: Add test helper for waiting for a selector to contain a given text Also fix an eslint complaint. We don't run eslint on that directory, but vim does. --- containers/flatpak/test/test-lib.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/containers/flatpak/test/test-lib.js b/containers/flatpak/test/test-lib.js index 18d929d8e47..65ed47a686d 100644 --- a/containers/flatpak/test/test-lib.js +++ b/containers/flatpak/test/test-lib.js @@ -64,6 +64,11 @@ function ph_wait_not_visible(sel) { }, 10000, "timed out waiting for " + sel + " to be visible"); } +function ph_wait_in_text(sel, match) { + return ph_wait_cond(() => window.cur_doc.querySelector(sel).textContent.includes(match), + 1000, `timed out waiting for ${sel} to contain ${match}`); +} + function ph_wait_count(sel, count) { return ph_wait_cond(() => (window.cur_doc.querySelectorAll(sel).length === count), 10000, "timed out waiting for " + sel + " to be visible"); } @@ -102,7 +107,7 @@ function ph_mouse(sel, type, x, y, btn, ctrlKey, shiftKey, altKey, metaKey) { bubbles: true, cancelable: true, view: window, - detail: detail, + detail, screenX: left + (x || 0), screenY: top + (y || 0), clientX: left + (x || 0), From 08eb0836b550967183239fee56340c5fa23d1de4 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 29 Sep 2023 13:46:16 +0200 Subject: [PATCH 02/10] flatpak: Add SSH login test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exercise the beiboot → SSH code path with actual interaction (user and password). Only run the test when giving `$COCKPIT_TEST_SSH_{HOST,PASS}`. That way, developers can run the test locally against our usual test VM: COCKPIT_TEST_SSH_HOST=admin@127.0.0.2:2201 COCKPIT_TEST_SSH_PASS=foobar Create a user in the GitHub VM. --- .github/workflows/flatpak-test.yml | 5 +++ containers/flatpak/test/test-browser | 14 ++++++++- .../flatpak/test/test-browser-login-ssh.js | 31 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 containers/flatpak/test/test-browser-login-ssh.js diff --git a/.github/workflows/flatpak-test.yml b/.github/workflows/flatpak-test.yml index 09e4893c3fc..6a34e8fe90c 100644 --- a/.github/workflows/flatpak-test.yml +++ b/.github/workflows/flatpak-test.yml @@ -27,6 +27,11 @@ jobs: - name: Smoke-test the installed flatpak run: | + export COCKPIT_TEST_SSH_PASS=foobar + sudo useradd -c User -s /bin/bash user + echo user:$COCKPIT_TEST_SSH_PASS | sudo chpasswd + export COCKPIT_TEST_SSH_HOST=user@127.0.0.1 + . /etc/profile.d/flatpak.sh xvfb-run sh -ec ' dbus-run-session containers/flatpak/test/test-ssh diff --git a/containers/flatpak/test/test-browser b/containers/flatpak/test/test-browser index 26759f7c91e..caf3eb59d29 100755 --- a/containers/flatpak/test/test-browser +++ b/containers/flatpak/test/test-browser @@ -63,10 +63,13 @@ class BrowserTest(unittest.TestCase): self.app_actions.activate_action("quit") self.assertEqual(self.p_client.wait(), 0) - def run_js(self, js_name): + def run_js(self, js_name, substitutions=None): with open(os.path.join(TEST_DIR, js_name)) as f: js = f.read() + for (pattern, replacement) in (substitutions or []): + js = js.replace(pattern, replacement) + # WebKit.run_javascript() needs some serializable return value js += '\ntrue\n' @@ -118,5 +121,14 @@ class BrowserTest(unittest.TestCase): # check that `invalid` was not added and try to remove `localhost` from the list self.assertEqual(self.run_js("test-browser-login-remove.js"), "PASS") + # SSH login + host = os.environ.get("COCKPIT_TEST_SSH_HOST") + if host: + substs = [ + ("%HOST%", host), + ("%PASS%", os.environ["COCKPIT_TEST_SSH_PASS"]), + ] + self.assertEqual(self.run_js("test-browser-login-ssh.js", substs), "page-load") + unittest.main() diff --git a/containers/flatpak/test/test-browser-login-ssh.js b/containers/flatpak/test/test-browser-login-ssh.js new file mode 100644 index 00000000000..38e1129a97f --- /dev/null +++ b/containers/flatpak/test/test-browser-login-ssh.js @@ -0,0 +1,31 @@ +/* global ph_mouse ph_set_result ph_wait_present ph_wait_visible ph_wait_in_text */ + +async function assert_conversation(match) { + await ph_wait_present("#conversation-prompt"); + await ph_wait_visible("#conversation-prompt"); + await ph_wait_in_text("#conversation-prompt", match); +} + +async function test() { + try { + await ph_wait_present("#server-field"); + document.getElementById("server-field").value = "%HOST%"; + ph_mouse("#login-button", "click"); + + // unknown host key + await assert_conversation("authenticity of host"); + document.getElementById("conversation-input").value = "yes"; + ph_mouse("#login-button", "click"); + + await ph_wait_present("#conversation-prompt"); + await assert_conversation("password"); + document.getElementById("conversation-input").value = "%PASS%"; + + // this will cause a page load, ending the test + ph_mouse("#login-button", "click"); + } catch (e) { + ph_set_result(e); + } +} + +test(); From 2779f0aff4f580569f6db9debd32faf78800c7a8 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 28 Sep 2023 06:09:42 +0200 Subject: [PATCH 03/10] test: Factorize cockpit-ws startup in TestClient --- test/verify/check-client | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/verify/check-client b/test/verify/check-client index d592fe276ef..d42ca3180f7 100755 --- a/test/verify/check-client +++ b/test/verify/check-client @@ -82,17 +82,16 @@ Command = /usr/bin/env python3 -m cockpit.beiboot timeout=30) def testBeibootNoBridge(self): - self.m_client.spawn(f"runuser -u admin -- {self.libexecdir}/cockpit-ws --no-tls", "ws.log") # set up target machine: no cockpit self.m_target.execute("rm /usr/bin/cockpit-bridge; rm -r /usr/share/cockpit") - self.checkLoginScenarios(local_bridge=False) def testBeibootWithBridge(self): - self.m_client.spawn(f"runuser -u admin -- {self.libexecdir}/cockpit-ws --no-tls", "ws.log") self.checkLoginScenarios(local_bridge=True) def checkLoginScenarios(self, *, local_bridge=True): + self.m_client.spawn(f"runuser -u admin -- {self.libexecdir}/cockpit-ws --no-tls", "ws.log") + b = self.browser b.open("/") From 22d6c1aea342897e25cdfd740112a11ef55efbe5 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 28 Sep 2023 10:01:31 +0200 Subject: [PATCH 04/10] test: Use standard logout test API in check-client --- test/verify/check-client | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/verify/check-client b/test/verify/check-client index d42ca3180f7..7b760d77136 100755 --- a/test/verify/check-client +++ b/test/verify/check-client @@ -66,9 +66,7 @@ Command = /usr/bin/env python3 -m cockpit.beiboot def logout(self, check_last_host=None): b = self.browser - b.assert_no_oops() - b.open_session_menu() - b.click('#logout') + b.logout() # FIXME: This is broken, nothing appears # b.wait_text("#brand", "Connect to:") if check_last_host: From 4530e86104c75310bd700f94d92b26603fc96107 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 28 Sep 2023 10:34:48 +0200 Subject: [PATCH 05/10] test: Accept destroyed execution context when clicking logout This is quite literally what it is defined to do. This races with the CDP driver finishing the command, so sometimes it would fail the test on throwing that RuntimeError. --- test/common/testlib.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/common/testlib.py b/test/common/testlib.py index cb3644b5068..5794571cbed 100644 --- a/test/common/testlib.py +++ b/test/common/testlib.py @@ -807,7 +807,12 @@ def logout(self): else: # happens when cockpit is still running self.open_session_menu() - self.click('#logout') + try: + self.click('#logout') + except RuntimeError as e: + # logging out does destroy the current frame context, it races with the CDP driver finishing the command + if "Execution context was destroyed" not in str(e): + raise self.wait_visible('#login') self.machine.allow_restart_journal_messages() From a13a81716968eb659fbb186baa1bd8247b66c57a Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Tue, 26 Sep 2023 10:41:43 +0200 Subject: [PATCH 06/10] selinux: Document how to quickly iterate policy changes Knowing what to change in the policy is often not quite obvious, and takes several attempts. Show how to do this much more quickly than repeated rpm/image builds. --- selinux/HACKING.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 selinux/HACKING.md diff --git a/selinux/HACKING.md b/selinux/HACKING.md new file mode 100644 index 00000000000..42d080eed26 --- /dev/null +++ b/selinux/HACKING.md @@ -0,0 +1,11 @@ +# Changing cockpit's SELinux policy + +The clean way is to edit the policy files and then rebuild the rpms and image with +`test/image-prepare -q fedora-XX`. + +To iterate more quickly, locally `./configure` the build tree with +`--enable-selinux-policy=targeted`, then you can quickly recompile and install +the policy into the `c` SSH target with: +```sh +make cockpit.pp && scp cockpit.pp c:/tmp/ && ssh c semodule -i /tmp/cockpit.pp +``` From 28463c793c45103b0ab9cfb94ec21512fe8064ab Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Tue, 26 Sep 2023 09:18:05 +0200 Subject: [PATCH 07/10] selinux: Allow cockpit-ws to run ssh(1) This is a prerequisite for switching the login page from cockpit-ssh to the cockpit-beiboot implementation, which runs ssh(1) directly. --- selinux/cockpit.te | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/selinux/cockpit.te b/selinux/cockpit.te index c2977853f8f..d826333ec3d 100644 --- a/selinux/cockpit.te +++ b/selinux/cockpit.te @@ -49,8 +49,13 @@ can_exec(cockpit_ws_t,cockpit_ws_exec_t) # systemd can execute cockpit-session can_exec(init_t,cockpit_session_exec_t) -# cockpit-ws can execute cockpit-session +# cockpit-ws can execute cockpit-{ssh,session} can_exec(cockpit_ws_t,cockpit_session_exec_t) +# ... and the real ssh (through beiboot/ferny) +gen_require(` + type ssh_exec_t; +') +can_exec(cockpit_ws_t,ssh_exec_t) # cockpit-ws can read from /dev/urandom dev_read_urand(cockpit_ws_t) # for authkey From 7e94dc1b9e461e2356f4cf0fb25787dc181fb140 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 28 Sep 2023 09:03:45 +0200 Subject: [PATCH 08/10] selinux: Allow cockpit_ws_t to read /sys On Fedora 39 we get this rejection: avc: denied { read } for comm="cockpit-askpass" name="possible" dev="sysfs" scontext=system_u:system_r:cockpit_ws_t:s0 tcontext=system_u:object_r:sysfs_t:s0 This also happens for "cockpit-beiboot". Our code doesn't directly access sysfs (at least not /devices/system/cpu/possible), but glibc does, and Python somehow inherits this. There is no harm in letting it read sysfs, and it may even be necessary for hardware based authentication schemas, so allow it. --- selinux/cockpit.te | 1 + 1 file changed, 1 insertion(+) diff --git a/selinux/cockpit.te b/selinux/cockpit.te index d826333ec3d..6df2321e73d 100644 --- a/selinux/cockpit.te +++ b/selinux/cockpit.te @@ -42,6 +42,7 @@ allow cockpit_ws_t self:process setrlimit; allow cockpit_ws_t self:tcp_socket create_stream_socket_perms; kernel_read_system_state(cockpit_ws_t) +dev_read_sysfs(cockpit_ws_t) # cockpit-tls can execute cockpit-ws can_exec(cockpit_ws_t,cockpit_ws_exec_t) From d0e7acee86e5c504414af1a13b0c8e03d9109ba5 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 28 Sep 2023 12:14:02 +0200 Subject: [PATCH 09/10] pybridge: Restore cockpit.beiboot debug logging Commit ba933040be2 dropped the executable wrapper around beiboot.py, which broke `COCKPIT_DEBUG=cockpit.beiboot`. Hardcode the name to bring it back. --- src/cockpit/beiboot.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cockpit/beiboot.py b/src/cockpit/beiboot.py index a54fee0823d..0c36ab20667 100644 --- a/src/cockpit/beiboot.py +++ b/src/cockpit/beiboot.py @@ -40,7 +40,7 @@ from cockpit.router import Router, RoutingRule from cockpit.transports import StdioTransport -logger = logging.getLogger(__name__) +logger = logging.getLogger('cockpit.beiboot') def ensure_ferny_askpass() -> Path: From 33154ddf699fad090d311ae1670901cd8465c9b2 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Wed, 27 Sep 2023 10:49:01 +0200 Subject: [PATCH 10/10] pybridge: Let cockpit-beiboot use package-installed askpass helper We need and want ensure_ferny_askpass() *only* for the flatpak case. Add some comments about that, as we already forgot it once. When running it from distro packages or the cockpit/ws container, we always want to use the packaged cockpit-askpass helper. Re-use `patch_libexecdir()` for that, as it already has all the necessary magic. Turn that into a proper global function. This fixes running cockpit-beiboot as cockpit-wsinstance system user, as there is no existing/writable home directory, and trying to mkdir ~/.cache EPERMs. --- src/cockpit/beiboot.py | 24 +++++++++++-- src/cockpit/packages.py | 75 +++++++++++++++++++++-------------------- 2 files changed, 59 insertions(+), 40 deletions(-) diff --git a/src/cockpit/beiboot.py b/src/cockpit/beiboot.py index 0c36ab20667..3e2583e1903 100644 --- a/src/cockpit/beiboot.py +++ b/src/cockpit/beiboot.py @@ -34,7 +34,7 @@ from cockpit.channel import ChannelRoutingRule from cockpit.channels import PackagesChannel from cockpit.jsonutil import JsonObject -from cockpit.packages import Packages, PackagesLoader +from cockpit.packages import Packages, PackagesLoader, patch_libexecdir from cockpit.peer import Peer from cockpit.protocol import CockpitProblem from cockpit.router import Router, RoutingRule @@ -44,9 +44,15 @@ def ensure_ferny_askpass() -> Path: + """Create askpass executable + + We need this for the flatpak: ssh and thus the askpass program run on the host (via flatpak-spawn), + not the flatpak. Thus we cannot use the shipped cockpit-askpass program. + """ src_path = importlib.resources.files(ferny.__name__) / 'interaction_client.py' src_data = src_path.read_bytes() + # Create the file in $XDG_CACHE_HOME, one of the few locations that a flatpak can write to xdg_cache_home = os.environ.get('XDG_CACHE_HOME') if xdg_cache_home is None: xdg_cache_home = os.path.expanduser('~/.cache') @@ -185,9 +191,21 @@ async def do_connect_transport(self) -> None: cmd: Sequence[str] = ('python3', '-ic', '# cockpit-bridge') env: Sequence[str] = () + in_flatpak = os.path.exists('/.flatpak-info') + # Remote host? Wrap command with SSH if self.destination != 'localhost': - ssh_askpass = ensure_ferny_askpass() + if in_flatpak: + # we run ssh and thus the helper on the host, always use the xdg-cache helper + ssh_askpass = ensure_ferny_askpass() + else: + # outside of the flatpak we expect cockpit-ws and thus an installed helper + askpass = patch_libexecdir('${libexecdir}/cockpit-askpass') + assert isinstance(askpass, str) + ssh_askpass = Path(askpass) + if not ssh_askpass.exists(): + logger.error("Could not find cockpit-askpass helper at %r", askpass) + env = ( f'SSH_ASKPASS={ssh_askpass!s}', 'DISPLAY=x', @@ -203,7 +221,7 @@ async def do_connect_transport(self) -> None: cmd = ('ssh', *host_args, shlex.join(cmd)) # Running in flatpak? Wrap command with flatpak-spawn --host - if os.path.exists('/.flatpak-info'): + if in_flatpak: cmd = ('flatpak-spawn', '--host', *(f'--env={kv}' for kv in env), *cmd) diff --git a/src/cockpit/packages.py b/src/cockpit/packages.py index 938b6203fc2..28fffcd853a 100644 --- a/src/cockpit/packages.py +++ b/src/cockpit/packages.py @@ -92,6 +92,43 @@ def sortify_version(version: str) -> str: return '.'.join(part.zfill(8) for part in version.split('.')) +@functools.lru_cache() +def get_libexecdir() -> str: + """Detect libexecdir on current machine + + This only works for systems which have cockpit-ws installed. + """ + for candidate in ['/usr/local/libexec', '/usr/libexec', '/usr/local/lib/cockpit', '/usr/lib/cockpit']: + if os.path.exists(os.path.join(candidate, 'cockpit-askpass')): + return candidate + else: + logger.warning('Could not detect libexecdir') + # give readable error messages + return '/nonexistent/libexec' + + +# HACK: Type narrowing over Union types is not supported in the general case, +# but this works for the case we care about: knowing that when we pass in an +# JsonObject, we'll get an JsonObject back. +J = TypeVar('J', JsonObject, JsonDocument) + + +def patch_libexecdir(obj: J) -> J: + if isinstance(obj, str): + if '${libexecdir}/cockpit-askpass' in obj: + # extra-special case: we handle this internally + abs_askpass = shutil.which('cockpit-askpass') + if abs_askpass is not None: + return obj.replace('${libexecdir}/cockpit-askpass', abs_askpass) + return obj.replace('${libexecdir}', get_libexecdir()) + elif isinstance(obj, dict): + return {key: patch_libexecdir(value) for key, value in obj.items()} + elif isinstance(obj, list): + return [patch_libexecdir(item) for item in obj] + else: + return obj + + # A document is a binary stream with a Content-Type, optional Content-Encoding, # and optional Content-Security-Policy class Document(NamedTuple): @@ -281,42 +318,6 @@ class PackagesLoader: 'path-not-exists': lambda p: not os.path.exists(p), } - @staticmethod - @functools.lru_cache() - def get_libexecdir() -> str: - """Detect libexecdir on current machine - - This only works for systems which have cockpit-ws installed. - """ - for candidate in ['/usr/local/libexec', '/usr/libexec', '/usr/local/lib/cockpit', '/usr/lib/cockpit']: - if os.path.exists(os.path.join(candidate, 'cockpit-askpass')): - return candidate - else: - logger.warning('Could not detect libexecdir') - # give readable error messages - return '/nonexistent/libexec' - - # HACK: Type narrowing over Union types is not supported in the general case, - # but this works for the case we care about: knowing that when we pass in an - # JsonObject, we'll get an JsonObject back. - J = TypeVar('J', JsonObject, JsonDocument) - - @classmethod - def patch_libexecdir(cls, obj: J) -> J: - if isinstance(obj, str): - if '${libexecdir}/cockpit-askpass' in obj: - # extra-special case: we handle this internally - abs_askpass = shutil.which('cockpit-askpass') - if abs_askpass is not None: - return obj.replace('${libexecdir}/cockpit-askpass', abs_askpass) - return obj.replace('${libexecdir}', cls.get_libexecdir()) - elif isinstance(obj, dict): - return {key: cls.patch_libexecdir(value) for key, value in obj.items()} - elif isinstance(obj, list): - return [cls.patch_libexecdir(item) for item in obj] - else: - return obj - @classmethod def get_xdg_data_dirs(cls) -> Iterable[str]: try: @@ -368,7 +369,7 @@ def patch_manifest(cls, manifest: JsonObject, parent: Path) -> JsonObject: manifest = cls.merge_patch(manifest, override) - return cls.patch_libexecdir(manifest) + return patch_libexecdir(manifest) @classmethod def load_manifests(cls) -> Iterable[Manifest]: