Provide codesigned stub exe's #13721

Callek · 2024-03-22T19:41:25Z

Description

This provides codesigned exe's for conda's stubs
Relates to Authenticode Sign cli-32.exe and cli-64.exe prebuilt binaries #13685
Signatures provided on behalf of Anaconda

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
Add / update outdated documentation?

codspeed-hq · 2024-03-22T21:00:16Z

CodSpeed Performance Report

Merging #13721 will degrade performances by 66.63%

_{Comparing Callek:callek/sign-stubs (0434968) with main (298ca49)}

Summary

❌ 7 regressions
✅ 14 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`Callek:callek/sign-stubs`	Change
❌	`test_env_update[classic]`	1.7 s	2 s	-14.26%
❌	`test_update[classic-update]`	1.6 s	1.8 s	-11.22%
❌	`test_update[classic-upgrade]`	1.6 s	1.8 s	-11.95%
❌	`test_solve_1[classic]`	18.2 s	21.5 s	-15.06%
❌	`test_jlap_fetch_ssl[False]`	360.8 ms	937.6 ms	-61.51%
❌	`test_jlap_fetch_ssl[True]`	619.8 ms	1,857.2 ms	-66.63%
❌	`test_match_1`	39.2 ms	45.3 ms	-13.38%

tests/test_codesigned.py

news/13721-sign-stubs

jezdez

Some comments that we need to answer before merging

jezdez · 2024-03-25T19:39:24Z

tests/test_codesigned.py

+    # Common installation directories where signtool might be located
+    common_paths = [
+        "C:\\Program Files (x86)\\Windows Kits\\10\\bin",
+        "C:\\Program Files\\Windows Kits\\10\\bin",
+        "C:\\Windows\\System32",
+    ]
+
+    signtool_path = None
+    # Search for signtool in common paths
+    for path in common_paths:
+        if signtool_path:
+            # We found one already
+            return signtool_path
+        if not os.path.exists(path):
+            continue
+        signtool_path = os.path.join(path, "signtool.exe")
+        if os.path.exists(signtool_path):
+            return signtool_path
+        elif "Windows Kits" in path:
+            signtool_path = None
+            max_version = 0
+            for dirname in os.listdir(path):
+                # Use most recent signtool version
+                if not dirname.endswith(".0"):
+                    continue  # next dirname
+                if int(dirname.replace(".", "")) < max_version:
+                    continue  # next dirname
+
+                maybe_signtool_path = os.path.join(path, dirname, "x64", "signtool.exe")
+                if os.path.exists(maybe_signtool_path):
+                    signtool_path = maybe_signtool_path
+    return signtool_path


This looks way too complicated for discovering signtool, why is shutil.which not enough? Could we just pass the list of additional paths as the path parameter of shutil.which?

Also, I'm curios, have you taken this code from somewhere given how specific this is about discovering the most recent version? Where does this case happen?

I took some slight inspiration from https://github.com/Azure/azure-code-signing-action.

which isn't enough because on these images we do not have this in the PATH by default, due to the multiple SDKs that github installs. We could in theory use the path parameter but the version of windows SDK's does vary from system to system and I wanted to prefer what is already set in the path and not override it.

The version info comparison is explicitly so we always take the most recent among whatever sdk is installed

I've also seen evidence, via googling that "C:\\Program Files\\Windows Kits\\10\\bin" and "C:\\Windows\\System32" may contain signtool in some circulstances, so I wrote this additionally so that it is more likely to succeed on local developer systems.

jezdez · 2024-03-25T19:40:03Z

tests/test_codesigned.py

+@pytest.mark.skipif(signtool_unsupported(), reason=signtool_unsupported_because())
+@pytest.mark.parametrize("stub_file_name", ["cli-32.exe", "cli-64.exe"])
+def test_stub_exe_signatures(stub_file_name: str) -> None:
+    """Verify that signtool verifies the signature of the stub exes"""


To clarify, the purpose is to verify that the built stubs are passing? In what situation wouldn't that be the case?

This is to verify that the pre-built stubs are signed with a valid (chains to windows root) signature. This wouldn't be the case if someone went to refresh the exe's and didn't sign them.

The test is so that we don't get surprised down the line by someone updating the binaries and not having had them signed prior to the PR.

jezdez · 2024-03-25T19:40:33Z

