Fix updater workflow #1101
Fix updater workflow #1101
Conversation
| # The ANYKEY_SCHEMA check in verify_signature expects the keydict to | ||
| # include a keyid | ||
| key['keyid'] = keyid | ||
| valid_sig = securesystemslib.keys.verify_signature(key, signature, signed) |
There was a problem hiding this comment.
Should we bail on a single exception here? Feels like we need a try-catch...
There was a problem hiding this comment.
_get_metadata_file has some logic to collect all exceptions and pack them into NoWorkingMirrorError, so I think this just continues the existing pattern?
tuf/client/updater.py
Outdated
| # new threshold of new signatures contained within the downloaded root | ||
| # metadata object | ||
| if valid and metadata_role == 'root': | ||
| valid = self._verify_root_sigs(metadata_signable) |
There was a problem hiding this comment.
I also wouldn't call this function inside _verify_metadata_file(), because there is too much magic. Best to call it explicitly in the _update_root_metadata() code IMHO.
There was a problem hiding this comment.
Agreed, this does make _verify_metadata_file() much more magical. I opted to call it inside _verify_metadata_file() because this enable us to perform the check before the tempfile is persisted, and makes it easier to perform the check per download mirror.
If we do the check in _update_root_metadata() we need to clean up a file from the clients metadata directory and, perhaps less importantly, can only perform the check on a single file returned by _update_root_metadata()
There was a problem hiding this comment.
+1. I think the point of never writing unverified files is critical and should probably be encoded in comments or docs... If we do not adhere to that it's almost impossible to ensure that our state of metadata stays valid.
There was a problem hiding this comment.
Also, we want to make sure that other functions that depend on this function do the same thing. Let's leave the magic alone for now, but document clearly why it is there, and not somewhere else.
|
|
||
|
|
||
|
|
||
| def test_root_rotation_unmet_new_threshold(self): |
There was a problem hiding this comment.
Lovely test, but there's some overlap between this test and the next. Maybe clarify that there is at least one successful intermediate rotation involved or something.
|
I believe I've addressed all review comments. Once the PR is approved I'd like to squash some of the changes addressing review comments into the patches that introduced the changes. |
Co-authored-by: Joshua Lock <[email protected]> Signed-off-by: Teodora Sechkova <[email protected]> Signed-off-by: Joshua Lock <[email protected]>
We no longer handle compressed metadata files, so rename this method to be shorter and less confusing: _verify_uncompressed_metadata_file -> _verify_metadata_file Signed-off-by: Joshua Lock <[email protected]>
This method is duplicating verification steps which have already been completed before the file was written to disk. Signed-off-by: Joshua Lock <[email protected]>
Per the detailed client workflow in the specification step 1.2 "Version N+1 of the root metadata file MUST have been signed by: (1) a threshold of keys specified in the trusted root metadata file (version N), and (2) a threshold of keys specified in the new root metadata file being validated (version N+1)." Number 2 is implemented here as this step was not being performed by the Updater. Unfortunately we can't use existing signature verification methods in tuf.sig, because tuf.sig.signature_status() does not verify signatures for keys which are not listed in keydb (and tuf.sig.verify uses tuf.sig.signature_status) Therefore this patch introduces a method for verifying signatures with root keys listed in the signable being verified. Signed-off-by: Joshua Lock <[email protected]>
joshuagl
force-pushed
the
joshuagl/updater-verify-root
branch
from
eab64ac to
2fc25ad
Compare
When verifying newly downloaded root metadata with the keys listed in the root metadata being verified, multiple signatures with the same keyid should not be counted towards the threshold. A keyid should only count once towards the threshold. This fixes the _verify_root_self_signed() method introduced in PR theupdateframework#1101 to ensure that keyids are only counted once when verifying a threshold of new root signatures. Signed-off-by: Joshua Lock <[email protected]>
When verifying newly downloaded root metadata with the keys listed in the root metadata being verified, multiple signatures with the same keyid should not be counted towards the threshold. A keyid should only count once towards the threshold. This fixes the _verify_root_self_signed() method introduced in PR theupdateframework#1101 to ensure that keyids are only counted once when verifying a threshold of new root signatures. Signed-off-by: Joshua Lock <[email protected]>
Fixes issue #: N/A
Description of the changes being introduced by the pull request:
The updater was not adhering to the detailed client workflow. Specifically newly downloaded root metadata was not being verified with a threshold of signatures from itself.
Per the detailed client workflow in the specification step 1.2
This PR ensures that point two is verified by the updater.
Please verify and check that the pull request fulfills the following
requirements: