-
Notifications
You must be signed in to change notification settings - Fork 276
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
addd_root_verification #2650
Conversation
@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. |
Pull Request Test Coverage Report for Build 9362401677Warning: 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
💛 - Coveralls |
There was a problem hiding this 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.
I've made a first pass for a test set for tuf.repository in #2651.
(EDIT: looks like I have some coverage issues to sort out in that PR... I will probably look into those next week) |
There was a problem hiding this 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
Thanks @jku for test set, Yes we can wait for 2651. 👍 |
2651 is now merged. When you have the time please rebase on origin/main: the two tests marked
thanks! |
Signed-off-by: h4l0gen <ks3913688@gmail.com>
83349c4
to
51c445c
Compare
@jku expected failure tests (3.11) is failing. |
these look like the "unexpected" successes that we actually expected: you can remove the |
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
There was a problem hiding this 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
squash merged with slightly modified commit message. |
* 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>
* 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>
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