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

Add e2e tests using a TUF mirror #3938

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Nov 19, 2024

Add a test that, instead of overriding trusted keys with environment variables, actually initializes a TUF cache using a local TUF mirror. This will prepare us to safely make changes to the TUF client.

Metadata for the public good instance goes through a different code path, since those keys are embedded in the code. We can't test signing and verifying with the public good instance hermetically, so we rely on conformance tests to catch issues with that path.

Summary

Release Note

Documentation

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.38%. Comparing base (2ef6022) to head (fa951be).
Report is 241 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3938      +/-   ##
==========================================
- Coverage   40.10%   36.38%   -3.73%     
==========================================
  Files         155      209      +54     
  Lines       10044    13383    +3339     
==========================================
+ Hits         4028     4869     +841     
- Misses       5530     7889    +2359     
- Partials      486      625     +139     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

This looks great!

@haydentherapper
Copy link
Contributor

@cpanato any idea on why the goreleaser snapshot is failing?

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Looks good. Some notes:

  • ConsistentSnapshot = false means the client will use some code paths that are different than prod (using consistent snapshot here would require adding version prefixes to snapshot/targets filenames and hash prefixes to artifact filenames). I think this might be irrelevant in this case (considering you just want to verify the code calling the tuf client isn't broken)
  • if you want to simplify, you could use a single key to sign all roles (maybe not worth it if that part is copied from an example). Could get rid of the second loop in setupTUF().
  • this PR does not include trusted_root.json yet but I guess you'd add that once you have code that would use it?

Seems good to me as is.

@jku
Copy link
Member

jku commented Nov 21, 2024

hook failed: shell: '/bin/bash -c if [ -n "$(git --no-pager diff --exit-code go.mod go.sum)" ]; then exit 1; fi': exit status 1: [no output]

well that looks fun (and likely unrelated to the PR).

  • --exit-code looks like a red herring as the test does not actually use the git exit code (because "-n $(...)")
  • $() eats the git diff output so we can't see the what this failed on
  • ...but there might also be a quoting failure here: notice how in the log the required starting quote after bash -c is missing even though it is there in sources

fixing the test to use git exit code instead of comparing strings would at least allow us to see what happens and might avoid the potential quoting issue -- git --no-pager diff --exit-code go.mod go.sum looks like it should work as is: no bash -c needed?

@cmurphy
Copy link
Contributor Author

cmurphy commented Nov 21, 2024

ConsistentSnapshot = false means the client will use some code paths that are different than prod

I did this to avoid having to add the hash prefixes to artifacts, I could change it if it turns out to be a problem.

if you want to simplify, you could use a single key to sign all roles

Thanks for the tip, I'll update this in order to reduce LOC

this PR does not include trusted_root.json yet but I guess you'd add that once you have code that would use it?

Right, cosign doesn't currently use trusted_root.json, I'm adding this now so that we have a place to build on in #3844

Add a test that, instead of overriding trusted keys with environment
variables, actually initializes a TUF cache using a local TUF mirror.
This will prepare us to safely make changes to the TUF client.

Metadata for the public good instance goes through a different code
path, since those keys are embedded in the code. We can't test signing
and verifying with the public good instance hermetically, so we rely on
conformance tests to catch issues with that path.

Signed-off-by: Colleen Murphy <[email protected]>
@haydentherapper
Copy link
Contributor

@cpanato we might need to try to free up more disk space, IIRC this is usually the culprit for this test failing

@jku
Copy link
Member

jku commented Nov 22, 2024

@cpanato we might need to try to free up more disk space, IIRC this is usually the culprit for this test failing

this seems unlikely to be the reason in this case: it's not actually building anything yet at the point of failure.

I can try out the refactor I mentioned: it should give some more leads.

@jku
Copy link
Member

jku commented Nov 22, 2024

I can try out the refactor I mentioned: it should give some more leads.

I was not expecting it but turns out the patch does simply create a go mod tidy change.

There's two commits in https://github.com/jku/cosign/commits/tuf-e2e-tests/ -- @cmurphy feel free to take both or just do a go mod tidy (I can submit the the hook refactor separately).

Signed-off-by: Colleen Murphy <[email protected]>
@cmurphy
Copy link
Contributor Author

cmurphy commented Nov 22, 2024

@jku wow thanks for catching that, I updated go.mod here if you want to submit your git-diff fix as its own PR

@haydentherapper haydentherapper enabled auto-merge (squash) November 22, 2024 18:03
@haydentherapper haydentherapper merged commit 6aeb919 into sigstore:main Nov 22, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants