Mac: Sign the inner framework only once per bundle ID

For a given bundle ID, the inner framework is bit-for-bit identical, but
each attempt to sign it will result in a different code signature due to
at least the signature timestamp varying.

Binary diff updates rely on the framework being bit-for-bit identical,
including the code signature, for a given version and product ID. Builds
are produced that vary solely in parameters in the outer app bundle, not
the inner framework, such as the channel and brand code.

In particular, failing to have bit-for-bit identical frameworks in cases
where two copies should have varied only in brand code has resulted in
diff updaters that fail to apply to new installs the first time an
update is attempted.

By saving the first framework signed for a specific bundle ID and
recycling it for any subsequent attempts to sign the framework for that
same bundle ID, these new code signatures that only vary in their
timestamp can be avoided. This meets the bit-for-bit identical guarantee
for the framework.

Bug: 976827
Change-Id: I5326a05376c2a2e0aaa86c0e28c36409539b5352
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1687792Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676644}

chrome/installer/mac/signing/commands.py

@@ -18,7 +18,34 @@ def file_exists(path):
 
 def copy_files(source, dest):
     assert source[-1] != '/'
-    subprocess.check_call(['rsync', '-a', '--delete', source, dest])
+    subprocess.check_call(
+        ['rsync', '--archive', '--checksum', '--delete', source, dest])
+
+
+def copy_dir_overwrite_and_count_changes(source, dest, dry_run=False):
+    assert source[-1] != '/'
+    command = [
+        'rsync', '--archive', '--checksum', '--itemize-changes', '--delete',
+        source + '/', dest
+    ]
+    if dry_run:
+        command.append('--dry-run')
+    output = subprocess.check_output(command)
+
+    # --itemize-changes will print a '.' in the first column if the item is not
+    # being updated, created, or deleted. This happens if only attributes
+    # change, such as a timestamp or permissions. Timestamp changes are
+    # uninteresting for the purposes of determining changed content, but
+    # permissions changes are not. Columns 6-8 are also checked so that files
+    # that have potentially interesting attributes (permissions, owner, or
+    # group) changing are counted, but column 5 for the timestamp is not
+    # considered.
+    changes = 0
+    for line in output.split('\n'):
+        if line == '' or (line[0] == '.' and line[5:8] == '...'):
+            continue
+        changes += 1
+    return changes
 
 
 def move_file(source, dest):

chrome/installer/mac/signing/commands_test.py

@@ -29,6 +29,110 @@ class TestCommands(unittest.TestCase):
         os.unlink(path)
         self.assertFalse(commands.file_exists(path))
 