conda/shell/cli-32.exe

It'd be important to document how these files were created.

From my prior research these were created from https://github.com/conda/conda-build/tree/main/conda_build/launcher_sources - For my use-case here I did not rebuild what we've already been shipping for ages and merely took the existing binaries and signed them.

We likely will want to rebuild these in the future, and have stricter provenance guarantees around those binaries -- not to mention the upstream changes those sources have incurred since the last binary updates https://github.com/python/cpython/compare/main/PC/launcher.c -- and additionally since python3.7 there is now a launcher2 in upstream sources: https://github.com/python/cpython/blob/main/PC/launcher2.c

The base branch was changed.

jezdez · 2024-04-09T11:28:50Z

Retargeted at the release branch since we plan to do a 24.3.1 release with these changes.

jezdez · 2024-04-09T11:30:13Z

@Callek Hmm, the rebasing didn't properly reduce the changes to what you had planned, could you rebase on top of 24.3.x please?

Callek · 2024-04-09T11:56:38Z

@Callek Hmm, the rebasing didn't properly reduce the changes to what you had planned, could you rebase on top of 24.3.x please?

Sure, I'll do that within the hour

Co-authored-by: Bianca Henderson <beeankha@gmail.com>

Callek · 2024-04-09T12:49:00Z

@Callek Hmm, the rebasing didn't properly reduce the changes to what you had planned, could you rebase on top of 24.3.x please?

Sure, I'll do that within the hour

...and Done

jezdez · 2024-04-09T18:16:49Z

The performance regressions are odd, @Callek any idea what that could be?

Callek · 2024-04-09T18:35:59Z

The performance regressions are odd, @Callek any idea what that could be?

Really odd if this affected performance... it didn't error on my initial pr. Maybe it's comparing against the wrong base?

Callek · 2024-04-09T20:21:04Z

The performance regressions are odd, @Callek any idea what that could be?

Really odd if this affected performance... it didn't error on my initial pr. Maybe it's comparing against the wrong base?

EDIT: I didn't think it had a performance issue in initial PR... But now that I'm looking it may have

Taking one of the simpler tests test_env_update it seems this creates a new (temporary) environment...

https://github.com/Callek/conda/blob/0434968525bb2c956d7e9f6d5dcacf1af2008099/tests/cli/test_subcommands.py#L239-L240

This would be using the new binaries for stubs... Secondly the CodSpeed is showing a diff in call stacks between the base and head commits, which is puzzling a bit:

In All the regressed tests it seems to be adding a new PyObject_Vectorcall invocation...

@jezdez how should I proceed?

