Update TUF client to support options and add LiveTrustedRoot #41

codysoyland · 2023-12-11T22:38:11Z

Summary

This PR implements #38. This adds:

TUF client options consistent with Sigstore TUF client spec
Switch to go-tuf v2
A live-updating TrustedRoot implementation which is compatible with long-running processes to ensure that TUF metadata is synchronized daily.

Credit to @kommendorkapten for some parts of the client code, which we developed in collaboration at GitHub.

To do:

Update examples and commands
Update go.mod
Add tests

Release Note

Documentation

haydentherapper

Super excited to see the spec implemented!

go.mod

pkg/root/trusted_root.go

pkg/tuf/options.go

haydentherapper · 2023-12-13T20:56:11Z

pkg/tuf/client.go

-func checkEmbedded(tufRootURL string, fileJSONStore *filejsonstore.FileJSONStore) (json.RawMessage, error) {
-	embeddedRootPath := path.Join("repository", tufRootURL, RootTUFPath)
+// Refresh forces a refresh of the underlying TUF client.
+// As the tuf client does not support multiple refreshes during its


Out of scope, is this something that's being tracked in go-tuf-metadata?

This is a deliberate choice to maintain consistency with the reference implementation (python-tuf) which does not support multiple Refresh invocations: https://github.com/theupdateframework/python-tuf/blob/f711997a08cbb558fc8ab91406a846bbe4883d1a/tuf/ngclient/updater.py#L115

It has been discussed in this issue: rdimitrov/go-tuf-metadata#86

I don't think this is a blocker for go-tuf, as in the majority of cases I would reckon a TUF client is a short lived client, so multiple calls to Refresh() would not be needed.

I can chime in there, but I think the issue will come up if TUF is used by a server rather than client. Not a blocker for Sigstore since the services don't depend on TUF though.

pkg/tuf/client.go

haydentherapper · 2023-12-13T22:09:51Z

pkg/tuf/client.go

 	}
+	return c.up.Refresh()


How is concurrency handled with calling Refresh in paralell? By the underlying TUF repo?

No, this should be documented as a method unsafe for concurrent use. For concurrency it appears the LiveTrustedRoot implements required serialization via a Mutex.

I'll add an issue for that in go-tuf-metadata so we can work on it 👍

pkg/root/trusted_root.go

pkg/tuf/options.go

ramonpetgrave64 · 2024-01-10T21:46:21Z

fyi, with slsa-verifier, we are planning to use the npmjs delegation, pending this sigstore-go PR and in
slsa-framework/slsa-verifier#731

haydentherapper · 2024-01-16T21:22:04Z

pkg/root/trusted_root.go

+		TrustedRoot: tr,
+		mu:          sync.RWMutex{},
+	}
+	ticker := time.NewTicker(time.Hour * 24)


This should be configurable if metadata expires more frequently than 24 hours.

That makes sense to me, however since tuf.Options.CacheValidity is using days as the unit, I wonder if we should also change that? If that config value is >= 1, then this ticker will have no effect if configured <= 24 hours, as it will just pull the cached data.

To confirm, cache validity ignores whether the metadata is still valid?

Maybe it should be max(24 hours, CacheValidity)? If cache validity is set, just refresh once the cache expires, otherwise refresh every 24 hours?

Thinking more on this, I think it's simpler to leave this as-is and let the client initialization handle the check for cache validity. Worst case, it just means an unnecessary reinitialization of the TUF client from disk.

kommendorkapten · 2024-01-17T07:24:20Z

The go-tuf-metadata repo are expected to be the official v2 package in go-tuf soon, so we should wait with this PR for that to happen, to avoid too many changes.

haydentherapper · 2024-01-26T00:28:17Z

Move has begun: theupdateframework/go-tuf#583

Signed-off-by: Cody Soyland <codysoyland@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2024-01-29T17:25:47Z

go-tuf is now updated with the new v2 package, so I updated this PR to use that.

Co-authored-by: Hayden B <hblauzvern@google.com> Signed-off-by: Cody Soyland <codysoyland@github.com>

Signed-off-by: Cody Soyland <codysoyland@github.com>

codysoyland · 2024-02-05T21:30:59Z

I added a basic test to initialize a TUF repo and client in 96326fa.

I was hoping to use the RepositorySimulator from go-tuf to create the repo, but I think there are some bugs with the target support in that utility, and it looks like the authors intend to move it to an internal package. So using the basic_repository.go example as a starting point, I roughly created a TUF repo that I can use in tests. I also partially implemented the Fetcher interface, so we don't need to open a port for tests.

Assuming I'm on the right path, I can continue to add more tests based on this to validate the caching behavior.

