Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1889223 - Submit all files to notarization before polling #957

Merged
merged 8 commits into from
Apr 26, 2024
24 changes: 21 additions & 3 deletions signingscript/src/signingscript/script.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import scriptworker.client

from signingscript.exceptions import SigningScriptError
from signingscript.task import build_filelist_dict, sign, task_cert_type, task_signing_formats
from signingscript.task import apple_notarize_stacked, build_filelist_dict, sign, task_cert_type, task_signing_formats
from signingscript.utils import copy_to_dir, load_apple_notarization_configs, load_autograph_configs

log = logging.getLogger(__name__)
Expand All @@ -24,6 +24,7 @@ async def async_main(context):
context (Context): the signing context.

"""
work_dir = context.config["work_dir"]
async with aiohttp.ClientSession() as session:
all_signing_formats = task_signing_formats(context)
if "gpg" in all_signing_formats or "autograph_gpg" in all_signing_formats:
Expand All @@ -36,17 +37,24 @@ async def async_main(context):
if not context.config.get("widevine_cert"):
raise Exception("Widevine format is enabled, but widevine_cert is not defined")

if "apple_notarization" in all_signing_formats or "apple_notarization_geckodriver" in all_signing_formats:
if {"apple_notarization", "apple_notarization_geckodriver", "apple_notarization_stacked"}.intersection(all_signing_formats):
if not context.config.get("apple_notarization_configs", False):
raise Exception("Apple notarization is enabled but apple_notarization_configs is not defined")
setup_apple_notarization_credentials(context)

context.session = session
context.autograph_configs = load_autograph_configs(context.config["autograph_configs"])

work_dir = context.config["work_dir"]
# TODO: Make task.sign take in the whole filelist_dict and return a dict of output files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get an issue on file for this, please.

# That would likely mean changing all behaviors to accept and deal with multiple files at once.

filelist_dict = build_filelist_dict(context)
for path, path_dict in filelist_dict.items():
if path_dict["formats"] == ["apple_notarization_stacked"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this actually function correctly if some files are not apple_notarization_stacked? Eg: will we sign (or notarize) them correctly, and then move on and do the stacked ones correctly? If not, we should disallow apple_notarization_stacked for only some files, and bail at a higher level here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll disallow mixed formats when stacked is present

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did disallowing mixed formats get implemented?

# Skip if only format is notarization_stacked - handled below
continue
if "apple_notarization_stacked" in path_dict["formats"]:
raise SigningScriptError("apple_notarization_stacked cannot be mixed with other signing types")
copy_to_dir(path_dict["full_path"], context.config["work_dir"], target=path)
log.info("signing %s", path)
output_files = await sign(context, os.path.join(work_dir, path), path_dict["formats"], authenticode_comment=path_dict.get("comment"))
Expand All @@ -55,6 +63,16 @@ async def async_main(context):
copy_to_dir(os.path.join(work_dir, source), context.config["artifact_dir"], target=source)
if "gpg" in path_dict["formats"] or "autograph_gpg" in path_dict["formats"]:
copy_to_dir(context.config["gpg_pubkey"], context.config["artifact_dir"], target="public/build/KEY")

# notarization_stacked is a special format that takes in all files at once instead of sequentially like other formats
# Should be fixed in https://github.com/mozilla-releng/scriptworker-scripts/issues/980
notarization_dict = {path: path_dict for path, path_dict in filelist_dict.items() if "apple_notarization_stacked" in path_dict["formats"]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using a new format for this just to make it safer to roll out? If so, what are the plans for apple_notarization and apple_notarization_geckodriver after we switch to stacked? (Ideally, it seems like we ought to move everything to stacked.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I initially replaced apple_notarization but decided against it to allow for the feature to ride the train.
I'll add deprecation notices on the old functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave geckodriver alone for now, and refactor that as we move things around later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get an issue on file to remove this later, too.

if notarization_dict:
output_files = await apple_notarize_stacked(context, notarization_dict)
for source in output_files:
source = os.path.relpath(source, work_dir)
copy_to_dir(os.path.join(work_dir, source), context.config["artifact_dir"], target=source)

log.info("Done!")


Expand Down
94 changes: 92 additions & 2 deletions signingscript/src/signingscript/sign.py
Original file line number Diff line number Diff line change
Expand Up @@ -1604,7 +1604,11 @@ async def _notarize_geckodriver(context, path, workdir):


async def _notarize_all(context, path, workdir):
"""Notarizes all files in a tarball"""
"""
Notarizes all files in a tarball
@Deprecated: This function is deprecated and will be removed in the future. Use apple_notarize_stacked instead.
"""
_, extension = os.path.splitext(path)
# Attempt extracting
await _extract_tarfile(context, path, extension, tmp_dir=workdir)
Expand Down Expand Up @@ -1634,10 +1638,11 @@ async def _notarize_all(context, path, workdir):
async def apple_notarize(context, path, *args, **kwargs):
"""
Notarizes given package(s) using rcodesign.
@Deprecated: This function is deprecated and will be removed in the future. Use apple_notarize_stacked instead.
"""
# Setup workdir
notarization_workdir = os.path.join(context.config["work_dir"], "apple_notarize")
shutil.rmtree(notarization_workdir, ignore_errors=True)
utils.mkdir(notarization_workdir)

_, extension = os.path.splitext(path)
Expand All @@ -1658,3 +1663,88 @@ async def apple_notarize_geckodriver(context, path, *args, **kwargs):
utils.mkdir(notarization_workdir)

return await _notarize_geckodriver(context, path, notarization_workdir)


@time_async_function
async def apple_notarize_stacked(context, filelist_dict):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of overlap between this function and _notarize_all. If we're planning to keep the latter, some refactoring to avoid all of the duplication would be nice.

"""
Notarizes multiple packages using rcodesign.
Submits everything before polling for status.
"""
ATTEMPTS = 5

relpath_index_map = {}
paths_to_notarize = []
task_index = 0

# Create list of files to be notarized + check for potential problems
for relpath, path_dict in filelist_dict.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I seem to recall that we can upload a zipfile with multiple files we want notarized inside. Assuming that's true, I suggest we construct a new zipfile and make a single request rather than making a bunch of smaller ones. (This would avoid potential issues with rate limiting or similar problems.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what I've gathered it's supported - but on the apple side it will check the apps sequentially, making things slower.
With the current setup we're making the exact same amount of parallel requests, so not much of a risk to be rate-limited - unless they could by requests per ~minute(?), which would be weird.

task_index += 1
relpath_index_map[relpath] = task_index
notarization_workdir = os.path.join(context.config["work_dir"], f"apple_notarize-{task_index}")
shutil.rmtree(notarization_workdir, ignore_errors=True)
utils.mkdir(notarization_workdir)
_, extension = os.path.splitext(relpath)
if extension == ".pkg":
path = os.path.join(notarization_workdir, relpath)
utils.copy_to_dir(path_dict["full_path"], notarization_workdir, target=relpath)
paths_to_notarize.append(path)
elif extension == ".gz":
await _extract_tarfile(context, path_dict["full_path"], extension, notarization_workdir)
workdir_files = os.listdir(notarization_workdir)
supported_files = [filename for filename in workdir_files if _can_notarize(filename, (".app", ".pkg"))]
if not supported_files:
raise SigningScriptError(f"No supported files found for file {relpath}")
for file in supported_files:
path = os.path.join(notarization_workdir, file)
paths_to_notarize.append(path)
else:
raise SigningScriptError(f"Unsupported file extension: {extension} for file {relpath}")

# notarization submissions map (path -> submission_id)
submissions_map = {}
# Submit to notarization one by one
for path in paths_to_notarize:
submissions_map[path] = await retry_async(
func=rcodesign_notarize,
args=(path, context.apple_credentials_path),
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)

# Notary wait all files
for path, submission_id in submissions_map.items():
await retry_async(
func=rcodesign_notary_wait,
args=(submission_id, context.apple_credentials_path),
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)

# Staple files
for path in submissions_map.keys():
await retry_async(
func=rcodesign_staple,
args=[path],
attempts=ATTEMPTS,
retry_exceptions=RCodesignError,
)

# Wrap up
stapled_files = []
for relpath, path_dict in filelist_dict.items():
task_index = relpath_index_map[relpath]
notarization_workdir = os.path.join(context.config["work_dir"], f"apple_notarize-{task_index}")
target_path = os.path.join(context.config["work_dir"], relpath)
_, extension = os.path.splitext(relpath)
# Pkgs don't need to be tarred
if extension == ".pkg":
utils.copy_to_dir(os.path.join(notarization_workdir, relpath), os.path.dirname(target_path))
else:
all_files = []
for root, _, files in os.walk(notarization_workdir):
for f in files:
all_files.append(os.path.join(root, f))
await _create_tarfile(context, target_path, all_files, extension, notarization_workdir)
stapled_files.append(target_path)
return stapled_files
5 changes: 5 additions & 0 deletions signingscript/src/signingscript/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from signingscript.sign import (
apple_notarize,
apple_notarize_geckodriver,
apple_notarize_stacked, # noqa: F401
sign_authenticode,
sign_debian_pkg,
sign_file,
Expand All @@ -33,6 +34,7 @@

log = logging.getLogger(__name__)


FORMAT_TO_SIGNING_FUNCTION = immutabledict(
{
"autograph_hash_only_mar384": sign_mar384_with_autograph_hash,
Expand All @@ -58,6 +60,9 @@
"autograph_rsa": sign_file_detached,
"apple_notarization": apple_notarize,
"apple_notarization_geckodriver": apple_notarize_geckodriver,
# This format is handled in script.py
# Should be refactored in https://github.com/mozilla-releng/scriptworker-scripts/issues/980
# "apple_notarization_stacked": apple_notarize_stacked,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to map this format to a function once the first todo above (about sign) is addressed. When you file that issue, please reference it here as well.

"default": sign_file,
}
)
Expand Down
1 change: 0 additions & 1 deletion signingscript/src/signingscript/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def mkdir(path):
Args:
path (str): the path to mkdir
"""
try:
os.makedirs(path)
Expand Down
19 changes: 19 additions & 0 deletions signingscript/tests/test_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,17 @@ async def fake_sign(_, val, *args, authenticode_comment=None):
assert authenticode_comment == "Some authenticode comment"
return [val]

