Move and rename SSH master socket path to avoid path length limit (#3196

)

Putting SSH sockets into plan directory is nice and all, no chance to
re-use socket by mistake, no conflicts, but it turns out SSH will not
accept path longer than 104 characters. Or 108, depending who you ask.

So, moving sockets once again, this time then live in *run* workdir,
they no longer contain plan name, because that can be very long, and we
have two fallbacks:

* if the path is too long, we use `hashlib` and create less readable,
  but hopefully shorter name, and
* if the path is still too long, SSH multiplexing is disabled
  completely.

docs/releases.rst

@@ -5,6 +5,18 @@
 ======================
 
 
+tmt-1.37.0
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+tmt will now put SSH master control socket into ``ssh-socket``
+subdirectory of a workdir. Originally, sockets were stored in
+``/run/user/$UID`` directory, but this path led to conflicts when
+multiple tmt instances shared sockets incorrectly. A fix landed in
+1.36 that put sockets into ``provision`` subdirectory of each plan,
+but this solution will break for plans with longer names because of
+unavoidable UNIX socket path limit of 104 (or 108) characters.
+
+
 tmt-1.36.0
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

tests/provision/ssh-multiplexing/data/.fmf/version

@@ -0,0 +1 @@
+1

tests/provision/ssh-multiplexing/data/plan.fmf

@@ -0,0 +1,8 @@
+execute:
+    how: tmt
+
+discover:
+    how: shell
+    tests:
+    - name: smoke
+      test: /bin/true

tests/provision/ssh-multiplexing/main.fmf

@@ -0,0 +1,5 @@
+summary: Check that SSH multiplexing works as expected
+tag+:
+  - provision-only
+  - provision-connect
+  - provision-virtual

tests/provision/ssh-multiplexing/test.sh

@@ -0,0 +1,28 @@
+#!/bin/bash
+. /usr/share/beakerlib/beakerlib.sh || exit 1
+
+rlJournalStart
+    rlPhaseStartSetup
+        rlRun "PROVISION_HOW=${PROVISION_HOW:-virtual}"
+        rlRun "run=\$(mktemp -d)" 0 "Create run directory"
+        rlRun "long_run=\$(mktemp -d /tmp/tmp.veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongXXX)" 0 "Create run directory with a very long name"
+        rlRun "pushd data"
+    rlPhaseEnd
+
+    rlPhaseStartTest "SSH multiplexing should be enabled by default ($PROVISION_HOW)"
+        rlRun "tmt -vv run -i $run -a provision -h $PROVISION_HOW"
+        rlAssertGrep "Spawning the SSH master process" "$run/log.txt"
+    rlPhaseEnd
+
+    rlPhaseStartTest "SSH multiplexing should be disabled when SSH socket path gets too long ($PROVISION_HOW)"
+        rlRun "tmt -vv run -i $long_run -a provision -h $PROVISION_HOW"
+        rlAssertGrep "warn: SSH multiplexing will not be used because the SSH socket path '.*' is too long." "$long_run/log.txt"
+        rlAssertGrep "The SSH master process cannot be terminated because it is disabled." "$long_run/log.txt"
+    rlPhaseEnd
+
+    rlPhaseStartCleanup
+        rlRun "popd"
+        rlRun "rm -r $run" 0 "Remove run directory"
+        rlRun "rm -r $long_run" 0 "Remove run directory with the long name"
+    rlPhaseEnd
+rlJournalEnd

tmt/steps/provision/__init__.py

@@ -3,6 +3,7 @@
 import datetime
 import enum
 import functools
+import hashlib
 import os
 import re
 import secrets
@@ -116,6 +117,16 @@ def configure_ssh_options() -> tmt.utils.RawCommand:
 #: provided by environment variables.
 BASE_SSH_OPTIONS: tmt.utils.RawCommand = DEFAULT_SSH_OPTIONS + configure_ssh_options()
 
+#: SSH master socket path is limited to this many characters.
+#:
+#: * UNIX socket path is limited to either 108 or 104 characters, depending
+#:   on the platform. See `man 7 unix` and/or kernel sources, for example.
+#: * SSH client processes may create paths with added "connection hash"
+#:   when connecting to the master, that is a couple of characters we need
+#:   space for.
+#:
+SSH_MASTER_SOCKET_LENGTH_LIMIT = 104 - 20
+
 # Default rsync options
 DEFAULT_RSYNC_OPTIONS = [
     "-s", "-R", "-r", "-z", "--links", "--safe-links", "--delete"]
@@ -1414,22 +1425,80 @@ def _ssh_guest(self) -> str:
         """ Return user@guest """
         return f'{self.user}@{self.primary_address}'
 