+    def test_copy_dir_overwrite_and_count_changes(self):
+        source_dir = os.path.join(self.tempdir, 'source')
+        os.mkdir(source_dir)
+
+        os.mkdir(os.path.join(source_dir, 'dir'))
+        open(os.path.join(source_dir, 'dir', 'file'), 'w').close()
+        with open(os.path.join(source_dir, 'file'), 'w') as file:
+            file.write('contents')
+
+        dest_dir = os.path.join(self.tempdir, 'dest')
+
+        # Make sure that dry_run doesn't actually change anything by testing it
+        # a couple of times before doing any real work.
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=True), 4)
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=True), 4)
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 4)
+
+        # Now test that a subsequent copy of the same thing doesn't report any
+        # changes.
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=True), 0)
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 0)
+
+        self.assertTrue(os.path.isdir(dest_dir))
+        self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'file')))
+        self.assertTrue(os.path.isdir(os.path.join(dest_dir, 'dir')))
+        self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'dir', 'file')))
+
+        self.assertEqual(
+            os.path.getsize(os.path.join(dest_dir, 'dir', 'file')), 0)
+        with open(os.path.join(dest_dir, 'file')) as file:
+            self.assertEqual(file.read(), 'contents')
+
+        # No changes to source should result in no changes reported.
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 0)
+
+        # Changing a timestamp isn't reported, but the timestamp does get
+        # updated.
+        os.utime(os.path.join(source_dir, 'dir', 'file'), (0, 0))
+
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 0)
+
+        self.assertEqual(
+            os.path.getmtime(os.path.join(dest_dir, 'dir', 'file')), 0)
+
+        # Changing a file is reported.
+        with open(os.path.join(source_dir, 'file'), 'w') as file:
+            file.write('new contents')
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 1)
+
+        with open(os.path.join(dest_dir, 'file')) as file:
+            self.assertEqual(file.read(), 'new contents')
+
+        # Changing a file whose length doesn't change is reported.
+        with open(os.path.join(source_dir, 'file'), 'w') as file:
+            file.write('new_contents')
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 1)
+
+        with open(os.path.join(dest_dir, 'file')) as file:
+            self.assertEqual(file.read(), 'new_contents')
+
+        # Creating directories and files are reported.
+        os.mkdir(os.path.join(source_dir, 'new_dir'))
+        open(os.path.join(source_dir, 'new_file'), 'w').close()
+
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 2)
+
+        self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'new_file')))
+        self.assertTrue(os.path.isdir(os.path.join(dest_dir, 'new_dir')))
+
+        # Removing files and directories is also reported.
+        os.rmdir(os.path.join(source_dir, 'new_dir'))
+        os.unlink(os.path.join(source_dir, 'new_file'))
+        os.mkdir(os.path.join(source_dir, 'newer_dir'))
+        open(os.path.join(source_dir, 'newer_file'), 'w').close()
+
+        self.assertEqual(
+            commands.copy_dir_overwrite_and_count_changes(
+                source_dir, dest_dir, dry_run=False), 4)
+
+        self.assertFalse(os.path.exists(os.path.join(dest_dir, 'new_file')))
+        self.assertFalse(os.path.exists(os.path.join(dest_dir, 'new_dir')))
+        self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'newer_file')))
+        self.assertTrue(os.path.isdir(os.path.join(dest_dir, 'newer_dir')))
+
     def test_move_file(self):
         orig_path = os.path.join(self.tempdir, 'file.txt')
         new_path = os.path.join(self.tempdir, 'renamed.txt')

chrome/installer/mac/signing/modification_test.py

@@ -51,6 +51,34 @@ class TestModification(unittest.TestCase):
         self.paths = model.Paths('$I', '$O', '$W')
         self.config = test_config.TestConfig()
 
+    def _is_framework_unchanged(self, plistlib, mocks):
+        # Determines whether any modifications were made within the framework
+        # according to calls to plistlib.writePlist or any of the mocked calls
+        # in |mocks|. This is done by examining the calls' arguments for a
+        # substring pointing into the framework.
+
+        def _do_mock_calls_mention_framework(mock_calls):
+            for call in mock_calls:
+                for tup in call:
+                    for arg in tup:
+                        # Don't anchor this substring in a particular directory
+                        # because it may appear in any of $I, $O, or $W. Don't
+                        # anchor it with App Product.app either, because it may
+                        # be renamed (to App Product Canary.app).
+                        if 'Contents/Frameworks/Product Framework.framework' in arg:
+                            return True
+
+            return False
+
+        if _do_mock_calls_mention_framework(plistlib.writePlist.mock_calls):
+            return False
+
+        for mocked in mocks.values():
+            if _do_mock_calls_mention_framework(mocked):
+                return False
+
+        return True
+
     def test_base_distribution(self, plistlib, **kwargs):
         dist = model.Distribution()
         config = dist.to_config(self.config)
@@ -78,6 +106,8 @@ class TestModification(unittest.TestCase):
         self.assertEqual(0, kwargs['move_file'].call_count)
         self.assertEqual(0, kwargs['write_file'].call_count)
 
+        self.assertTrue(self._is_framework_unchanged(plistlib, kwargs))
+
     def test_distribution_with_brand(self, plistlib, **kwargs):
         dist = model.Distribution(branding_code='MOO')
         config = dist.to_config(self.config)