EDIT: most regressions seem to be < 1 second added... so maybe it relates to Windows running their certificate verification code under the hood (I'm unsure)

Callek · 2024-04-09T20:37:13Z

Sorry for additional noise, but looks like we're only running benchmarks on py3.12 on linux so I'd have ZERO idea why this could be impactful here... and even posit that this change is mere noise-in-the-machine

jezdez · 2024-04-10T18:09:00Z

tests/test_codesigned.py

+    return bool(signtool_unsupported_because())
+
+
+@pytest.mark.skipif(signtool_unsupported(), reason=signtool_unsupported_because())


Let's see if inlining this skipping into the test may prevent the tests to degrade performance wise

jezdez · 2024-04-10T19:21:59Z

As proven by the test PR #13787 the performance degradation seems to be something entirely different, or just a branch targeting issue in Codspeed. Let's merge this as-is.

* Add missing news for #13653 (#13711) * add missing news for #13653 * Provide codesigned stub exe's (#13721) Co-authored-by: Bianca Henderson <beeankha@gmail.com> * Changelog 24.4.0 (#13847) * Update .authors.yml * Update .mailmap * Update news * Updated authorship for 24.4.0 * Updated CHANGELOG for 24.4.0 * Add first-time contributions * Disable anaconda-anon-usage for CI. (#13803) (#13848) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Fix `pluggy 1.5.0` failure in tests (#13838) (#13849) Co-authored-by: Ken Odegard <kodegard@anaconda.com> * Bump conda-incubator/setup-miniconda in /.github/workflows (#13855) Bumps [conda-incubator/setup-miniconda](https://github.com/conda-incubator/setup-miniconda) from 3.0.3 to 3.0.4. - [Release notes](https://github.com/conda-incubator/setup-miniconda/releases) - [Changelog](https://github.com/conda-incubator/setup-miniconda/blob/main/CHANGELOG.md) - [Commits](conda-incubator/setup-miniconda@0301788...a426040) --- updated-dependencies: - dependency-name: conda-incubator/setup-miniconda dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump peter-evans/create-pull-request from 6.0.4 to 6.0.5 in /.github/workflows (#13856) Bumps [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request) from 6.0.4 to 6.0.5. - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](peter-evans/create-pull-request@9153d83...6d6857d) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * 🔄 synced file(s) with conda/infrastructure (#13857) Co-authored-by: Conda Bot <conda-bot@users.noreply.github.com> * Explicitly use macos-13 for macOS Intel runners (#13836) * Bump conda/actions from 24.2.0 to 24.4.0 in /.github/workflows (#13858) Bumps [conda/actions](https://github.com/conda/actions) from 24.2.0 to 24.4.0. - [Release notes](https://github.com/conda/actions/releases) - [Commits](conda/actions@f46142e...1e442e0) --- updated-dependencies: - dependency-name: conda/actions dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump conda/actions from 24.2.0 to 24.4.0 in /.github/workflows (#13859) Bumps [conda/actions](https://github.com/conda/actions) from 24.2.0 to 24.4.0. - [Release notes](https://github.com/conda/actions/releases) - [Commits](conda/actions@f46142e...1e442e0) Refs #13858. --- updated-dependencies: - dependency-name: conda/actions dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Explicitly use macos-13 for macOS Intel runners (#13836) * Add arm64 review builds (#13845) * Fix `conda env create/update --json` regression (#13860) * Add test to check json output when creating envs with yaml file * Add environment_with_pip.yml * Reformat test, change import --------- Co-authored-by: Ken Odegard <kodegard@anaconda.com> * Pass CODECOV_TOKEN to codecov GitHub Action step. (#13867) * Update test durations (#13873) * [pre-commit.ci] pre-commit autoupdate (#13876) * [pre-commit.ci] pre-commit autoupdate updates: - [github.com/astral-sh/ruff-pre-commit: v0.3.7 → v0.4.2](astral-sh/ruff-pre-commit@v0.3.7...v0.4.2) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Enable ruff --unsafe-fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Disable ruff --unsafe-fixes again * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Bast <2790401+dbast@users.noreply.github.com> * Bump CodSpeedHQ/action from 2.3.1 to 2.4.0 in /.github/workflows (#13875) * Bump CodSpeedHQ/action from 2.3.1 to 2.4.0 in /.github/workflows Bumps [CodSpeedHQ/action](https://github.com/codspeedhq/action) from 2.3.1 to 2.4.0. - [Release notes](https://github.com/codspeedhq/action/releases) - [Changelog](https://github.com/CodSpeedHQ/action/blob/main/CHANGELOG.md) - [Commits](CodSpeedHQ/action@aa99394...ac302de) --- updated-dependencies: - dependency-name: CodSpeedHQ/action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Update .github/workflows/tests.yml --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ken Odegard <kodegard@anaconda.com> * Small typo fix (#13877) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Daniel Holth <dholth@anaconda.com> Co-authored-by: Justin Wood (Callek) <callek@gmail.com> Co-authored-by: Bianca Henderson <beeankha@gmail.com> Co-authored-by: Jannis Leidel <jannis@leidel.info> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ken Odegard <kodegard@anaconda.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: conda-bot <18747875+conda-bot@users.noreply.github.com> Co-authored-by: Conda Bot <conda-bot@users.noreply.github.com> Co-authored-by: jaimergp <jaimergp@users.noreply.github.com> Co-authored-by: Bianca Henderson <bhenderson@anaconda.com> Co-authored-by: Daniel Bast <2790401+dbast@users.noreply.github.com> Co-authored-by: Erik Whiting <erik@erikwhiting.com>

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 22, 2024

Callek force-pushed the callek/sign-stubs branch 2 times, most recently from 29cacdc to 0734113 Compare March 22, 2024 20:20

Callek force-pushed the callek/sign-stubs branch 9 times, most recently from b291430 to 09fd036 Compare March 23, 2024 02:37

Callek marked this pull request as ready for review March 23, 2024 02:43

Callek requested a review from a team as a code owner March 23, 2024 02:43

Callek force-pushed the callek/sign-stubs branch from 3bad5c6 to fc341d6 Compare March 23, 2024 02:43

travishathaway reviewed Mar 25, 2024

View reviewed changes

tests/test_codesigned.py Outdated Show resolved Hide resolved

travishathaway reviewed Mar 25, 2024

View reviewed changes

tests/test_codesigned.py Outdated Show resolved Hide resolved

travishathaway reviewed Mar 25, 2024

View reviewed changes

tests/test_codesigned.py Show resolved Hide resolved

Callek force-pushed the callek/sign-stubs branch from fc341d6 to 11729f6 Compare March 25, 2024 13:37

beeankha reviewed Mar 25, 2024

View reviewed changes

news/13721-sign-stubs Outdated Show resolved Hide resolved

Callek requested review from beeankha and travishathaway March 25, 2024 16:20

jezdez reviewed Mar 25, 2024

View reviewed changes

jezdez mentioned this pull request Mar 25, 2024

Provide codesigned stub exe's conda/conda-build#5252

Merged

3 tasks

jezdez previously approved these changes Apr 3, 2024

View reviewed changes

jezdez requested a review from chenghlee April 3, 2024 13:57

jezdez changed the base branch from main to 24.3.x April 9, 2024 11:28

jezdez previously approved these changes Apr 9, 2024

View reviewed changes

jezdez force-pushed the callek/sign-stubs branch from 1170bde to 9cd35fd Compare April 9, 2024 11:29

Callek and others added 5 commits April 9, 2024 08:48

Test that the stub exe's are codesigned

09f2b4f

Sign existing copies of cli-32 and cli-64

63c8166

Add a news entry

ecfb571

Update news/13721-sign-stubs

194e53a

Co-authored-by: Bianca Henderson <beeankha@gmail.com>

Address review comment from identical code in a conda-build PR

0434968

Callek dismissed jezdez’s stale review via 0434968 April 9, 2024 12:48

Callek force-pushed the callek/sign-stubs branch from 9cd35fd to 0434968 Compare April 9, 2024 12:48

Callek requested a review from jezdez April 9, 2024 12:49

jezdez approved these changes Apr 9, 2024

View reviewed changes

chenghlee approved these changes Apr 9, 2024

View reviewed changes

jezdez reviewed Apr 10, 2024

View reviewed changes

Callek mentioned this pull request Apr 10, 2024

Add a docstring to is_server_available test #13787

Closed

jezdez merged commit 925a448 into conda:24.3.x Apr 10, 2024

jezdez mentioned this pull request Apr 24, 2024

Authenticode Sign cli-32.exe and cli-64.exe prebuilt binaries #13685

Closed

2 tasks

jaimergp mentioned this pull request Aug 29, 2024

How to distribute codesigned launchers? conda/conda-launchers#5

Open

github-actions bot added the locked [bot] locked due to inactivity label Apr 11, 2025

github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2025

		return bool(signtool_unsupported_because())


		@pytest.mark.skipif(signtool_unsupported(), reason=signtool_unsupported_because())

Provide codesigned stub exe's #13721

Provide codesigned stub exe's #13721

Conversation

Callek commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist - did you ...

Uh oh!

codspeed-hq bot commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #13721 will degrade performances by 66.63%

Summary

Benchmarks breakdown

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jezdez left a comment

Choose a reason for hiding this comment

Uh oh!

jezdez Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

Callek Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

jezdez Mar 25, 2024 • 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!

Callek Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

jezdez Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

Callek Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

jezdez commented Apr 9, 2024

Uh oh!

jezdez commented Apr 9, 2024

Uh oh!

Callek commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Callek commented Apr 9, 2024

Uh oh!

jezdez commented Apr 9, 2024

Uh oh!

Callek commented Apr 9, 2024

Uh oh!

Callek commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Callek commented Apr 9, 2024

Uh oh!

jezdez Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

jezdez commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Callek commented Mar 22, 2024 •

edited

Loading

codspeed-hq bot commented Mar 22, 2024 •

edited

Loading

jezdez Mar 25, 2024 •

edited

Loading

Callek commented Apr 9, 2024 •

edited

Loading

Callek commented Apr 9, 2024 •

edited

Loading