/cc @kommendorkapten

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Cody Soyland <codysoyland@github.com>

It is impossible to reach the timestamp checks in loadMetadata, as the preceeding code to load metadata and verify it will force an online refresh anyway, so at this point, the cache has already been updated. Setting RemoteTargetsURL is not necessary as go-tuf will set that correctly by default. Signed-off-by: Cody Soyland <codysoyland@github.com>

pkg/tuf/config.go

codysoyland · 2024-02-07T22:19:48Z

I added tests for the tuf package, so we're up to 88.0% statement coverage in that package.

It turns out that writing some tests has already paid off as I was able to identify a few bugs and dead code.

kommendorkapten

Nice cody!

@haydentherapper Have some time to give this a review?

haydentherapper · 2024-02-09T06:15:24Z

Yep reviewing now!

haydentherapper

Just a few questions! This looks really good, nice work on the tests.

haydentherapper · 2024-02-09T06:13:32Z

pkg/tuf/client.go

+	// Based on that metadata we take a decision if a full TUF
+	// refresh should be done or not. As so, the tmpCfg is only needed
+	// here and not in future invocations.
+	tmpCfg.UnsafeLocalMode = true


To confirm, this means that if the timestamp along with all other metadata is not expired, then we will not make any network connections, correct?

examples/oci-image-verification/go.mod

pkg/root/trusted_root.go

haydentherapper · 2024-02-09T06:54:00Z

pkg/root/trusted_root.go

+		TrustedRoot: tr,
+		mu:          sync.RWMutex{},
+	}
+	ticker := time.NewTicker(time.Hour * 24)


To confirm, cache validity ignores whether the metadata is still valid?

Maybe it should be max(24 hours, CacheValidity)? If cache validity is set, just refresh once the cache expires, otherwise refresh every 24 hours?

pkg/tuf/client.go

haydentherapper · 2024-02-09T07:02:26Z

pkg/tuf/options.go

+		// Fall back to using a TUF repository in the temp location
+		home = os.TempDir()
+	}
+	opts.CacheValidity = 1


Should we have a cache by default? I think cache is expected to be used in the case of airgapped environments, so should we have this be 0 and just rely on timestamp expiration for when to fetch metadata by default?

Back in the summer of 2022 when we where discussing this, 1 day was proposed https://docs.google.com/document/d/1QWBvpwYxOy9njAmd8vpizNQpPti9rd5ugVhji0r3T4c/edit#heading=h.s9pcbj5k0o1z

In retrospect, I think that the tuf package maybe should set the cache validity to 0 (i.e no cache) but each client should set the value to the best in their domain. This way we minimize any possible footguns (i.e secure by default). Even if a single is quick, I could still see possible issues being opened when the client (e.g. cosign) does not immediately get the new root.

I can change this. cc @codysoyland

How does this interact with the target cache files? I don’t believe it does, correct? As in, the client will still cache targets on disk regardless of this configuration.

Reading this comment again:

Should we have a cache by default? I think cache is expected to be used in the case of airgapped environments, so should we have this be 0 and just rely on timestamp expiration for when to fetch metadata by default?

This is not really how it works. If caching is not used, the client will always try to get a new timestamp and root, regardless if the local timestamp is expired or not (this is according to the TUF spec).

The cache allows us to control if this (full TUF online refresh) should be performed on every invocation, or only once per day. But I'm still thinking that maybe having the cache off by default is a better option, as a full TUF update is not that expensive, if the client is up to date, it's two egress HTTP requests (one for root and one for timestamp). So the cache is not really with arigapped, it's more a way to speed up invocations where e.g. frequent cosign verify is performed in a short period of time.

How does this interact with the target cache files? I don’t believe it does, correct? As in, the client will still cache targets on disk regardless of this configuration.

Exactly, it's only the metadata that is cached in non-standard way to avoid a full metadata refresh against the remote TUF repository.

Since we're using the UnsafeLocalMode updater for the initial update, won't cache validity only matter if the timestamp is expired?

No, timestamp expiration is always respected. As the timestamp may expire at any time, or a new root (or target) can be published, so the client always looks for that on every invocation. This is the behaviour that's controlled via CacheValidity

Also, should cache validity be checked in GetTarget too? Otherwise GetTargetInfo forces a refresh if metadata is expired.

Yes, if the metadata is expired, this will happen. If this is in an airgapped environment this would fail, but for that, I would think a better approach is to just provide the trusted_root.json instead of using TUF, as we won't get any real security benefit from it. And also, I would think that the normal scenario is that a client does:

Create a TUF client

