Use TUF to download key/cert material #351

jku · 2022-12-20T16:27:52Z

Should fix #25 by using TUF to download all certs and keys. It is based on code by @joshuagl, @wxjdsr (co-authors on first commit in this branch) but it is a significant rewrite compared to their branches.

FYI also @woodruffw, @di, @SantiagoTorres.

I'm allowing edits by maintainers, go wild if you feel like it.

Status (updated dec 22):

Works for signing and verifying
The embedded keys/certs are no longer used or included in git (except staging keys as test assets)
if the default (production) or --staging flows are used, TUF is used to fetch key material
TUF is also used in some other cases (code does the same thing as the previous implementation, and uses production key material if none is provided)
the implementation peeks into python-tuf internals currently (to get access to top-level target metadata). This should be okay for now but a long-term solution might be RFE: expose delegated metadata to client application theupdateframework/python-tuf#1995
When the caches are hot (certs, keys and metadata have been downloaded already), every "sigstore verify" and "sigstore sign" (with the production or staging flows) now results in a minimum of two network requests (404 and a small download). This may be avoidable in future but currently always happens
with empty caches we download ~10 files, in the order of 20kB altogether. These files are then cached for later use

Some TODO items (there are certainly more)

testing: many existing tests now require network
- we could mock the requests for staging in the tests
- longer term could make it possible to not do network requests when metadata is not expired
testing: the TUF module has no tests (it does get tested by all tests using staging though)
TUF metadata is stored in ~/.local/share/sigstore-python/tuf/<url>/ and targets are cached in ~/.cache/sigstore-python/tuf/<url>/ -- this is not great for windows
logging in python-tuf is pretty verbose at INFO: we'll have to change that at some level
error handling is almost non-existent, but let's handle in later PR _internal.tuf: exception handling #357 . The situations I've identified are:
- python-tuf RepositoryError (metadata from repo is not valid in some way)
- python-tuf DownloadError (trouble downloading things)
- python-tuf OSError (trouble storing things on disk)
- discovery errors in our _tuf module: the repository content was not what we expected
should remove the embedded key/cert material from the repo

TrustUpdater can be used to fetch specific trust roots: * Currently supports fetching * ctfe keys and * the rekor key * Caches target files in ~/.cache/sigstore-python/ * Stores metadata in ~/.local/share/sigstore-python/ * Expects to either * find the metadata _for the given URL_ in metadata store * (for prod and stage only) find the boostrap root.json in sigstore/_store The "API" that TrustUpdater provides is not meant to be final: it is the minimal one that should fulfill current needs. Nothing uses the TrustUpdater yet, but it's testable: >>> from sigstore._tuf import TrustUpdater, DEFAULT_TUF_URL >>> updater = TrustUpdater(DEFAULT_TUF_URL) >>> rekor_key_bytes = updater.get_rekor_key() Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: wxjdsr <wxjdsr@126.com> Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

CTKeyring: * Take bytes as constructore input: this makes it easier to feed things from either CLI arguments or the TUF trust updater. * Remove tests that no longer make sense. The prod/staging contant should still be tested but TUF is now used in the same flows: Unsure how to best test this. Use TUF to find the CTFE and rekor key when using "production" or "staging". Note that "staging" is currently untested: I am not sure even the URL makes sense. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com> Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Use TUF to get CTFE/Rekor keys in the non-staging, non-production flow. As before, the assumption is that user wants production keys in this case. Refactor TrustUpdater so that it does not do network traffic if nothing is requested. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

* Assume that the active fulcio certs in the repository form a certificate chain that cryptography can ingest * Refactor RekorClient construction so that we avoid constructing multiple TrustTupdaters Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

If this is not production or staging but rekor key is not given, use production: this is what original (non-tuf) code was doing as well. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

https://tuf-root-staging.storage.googleapis.com/ does work as staging repository. There is still a bug somewhere as staging verify currently fails. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

This is needed to bootstrap the TUF metadata with --staging. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

jku · 2022-12-20T16:37:31Z

Oh, a note on fulcio: I now feed all certs with usage "Fulcio" and status "Active" to Verifier with the assumption that