async def fake_notarize_stacked(_, filelist_dict, *args, **kwargs):
return filelist_dict.keys()

mocker.patch.object(script, "load_autograph_configs", new=noop_sync)
mocker.patch.object(script, "load_apple_notarization_configs", new=noop_sync)
mocker.patch.object(script, "setup_apple_notarization_credentials", new=noop_sync)
# mocker.patch.object(script, "task_cert_type", new=noop_sync)
mocker.patch.object(script, "task_signing_formats", return_value=formats)
mocker.patch.object(script, "build_filelist_dict", new=fake_filelist_dict)
mocker.patch.object(script, "sign", new=fake_sign)
mocker.patch.object(script, "apple_notarize_stacked", new=fake_notarize_stacked)
context = mock.MagicMock()
context.config = {"work_dir": tmpdir, "artifact_dir": tmpdir, "autograph_configs": {}, "apple_notarization_configs": "fake"}
context.config.update(extra_config)
Expand Down Expand Up @@ -99,6 +103,21 @@ async def test_async_main_apple_notarization(tmpdir, mocker):
await async_main_helper(tmpdir, mocker, formats)


@pytest.mark.asyncio
async def test_async_main_apple_notarization_stacked(tmpdir, mocker):
formats = ["apple_notarization_stacked"]
mocker.patch.object(script, "copy_to_dir", new=noop_sync)
await async_main_helper(tmpdir, mocker, formats)