Get trusted_root.json target
and the two calls happens during initialization, pretty close to each other in time. And once the trusted_root.json is retrieved, it's used for the rest of the program's execution?

Ah, I took another pass over the code and now see what I was missing. In the current Sigstore TUF client, we have it set up where if the timestamp is not expired, no additional metadata is fetched. With the current implementation, if you wanted to replicate that behavior, you would set CacheValidity to be greater than the timestamp expiration - Then it would not refresh until the timestamp expires, which will then force a refresh.

Have you considered having this behavior controlled via a boolean rather than a time period? The current implementation does give finer control though. One idea to improve UX, maybe a NO_CACHE and MAX_CACHE constant, set to 0 and MAX_INT respectively? Then when setting up the config, you can choose to a) follow the TUF spec with NO_CACHE, b) only refresh when absolutely necessary with MAX_CACHE, or c) fine-tune based on needs.

I might also add a comment making this explicit that CacheValidity is not for airgapped environments, since it still does respect timestamp expiration.

Have you considered having this behavior controlled via a boolean rather than a time period?

I can't speak for @kommendorkapten, but in my opinion, the CacheValidity flag works well for systems that perform a lot of verifications, without having to manually bump the cache daily, and avoids the risk of missing new metadata if the repo's timestamp interval is long. However, I could see the argument of changing it to hours or time.Duration, or adding a few constants as you suggested.

I might also add a comment making this explicit that CacheValidity is not for airgapped environments, since it still does respect timestamp expiration.

What is your opinion on the correct behavior for airgapped environments? Would you suggest having an option to disregard an expired timestamp? That is certainly possible, but I would probably want to slap the Insecure prefix in front of that option. 😆

CacheValidity flag works well for systems that perform a lot of verifications, without having to manually bump the cache daily, and avoids the risk of missing new metadata if the repo's timestamp interval is long

This sounds good. I think the constants are a nice compromise for those who are configuring the client and want a simple "boolean" approach of "no cache" vs "cache up to timestamp expiration".

What is your opinion on the correct behavior for airgapped environments? Would you suggest having an option to disregard an expired timestamp?

I think we should omit airgapped support for now, there's much more nuance with getting this right, given it doesn't follow the TUF spec. I agree with @kommendorkapten that for airgapped clients, a reasonable approach is to fetch the trust root out of band. The python client has been thinking about what it means to be truly offline too.

Got it, I missed @kommendorkapten's comment on that, but I agree with you both!

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2024-02-09T07:48:53Z

I broke some tests when I updated to latest go-tuf, I will fix that.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Cody Soyland <codysoyland@github.com>

haydentherapper

Left one comment about how to clarify the purpose of cache validity and use consts, but it's not a blocker. Thanks for all of the work on this!

Signed-off-by: Cody Soyland <codysoyland@github.com>

haydentherapper

🎉

Addresses: #547 - [x] Pending: sigstore/sigstore-go#41 Uses the new [sigstore-go@0.2.0](https://github.com/sigstore/sigstore-go/releases/tag/v0.2.0) Currently slsa-verifier has npmjs' attestation key hardcoded. But sigstore now stores the same key within their own TUF root. This PR - dynamically use the keyid specified in the sigstore bundle, rather than the hardcoded keyid. - uses an updated ([pending]( sigstore/sigstore-go#41)) sigstore-go library that allows us to fetch a signed and verified copy of the same key. --------- Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>

codysoyland force-pushed the tuf-client-2 branch from 2235c6a to 0e74150 Compare December 12, 2023 18:39

haydentherapper requested changes Dec 13, 2023

View reviewed changes

kommendorkapten reviewed Dec 14, 2023

View reviewed changes

pkg/tuf/options.go Outdated Show resolved Hide resolved

kommendorkapten reviewed Dec 15, 2023

View reviewed changes

pkg/tuf/options.go Outdated Show resolved Hide resolved

rdimitrov mentioned this pull request Dec 15, 2023

Make go-tuf-metadata concurrency-safe rdimitrov/go-tuf-metadata#91

Closed

This was referenced Jan 9, 2024

feat: [draft] add experimental npm sigstore-tuf suport ramonpetgrave64/slsa-verifier#1

Closed

feat: fixes #547: add npm sigstore-tuf suport slsa-framework/slsa-verifier#731

Merged

haydentherapper reviewed Jan 16, 2024

View reviewed changes

codysoyland and others added 10 commits January 29, 2024 11:02

Update TUF client to support options and add LiveTrustedRoot

444736a

Signed-off-by: Cody Soyland <codysoyland@github.com>

Make sure DefaultOptions never fails

035c084

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

avoid empty strings for arguments, use named attributes

