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

addd_root_verification #2650

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Conversation

h4l0gen
Copy link
Contributor

@h4l0gen h4l0gen commented May 30, 2024

Description of the changes being introduced by the pull request:
Add root verification step in do_snapshot and do_timestamp for catching changes in signing keys.

Fixes #2438

@h4l0gen
Copy link
Contributor Author

h4l0gen commented May 30, 2024

@jku I don't have strong idea on how to check this locally, but it should works for us. PTAL and tell me if you see any required changes.
Thanks you 👍

@coveralls
Copy link

coveralls commented May 30, 2024

Pull Request Test Coverage Report for Build 9362401677

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 96.805%

Totals Coverage Status
Change from base Build 9362277470: 0.02%
Covered Lines: 1554
Relevant Lines: 1591

💛 - Coveralls

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.

Thanks. The core idea seems valid but you will need the Metadata object that contains Snapshot: otherwise you can't verify that it is correctly signed.

We'll need to figure out the testing before merging this... either we need a unit test (unsure yet how easy that is) or we need to modify the repository example so that it actually hits this code path. I can try to have a look tomorrow.

tuf/repository/_repository.py Outdated Show resolved Hide resolved
@jku
Copy link
Member

jku commented May 31, 2024

I've made a first pass for a test set for tuf.repository in #2651.

  • Feel free to merge that commit into this PR to test if you want (you'll need to remove the ExpectedFailure mark though) -- this way we could merge both together
  • otherwise I'll probably make 2651 into a proper PR next week and you can rebase this after that one is merged

(EDIT: looks like I have some coverage issues to sort out in that PR... I will probably look into those next week)

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.

I think this is correct, thanks. I'll refrain from approving until we have a test or two

@h4l0gen
Copy link
Contributor Author

h4l0gen commented Jun 1, 2024

Thanks @jku for test set, Yes we can wait for 2651. 👍

@jku
Copy link
Member

jku commented Jun 3, 2024

2651 is now merged. When you have the time please rebase on origin/main: the two tests marked ExpectedFailure should start "unexpectedly" succeeding:

  • if they do, just remove the marks from the two tests
  • if not, let me now

thanks!

Signed-off-by: h4l0gen <ks3913688@gmail.com>
Signed-off-by: h4l0gen <ks3913688@gmail.com>
@h4l0gen
Copy link
Contributor Author

h4l0gen commented Jun 4, 2024

@jku expected failure tests (3.11) is failing.

@jku
Copy link
Member

jku commented Jun 4, 2024

@jku expected failure tests (3.11) is failing.

these look like the "unexpected" successes that we actually expected: you can remove the @unittest.expectedFailure mark from them and we should be good to merge

Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
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.

This is great, thanks

@jku jku merged commit 0ac86c6 into theupdateframework:develop Jun 4, 2024
14 checks passed
@jku
Copy link
Member

jku commented Jun 4, 2024

squash merged with slightly modified commit message.

@h4l0gen h4l0gen deleted the add_root_verification branch June 4, 2024 07:01
shubhusion pushed a commit to shubhusion/python-tuf that referenced this pull request Jun 17, 2024
* repository: Handle online key change situations in do_snapshot() and do_timestamp():
  always create a new version if current version is not correctly signed
* remove expectedFailure marks from the related tests

Signed-off-by: h4l0gen <ks3913688@gmail.com>
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
Signed-off-by: shubhusion <shubham27.sharma03@gmail.com>
shubhusion pushed a commit to shubhusion/python-tuf that referenced this pull request Jul 27, 2024
* repository: Handle online key change situations in do_snapshot() and do_timestamp():
  always create a new version if current version is not correctly signed
* remove expectedFailure marks from the related tests

Signed-off-by: h4l0gen <ks3913688@gmail.com>
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
Signed-off-by: shubhusion <shubham27.sharma03@gmail.com>
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.

repository: Tweak snapshot/timestamp triggers
3 participants