@pytest.mark.asyncio
async def test_async_main_apple_notarization_stacked_mixed_fail(tmpdir, mocker):
formats = ["autograph_mar", "apple_notarization_stacked"]
mocker.patch.object(script, "copy_to_dir", new=noop_sync)
with pytest.raises(SigningScriptError):
await async_main_helper(tmpdir, mocker, formats)


@pytest.mark.asyncio
async def test_async_main_apple_notarization_no_config(tmpdir, mocker):
formats = ["apple_notarization"]
Expand Down
61 changes: 61 additions & 0 deletions signingscript/tests/test_sign.py
Original file line number Diff line number Diff line change
Expand Up @@ -1481,3 +1481,64 @@ async def test_apple_notarize_geckodriver(mocker, context):

await sign.apple_notarize_geckodriver(context, "/foo/bar.pkg")
notarize_geckodriver.assert_awaited_once()


@pytest.mark.asyncio
async def test_apple_notarize_stacked(mocker, context):
notarize = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notarize", notarize)
wait = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_notary_wait", wait)
staple = mock.AsyncMock()
mocker.patch.object(sign, "rcodesign_staple", staple)

mocker.patch.object(sign, "_extract_tarfile", noop_async)
mocker.patch.object(sign, "_create_tarfile", noop_async)
mocker.patch.object(sign.os, "listdir", lambda *_: ["/foo.pkg", "/baz.app", "/foobar"])
mocker.patch.object(sign.os, "walk", lambda *_: [("/", None, ["foo.pkg", "baz.app"])])
mocker.patch.object(sign.shutil, "rmtree", noop_sync)
mocker.patch.object(sign.utils, "mkdir", noop_sync)
mocker.patch.object(sign.utils, "copy_to_dir", noop_sync)

