[signingscript] Add apple notarization support #681

hneiva · 2023-03-02T22:12:09Z

DO NOT MERGE

~~Secrets still need to be added to SOPS~~

It's ready to roll

bhearsum

This looks quite good overall! I'd like to see a few things before this lands:

Agreement on what pool(s) we're going to use for these new workers
Reworking the creds to set-up the files at init (instead of with each task)
Some testing through dev before this lands (perhaps through adhoc-signing as we discussed before?).

signingscript/Dockerfile

signingscript/docker.d/build_msix_packaging.sh

signingscript/docker.d/apple_notarization_creds.yml

bhearsum · 2023-03-03T14:02:13Z

signingscript/src/signingscript/sign.py

+        command = [
+            "rcodesign",
+            "notary-submit",
+            "--staple",


This causes rcodesign to block on notarization completing. This is a notable change from our current process, and will tie up available workers in whatever pool this runs in.

I think this is OK to do, however, this argues for creating a new pool that is exclusively responsible for notarization and stapling (to avoid other signing types getting blocked or delayed if there's a spike in notarization requests, or Apple is slow, etc, etc.). This will end up being a nice simplification and clean up of the current graph (where we have the confusing "sign1" -> "poll" -> "sign3" flow).

(I don't think we need to use the current model of separate workers for notarization and stapling, as long as we do use a separate worker type here, and make sure its maxCapacity is set fairly high.)

In an ideal world, each signingscript that is handling notarization would be able to claim multiple tasks (since most of its work is just sleeping and waiting on Apple) -- but that is well into the realm of future improvements, and absolutely not part of this initial work.

Let's get some measurements on how quick this goes with adhoc, then we'll have a better idea of how bad it could be.

bhearsum · 2023-03-03T14:12:54Z

signingscript/src/signingscript/sign.py

+
+    # Notarize
+    with tempfile.NamedTemporaryFile() as temp_creds:
+        temp_creds.write(json.dumps(credential).encode("ascii"))


Is there a reason we can't simply deploy creds as this, rather than write them out with each task? We do this, for example, with Google credentials in pushapk workers: creating files as part of init, setting the file path as part of the worker config

I'm not sure I love the idea of either:
a. Having the credentials in SOPS base64'd
b. Having to construct the json file in init_worker.sh

What if we moved the credentials creation to async_main? (if !credential.exist, create it sort of deal)

It will have to be in SOPS either way, no? (I don't think we have any other place to put secrets that these workers need?)

As far as init_worker vs. async_main - I don't feel terribly strongly either way. Although requiring an intermediary format to async_main makes local testing a bit more annoying? I'm not going to block on either approach - as long as it's not being done per-task I'm pretty happy.

hneiva · 2023-03-08T19:30:10Z

Tested on dev pool: mozilla-releng/adhoc-signing#152

pyproject.toml

bhearsum · 2023-03-09T01:17:03Z

signingscript/src/signingscript/script.py

+        os.path.dirname(context.config["apple_notarization_configs"]),
+        'apple_api_key.json',
+    )
+    if os.path.exists(context.apple_credentials_path):


This code path means that this function doesn't do what it claims ("write credentials"), and also doesn't throw an error. Why does it exist, and what can we do to improve this? Is it a remnant from when this was being called per task?

Basically it will write the first time, and skip for any subsequent tasks.
The function still checks for scope issues, so there's a reason for it to exist.
Any specific suggestions on improving it?

In that case, it ought to have a different name, or move the scope checks out :).

bhearsum · 2023-03-09T01:20:10Z

signingscript/src/signingscript/sign.py

+    await rcodesign_notarize(workdir_files, notarization_workdir, context.apple_credentials_path)
+
+    # Compress files and return path to tarball
+    return await _create_tarfile(context, path, all_file_names, compression, notarization_workdir)


Is there a particular reason we're sending these back as tarballs instead of pkg or apps (as we do with authenticode signing)?

.apps are not compressed at all.
Since this is a "new" type of task, defaulting to a compressed output seems reasonable.
That being said, we might adjust it later if required 🙃

Ah right - I totally lost the plot on .app format 😆 . This is fine.

bhearsum

I'm good with this with the write_credentials function renamed (or the scope check moved out).

hneiva requested a review from a team March 2, 2023 22:12

bhearsum requested changes Mar 3, 2023

View reviewed changes

hneiva force-pushed the hneiva/rcodesign-signingscript branch 3 times, most recently from 2ae1178 to 01ecc91 Compare March 8, 2023 18:49

hneiva commented Mar 8, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

hneiva marked this pull request as ready for review March 8, 2023 20:20

hneiva requested a review from bhearsum March 8, 2023 20:20

bhearsum reviewed Mar 9, 2023

View reviewed changes

hneiva force-pushed the hneiva/rcodesign-signingscript branch from 949063d to 8275397 Compare March 9, 2023 22:56

bhearsum approved these changes Mar 9, 2023

View reviewed changes

jcristau added the signingscript label Mar 10, 2023

hneiva added 11 commits March 15, 2023 12:22

feat: signingscript: add rcodesign + docker build improvements

2b5f1af

feat: Add apple notarization support

51168f7

Fix signingscript tests

32b0584

Move apple notarization creds persistance

07f66a5

fix: Remove COPY --chown for signingscript

4dc8cfa

fix: remove comma from apple notarization yaml

2cc07b6

tests: signinscript: Added tests for docker.d yml files

45d9ce7

fix: Remove spaces from empty line

4968378

fix: add vendored to signingscript dockerfile

4e91a61

Minor syntax fix

f199134

PR bits: Rename function

8b17c13

hneiva force-pushed the hneiva/rcodesign-signingscript branch from 359fefe to 8b17c13 Compare March 15, 2023 19:23

hneiva merged commit e1994ea into mozilla-releng:master Mar 15, 2023

hneiva deleted the hneiva/rcodesign-signingscript branch March 15, 2023 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[signingscript] Add apple notarization support #681

[signingscript] Add apple notarization support #681

hneiva commented Mar 2, 2023 •

edited

Loading

bhearsum left a comment

bhearsum Mar 3, 2023

hneiva Mar 8, 2023

bhearsum Mar 3, 2023

hneiva Mar 3, 2023

bhearsum Mar 3, 2023

hneiva commented Mar 8, 2023

bhearsum Mar 9, 2023

hneiva Mar 9, 2023 •

edited

Loading

bhearsum Mar 9, 2023

bhearsum Mar 9, 2023

hneiva Mar 9, 2023

bhearsum Mar 9, 2023

bhearsum left a comment

[signingscript] Add apple notarization support #681

[signingscript] Add apple notarization support #681

Conversation

hneiva commented Mar 2, 2023 • edited Loading

DO NOT MERGE

bhearsum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hneiva commented Mar 8, 2023

Choose a reason for hiding this comment

hneiva Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhearsum left a comment

Choose a reason for hiding this comment

hneiva commented Mar 2, 2023 •

edited

Loading

hneiva Mar 9, 2023 •

edited

Loading