mac-arm64: Add tests for 8e602f66

8e602f66 (https://chromium-review.googlesource.com/c/2420529) was
checked in expediently over the weekend for ${reasons}. This addresses
review feedback by refactoring the implementation into library code as
needed, and adding full proper tests.

Bug: 1130270
Test: chrome/installer/mac/signing/run_mac_signing_tests.py
Change-Id: I23a2a2d5ab43a05849f1211f021577cd4ee4b476
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422275
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809502}

chrome/installer/mac/signing/commands.py

@@ -6,6 +6,7 @@ The commands module wraps operations that have side-effects.
 """
 
 import os
+import platform
 import plistlib
 import shutil
 import stat
@@ -90,6 +91,36 @@ def run_command_output(args, **kwargs):
     return subprocess.check_output(args, **kwargs)
 
 
+def lenient_run_command_output(args, **kwargs):
+    """Runs a command, being fairly tolerant of errors.
+
+    Returns:
+        A tuple of (returncode, stdoutdata, stderrdata), or if an OSError was
+        raised, (None, None, None).
+    """
+    logger.info('Running command: %s', args)
+
+    try:
+        process = subprocess.Popen(
+            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs)
+    except OSError:
+        return (None, None, None)
+
+    (stdout, stderr) = process.communicate()
+
+    return (process.wait(), stdout, stderr)
+
+
+def macos_version():
+    """Determines the macOS version of the running system.
+
+    Returns:
+        A list containing one element for each component of the version number,
+        such as [10, 15, 6] and [11, 0].
+    """
+    return [int(x) for x in platform.mac_ver()[0].split('.')]
+
+
 class PlistContext(object):
     """
     PlistContext is a context manager that reads a plist on entry, providing

chrome/installer/mac/signing/commands_test.py

@@ -171,3 +171,38 @@ class TestCommands(unittest.TestCase):
     def test_run_command_output(self):
         output = commands.run_command_output(['echo', 'hello world'])
         self.assertEqual(b'hello world\n', output)
+
+    def test_lenient_run_command_output(self):
+        # Successful command, output on stdout.
+        (returncode, stdout,
+         stderr) = commands.lenient_run_command_output(['echo', 'hello'])
+        self.assertEqual(returncode, 0)
+        self.assertEqual(stdout, b'hello\n')
+        self.assertEqual(stderr, b'')
+
+        # Failure, error on stderr.
+        (returncode, stdout,
+         stderr) = commands.lenient_run_command_output(['cp'])
+        self.assertNotEqual(returncode, 0)
+        self.assertEqual(stdout, b'')
+        self.assertTrue(b'usage: ' in stderr or b'cp: ' in stderr)
+
+        # EACCES
+        (returncode, stdout,
+         stderr) = commands.lenient_run_command_output(['/etc/shells'])
+        self.assertIsNone(returncode)
+        self.assertIsNone(stdout)
+        self.assertIsNone(stderr)
+
+        # ENOENT
+        (returncode, stdout,
+         stderr) = commands.lenient_run_command_output(['/var/empty/enoent'])
+        self.assertIsNone(returncode)
+        self.assertIsNone(stdout)
+        self.assertIsNone(stderr)
+
+    def test_macos_version(self):
+        version = commands.macos_version()
+        self.assertGreaterEqual(len(version), 2)
+        self.assertGreaterEqual(version, [10, 10])
+        self.assertLess(version, [30])

chrome/installer/mac/signing/signing.py

