MSI: Add support for signing of installers #1224
Conversation
| certificate_file=certificate_file, | ||
| ) | ||
|
|
||
| def _get_signing_params(self): |
There was a problem hiding this comment.
About the changes in this file to simplify review: The new code is based on existing patterns, _get_signing_params() extracts the same environment variables already used in get_signing_command(), and sign() builds the same command structure but as a list for direct execution. The create_signing_tool() function is essentially the same logic that was already in winexe.py, just moved to reduce duplicated code, but reuses existing logic to enable MSI signing.
| CONSTRUCTOR_SIGNING_CERTIFICATE=str(cert_path), | ||
| CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD=cert_pwd, | ||
| ): | ||
| _verify_windows_signature(installer) |
There was a problem hiding this comment.
I added this because I thought it was odd that this test was even passing before signing of installers was supported for MSI Installers. This example was already running for EXE and MSI installers before this PR, and passed for MSI due to lack of actual verification during the testing.
6173fcf to
c9cb1bd
Compare
| outpath.unlink(missing_ok=True) | ||
| shutil.move(msi_paths[0], outpath) | ||
|
|
||
| if signing_tool: |
There was a problem hiding this comment.
Should we sign before moving the installer out of the staging directory?
| command += ' /p "%CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD%"' | ||
| return command | ||
|
|
||
| def sign(self, file_path: str | Path): |
There was a problem hiding this comment.
There is still some duplication here since get_signing_command is just the " ".join() on the list you're building inside sign()
There was a problem hiding this comment.
Yeah this is with some code duplication but its kept intentionally because I tried to keep it simple, here get_signing_command and sign have very different requirements.
To unify them, we'd need some kind of helper function that returns the command as a list of tuples like (value, needs_escaping, is_env_reference). Then sign() would extract just the values, while get_signing_command() would iterate through, apply win_str_esc() where needs_escaping=True, and replace actual values with %VAR% syntax where is_env_reference=True. For context:
- paths:
sign()uses actual value,get_signing_command()needswin_str_esc(value) - password: sign() uses actual value,
get_signing_command()emits"%ENV_VAR%"literal for NSIS to expand during runtime.
Most elements just need optional escaping, but password is fundamentally different.
I'm happy to do this refactoring if we are OK with the added complexity.
There was a problem hiding this comment.
In that case, I think keeping them separate is good.
| "tenant_id": os.environ.get("AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID"), | ||
| } | ||
|
|
||
| def sign(self, file_path: str | Path): |
There was a problem hiding this comment.
Same here: get_signing_command and what you need for sign are just two different representations of the same data.
|
|
||
|
|
||
| def create_signing_tool(info: dict) -> SigningTool | None: | ||
| """Create a signing tool based on construct.yaml configuration. |
There was a problem hiding this comment.
| """Create a signing tool based on construct.yaml configuration. | |
| """Create a signing tool based on construct.yaml. |
| signing_tool = WindowsSignTool(certificate_file=info.get("signing_certificate")) | ||
| elif signing_tool_name == "azuresigntool": | ||
| signing_tool = AzureSignTool() | ||
| else: |
There was a problem hiding this comment.
This function seems to only cover Windows, but this file also contains macOS signing tools.
There was a problem hiding this comment.
I renamed it to make it more clear 72d0414
|
@marcoesters I just added two more commits
|
This is strange. The environment should have been activated (and it is, or |
@marcoesters Im not sure, this branch is still pinned to the previous version |
0bc0fc6 to
f0ec7b7
Compare
|
@marcoesters this has also been rebased and I removed the python/pip specific commits after the rebase so it also needs a new approval. |
* add signing of installers - work in progress * Review fixes: sign before move, docstring update, rename func
Description
Adds code signing support for MSI installers built via Briefcase, reusing the existing EXE signing infrastructure.
sign()method toWindowsSignToolandAzureSignToolfor direct file signing (post-build)create_signing_tool()factory to reduce duplicationwindows_signing_tool/signing_certificateconfig as EXEtest_example_signingChecklist - did you ...
newsdirectory (using the template) for the next release's release notes?