[crmsh-4.6] Fix: sh: Avoid unnecessary sudo calls in ClusterShell SSH commands (bsc#1229416) (#1965)

The `subprocess_run_without_input` method in `ClusterShell` was
unconditionally including `sudo -H -u ${user}` in its SSH command
construction, even when the target user is the same as the remote user.

This commit modifies the logic to conditionally include `sudo -H -u
${user}` only when the target `user` is different from the
`remote_user`. This prevents unnecessary `sudo` calls, which is
important in environments where `sudo` might not be installed or
configured, and improves the robustness of SSH command execution.

Author: liangxin1300

crmsh/bootstrap.py

@@ -182,29 +182,45 @@ def initialize_user(self):
         """
         users_of_specified_hosts: 'not_specified', 'specified', 'no_hosts'
         """
-        if self.cluster_node is None and self.user_at_node_list is None:
-            users_of_specified_hosts = 'no_hosts'
-        elif self.cluster_node is not None and '@' in self.cluster_node:
-            users_of_specified_hosts = 'specified'
-        elif self.user_at_node_list is not None and any('@' in x for x in self.user_at_node_list):
-            users_of_specified_hosts = 'specified'
-        else:
-            users_of_specified_hosts = 'not_specified'
         sudoer = userdir.get_sudoer()
-        has_sudoer = sudoer is not None
-        if users_of_specified_hosts == 'specified':
-            if has_sudoer:
-                self.current_user = sudoer
+        if self.cluster_node is not None:
+            parts = self.cluster_node.split('@', 1)
+            if len(parts) == 2:
+                user, host = parts
+                cluster_node_user = user
             else:
+                cluster_node_user = 'root'
+            if cluster_node_user == 'root':
+                assert userdir.getuser() == 'root'
+                self.current_user = 'root'
+            elif sudoer is None:
                 utils.fatal("Unsupported config: local node is using root and remote nodes is using non-root users.")
-        elif users_of_specified_hosts == 'not_specified':
-            assert userdir.getuser() == 'root'
-            self.current_user = 'root'
-        elif users_of_specified_hosts == 'no_hosts':
+            else:
+                self.current_user = sudoer
+        elif self.user_at_node_list:
+            has_root = False
+            has_non_root = False
+            for item in self.user_at_node_list:
+                parts = item.split('@', 1)
+                if len(parts) == 2:
+                    user, host = parts
+                    has_root = has_root or user == 'root'
+                    has_non_root = has_non_root or user != 'root'
+                else:
+                    has_root = True
+            if has_root and has_non_root:
+                utils.fatal("Unsupported config: mixing root and non-root users in a cluster.")
+            elif has_root:
+                assert userdir.getuser() == 'root'
+                self.current_user = 'root'
+            else:
+                if sudoer is None:
+                    utils.fatal("Unsupported config: local node is using root and remote nodes is using non-root users.")
+                else:
+                    self.current_user = sudoer
+        else:
             assert userdir.getuser() == 'root'
             self.current_user = 'root'
-        else:
-            raise AssertionError('Bad parameter user_of_specified_hosts: {}'.format(users_of_specified_hosts))
 
     def _validate_sbd_option(self):
         """

crmsh/prun/prun.py

@@ -12,7 +12,7 @@
 
 _DEFAULT_CONCURRENCY = 32
 
-_SUDO_SFTP_SERVER = 'sudo PATH=/usr/lib/ssh:/usr/lib/openssh:/usr/libexec/ssh:/usr/libexec/openssh /bin/sh -c "exec sftp-server"'
+_SUDO_SFTP_SERVER_OPTION = '-s \'sudo PATH=/usr/lib/ssh:/usr/lib/openssh:/usr/libexec/ssh:/usr/libexec/openssh /bin/sh -c "exec sftp-server"\''
 
 
 class ProcessResult:
@@ -117,7 +117,10 @@ def _build_run_task(remote: str, cmdline: str) -> Task:
         )
     else:
         local_sudoer, remote_sudoer = UserOfHost.instance().user_pair_for_ssh(remote)
-        shell = 'ssh {} {}@{} sudo -H /bin/sh'.format(crmsh.constants.SSH_OPTION, remote_sudoer, remote)
+        if remote_sudoer == "root":
+            shell = 'ssh {} {}@{} /bin/bash'.format(crmsh.constants.SSH_OPTION, remote_sudoer, remote)
+        else:
+            shell = 'ssh {} {}@{} sudo -H /bin/bash'.format(crmsh.constants.SSH_OPTION, remote_sudoer, remote)
         if local_sudoer == crmsh.userdir.getuser():
             args = ['/bin/sh', '-c', shell]
         elif os.geteuid() == 0:
@@ -188,10 +191,10 @@ def pcopy_to_remote(
 
 def _build_copy_task(ssh: str, script: str, host: str):
     _, remote_sudoer = UserOfHost.instance().user_pair_for_ssh(host)
-    cmd = "sftp {} {} -o BatchMode=yes -s '{}' -b - {}@{}".format(
+    cmd = "sftp {} {} -o BatchMode=yes {} -b - {}@{}".format(
         ssh,
         crmsh.constants.SSH_OPTION,
-        _SUDO_SFTP_SERVER,
+        _SUDO_SFTP_SERVER_OPTION if remote_sudoer != 'root' else '',
         remote_sudoer, _enclose_inet6_addr(host),
     )
     return Task(
@@ -254,10 +257,10 @@ def pfetch_from_remote(
 
 def _build_fetch_task( ssh: str, host: str, src: str, dst: str, flags: str) -> Task:
     _, remote_sudoer = UserOfHost.instance().user_pair_for_ssh(host)
-    cmd = "sftp {} {} -o BatchMode=yes -s '{}' -b - {}@{}".format(
+    cmd = "sftp {} {} -o BatchMode=yes {} -b - {}@{}".format(
         ssh,
         crmsh.constants.SSH_OPTION,
-        _SUDO_SFTP_SERVER,
+        _SUDO_SFTP_SERVER_OPTION if remote_sudoer != 'root' else '',
         remote_sudoer, _enclose_inet6_addr(host),
     )
     os.makedirs(f"{dst}/{host}", exist_ok=True)

crmsh/prun/runner.py

@@ -36,6 +36,8 @@ def __init__(
         # Caller can pass arbitrary data to context, it is kept untouched.
         self.context = context
 
+    def __repr__(self):
+        return f"TaskArgumentsEq({self.args}, {self.input}, {self.stdout_config}, {self.stderr_config}, {self.context}"
 
 class Runner:
     def __init__(self, concurrency):

crmsh/sh.py

@@ -339,17 +339,16 @@ def subprocess_run_without_input(self, host: typing.Optional[str], user: typing.
             if user is None:
                 user = 'root'
             local_user, remote_user = self.user_of_host.user_pair_for_ssh(host)
+            ssh_agent_options = '-A' if self.forward_ssh_agent else ''
+            if user == remote_user:
+                shell = f'ssh {ssh_agent_options} {constants.SSH_OPTION} -o BatchMode=yes {remote_user}@{host} /bin/bash'
+            else:
+                preserve_env_options = '--preserve-env=SSH_AUTH_SOCK' if self.forward_ssh_agent else ''
+                shell = f'ssh {ssh_agent_options} {constants.SSH_OPTION} -o BatchMode=yes {remote_user}@{host}' \
+                        f' sudo -H -u {user} {preserve_env_options} /bin/bash'
             result = self.local_shell.su_subprocess_run(
                 local_user,
-                'ssh {} {} -o BatchMode=yes {}@{} sudo -H -u {} {} /bin/sh'.format(
-                    '-A' if self.forward_ssh_agent else '',
-                    constants.SSH_OPTION,
-                    remote_user,
-                    host,
-                    user,
-                    '--preserve-env=SSH_AUTH_SOCK' if self.forward_ssh_agent else '',
-                    constants.SSH_OPTION,
-                ),
+                shell,
                 input=cmd.encode('utf-8'),
                 start_new_session=True,
                 **kwargs,

test/features/qdevice_setup_remove.feature

@@ -148,6 +148,15 @@ Feature: corosync qdevice/qnetd setup/remove process
     Then    Service "corosync-qdevice" is "started" on "hanode1"
     And     Service "corosync-qdevice" is "started" on "hanode2"
 
+  @skip_non_root
+  @clean
+  Scenario: Initialize qnetd without "sudo" installed (bsc#1229416)
+    When    Run "mv /usr/bin/sudo /usr/bin/sudo.bak" on "qnetd-node"
+    When    Run "crm cluster init -y --qnetd-hostname root@qnetd-node" on "hanode1"
+    Then    Service "corosync-qdevice" is "started" on "hanode1"
+    Then    Service "corosync-qnetd" is "started" on "qnetd-node"
+    When    Run "mv /usr/bin/sudo.bak /usr/bin/sudo" on "qnetd-node"
+
   @skip_non_root
   @clean
   Scenario: Missing crm/crm.conf (bsc#1209193)

test/unittests/test_bootstrap.py

@@ -324,6 +324,10 @@ def test_initialize_user_cluster_node_with_user_without_sudoer(self, mock_getuse
         context.user_at_node_list = None
         with self.assertRaises(ValueError):
             context.initialize_user()
+        context.cluster_node = 'root@node1'
+        context.user_at_node_list = None
+        context.initialize_user()
+        self.assertEqual('root', context.current_user)
 
     @mock.patch('crmsh.userdir.get_sudoer')
     @mock.patch('crmsh.userdir.getuser')
@@ -346,6 +350,10 @@ def test_initialize_user_cluster_node_with_user_with_sudoer(self, mock_getuser:
         context.user_at_node_list = None
         context.initialize_user()
         self.assertEqual('bob', context.current_user)
+        context.cluster_node = 'root@node1'
+        context.user_at_node_list = None
+        context.initialize_user()
+        self.assertEqual('root', context.current_user)
 
     @mock.patch('crmsh.userdir.get_sudoer')
     @mock.patch('crmsh.userdir.getuser')
@@ -368,6 +376,10 @@ def test_initialize_user_node_list_with_user_without_sudoer(self, mock_getuser:
         context.cluster_node = None
         with self.assertRaises(ValueError):
             context.initialize_user()
+        context.user_at_node_list = ['root@node1', 'root@node2']
+        context.cluster_node = None
+        context.initialize_user()
+        self.assertEqual('root', context.current_user)
 
     @mock.patch('crmsh.userdir.get_sudoer')
     @mock.patch('crmsh.userdir.getuser')
@@ -386,10 +398,18 @@ def test_initialize_user_node_list_with_user_with_sudoer(self, mock_getuser: moc
         mock_getuser.return_value = 'root'
         mock_get_sudoer.return_value = 'bob'
         context = bootstrap.Context()
+        context.user_at_node_list = ['alice@node1', 'root@node2']
+        context.cluster_node = None
+        with self.assertRaises(ValueError):
+            context.initialize_user()
         context.user_at_node_list = ['alice@node1', 'alice@node2']
         context.cluster_node = None
         context.initialize_user()
         self.assertEqual('bob', context.current_user)
+        context.user_at_node_list = ['root@node1', 'root@node2']
+        context.cluster_node = None
+        context.initialize_user()
+        self.assertEqual('root', context.current_user)
 
 
 class TestBootstrap(unittest.TestCase):

test/unittests/test_prun.py

@@ -40,14 +40,14 @@ def test_prun(
         ])
         mock_runner_add_task.assert_has_calls([
             mock.call(TaskArgumentsEq(
-                ['su', 'alice', '--login', '-c', 'ssh {} bob@host1 sudo -H /bin/sh'.format(crmsh.constants.SSH_OPTION)],
+                ['su', 'alice', '--login', '-c', 'ssh {} bob@host1 sudo -H /bin/bash'.format(crmsh.constants.SSH_OPTION)],
                 b'foo',
                 stdout=crmsh.prun.runner.Task.Capture,
                 stderr=crmsh.prun.runner.Task.Capture,
                 context={"host": 'host1', "ssh_user": 'bob'},
             )),
             mock.call(TaskArgumentsEq(
-                ['su', 'alice', '--login', '-c', 'ssh {} bob@host2 sudo -H /bin/sh'.format(crmsh.constants.SSH_OPTION)],
+                ['su', 'alice', '--login', '-c', 'ssh {} bob@host2 sudo -H /bin/bash'.format(crmsh.constants.SSH_OPTION)],
                 b'bar',
                 stdout=crmsh.prun.runner.Task.Capture,
                 stderr=crmsh.prun.runner.Task.Capture,
@@ -90,14 +90,14 @@ def test_prun_root(
         ])
         mock_runner_add_task.assert_has_calls([
             mock.call(TaskArgumentsEq(
-                ['/bin/sh', '-c', 'ssh {} root@host1 sudo -H /bin/sh'.format(crmsh.constants.SSH_OPTION)],
+                ['/bin/sh', '-c', 'ssh {} root@host1 /bin/bash'.format(crmsh.constants.SSH_OPTION)],
                 b'foo',
                 stdout=crmsh.prun.runner.Task.Capture,
                 stderr=crmsh.prun.runner.Task.Capture,
                 context={"host": 'host1', "ssh_user": 'root'},
             )),
             mock.call(TaskArgumentsEq(
-                ['/bin/sh', '-c', 'ssh {} root@host2 sudo -H /bin/sh'.format(crmsh.constants.SSH_OPTION)],
+                ['/bin/sh', '-c', 'ssh {} root@host2 /bin/bash'.format(crmsh.constants.SSH_OPTION)],
                 b'bar',
                 stdout=crmsh.prun.runner.Task.Capture,
                 stderr=crmsh.prun.runner.Task.Capture,
@@ -155,3 +155,6 @@ def __eq__(self, other):
             and self.stdout_config == other.stdout_config \
             and self.stderr_config == other.stderr_config \
             and self.context == other.context
+
+    def __repr__(self):
+        return f"TaskArgumentsEq({self.args}, {self.input}, {self.stdout_config}, {self.stderr_config}, {self.context}"