@@ -7,9 +7,7 @@ bundle that need to be signed, as well as providing utilities to sign them.
 """
 
 import os.path
-import platform
 import re
-import subprocess
 
 from . import commands
 
@@ -28,30 +26,23 @@ def _linker_signed_arm64_needs_force(path):
     # On macOS 11.0 and later, codesign handles linker-signed code properly
     # without the --force hand-holding. Check OS >= 10.16 because that's what
     # Python will think the OS is if it wasn't built with the 11.0 SDK or later.
-    if [int(x) for x in platform.mac_ver()[0].split('.')] >= [10, 16]:
+    if commands.macos_version() >= [10, 16]:
         return False
 
-    try:
-        # Look just for --arch=arm64 because that's the only architecture that
-        # has linker-signed code by default. If this were used with universal
-        # code (if there were any), --display without --arch would default to
-        # the native architecture, which almost certainly wouldn't be arm64 and
-        # therefore would be wrong.
-        codesign = subprocess.Popen(
-            ['codesign', '--display', '--verbose', '--arch=arm64', '--', path],
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE)
-    except OSError:
+    # Look just for --arch=arm64 because that's the only architecture that has
+    # linker-signed code by default. If this were used with universal code (if
+    # there were any), --display without --arch would default to the native
+    # architecture, which almost certainly wouldn't be arm64 and therefore would
+    # be wrong.
+    (returncode, stdout, stderr) = commands.lenient_run_command_output(
+        ['codesign', '--display', '--verbose', '--arch=arm64', '--', path])
+
+    if returncode != 0:
         # Problem running codesign? Don't make the error about this confusing
         # function. Just return False and let some less obscure codesign
-        # invocation be the error.
-        return False
-
-    (stdout, stderr) = codesign.communicate()
-    if codesign.wait() != 0:
-        # Not signed at all? No problem. No arm64 code? No problem either. Not
-        # code at all? File not found? Well, those don't count as linker-signed
-        # either.
+        # invocation be the error. Not signed at all? No problem. No arm64 code?
+        # No problem either. Not code at all? File not found? Well, those don't
+        # count as linker-signed either.
         return False
 
     # Yes, codesign --display puts all of this on stderr.

chrome/installer/mac/signing/signing_test.py

@@ -15,6 +15,86 @@ except NameError:
     FileNotFoundError = IOError
 
 
+@mock.patch('signing.commands.lenient_run_command_output')
+@mock.patch('signing.commands.macos_version', return_value=[10, 15])
+class TestLinkerSignedArm64NeedsForce(unittest.TestCase):
+
+    def test_oserror(self, macos_version, lenient_run_command_output):
+        lenient_run_command_output.return_value = (None, None, None)
+        self.assertFalse(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_called_once()
+
+    def test_unsigned(self, macos_version, lenient_run_command_output):
+        lenient_run_command_output.return_value = (
+            1, b'', b'test: code object is not signed at all\n')
+        self.assertFalse(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_called_once()
+
+    def test_not_linker_signed(self, macos_version, lenient_run_command_output):
+        lenient_run_command_output.return_value = (0, b'', b'''Executable=test
+Identifier=test
+Format=Mach-O thin (arm64)
+CodeDirectory v=20100 size=592 flags=0x2(adhoc) hashes=13+2 location=embedded
+Signature=adhoc
+Info.plist=not bound
+TeamIdentifier=not set
+Sealed Resources=none
+Internal requirements count=0 size=12
+''')
+        self.assertFalse(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_called_once()
+
+    def test_linker_signed_10_15(self, macos_version,
+                                 lenient_run_command_output):
+        lenient_run_command_output.return_value = (0, b'', b'''Executable=test
+Identifier=test
+Format=Mach-O thin (arm64)
+CodeDirectory v=20400 size=512 flags=0x20002(adhoc,???) hashes=13+0 location=embedded
+Signature=adhoc
+Info.plist=not bound
+TeamIdentifier=not set
+Sealed Resources=none
+Internal requirements=none
+''')
+        self.assertTrue(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_called_once()
+
+    def test_linker_signed_10_16(self, macos_version,
+                                 lenient_run_command_output):
+        # 10.16 is what a Python built against an SDK < 11.0 will see 11.0 as.
+        macos_version.return_value = [10, 16]
+        lenient_run_command_output.return_value = (0, b'', b'''Executable=test
+Identifier=test
+Format=Mach-O thin (arm64)
+CodeDirectory v=20400 size=250 flags=0x20002(adhoc,linker-signed) hashes=5+0 location=embedded
+Signature=adhoc
+Info.plist=not bound
+TeamIdentifier=not set
+Sealed Resources=none
+Internal requirements=none
+''')
+        self.assertFalse(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_not_called()
+
+    def test_linker_signed_11_0(self, macos_version,
+                                lenient_run_command_output):
+        macos_version.return_value = [11, 0]
+        lenient_run_command_output.return_value = (0, b'', b'''Executable=test
+Identifier=test
+Format=Mach-O thin (arm64)
+CodeDirectory v=20400 size=250 flags=0x20002(adhoc,linker-signed) hashes=5+0 location=embedded
+Signature=adhoc
+Info.plist=not bound
+TeamIdentifier=not set
+Sealed Resources=none
+Internal requirements=none
+''')
+        self.assertFalse(signing._linker_signed_arm64_needs_force(None))
+        lenient_run_command_output.assert_not_called()
+
+
+@mock.patch(
+    'signing.signing._linker_signed_arm64_needs_force', return_value=False)
 @mock.patch('signing.commands.run_command')
 class TestSignPart(unittest.TestCase):
 
@@ -22,7 +102,7 @@ class TestSignPart(unittest.TestCase):
         self.paths = model.Paths('/$I', '/$O', '/$W')
         self.config = test_config.TestConfig()
 
-    def test_sign_part(self, run_command):
+    def test_sign_part(self, run_command, linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct('Test.app', 'test.signing.app')
         signing.sign_part(self.paths, self.config, part)
         run_command.assert_called_once_with([
@@ -30,7 +110,19 @@ class TestSignPart(unittest.TestCase):
             '=designated => identifier "test.signing.app"', '/$W/Test.app'
         ])
 
-    def test_sign_part_no_notary(self, run_command):
+    def test_sign_part_needs_force(self, run_command,
+                                   linker_signed_arm64_needs_force):
+        linker_signed_arm64_needs_force.return_value = True
+        part = model.CodeSignedProduct('Test.app', 'test.signing.app')
+        signing.sign_part(self.paths, self.config, part)
+        run_command.assert_called_once_with([
+            'codesign', '--sign', '[IDENTITY]', '--force', '--timestamp',
+            '--requirements', '=designated => identifier "test.signing.app"',
+            '/$W/Test.app'
+        ])
+
+    def test_sign_part_no_notary(self, run_command,
+                                 linker_signed_arm64_needs_force):
         config = test_config.TestConfig(notary_user=None, notary_password=None)
         part = model.CodeSignedProduct('Test.app', 'test.signing.app')
         signing.sign_part(self.paths, config, part)
@@ -39,14 +131,16 @@ class TestSignPart(unittest.TestCase):
             '=designated => identifier "test.signing.app"', '/$W/Test.app'
         ])
 
-    def test_sign_part_no_identifier_requirement(self, run_command):
+    def test_sign_part_no_identifier_requirement(
+            self, run_command, linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app', 'test.signing.app', identifier_requirement=False)
         signing.sign_part(self.paths, self.config, part)
         run_command.assert_called_once_with(
             ['codesign', '--sign', '[IDENTITY]', '--timestamp', '/$W/Test.app'])
 
-    def test_sign_with_identifier(self, run_command):
+    def test_sign_with_identifier(self, run_command,
+                                  linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app', 'test.signing.app', sign_with_identifier=True)
         signing.sign_part(self.paths, self.config, part)
@@ -56,7 +150,8 @@ class TestSignPart(unittest.TestCase):
             '=designated => identifier "test.signing.app"', '/$W/Test.app'
         ])
 
-    def test_sign_with_identifier_no_requirement(self, run_command):
+    def test_sign_with_identifier_no_requirement(
+            self, run_command, linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app',
             'test.signing.app',
@@ -68,7 +163,8 @@ class TestSignPart(unittest.TestCase):
             'test.signing.app', '/$W/Test.app'
         ])
 
-    def test_sign_part_with_options(self, run_command):
+    def test_sign_part_with_options(self, run_command,
+                                    linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app',
             'test.signing.app',
@@ -81,7 +177,8 @@ class TestSignPart(unittest.TestCase):
             'restrict,library', '/$W/Test.app'
         ])
 
-    def test_sign_part_with_entitlements(self, run_command):
+    def test_sign_part_with_entitlements(self, run_command,
+                                         linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app',
             'test.signing.app',
@@ -93,7 +190,7 @@ class TestSignPart(unittest.TestCase):
             '/$W/entitlements.plist', '/$W/Test.app'
         ])
 
-    def test_verify_part(self, run_command):
+    def test_verify_part(self, run_command, linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct('Test.app', 'test.signing.app')
         signing.verify_part(self.paths, part)
         self.assertEqual(run_command.mock_calls, [
@@ -104,7 +201,8 @@ class TestSignPart(unittest.TestCase):
             mock.call(['codesign', '--verify', '--verbose=6', '/$W/Test.app']),
         ])
 
-    def test_verify_part_with_options(self, run_command):
+    def test_verify_part_with_options(self, run_command,
+                                      linker_signed_arm64_needs_force):
         part = model.CodeSignedProduct(
             'Test.app',
             'test.signing.app',