Support azure signtool #771
Conversation
…ructor into support-azure-signtool
conda-bot
added
the
cla-signed
| @@ -109,6 +109,12 @@ def main_build(dir_path, output_dir='.', platform=cc_platform, | |||
| if info.get(key): # only join if there's a truthy value set | |||
| info[key] = abspath(join(dir_path, info[key])) | |||
|
|
|||
| # Normalize name and set default value | |||
| if info.get("windows_signing_tool"): | |||
| info["windows_signing_tool"] = info["windows_signing_tool"].lower().replace(".exe", "") | |||
There was a problem hiding this comment.
What happens if a user tries to set this to a full path? Might be tempting 😬
There was a problem hiding this comment.
At the same time, if we are validating the input in construct.py:805, why do we need normalization here? 🤔 For library usage? In that case it should be the same logic (or move the logic here).
There was a problem hiding this comment.
What happens if a user tries to set this to a full path?
constructor will and should fail. windows_signing_tool is the name. Otherwise, I would have to infer from the binary what the appropriate signing too is.
In that case it should be the same logic
Yes, that's true, good catch!
(or move the logic here).
I'm not sure about this. Setting default values and manipulating input is done in main_build. validate() performs validation here. If we move this line into validate(), we are mixing scopes, which makes things harder to track.
There was a problem hiding this comment.
Very nice work. I'm a little concerned about the lack of tests, so I wonder if it's feasible to have a some secrets added to a dummy vault (or something that implements the same API). However, that might be too complicated and this feature will be used routinely by Anaconda anyway, so I guess we'll know when it breaks.
Double check that the docs render correctly locally, I found a little typo with the admonition syntax. Other than that, this is good to go after addressing the comments.
Co-authored-by: jaimergp <[email protected]>
I will work internally on this and will see that I can get tests into this PR. |
|
I added integration tests and secrets were added to the repository. However, the secrets will not be available for anybody who submits a PR from the fork, which will cause the tests to fail. The tests do execute the correct command though: https://github.com/conda/constructor/actions/runs/8760110611/job/24044501617#step:9:2133 Those tests will have to be skipped until merged into This requires a token, so I will convert this PR into draft until that infrastructure is in place. Additional things I found:
|
| ) | ||
| command = ( | ||
| f"{win_str_esc(self.executable)} sign /f {win_str_esc(self.certificate_file)} " | ||
| f"/tr {win_str_esc(timestamp_server)} /td {timestamp_digest} /fd {file_digest}" |
There was a problem hiding this comment.
Just noting here that no quotes are needed because we don't expect spaces here.
constructor/signing.py
Outdated
| command = ( | ||
| f"{self.executable} sign -v" | ||
| ' -kvu %AZURE_SIGNTOOL_KEY_VAULT_URL%' | ||
| ' -kvc %AZURE_SIGNTOOL_KEY_VAULT_CERTIFICATE%' |
There was a problem hiding this comment.
We can. I just tested it locally and the test worked fine with quotes, too.
constructor/signing.py
Outdated
| " -kvi %AZURE_SIGNTOOL_KEY_VAULT_CLIENT_ID%" | ||
| " -kvt %AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID%" | ||
| " -kvs %AZURE_SIGNTOOL_KEY_VAULT_SECRET%" |
There was a problem hiding this comment.
Same, do we need to quote any of these?
| raise RuntimeError(f"Signature verification failed.\n{proc.stderr}") | ||
| try: | ||
| status, status_message = proc.stdout.strip().split("\n") | ||
| status = int(status) |
There was a problem hiding this comment.
Can this fail if status is empty or None?
There was a problem hiding this comment.
I'm not sure how this could be None unless proc.stdout can somehow be None.
If status is an empty string, int(status) raises a ValueError, which is caught further down.
| ) | ||
| @pytest.mark.parametrize( | ||
| "auth_method", | ||
| os.environ.get("AZURE_SIGNTOOL_TEST_AUTH_METHODS", "token,secret").split(","), |
There was a problem hiding this comment.
Why do we need this "parametrized" parametrization? Are you using it locally while debugging? I don't see it in the CI setup 🤔
There was a problem hiding this comment.
I wanted to make sure that all authentication methods could potentially be covered. For tokens and secrets, I can just check the environment variables and skip if needed. For managed identities, however, this does not work because there isn't an environment variable to check for (logging into the vault must happen before AzureSignTool is executed).
So, the only "safe" methods to add into the parametrization are token and secret. However, I still wanted to allow testing with a managed identity.
Does that make sense? I tried to explain this briefly in the docstring.
| """Test signing installers with AzureSignTool. | ||
|
|
||
| There are three ways to authenticate with Azure: tokens, secrets, and managed identities. | ||
| There is no good sentinel environment for manged identities, so an environment variable |
There was a problem hiding this comment.
| There is no good sentinel environment for manged identities, so an environment variable | |
| There is no good sentinel environment for managed identities, so an environment variable |
I think?
|
|
||
| If neither `AZURE_SIGNTOOL_KEY_VAULT_ACCESSTOKEN` nor `AZURE_SIGNTOOL_KEY_VAULT_SECRET` are set, `constructor` will use a Managed Identity (`-kvm` CLI option). | ||
| ::: | ||
| ### Signing and notarizing PKG installers |
There was a problem hiding this comment.
| ### Signing and notarizing PKG installers | |
| ### Signing and notarizing PKG installers |
Description
Add support for AzureSignTool to sign Windows installers.
AzureSignTooluses Azure key vaults, which do not use certificate files on disk. To achieve this, this PR:windows_signing_toolkeyword that selects the tool to sign. This key determines whether to sign the installer instead ofsigning_certificate, which makes signing more extensible.SignToolandAzureSignTool.signtool.exeto use a different digest algorithms.Since this requires access to Azure, tests cannot be added for this feature.
Closes #767.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?