@@ -105,6 +135,8 @@ class TestModification(unittest.TestCase):
         ])
         self.assertEqual(0, kwargs['move_file'].call_count)
 
+        self.assertTrue(self._is_framework_unchanged(plistlib, kwargs))
+
     def test_distribution_with_channel(self, plistlib, **kwargs):
         dist = model.Distribution(channel='dev')
         config = dist.to_config(self.config)
@@ -134,6 +166,8 @@ class TestModification(unittest.TestCase):
         self.assertEqual(0, kwargs['move_file'].call_count)
         self.assertEqual(0, kwargs['write_file'].call_count)
 
+        self.assertTrue(self._is_framework_unchanged(plistlib, kwargs))
+
     def test_distribution_with_product_dirname(self, plistlib, **kwargs):
         dist = model.Distribution(product_dirname='Farmland/Cows')
         config = dist.to_config(self.config)
@@ -162,6 +196,8 @@ class TestModification(unittest.TestCase):
         self.assertEqual(0, kwargs['move_file'].call_count)
         self.assertEqual(0, kwargs['write_file'].call_count)
 
+        self.assertTrue(self._is_framework_unchanged(plistlib, kwargs))
+
     def test_distribution_with_creator_code(self, plistlib, **kwargs):
         dist = model.Distribution(creator_code='Mooo')
         config = dist.to_config(self.config)
@@ -292,3 +328,5 @@ class TestModification(unittest.TestCase):
             }, '$W/App Product Canary.app/Contents/Resources/test.signing.bundle_id.canary.manifest/Contents/Resources/test.signing.bundle_id.canary.manifest'
                      )
         ])
+
+        self.assertFalse(self._is_framework_unchanged(plistlib, kwargs))

chrome/installer/mac/signing/pipeline.py

@@ -14,7 +14,7 @@ import os.path
 from . import commands, model, modification, notarize, signing
 
 
