Use --non-interactive instead of -A (bugfix) (#2161)

* Use --non-interactive instead of -A

* Improve sudo_broker system exit output

* Update tests now using check_output

Minor: Black

* Fix new bug in untested function

Author: Hook25

checkbox-ng/plainbox/impl/secure/sudo_broker.py

@@ -1,8 +1,9 @@
 # This file is part of Checkbox.
 #
-# Copyright 2017-2020 Canonical Ltd.
+# Copyright 2017-2025 Canonical Ltd.
 # Written by:
 #   Maciej Kisielewski <maciej.kisielewski@canonical.com>
+#   Massimiliano Girardi <massimiliano.girardi@canonical.com>
 #
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
@@ -30,7 +31,14 @@
 import sys
 
 from plainbox.i18n import gettext as _
-from subprocess import check_call, CalledProcessError, DEVNULL, SubprocessError
+from subprocess import (
+    check_output,
+    check_call,
+    CalledProcessError,
+    STDOUT,
+    DEVNULL,
+    SubprocessError,
+)
 
 
 logger = logging.getLogger("sudo_broker")
@@ -40,27 +48,36 @@ def is_passwordless_sudo():
     """
     Check if system can run sudo without pass.
     """
+    # this command fails if passowrdless sudo is not configured because
+    # --reset-timestamp will trigger re-authentication
+    # --non-interactive will make the command fail if user interaction is due
+    # We no longer use `-A` (ASKPASS) because sudo-rs doesn't currently support
+    # it
+    check_passwordless_sudo_cmd = [
+        "sudo",
+        "--non-interactive",
+        "--reset-timestamp",
+        "true",
+    ]
     if os.geteuid() == 0:
         # even though we run as root, we still may need to use sudo to switch
         # to a normal user for jobs not requiring root, so let's see if sudo
         # actually works.
         try:
-            check_call(
-                ["sudo", "-A", "-k", "true"], stdout=DEVNULL, stderr=DEVNULL
+            check_output(
+                check_passwordless_sudo_cmd,
+                stderr=STDOUT,
+                universal_newlines=True,
             )
         except (SubprocessError, OSError) as exc:
-            logger.error(_("Unable to run sudo %s"), exc)
-            raise SystemExit(1)
+            try:
+                print(exc.output)
+            except AttributeError:
+                pass
+            raise SystemExit("Checkbox is unable to run sudo: {}".format(exc))
         return True
-    # running sudo with -A will try using ASKPASS envvar that should specify
-    # the program to use when asking for password
-    # If the system is configured to not ask for password, this will silently
-    # succeed. If the pass is required, it'll return 1 and not ask for pass,
-    # as the askpass program is not provided
     try:
-        check_call(
-            ["sudo", "-A", "-k", "true"], stdout=DEVNULL, stderr=DEVNULL
-        )
+        check_output(check_passwordless_sudo_cmd, stderr=STDOUT)
     except CalledProcessError:
         return False
     return True

checkbox-ng/plainbox/impl/secure/test_sudo_broker.py

@@ -17,32 +17,46 @@
 from subprocess import CalledProcessError
 from unittest import TestCase, mock
 
-from plainbox.impl.secure.sudo_broker import is_passwordless_sudo
+from plainbox.impl.secure.sudo_broker import (
+    is_passwordless_sudo,
+    validate_pass,
+)
 
 
 class IsPasswordlessSudoTests(TestCase):
     @mock.patch("os.geteuid", return_value=0)
-    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
-    def test_root_happy(self, mock_check_call, mock_getuid):
-        mock_check_call.return_value = 0
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_output")
+    def test_root_happy(self, mock_check_output, mock_getuid):
+        mock_check_output.return_value = 0
         self.assertTrue(is_passwordless_sudo())
 
     @mock.patch("os.geteuid", return_value=0)
-    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
-    def test_root_raising(self, mock_check_call, mock_getuid):
-        mock_check_call.side_effect = OSError
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_output")
+    def test_root_raising(self, mock_check_output, mock_getuid):
+        mock_check_output.side_effect = OSError
 
         with self.assertRaises(SystemExit):
             is_passwordless_sudo()
 
     @mock.patch("os.geteuid", return_value=1000)
-    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
-    def test_non_root_happy(self, mock_check_call, mock_getuid):
-        mock_check_call.return_value = 0
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_output")
+    def test_non_root_happy(self, mock_check_output, mock_getuid):
+        mock_check_output.return_value = 0
         self.assertTrue(is_passwordless_sudo())
 
     @mock.patch("os.geteuid", return_value=1000)
-    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
-    def test_non_root_raising(self, mock_check_call, mock_getuid):
-        mock_check_call.side_effect = CalledProcessError(1, "oops")
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_output")
+    def test_non_root_raising(self, mock_check_output, mock_getuid):
+        mock_check_output.side_effect = CalledProcessError(1, "oops")
         self.assertFalse(is_passwordless_sudo())
+
+
+class ValidatePassTest(TestCase):
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
+    def test_validate_pass_wrong(self, check_output_mock):
+        check_output_mock.side_effect = CalledProcessError(1, "oops")
+        self.assertFalse(validate_pass(b"password"))
+
+    @mock.patch("plainbox.impl.secure.sudo_broker.check_call")
+    def test_validate_pass_ok(self, check_output_mock):
+        self.assertTrue(validate_pass(b"password"))