these all form a chain
cryptography copes with the (non-controlled) order of these certs

hopefully there are no problems with these assumptions

sigstore/_internal/rekor/client.py

jku · 2022-12-20T16:50:41Z

@asraa if you want to have a look at the TrustUpdater.get_* methods (https://github.com/jku/sigstore-python/blob/tuf-refactor/sigstore/_internal/tuf.py#L90) and comment whether my uses of "usage" and "status" seem sensible (from perspective of root-signing repository maintenance), that would be great.

woodruffw · 2022-12-20T17:35:54Z

Oh, a note on fulcio: I now feed all certs with usage "Fulcio" and status "Active" to Verifier with the assumption that

these all form a chain

cryptography copes with the (non-controlled) order of these certs

hopefully there are no problems with these assumptions

These assumptions should be fine! We throw everything into an OpenSSL X.509 store, and I don't believe it imposes any order of insertion.

sigstore/_internal/tuf.py

Signed-off-by: William Woodruff <william@trailofbits.com>

woodruffw · 2022-12-20T21:26:44Z

Online tests now pass, but the forced offline tests are still failing (as expected).

woodruffw · 2022-12-20T21:27:21Z

logging in python-tuf is pretty verbose at INFO: we'll have to change that at some level

IMO this is worth an upstream PR -- looks like most of these should really be DEBUG level instead. I can look into that.

Edit: Opened: theupdateframework/python-tuf#2243

sigstore/_verify/verifier.py

* Bring back RekorClient.production() and RekorClient.staging(): these are simple but make the calling code slightly clearer maybe * Add a TrustUpdater argument to those methods: if you use production/staging, you need a TrustUpdater * The TrustUpdater can not be constructed inside RekorClient as other components may need it as well. It's not perfectly elegant for the caller but it's not horrible either: updater = TrustUpdater.staging() client = RekorClient.staging(updater) This design means TrustUpdater does not know anything about the sigstore mechanisms: it just discovers and downloads files. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

No functional change, just refactor the target discovery into a single method. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

The test doesn't make a lot of sense now that the keys are not being read from the _store. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Signed-off-by: William Woodruff <william@trailofbits.com>

Annoying. Signed-off-by: William Woodruff <william@trailofbits.com>

sigstore/_internal/tuf.py

woodruffw · 2022-12-21T18:45:27Z

This is looking pretty good to me! I think we can move it out of a draft for now; there are only a few small cleanup tasks that I think should get done before we merge.

sigstore/_cli.py

sigstore/_internal/tuf.py

Signed-off-by: Alex Cameron <asc@tetsuo.sh>

tetsuo-cpp · 2022-12-22T06:01:27Z

I'm seeing some weird lint errors on this branch that I don't see in main, CI doesn't seem to have this issue as it's getting all the way to bandit:

sigstore/_utils.py:161: error: Returning Any from function declared to return "bytes"  [no-any-return]
Found 1 error in 1 file (checked 24 source files)

Looks like that file hasn't even been touched.

jku · 2022-12-22T08:51:52Z

I'm seeing some weird lint errors on this branch that I don't see in main, CI doesn't seem to have this issue as it's getting all the way to bandit:
sigstore/_utils.py:161: error: Returning Any from function declared to return "bytes"  [no-any-return]
Found 1 error in 1 file (checked 24 source files)
Looks like that file hasn't even been touched.

This is unrelated, it's not showing up in main because William added an ignore in 0647d75.

tetsuo-cpp · 2022-12-22T09:03:28Z

This is unrelated, it's not showing up in main because William added an ignore in 0647d75.

I assumed that was necessary somehow because we switched to Ruff, but your branch doesn't have that commit. Anyway, if you don't mind, I'll merge in main and get this branch up to date so hopefully this goes away.

Signed-off-by: Alex Cameron <asc@tetsuo.sh>

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

This is a potential improvement, not a necessary one. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

jku · 2022-12-22T09:40:36Z

I pushed some tiny documentation tweaks but I would agree: this looks good to me apart from the final linting issue #351 (comment)

tetsuo-cpp

LGTM provided that the linting is sorted out. Nice work.

Also tweak one annotation (remove unneeded quotes) Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

jku · 2022-12-22T11:25:19Z

I added the "#nosec" to get tests passing: feel free to solve the issue in some other way if you want.

di · 2022-12-22T14:44:00Z

/gcbrun

Signed-off-by: William Woodruff <william@trailofbits.com>

woodruffw · 2022-12-22T16:04:09Z

/gcbrun

woodruffw

LGTM!

sigstore/_internal/tuf.py

asraa · 2022-12-29T16:04:02Z

sigstore/_internal/tuf.py

+
+        # NOTE: _updater has been fully initialized at this point, but mypy can't see that.
+        targets = self._updater._trusted_set.targets.signed.targets  # type: ignore[union-attr]
+        for target_info in targets.values():


I actually would caution against using the custom metadata and use the path filters "rekor*", "fulcio*", "ctfe*", instead as the filtering mechanism. While we'll keep some of the metadata, I think we are aiming to change them (for example, status will be deprecated in favor of explicit NotBefore/NotAfter ranges)

That being said "usage" should match here and be stable

Yeah I knew the custom metadata was not a permanent solution but I'm not sure filtering by path is going to be either -- maybe makes sense to update once the target discovery and delegation plans are properly decided

I'm not sure filtering by path is going to be either -- maybe makes sense to update once the target discovery and delegation plans are properly decided

That's fair! Yeah, in the end distributing as a single bundle would probably fix that and that likely will be the case next year. The migration to that will be slow anyway (as in, the existing targets will continue to be distributed like this).

jku and others added 8 commits December 20, 2022 14:09

Fix linter issues in TUF related code

b6325fd

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

cli: Use production rekor key by default

d7b75c1

If this is not production or staging but rekor key is not given, use production: this is what original (non-tuf) code was doing as well. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

tuf: Enable staging support

6386af7

https://tuf-root-staging.storage.googleapis.com/ does work as staging repository. There is still a bug somewhere as staging verify currently fails. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

_store: Add missing staging root.json

29885ff

This is needed to bootstrap the TUF metadata with --staging. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

jku commented Dec 20, 2022

View reviewed changes

sigstore/_internal/rekor/client.py Show resolved Hide resolved

woodruffw reviewed Dec 20, 2022

View reviewed changes

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

woodruffw added 6 commits December 20, 2022 14:12

verifier: blacken

e1712a7

Signed-off-by: William Woodruff <william@trailofbits.com>

pyproject, sigstore/tuf: use appdirs for local state

b62a410

Signed-off-by: William Woodruff <william@trailofbits.com>

verifier: unused import

a8d1e4e

Signed-off-by: William Woodruff <william@trailofbits.com>

_internal/tuf: disambiguate caches correctly

f75c866

Signed-off-by: William Woodruff <william@trailofbits.com>

sign, verify, internal: refactor rekor client handling

3a8f026

Signed-off-by: William Woodruff <william@trailofbits.com>

test/verify: fix TestVerificationMaterials test

7d80e93

Signed-off-by: William Woodruff <william@trailofbits.com>

woodruffw added component:signing Core signing functionality component:verification Core verification functionality labels Dec 20, 2022

woodruffw mentioned this pull request Dec 20, 2022

tuf: move INFO logs to DEBUG or WARNING theupdateframework/python-tuf#2243

Merged

tetsuo-cpp self-requested a review December 20, 2022 23:36

jku commented Dec 21, 2022

View reviewed changes

sigstore/_verify/verifier.py Show resolved Hide resolved

jku added 4 commits December 21, 2022 11:28

internal: Improve tuf docstrings

8072f1d

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

internal: Refactor tuf

e85d6f4

No functional change, just refactor the target discovery into a single method. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

tests: Remove test for _store

238f191

The test doesn't make a lot of sense now that the keys are not being read from the _store. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

woodruffw added 3 commits December 21, 2022 13:30

tuf: re-use our read_embedded helper

03bdaf7

Signed-off-by: William Woodruff <william@trailofbits.com>

README: update --help texts

476b8f4

Signed-off-by: William Woodruff <william@trailofbits.com>

gitignore, test: allow staging-tuf assets

3c88b26

Annoying. Signed-off-by: William Woodruff <william@trailofbits.com>

woodruffw reviewed Dec 21, 2022

View reviewed changes

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

woodruffw marked this pull request as ready for review December 21, 2022 18:45

tetsuo-cpp reviewed Dec 22, 2022

View reviewed changes

sigstore/_cli.py Show resolved Hide resolved

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

tuf: Switch to using f-strings for logging

d9aa72c

Signed-off-by: Alex Cameron <asc@tetsuo.sh>

jku mentioned this pull request Dec 22, 2022

CLI: Limit user confusion over Rekor/Fulcio instance and state URLs #324

Closed

jku mentioned this pull request Dec 22, 2022

_internal.tuf: exception handling #357

Closed

tetsuo-cpp and others added 4 commits December 22, 2022 20:24

Merge remote-tracking branch 'origin/main' into tuf-refactor

deadd3c

Signed-off-by: Alex Cameron <asc@tetsuo.sh>

test: document TUF staging mock better

b1fdc9f

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

_internal/rekor: Mention updater arg in docsstrings

cf4e46f

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

_internal/tuf: Reword a TODO into a NOTE

be7a6d7

This is a potential improvement, not a necessary one. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

tetsuo-cpp previously approved these changes Dec 22, 2022

View reviewed changes

_internal/tuf: Add nosec for mypy-related assert

b7c0bdb

Also tweak one annotation (remove unneeded quotes) Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

jku dismissed tetsuo-cpp’s stale review via b7c0bdb December 22, 2022 11:22

woodruffw added 2 commits December 22, 2022 10:58

Merge branch 'main' into tuf-refactor

e94d78c

_internal/tuf: replace nosec with type ignore

4e7f680

Signed-off-by: William Woodruff <william@trailofbits.com>

woodruffw approved these changes Dec 22, 2022

View reviewed changes

woodruffw merged commit 5a68d1e into sigstore:main Dec 22, 2022

jku mentioned this pull request Dec 27, 2022

tuf: review the cache / local storage directory names #370

Closed

asraa reviewed Dec 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TUF to download key/cert material #351

Use TUF to download key/cert material #351

jku commented Dec 20, 2022 •

edited

jku commented Dec 20, 2022

jku commented Dec 20, 2022

woodruffw commented Dec 20, 2022

woodruffw commented Dec 20, 2022

woodruffw commented Dec 20, 2022 •

edited

woodruffw commented Dec 21, 2022

tetsuo-cpp commented Dec 22, 2022

jku commented Dec 22, 2022

tetsuo-cpp commented Dec 22, 2022 •

edited

jku commented Dec 22, 2022

tetsuo-cpp left a comment

jku commented Dec 22, 2022

di commented Dec 22, 2022

woodruffw commented Dec 22, 2022

woodruffw left a comment

asraa Dec 29, 2022

asraa Dec 29, 2022

jku Dec 29, 2022

asraa Dec 29, 2022

Use TUF to download key/cert material #351

Use TUF to download key/cert material #351

Conversation

jku commented Dec 20, 2022 • edited

jku commented Dec 20, 2022

jku commented Dec 20, 2022

woodruffw commented Dec 20, 2022

woodruffw commented Dec 20, 2022

woodruffw commented Dec 20, 2022 • edited

woodruffw commented Dec 21, 2022

tetsuo-cpp commented Dec 22, 2022

jku commented Dec 22, 2022

tetsuo-cpp commented Dec 22, 2022 • edited

jku commented Dec 22, 2022

tetsuo-cpp left a comment

Choose a reason for hiding this comment

jku commented Dec 22, 2022

di commented Dec 22, 2022

woodruffw commented Dec 22, 2022

woodruffw left a comment

Choose a reason for hiding this comment

asraa Dec 29, 2022

Choose a reason for hiding this comment

asraa Dec 29, 2022

Choose a reason for hiding this comment

jku Dec 29, 2022

Choose a reason for hiding this comment

asraa Dec 29, 2022

Choose a reason for hiding this comment

jku commented Dec 20, 2022 •

edited

woodruffw commented Dec 20, 2022 •

edited

tetsuo-cpp commented Dec 22, 2022 •

edited