tuf: use bundled trusted root if available#542
Conversation
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
tnytown
force-pushed
the
andrew/bundled-trust-root
branch
from
f25a78c to
27f6dc9
Compare
Signed-off-by: Andrew Pan <[email protected]>
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
@tetsuo-cpp can do a full review, but as a small nit: needs a CHANGELOG entry 🙂
|
I've left this one on draft because I haven't adjusted the tests to exercise the new codepaths. Most of the new logic seems trivial except for the stuff that ensures that a given target in a trusted root is active/expired. I'll work on adding some basic testing for that. |
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
tnytown
force-pushed
the
andrew/bundled-trust-root
branch
from
8737363 to
ce334bc
Compare
Signed-off-by: Andrew Pan <[email protected]>
tnytown
changed the title
Signed-off-by: Andrew Pan <[email protected]>
Co-authored-by: William Woodruff <[email protected]> Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
|
| if not _is_timerange_valid(ca.valid_for, allow_expired=True): | ||
| continue |
There was a problem hiding this comment.
This is a no-op, right? If so maybe we can just omit it entirely, and perhaps just leave a comment to the effect of "we could check _is_timerange_valid here but don't bother to, since expired Fulcio chains are considered valid for verification purposes."
There was a problem hiding this comment.
_is_timerange_valid also checks that the current timestamp is after valid_for's start, so in the off chance where that is not the case, this check will fail.
That being said, this detail is not too clear. I'll add a docstring to _is_timerange_valid :)
Signed-off-by: Andrew Pan <[email protected]>
Co-authored-by: William Woodruff <[email protected]> Signed-off-by: Andrew Pan <[email protected]>
| @lru_cache() | ||
| def _updater(self) -> Updater: |
There was a problem hiding this comment.
Once we upgrade to 3.8 this can become cached_property, which will be nice. But until then, this LGTM!
There was a problem hiding this comment.
yep! I was intending to use cached_property until I saw that it was 3.8+ :( Apparently cryptography polyfills it though!
There was a problem hiding this comment.
LGTM. Two thoughts:
-
Assuming the Sigstore TUF repo maintainers are okay with it, I think we can transition to codebase to assuming a trust root bundle (and failing if not present) after a migration period. We could retain an explicit opt-in flag (like
--tuf-unbundled-trust-root), if necessary. CC @asraa for thoughts 🙂 -
We should add some more unit tests for the bundled trust root. @tnytown could you make a follow-up issue for that?
asraa
left a comment
asraa
left a comment
There was a problem hiding this comment.
This is amazing!!
Yes - we are planning to have this be the stable trusted root target from now on. We will leave the existing targets in there for likely a year, and then deprecate them, so the current code with defaults for the trusted_root.json looks good to me.
Honestly, I wouldn't add an option to use the fallback old unbundled targets. IMO there's no need to, the behavior and targets are the same in both the bundled and unbundled so users wouldn't see any difference
4 participants
Summary
This changeset modifies the TUF component to fetch keys from the
trusted_root.jsontarget, if available.trusted_root.jsonis a rollup of Sigstore component public keys. Using this target reduces the number of network roundtrips necessary to fetch keys individually.Resolves #488.
Release Note
Documentation