-def _customize_and_sign_chrome(paths, dist_config, dest_dir):
+def _customize_and_sign_chrome(paths, dist_config, dest_dir, signed_frameworks):
     """Does channel customization and signing of a Chrome distribution. The
     resulting app bundle is moved into |dest_dir|.
 
@@ -23,6 +23,14 @@ def _customize_and_sign_chrome(paths, dist_config, dest_dir):
         dist_config: A |config.CodeSignConfig| for the |model.Distribution|.
         dest_dir: The directory into which the product will be placed when
             the operations are completed.
+        signed_frameworks: A dict that will store paths and change counts of
+            already-signed inner frameworks keyed by bundle ID. Paths are used
+            to recycle already-signed frameworks instead of re-signing them.
+            Change counts are used to verify equivalence of frameworks when
+            recycling them. Callers can pass an empty dict on the first call,
+            and reuse the same dict for subsequent calls. This function will
+            produce and consume entries in the dict. If this sharing is
+            undesired, pass None instead of a dict.
     """
     # Copy the app to sign into the work dir.
     commands.copy_files(
@@ -32,7 +40,53 @@ def _customize_and_sign_chrome(paths, dist_config, dest_dir):
     modification.customize_distribution(paths, dist_config.distribution,
                                         dist_config)
 
-    signing.sign_chrome(paths, dist_config)
+    work_dir_framework_path = os.path.join(paths.work,
+                                           dist_config.framework_dir)
+    if signed_frameworks is not None and dist_config.base_bundle_id in signed_frameworks:
+        # If the inner framework has already been modified and signed for this
+        # bundle ID, recycle the existing signed copy without signing a new
+        # copy. This ensures that bit-for-bit identical input will result in
+        # bit-for-bit identical signatures not affected by differences in, for
+        # example, the signature's timestamp. All variants of a product sharing
+        # the same bundle ID are assumed to have bit-for-bit identical
+        # frameworks.
+        #
+        # This is significant because of how binary diff updates work. Binary
+        # diffs are built between two successive versions on the basis of their
+        # inner frameworks being bit-for-bit identical without regard to any
+        # customizations applied only to the outer app. In order for these to
+        # apply to all installations regardless of the presence or specific
+        # values of any app-level customizations, all inner frameworks for a
+        # single version and base bundle ID must always remain bit-for-bit
+        # identical, including their signatures.
+        (signed_framework_path, signed_framework_change_count
+        ) = signed_frameworks[dist_config.base_bundle_id]
+        actual_framework_change_count = commands.copy_dir_overwrite_and_count_changes(
+            os.path.join(dest_dir, signed_framework_path),
+            work_dir_framework_path,
+            dry_run=False)
+
+        if actual_framework_change_count != signed_framework_change_count:
+            raise ValueError(
+                'While customizing and signing {} ({}), actual_framework_change_count {} != signed_framework_change_count {}'
+                .format(dist_config.base_bundle_id, dist_config.dmg_basename,
+                        actual_framework_change_count,
+                        signed_framework_change_count))
+
+        signing.sign_chrome(paths, dist_config, sign_framework=False)
+    else:
+        unsigned_framework_path = os.path.join(paths.work,
+                                               'modified_unsigned_framework')
+        commands.copy_dir_overwrite_and_count_changes(
+            work_dir_framework_path, unsigned_framework_path, dry_run=False)
+        signing.sign_chrome(paths, dist_config, sign_framework=True)
+        actual_framework_change_count = commands.copy_dir_overwrite_and_count_changes(
+            work_dir_framework_path, unsigned_framework_path, dry_run=True)
+        if signed_frameworks is not None:
+            dest_dir_framework_path = os.path.join(dest_dir,
+                                                   dist_config.framework_dir)
+            signed_frameworks[dist_config.base_bundle_id] = (
+                dest_dir_framework_path, actual_framework_change_count)
 
     app_path = os.path.join(paths.work, dist_config.app_dir)
     commands.make_dir(dest_dir)
@@ -212,6 +266,7 @@ def sign_all(orig_paths, config, package_dmg=True, do_notarization=True):
         # First, sign all the distributions and optionally submit the
         # notarization requests.
         uuids_to_config = {}
+        signed_frameworks = {}
         for dist in config.distributions:
             with commands.WorkDirectory(orig_paths) as paths:
                 dist_config = dist.to_config(config)
@@ -224,7 +279,8 @@ def sign_all(orig_paths, config, package_dmg=True, do_notarization=True):
                     dest_dir = notary_paths.work
 
                 dest_dir = os.path.join(dest_dir, dist_config.dmg_basename)
-                _customize_and_sign_chrome(paths, dist_config, dest_dir)
+                _customize_and_sign_chrome(paths, dist_config, dest_dir,
+                                           signed_frameworks)
 
                 # If the build products are to be notarized, ZIP the app bundle
                 # and submit it for notarization.

chrome/installer/mac/signing/signing.py

@@ -85,8 +85,7 @@ def get_parts(config):
                 '{}.helper.renderer'.format(uncustomized_bundle_id),
                 # Do not use |full_hardened_runtime_options| because library
                 # validation is incompatible with the JIT entitlement.
-                options=CodeSignOptions.RESTRICT +
-                CodeSignOptions.KILL +
+                options=CodeSignOptions.RESTRICT + CodeSignOptions.KILL +
                 CodeSignOptions.HARDENED_RUNTIME,
                 entitlements='helper-renderer-entitlements.plist',
                 verify_options=VerifyOptions.DEEP),
@@ -98,8 +97,7 @@ def get_parts(config):
                 # Do not use |full_hardened_runtime_options| because library
                 # validation is incompatible with the disable-library-validation
                 # entitlement.
-                options=CodeSignOptions.RESTRICT +
-                CodeSignOptions.KILL +
+                options=CodeSignOptions.RESTRICT + CodeSignOptions.KILL +
                 CodeSignOptions.HARDENED_RUNTIME,
                 entitlements='helper-plugin-entitlements.plist',
                 verify_options=VerifyOptions.DEEP),
@@ -226,7 +224,7 @@ def _validate_chrome(paths, config, app):
         commands.run_command(['spctl', '--assess', '-vv', app_path])
 
 
-def sign_chrome(paths, config):
+def sign_chrome(paths, config, sign_framework=False):
     """Code signs the Chrome application bundle and all of its internal nested
     code parts.
 
@@ -234,6 +232,9 @@ def sign_chrome(paths, config):
         paths: A |model.Paths| object.
         config: The |model.CodeSignConfig| object. The |app_product| binary and
             nested binaries must exist in |paths.work|.
+        sign_framework: True if the inner framework is to be signed in addition
+            to the outer application. False if only the outer application is to
+            be signed.
     """
     parts = get_parts(config)
 
@@ -247,18 +248,20 @@ def sign_chrome(paths, config):
 
     _sanity_check_version_keys(paths, parts)
 
-    # To sign an .app bundle that contains nested code, the nested components
-    # themselves must be signed. Each of these components is signed below. Note
-    # that unless a framework has multiple versions (which is discouraged),
-    # signing the entire framework is equivalent to signing the Current version.
-    # https://developer.apple.com/library/content/technotes/tn2206/_index.html#//apple_ref/doc/uid/DTS40007919-CH1-TNTAG13
-    for name, part in parts.items():
-        if name in ('app', 'framework'):
-            continue
-        sign_part(paths, config, part)
-
-    # Sign the framework bundle.
-    sign_part(paths, config, parts['framework'])
+    if sign_framework:
+        # To sign an .app bundle that contains nested code, the nested
+        # components themselves must be signed. Each of these components is
+        # signed below. Note that unless a framework has multiple versions
+        # (which is discouraged), signing the entire framework is equivalent to
+        # signing the Current version.
+        # https://developer.apple.com/library/content/technotes/tn2206/_index.html#//apple_ref/doc/uid/DTS40007919-CH1-TNTAG13
+        for name, part in parts.items():
+            if name in ('app', 'framework'):
+                continue
+            sign_part(paths, config, part)
+
+        # Sign the framework bundle.
+        sign_part(paths, config, parts['framework'])
 
     provisioning_profile_basename = config.provisioning_profile_basename
     if provisioning_profile_basename:

chrome/installer/mac/signing/signing_test.py

@@ -57,7 +57,6 @@ class TestGetParts(unittest.TestCase):
         self.assertEqual('test.signing.bundle_id.helper',
                          all_parts['helper-app'].identifier)
 
-
     def test_part_options(self):
         parts = signing.get_parts(test_config.TestConfig())
         self.assertEqual(
@@ -73,13 +72,11 @@ class TestGetParts(unittest.TestCase):
                 model.CodeSignOptions.HARDENED_RUNTIME),
             set(parts['helper-app'].options))
         self.assertEqual(
-            set(model.CodeSignOptions.RESTRICT +
-                model.CodeSignOptions.KILL +
+            set(model.CodeSignOptions.RESTRICT + model.CodeSignOptions.KILL +
                 model.CodeSignOptions.HARDENED_RUNTIME),
             set(parts['helper-renderer-app'].options))
         self.assertEqual(
-            set(model.CodeSignOptions.RESTRICT +
-                model.CodeSignOptions.KILL +
+            set(model.CodeSignOptions.RESTRICT + model.CodeSignOptions.KILL +
                 model.CodeSignOptions.HARDENED_RUNTIME),
             set(parts['helper-plugin-app'].options))
         self.assertEqual(
@@ -266,7 +263,7 @@ class TestSignChrome(unittest.TestCase):
         dist = model.Distribution()
         config = dist.to_config(test_config.TestConfig())
 
-        signing.sign_chrome(self.paths, config)
+        signing.sign_chrome(self.paths, config, sign_framework=True)
 
         # No files should be moved.
         self.assertEqual(0, kwargs['move_file'].call_count)
@@ -314,7 +311,7 @@ class TestSignChrome(unittest.TestCase):
 
         config = dist.to_config(Config())
 
-        signing.sign_chrome(self.paths, config)
+        signing.sign_chrome(self.paths, config, sign_framework=True)
 
         self.assertEqual(kwargs['run_command'].mock_calls, [
             mock.call.run_command([
@@ -334,7 +331,7 @@ class TestSignChrome(unittest.TestCase):
                 return None
 
         config = dist.to_config(Config())
-        signing.sign_chrome(self.paths, config)
+        signing.sign_chrome(self.paths, config, sign_framework=True)
 
         self.assertEqual(0, kwargs['copy_files'].call_count)
 
@@ -355,8 +352,9 @@ class TestSignChrome(unittest.TestCase):
             # If the file is missing, signing should fail since TestConfig has
             # no optional parts.
             config = model.Distribution().to_config(test_config.TestConfig())
-            self.assertRaises(FileNotFoundError,
-                              lambda: signing.sign_chrome(self.paths, config))
+            self.assertRaises(
+                FileNotFoundError,
+                lambda: signing.sign_chrome(self.paths, config, sign_framework=True))
 
             class Config(test_config.TestConfig):
 
@@ -366,14 +364,50 @@ class TestSignChrome(unittest.TestCase):
 
             # With the part marked as optional, it should succeed.
             config = model.Distribution().to_config(Config())
-            signing.sign_chrome(self.paths, config)
+            signing.sign_chrome(self.paths, config, sign_framework=True)
+
+    @mock.patch('signing.signing._sanity_check_version_keys')
+    def test_sign_chrome_no_framework(self, *args, **kwargs):
+        manager = mock.Mock()
+        for kwarg in kwargs:
+            manager.attach_mock(kwargs[kwarg], kwarg)
+
+        dist = model.Distribution()
+        config = dist.to_config(test_config.TestConfig())
+
+        signing.sign_chrome(self.paths, config, sign_framework=False)
+
+        # No files should be moved.
+        self.assertEqual(0, kwargs['move_file'].call_count)
+
+        # Test that the provisioning profile is copied.
+        self.assertEqual(kwargs['copy_files'].mock_calls, [
+            mock.call.copy_files(
+                '$I/Product Packaging/provisiontest.provisionprofile',
+                '$W/App Product.app/Contents/embedded.provisionprofile')
+        ])
+
+        # Ensure that only the app is signed.
+        signed_paths = [
+            call[1][2].path for call in kwargs['sign_part'].mock_calls
+        ]
+        self.assertEqual(signed_paths, ['App Product.app'])
+
+        self.assertEqual(kwargs['run_command'].mock_calls, [
+            mock.call.run_command([
+                'codesign', '--display', '--requirements', '-', '--verbose=5',
+                '$W/App Product.app'
+            ]),
+            mock.call.run_command(
+                ['spctl', '--assess', '-vv', '$W/App Product.app']),
+        ])
 
     @mock.patch(
         'signing.commands.plistlib.readPlist',
         side_effect=_get_plist_read('99.0.9999.99'))
     def test_sanity_check_ok(self, read_plist, **kwargs):
         config = model.Distribution().to_config(test_config.TestConfig())
-        signing.sign_chrome(self.paths, config)
+        signing.sign_chrome(self.paths, config, sign_framework=True)
 
     @mock.patch(
         'signing.commands.plistlib.readPlist',
@@ -381,4 +415,4 @@ class TestSignChrome(unittest.TestCase):
     def test_sanity_check_bad(self, read_plist, **kwargs):
         config = model.Distribution().to_config(test_config.TestConfig())
         self.assertRaises(ValueError,
-                          lambda: signing.sign_chrome(self.paths, config))
+                          lambda: signing.sign_chrome(self.paths, config, sign_framework=True))