+    @functools.cached_property
+    def _is_ssh_master_socket_path_acceptable(self) -> bool:
+        """ Whether the SSH master socket path we create is acceptable by SSH """
+
+        if len(str(self._ssh_master_socket_path)) >= SSH_MASTER_SOCKET_LENGTH_LIMIT:
+            self.warn("SSH multiplexing will not be used because the SSH socket path "
+                      f"'{self._ssh_master_socket_path}' is too long.")
+            return False
+
+        return True
+
+    @property
+    def is_ssh_multiplexing_enabled(self) -> bool:
+        """ Whether SSH multiplexing should be used """
+
+        if self.primary_address is None:
+            return False
+
+        if not self._is_ssh_master_socket_path_acceptable:
+            return False
+
+        return True
+
     @functools.cached_property
     def _ssh_master_socket_path(self) -> Path:
         """ Return path to the SSH master socket """
 
         assert isinstance(self.parent, tmt.steps.provision.Provision)
-        assert self.parent.workdir is not None
+        assert self.parent.plan.my_run is not None
+        assert self.parent.plan.my_run.workdir is not None
 
-        socket_dir = self.parent.workdir / 'ssh-sockets'
+        socket_dir = self.parent.plan.my_run.workdir / 'ssh-sockets'
 
         try:
             socket_dir.mkdir(parents=True, exist_ok=True)
 
         except Exception as exc:
             raise ProvisionError(f"Failed to create SSH socket directory '{socket_dir}'.") from exc
 
-        return socket_dir / f'{self.pathless_safe_name}.socket'
+        # Try more informative, but possibly too long path, constructed from pieces
+        # humans can easily understand and follow.
+        #
+        # The template is what seems to be a common template in general SSH discussions,
+        # hostname, port, username. Can we use guest name? Maybe, on the other hand, guest
+        # name is meaningless outside of its plan, it might be too ambiguous. Starting with
+        # what SSH folk uses, we may amend it later.
+
+        # This should be true, otherwise `is_ssh_multiplexing_enabled` would return `False`
+        # and nobody would need to use SSH master socket path.
+        assert self.primary_address
+
+        guest_id_components: list[str] = [self.primary_address]
+
+        if self.port:
+            guest_id_components.append(str(self.port))
+
+        if self.user:
+            guest_id_components.append(self.user)
+
+        guest_id = '-'.join(guest_id_components)
+
+        socket_path = socket_dir / f'{guest_id}.socket'
+
+        if len(str(socket_path)) < SSH_MASTER_SOCKET_LENGTH_LIMIT:
+            return socket_path
+
+        # Fall back to a less readable, but hopefully shorter and therefore acceptable filename.
+        # Note that we don't check the length anymore: giving up, this is the path, take it
+        # or leave it. And callers may very well leave it, we tried our best.
+        digest = hashlib \
+            .shake_128(guest_id.encode()) \
+            .hexdigest(16)
+
+        return socket_dir / f'{digest}.socket'
 
     @property
     def _ssh_options(self) -> Command:
@@ -1456,7 +1525,8 @@ def _ssh_options(self) -> Command:
             options.extend(['-oPasswordAuthentication=no'])
 
         # Include the SSH master process
-        options.append(f'-S{self._ssh_master_socket_path}')
+        if self.is_ssh_multiplexing_enabled:
+            options.append(f'-S{self._ssh_master_socket_path}')
 
         options.extend([f'-o{option}' for option in self.ssh_option])
 
@@ -1496,6 +1566,12 @@ def _cleanup_ssh_master_process(
             logger: Optional[tmt.log.Logger] = None) -> None:
         logger = logger or self._logger
 
+        if not self.is_ssh_multiplexing_enabled:
+            logger.debug('The SSH master process cannot be terminated because it is disabled.',
+                         level=3)
+
+            return
+
         with self._ssh_master_process_lock:
             if self._ssh_master_process is None:
                 logger.debug('The SSH master process cannot be terminated because it is unset.',
@@ -1525,13 +1601,17 @@ def _cleanup_ssh_master_process(
     def _ssh_command(self) -> Command:
         """ A base SSH command shared by all SSH processes """
 
-        with self._ssh_master_process_lock:
-            if self._ssh_master_process is None:
-                self._ssh_master_process = self._spawn_ssh_master_process()
+        if self.is_ssh_multiplexing_enabled:
+            with self._ssh_master_process_lock:
+                if self._ssh_master_process is None:
+                    self._ssh_master_process = self._spawn_ssh_master_process()
 
         return self._base_ssh_command
 
     def _unlink_ssh_master_socket_path(self) -> None:
+        if not self.is_ssh_multiplexing_enabled:
+            return
+
         with self._ssh_master_process_lock:
             if not self._ssh_master_socket_path:
                 return