tuf: use bundled trusted root if available #542

tnytown · 2023-03-12T04:08:33Z

Summary

This changeset modifies the TUF component to fetch keys from the trusted_root.json target, if available. trusted_root.json is 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

Signed-off-by: Andrew Pan <a@tny.town>

woodruffw

@tetsuo-cpp can do a full review, but as a small nit: needs a CHANGELOG entry 🙂

tnytown · 2023-03-14T16:17:19Z

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 <a@tny.town>

…root

Signed-off-by: Andrew Pan <a@tny.town>

sigstore/_internal/keyring.py

sigstore/_internal/tuf.py

Signed-off-by: Andrew Pan <a@tny.town>

CHANGELOG.md

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

tetsuo-cpp

LGTM!

Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

woodruffw · 2023-03-23T02:59:52Z

sigstore/_internal/keyring.py:81:39: F841 [*] Local variable `e` is assigned to but never used

woodruffw

Small nits 🙂

sigstore/_internal/keyring.py

woodruffw · 2023-03-23T03:02:54Z

sigstore/_internal/tuf.py

+                if not _is_timerange_valid(ca.valid_for, allow_expired=True):
+                    continue


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."

_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 :)

Got it, thanks!

sigstore/_internal/tuf.py

Signed-off-by: Andrew Pan <a@tny.town>

…root

sigstore/_internal/keyring.py

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

woodruffw · 2023-03-23T23:12:14Z

sigstore/_internal/tuf.py

+    @lru_cache()
+    def _updater(self) -> Updater:


Once we upgrade to 3.8 this can become cached_property, which will be nice. But until then, this LGTM!

yep! I was intending to use cached_property until I saw that it was 3.8+ :( Apparently cryptography polyfills it though!

woodruffw

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

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

woodruffw · 2023-03-25T02:47:19Z

Makes sense, thanks @asraa!

@tnytown: In that case, let's plan to remove support for the old "standalone" targets in an upcoming release. This is entirely an implementation detail in a private API, so we can do it with a patch release.

tnytown added 4 commits March 11, 2023 22:02

tuf: use "trusted root" if available

23a4481

Signed-off-by: Andrew Pan <a@tny.town>

tuf, test_tuf: adapt tests for bundled root

3ed59b6

Signed-off-by: Andrew Pan <a@tny.town>

keyring, _utils, test_utils: DER cert loading

40ea116

Signed-off-by: Andrew Pan <a@tny.town>

tuf: allow missing expiries in key validity period

27f6dc9

Signed-off-by: Andrew Pan <a@tny.town>

tnytown force-pushed the andrew/bundled-trust-root branch from f25a78c to 27f6dc9 Compare March 12, 2023 17:49

tuf, test_tuf: _get should never return None

c257dcd

Signed-off-by: Andrew Pan <a@tny.town>

woodruffw requested a review from tetsuo-cpp March 13, 2023 17:06

woodruffw added the component:tuf TUF related components label Mar 13, 2023

tnytown added the safe to test label Mar 13, 2023

woodruffw reviewed Mar 14, 2023

View reviewed changes

tnytown added 4 commits March 15, 2023 15:24

tuf: revert enums

ba3ef22

Signed-off-by: Andrew Pan <a@tny.town>

test_tuf: test_is_timerange_valid

77a3ce8

Signed-off-by: Andrew Pan <a@tny.town>

Merge remote-tracking branch 'origin/main' into andrew/bundled-trust-…

3d0d852

…root

_utils: _errors -> errors

ce334bc

Signed-off-by: Andrew Pan <a@tny.town>

tnytown force-pushed the andrew/bundled-trust-root branch from 8737363 to ce334bc Compare March 15, 2023 20:39

CHANGELOG: blurb

a763ed4

Signed-off-by: Andrew Pan <a@tny.town>

tnytown marked this pull request as ready for review March 15, 2023 20:44

tnytown changed the title ~~[WIP] tuf: use bundled trusted root if available~~ tuf: use bundled trusted root if available Mar 15, 2023

tetsuo-cpp reviewed Mar 17, 2023

View reviewed changes

sigstore/_internal/keyring.py Outdated Show resolved Hide resolved

tetsuo-cpp reviewed Mar 17, 2023

View reviewed changes

sigstore/_internal/tuf.py Show resolved Hide resolved

keyring: handle all errors in except cases

fb5c17b

Signed-off-by: Andrew Pan <a@tny.town>

woodruffw reviewed Mar 17, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

735321c

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

tetsuo-cpp previously approved these changes Mar 22, 2023

View reviewed changes

Merge branch 'main' into andrew/bundled-trust-root

8a7ac8c

Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

tnytown dismissed tetsuo-cpp’s stale review via 8a7ac8c March 23, 2023 01:28

woodruffw reviewed Mar 23, 2023

View reviewed changes

Implement suggestions from code review

d79bd1a

Signed-off-by: Andrew Pan <a@tny.town>

tnytown requested a review from woodruffw March 23, 2023 05:13

tnytown self-assigned this Mar 23, 2023

Merge remote-tracking branch 'origin/main' into andrew/bundled-trust-…

fb7d9bd

…root

woodruffw reviewed Mar 23, 2023

View reviewed changes

sigstore/_internal/keyring.py Outdated Show resolved Hide resolved

Apply suggestions from code review

acdcbd3

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>

woodruffw reviewed Mar 23, 2023

View reviewed changes

woodruffw approved these changes Mar 23, 2023

View reviewed changes

tnytown mentioned this pull request Mar 23, 2023

More unit testing for bundled trust root changes #580

Closed

5 tasks

woodruffw merged commit 7b390f4 into sigstore:main Mar 23, 2023

woodruffw deleted the andrew/bundled-trust-root branch March 23, 2023 23:22

asraa reviewed Mar 24, 2023

View reviewed changes

woodruffw mentioned this pull request Mar 25, 2023

TUF: Remove support for non-bundled roots of trust #584

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tuf: use bundled trusted root if available #542

tuf: use bundled trusted root if available #542

tnytown commented Mar 12, 2023 •

edited

woodruffw left a comment

tnytown commented Mar 14, 2023

tetsuo-cpp left a comment

woodruffw commented Mar 23, 2023

woodruffw left a comment

woodruffw Mar 23, 2023

tnytown Mar 23, 2023

woodruffw Mar 23, 2023

woodruffw Mar 23, 2023

tnytown Mar 23, 2023

woodruffw left a comment •

edited

asraa left a comment

woodruffw commented Mar 25, 2023

		if not _is_timerange_valid(ca.valid_for, allow_expired=True):
		continue

tuf: use bundled trusted root if available #542

tuf: use bundled trusted root if available #542

Conversation

tnytown commented Mar 12, 2023 • edited

Summary

Release Note

Documentation

woodruffw left a comment

Choose a reason for hiding this comment

tnytown commented Mar 14, 2023

tetsuo-cpp left a comment

Choose a reason for hiding this comment

woodruffw commented Mar 23, 2023

woodruffw left a comment

Choose a reason for hiding this comment

woodruffw Mar 23, 2023

Choose a reason for hiding this comment

tnytown Mar 23, 2023

Choose a reason for hiding this comment

woodruffw Mar 23, 2023

Choose a reason for hiding this comment

woodruffw Mar 23, 2023

Choose a reason for hiding this comment

tnytown Mar 23, 2023

Choose a reason for hiding this comment

woodruffw left a comment • edited

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

woodruffw commented Mar 25, 2023

tnytown commented Mar 12, 2023 •

edited

woodruffw left a comment •

edited