await sign.apple_notarize_stacked(
context,
{
"/app.tar.gz": {"full_path": "/app.tar.gz", "formats": ["apple_notarize_stacked"]},
"/app2.pkg": {"full_path": "/app2.pkg", "formats": ["apple_notarize_stacked"]},
},
)
# one for each file format
assert notarize.await_count == 3
assert wait.await_count == 3
assert staple.await_count == 3


@pytest.mark.asyncio
async def test_apple_notarize_stacked_unsupported(mocker, context):
"""Test unsupported file extensions"""

mocker.patch.object(sign, "_extract_tarfile", noop_async)
mocker.patch.object(sign.shutil, "rmtree", noop_sync)
mocker.patch.object(sign.utils, "mkdir", noop_sync)
mocker.patch.object(sign.utils, "copy_to_dir", noop_sync)

# Returns unsupported file formats
mocker.patch.object(sign.os, "listdir", lambda *_: ["/foo.aaa", "/baz.bbb", "/foobar"])

with pytest.raises(SigningScriptError):
# Main file is supported, contents uses the above os.listdir
await sign.apple_notarize_stacked(
context,
{
"/app.tar.gz": {"full_path": "/app.tar.gz", "formats": ["apple_notarize_stacked"]},
},
)

with pytest.raises(SigningScriptError):
# Main file extension is unsupported
await sign.apple_notarize_stacked(
context,
{
"/app.bbb": {"full_path": "/app.bbb", "formats": ["apple_notarize_stacked"]},
},
)
1 change: 1 addition & 0 deletions signingscript/tests/test_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def fake_log(context, new_files, *args):
("widevine", stask.sign_widevine),
("autograph_authenticode", stask.sign_authenticode),
("autograph_authenticode_stub", stask.sign_authenticode),
("apple_notarization", stask.apple_notarize),
("default", stask.sign_file),
# Key id cases
("autograph_hash_only_mar384:firefox_20190321_dev", stask.sign_mar384_with_autograph_hash),
Expand Down