feat: Mac hardened signing on signingscript by hneiva · Pull Request #872 · mozilla-releng/scriptworker-scripts

hneiva · 2023-11-16T18:20:23Z

No description provided.

bhearsum

(I'm going to try out Coventional Comments for this review.)

I'm marking this as request changes for the few blocking items, questions, and because I'd like a link to one or more test runs of this.

Overall it looks very good though, nice work!

bhearsum · 2023-12-08T14:14:16Z

signingscript/Dockerfile

 # Install signingscript + configloader + widevine
 RUN python -m venv /app \
 && cd signingscript \
+ && /app/bin/pip install /app/scriptworker_client \


question: why are we installing scriptworker_client twice? (and its related requirements). If this is not necessary, please remove it from the above block.

I meant to remove it from the block above since it has no effect whatsoever (root user)

bhearsum · 2023-12-08T14:20:20Z

signingscript/docker.d/apple_signing_creds.yml

+    $match:
+      'ENV == "prod" && scope_prefix':
+        '${scope_prefix[0]}cert:release-signing':
+          - "app_credentials": {"$eval": "APPLE_APP_SIGNING_CREDENTIALS"}


question: Are these usernames, a username+password, or something else? If they're usernames or other singular values, app_username or similar would be better here.

(After reading further down, it looks like these might be base64 encoded pkcs12 bundles? If that's the case, I suggest names like app_pkcs12_bundle, and pkcs12_passphrase)

Yeah that sounds better.

bhearsum · 2023-12-08T14:21:08Z

signingscript/docker.d/apple_signing_creds.yml

+        '${scope_prefix[0]}cert:release-signing':
+          - "app_credentials": {"$eval": "APPLE_APP_SIGNING_CREDENTIALS"}
+            "installer_credentials": {"$eval": "APPLE_INSTALLER_SIGNING_CREDENTIALS"}
+            "password": {"$eval": "APPLE_SIGNING_CREDS_PASSWORD"}


suggestion: Similarly, what is this password associated with? (Can it named in a way that makes this clear?)

bhearsum · 2023-12-08T14:32:45Z

signingscript/docker.d/build_msix_packaging.sh

+cp msix-packaging/.vs/bin/makemsix /usr/bin
+cp msix-packaging/.vs/lib/libmsix.so /usr/lib
+
+rm -rf msix-packaging


praise: thank you for cleaning up the left I mess here :)

bhearsum · 2023-12-08T14:33:37Z

signingscript/docker.d/build_rcodesign.sh

@@ -0,0 +1,16 @@
+#!/bin/bash


todo: rename this script to install_rcodesign.sh (we are not building it, so let's be clear about that in the name)

bhearsum · 2023-12-08T16:26:36Z

signingscript/src/signingscript/sign.py

+
+    signed = False
+    for file in os.scandir(signing_dir):
+        if file.is_dir() and file.name.endswith(".app"):


question: How confident are you about which creds to use based on filenames, or how often might these change? I'm wondering if we should have the payload be selecting which cert to use instead of hardcoding it in the code.

Either way, I think ipa files should be handled now, or soon?

If we are not naming the bundles .app or not naming installers .pkg, I think we have bigger problems.

So far I haven't found a way of signing ipa bundles with rcodesign 😢

bhearsum · 2023-12-08T16:27:16Z

signingscript/src/signingscript/sign.py

+            # Use installer credentials
+            creds = context.apple_installer_signing_creds_path
+        else:
+            # If not pkg AND not a directory (.app) - then skip file


todo: log any files that are skipped to make debugging easier.

bhearsum · 2023-12-08T16:28:12Z

signingscript/src/signingscript/sign.py

+        if provisioning_profile_config:
+            copy_provisioning_profiles(bundle_path, provisioning_profile_config)
+
+        # TODO: widevine and omnija signing should run from formats?


todo: address this todo :). (I assume this means we should only be doing this if widevine & omnija signing are explicitly requested in the payload? But please correct me if that's wrong...)

I think this should stay a TODO, and we find a better way of signing them in the future without failing due to the DMG symlink. Currently they are always signed together, so it's safe to assume here in the meantime.

OK, let's file an issue or bug and point at that for the follow-up, in that case.

Did this get filed somewhere?

bhearsum · 2023-12-08T16:28:31Z

signingscript/src/signingscript/utils.py

    private_key: str


+@dataclass


praise: Nice usage of dataclasses in this patch!

bhearsum · 2023-12-08T16:29:28Z

signingscript/src/signingscript/utils.py

+    hfsplus = context.config["hfsplus"]
+    undmg_cmd = [dmg, "extract", source, "tmp.hfs"]
+    log.info(undmg_cmd)
+    await execute_subprocess(undmg_cmd, cwd=context.config["work_dir"], log_level=logging.DEBUG)


question: Did we ever decide what to do about the symlinks? If we're not handling them here, have you tested to make sure the outputs from repackage-signing still contain working symlinks?

I was testing this in adhoc signer, which doesn't have repackaging setup.
I guess we could try this on Try?

That sounds like a good next step!

Did this testing happen?

hneiva

Updating PR changes on ~~next commit~~ the commit above....?

hneiva · 2023-12-08T21:24:04Z

signingscript/Dockerfile

 # Install signingscript + configloader + widevine
 RUN python -m venv /app \
 && cd signingscript \
+ && /app/bin/pip install /app/scriptworker_client \


I meant to remove it from the block above since it has no effect whatsoever (root user)

hneiva · 2023-12-08T21:45:37Z

signingscript/docker.d/apple_signing_creds.yml

+    $match:
+      'ENV == "prod" && scope_prefix':
+        '${scope_prefix[0]}cert:release-signing':
+          - "app_credentials": {"$eval": "APPLE_APP_SIGNING_CREDENTIALS"}


Yeah that sounds better.

hneiva · 2023-12-08T21:46:03Z

signingscript/docker.d/apple_signing_creds.yml

+        '${scope_prefix[0]}cert:release-signing':
+          - "app_credentials": {"$eval": "APPLE_APP_SIGNING_CREDENTIALS"}
+            "installer_credentials": {"$eval": "APPLE_INSTALLER_SIGNING_CREDENTIALS"}
+            "password": {"$eval": "APPLE_SIGNING_CREDS_PASSWORD"}


hneiva · 2023-12-08T21:46:34Z

signingscript/docker.d/build_rcodesign.sh

@@ -0,0 +1,16 @@
+#!/bin/bash


hneiva · 2023-12-08T21:55:36Z

signingscript/src/signingscript/apple.py

+PROVISIONING_PROFILE_FILENAMES = {
+    "firefox": "orgmozillafirefox.provisionprofile",
+    "devedition": "orgmozillafirefoxdeveloperedition.provisionprofile",
+    "nightly": "orgmozillanightly.provisionprofile",


Added the missing files

hneiva · 2023-12-08T21:57:03Z

signingscript/src/signingscript/apple.py

+        # Resolve absolute destination path
+        target_abs_path = os.path.join(bundlepath, target_path if target_path[0] != "/" else target_path[1:])
+        if os.path.exists(target_abs_path):
+            log.warning("Provisioning profile at {target_path} already exists, overriding.")


Under normal scenarios it shouldn't happen. But when testing things, this makes sure the profile is always "what it should be".

hneiva · 2023-12-08T22:01:37Z

signingscript/src/signingscript/sign.py

+
+    signed = False
+    for file in os.scandir(signing_dir):
+        if file.is_dir() and file.name.endswith(".app"):


If we are not naming the bundles .app or not naming installers .pkg, I think we have bigger problems.

So far I haven't found a way of signing ipa bundles with rcodesign 😢

hneiva · 2023-12-08T22:02:50Z

signingscript/src/signingscript/sign.py

+            # Use installer credentials
+            creds = context.apple_installer_signing_creds_path
+        else:
+            # If not pkg AND not a directory (.app) - then skip file


hneiva · 2023-12-08T22:05:14Z

signingscript/src/signingscript/sign.py

+        if provisioning_profile_config:
+            copy_provisioning_profiles(bundle_path, provisioning_profile_config)
+
+        # TODO: widevine and omnija signing should run from formats?


I think this should stay a TODO, and we find a better way of signing them in the future without failing due to the DMG symlink. Currently they are always signed together, so it's safe to assume here in the meantime.

hneiva · 2023-12-08T22:08:41Z

signingscript/src/signingscript/utils.py

+    hfsplus = context.config["hfsplus"]
+    undmg_cmd = [dmg, "extract", source, "tmp.hfs"]
+    log.info(undmg_cmd)
+    await execute_subprocess(undmg_cmd, cwd=context.config["work_dir"], log_level=logging.DEBUG)


I was testing this in adhoc signer, which doesn't have repackaging setup.
I guess we could try this on Try?

bhearsum

Approving, but please make sure that the symlink situation is verified before this is used in prod. (You can deploy this is to prod safely, I think, this it's a new format, but it shouldn't be used outside of try or a project branch until we know the symlinks are fine.)

Nice work!

bhearsum · 2024-01-09T14:26:48Z

signingscript/src/signingscript/sign.py

+        if provisioning_profile_config:
+            copy_provisioning_profiles(bundle_path, provisioning_profile_config)
+
+        # TODO: widevine and omnija signing should run from formats?


Did this get filed somewhere?

bhearsum · 2024-01-09T14:27:15Z

signingscript/src/signingscript/utils.py

+    hfsplus = context.config["hfsplus"]
+    undmg_cmd = [dmg, "extract", source, "tmp.hfs"]
+    log.info(undmg_cmd)
+    await execute_subprocess(undmg_cmd, cwd=context.config["work_dir"], log_level=logging.DEBUG)


Did this testing happen?

hneiva force-pushed the hneiva/rcodesign branch 6 times, most recently from 80d3d53 to f676f83 Compare November 17, 2023 19:38

hneiva force-pushed the hneiva/rcodesign branch from f676f83 to 2f64c3e Compare November 27, 2023 20:27

hneiva force-pushed the hneiva/rcodesign branch 2 times, most recently from a1e4a4d to fb47722 Compare December 7, 2023 22:12

hneiva marked this pull request as ready for review December 7, 2023 22:49

hneiva requested a review from a team December 7, 2023 22:50

bhearsum requested changes Dec 8, 2023

View reviewed changes

hneiva commented Dec 8, 2023

View reviewed changes

feat: Mac hardened signing on signingscript

739dcdb

hneiva force-pushed the hneiva/rcodesign branch from b52c549 to aac1754 Compare January 3, 2024 16:43

PR changes

98cc8ba

hneiva force-pushed the hneiva/rcodesign branch from aac1754 to 98cc8ba Compare January 3, 2024 18:34

hneiva requested a review from bhearsum January 8, 2024 20:04

bhearsum approved these changes Jan 9, 2024

View reviewed changes

Conversation

hneiva commented Nov 16, 2023

Uh oh!

bhearsum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hneiva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhearsum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hneiva left a comment •

edited

Loading