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

Change conda-content-trust from pre-solve to post-solve #11545

Merged

Conversation

kenodegard
Copy link
Contributor

@kenodegard kenodegard commented Jun 14, 2022

Description

In an effort to speed up package signature verification we move the verification process to occur after solving. This way only the packages included in the transaction are validated (tens of packages, not thousands).

A secondary goal was to move even more of the signature verification logic into conda-content-trust itself. This is in preparation for making this a conda plugin once the framework is available (#11435). With this change, conda knows/cares even less about what and how signature verification happens.

Depends on #11646
Resolves #11154
See conda/conda-content-trust#29

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?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jun 14, 2022
@kenodegard kenodegard requested a review from awwad June 14, 2022 20:07
@kenodegard kenodegard self-assigned this Jun 14, 2022
@travishathaway travishathaway added the source::anaconda created by members of Anaconda, Inc. label Jun 27, 2022
@kenodegard kenodegard marked this pull request as ready for review July 6, 2022 02:32
@kenodegard kenodegard requested a review from a team as a code owner July 6, 2022 02:32
@kenodegard kenodegard changed the title Move all conda-content-trust logic into conda-content-trust Change conda-content-trust from pre-solve to post-solve Jul 6, 2022
@kenodegard kenodegard marked this pull request as draft July 21, 2022 16:56
@LtDan33
Copy link
Member

LtDan33 commented Jul 29, 2022

Is there any info on how much the process will be sped up?

@kenodegard
Copy link
Contributor Author

@LtDan33, @awwad did some super rough testing a few weeks ago using conda install django --dry-run and got this:

Pre- & Disabled Pre- & Enabled Post- & Disabled Post- & Enabled
9.402 24.58 11.375 10.89
9.785 25.617 10.884 10.842
9.322 25.951 10.884 13.834
11.398 26.779 11.465 11.256
9.956 24.58 11.766 11.102
9.803 25.912 11.521
26.159 11.498
25.795
Average 9.94 25.67 11.27 11.56

We see that with post-solve and sig verification disabled a slight slowdown occurred. This is likely due to the deepcopy (which can be addressed with changes to CCT itself) that I had to add but overall we see a good speedup from the pre-solve to post-solve.

@kenodegard kenodegard force-pushed the sig-verification branch 3 times, most recently from e93c051 to 5131ef0 Compare August 10, 2022 13:37
@kenodegard kenodegard marked this pull request as ready for review August 10, 2022 13:37
Comment on lines +393 to +395
[
signature_verification(self),
],
Copy link
Contributor Author

@kenodegard kenodegard Aug 16, 2022

Choose a reason for hiding this comment

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

This can be refactored pretty nicely (in the future) using the plugin framework:

@conda.plugins.register
def conda_metadata():
    return signature_verification
Suggested change
[
signature_verification(self),
],
pm.hook.conda_metadata(),

@awwad
Copy link
Contributor

awwad commented Aug 31, 2022

Performance is good now. Will share times later. Code looks good to me. Basic happy path tests are good. Still doing manual testing due to missing tests in conda.

Ken, how do you feel about a feature branch for this? I'd approve this PR for a feature branch, but it can't get released without more testing. (TBC, this can still be manual testing for now, and I'm still working on that.)

@kenodegard kenodegard changed the base branch from main to feature/signature-verification August 31, 2022 18:47
@kenodegard kenodegard force-pushed the sig-verification branch 2 times, most recently from e509484 to e0a206c Compare August 31, 2022 18:49
It's still looking for an fn and signatures in this line, which are no longer arguments.

Note that this function is now fed only one record, so it is now implicit that the record is the correct one.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
as Ken suggested.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
conda/trust/signature_verification.py Outdated Show resolved Hide resolved
@awwad awwad merged commit 4d49fa5 into conda:feature/signature-verification Sep 1, 2022
@kenodegard kenodegard deleted the sig-verification branch September 1, 2022 15:54
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Sep 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity source::anaconda created by members of Anaconda, Inc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Significant performance impact when enabling --enable-signature-verification with conda token
5 participants