5f4fafa

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Ignore emacs backup files

511a0b9

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Created a bascig config file for the tuf client

e680b4f

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Style fixes

7be28c0

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Made consistent snapshot configurable

780beb3

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Clarified the use of unsafe local mode

b1f195f

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Updated to go-tuf/v2@master

3e2ab65

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Resolved merge conflict

8297aeb

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten force-pushed the tuf-client-2 branch from b036ee1 to 8297aeb Compare January 29, 2024 10:08

kommendorkapten added 2 commits January 29, 2024 11:22

Merge branch 'main' into tuf-client-2

9c4e1c4

Fixed errors from linter

11dedbd

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

codysoyland and others added 4 commits January 29, 2024 14:05

Use short variable declaration syntax

dc7e979

Co-authored-by: Hayden B <hblauzvern@google.com> Signed-off-by: Cody Soyland <codysoyland@github.com>

Remove old unused embedded root

8bc63cf

Signed-off-by: Cody Soyland <codysoyland@github.com>

Add func to fetch TUF root with given options

c270ed8

Signed-off-by: Cody Soyland <codysoyland@github.com>

Add chainable functional options to Options struct

a8fd9e0

Signed-off-by: Cody Soyland <codysoyland@github.com>

codysoyland changed the title ~~WIP: Update TUF client to support options and add LiveTrustedRoot~~ Update TUF client to support options and add LiveTrustedRoot Jan 29, 2024

kommendorkapten and others added 3 commits February 6, 2024 09:06

Made the tuf root file configurable via the command line

057aa83

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Merge branch 'main' into tuf-client-2

4ac2b31

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Use consts from go-tuf

72edefd

Signed-off-by: Cody Soyland <codysoyland@github.com>

codysoyland force-pushed the tuf-client-2 branch from 88a2c04 to 72edefd Compare February 6, 2024 14:38

codysoyland added 5 commits February 6, 2024 15:50

Add test to fetch target

0def807

Signed-off-by: Cody Soyland <codysoyland@github.com>

Breakout publish

f4d0556

Signed-off-by: Cody Soyland <codysoyland@github.com>

Add target support and refresh test

ee12af4

Signed-off-by: Cody Soyland <codysoyland@github.com>

Add TUF caching tests

fe78b34

Signed-off-by: Cody Soyland <codysoyland@github.com>

codysoyland commented Feb 7, 2024

View reviewed changes

pkg/tuf/config.go Show resolved Hide resolved

kommendorkapten previously approved these changes Feb 8, 2024

View reviewed changes

haydentherapper reviewed Feb 9, 2024

View reviewed changes

kommendorkapten added 3 commits February 9, 2024 08:35

Updated go-tuf

fd475da

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Updated to latest go-tuf

616ee98

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Clarified that the updates is replaced, not the actual tuf client

51aaf34

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten dismissed their stale review via 51aaf34 February 9, 2024 07:44

kommendorkapten and others added 2 commits February 9, 2024 08:53

Updated to new error type (pointer)

10be16d

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Use 0 days for default CacheValidity

1d0f156

Signed-off-by: Cody Soyland <codysoyland@github.com>

haydentherapper previously approved these changes Feb 9, 2024

View reviewed changes

Clarify CacheValidity option and add NoCache/MaxCache consts

37bb81f

Signed-off-by: Cody Soyland <codysoyland@github.com>

codysoyland dismissed haydentherapper’s stale review via 37bb81f February 9, 2024 21:57

haydentherapper approved these changes Feb 9, 2024

View reviewed changes

codysoyland merged commit 40d7422 into main Feb 10, 2024
9 checks passed

codysoyland deleted the tuf-client-2 branch February 10, 2024 18:29

codysoyland mentioned this pull request Feb 12, 2024

Implement Sigstore TUF Client Spec #38

Closed

Update TUF client to support options and add LiveTrustedRoot #41

Update TUF client to support options and add LiveTrustedRoot #41

Conversation

codysoyland commented Dec 11, 2023 • edited Loading

Summary

Release Note

Documentation

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonpetgrave64 commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten commented Jan 17, 2024

haydentherapper commented Jan 26, 2024 • edited Loading

kommendorkapten commented Jan 29, 2024

codysoyland commented Feb 5, 2024

codysoyland commented Feb 7, 2024

kommendorkapten left a comment

Choose a reason for hiding this comment

haydentherapper commented Feb 9, 2024

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten commented Feb 9, 2024

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

codysoyland commented Dec 11, 2023 •

edited

Loading

haydentherapper commented Jan 26, 2024 •

edited

Loading

haydentherapper Feb 9, 2024 •

edited

Loading