tuf: improve TUF client concurrency and caching #1953

asraa · 2022-06-02T17:39:17Z

Summary

Moves Rekor public key retrieval from API inside GetRekorPubs.
TUF objects always use in-memory local stores and target store. If caching is enabled, we sync with the on-disk store on (one-time) start and re-sync on any updates. We do not keep the local disk cache open, so we don't hog the OS file lock.
TUF objects are now singletons (see POC from @vaikas DNM concurrent NewFromEnv tests #1941): this means no re-initialization for each process, we can re-use the TUF object.

Ticket Link

Partial fix #1935

Release Note

Signed-off-by: Asra Ali <[email protected]>

Signed-off-by: Asra Ali <[email protected]> update Signed-off-by: Asra Ali <[email protected]>

codecov-commenter · 2022-06-02T18:18:46Z

Codecov Report

Merging #1953 (8719e02) into main (ae90c74) will increase coverage by 0.69%.
The diff coverage is 53.19%.

@@            Coverage Diff             @@
##             main    #1953      +/-   ##
==========================================
+ Coverage   34.00%   34.69%   +0.69%     
==========================================
  Files         153      153              
  Lines        9981    10076      +95     
==========================================
+ Hits         3394     3496     +102     
  Misses       6208     6208              
+ Partials      379      372       -7

Impacted Files	Coverage Δ
cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go	`37.83% <ø> (+1.47%)`	⬆️
cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go	`50.00% <ø> (+0.73%)`	⬆️
cmd/cosign/cli/verify/verify_blob.go	`10.25% <0.00%> (-0.04%)`	⬇️
pkg/cosign/tuf/testutils.go	`0.00% <0.00%> (ø)`
pkg/cosign/tlog.go	`30.07% <11.11%> (+0.94%)`	⬆️
pkg/cosign/tuf/client.go	`64.72% <66.03%> (+1.91%)`	⬆️
pkg/cosign/verify.go	`28.85% <75.00%> (-3.93%)`	⬇️
...is/policy/v1beta1/clusterimagepolicy_validation.go	`93.06% <0.00%> (-0.78%)`	⬇️
...s/policy/v1alpha1/clusterimagepolicy_validation.go	`93.06% <0.00%> (-0.69%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae90c74...8719e02. Read the comment docs.

dlorenc · 2022-06-02T20:27:45Z

Should we drop WIP from the title?

asraa · 2022-06-02T20:35:18Z

Should we drop WIP from the title?

I just realized one more clean-up I can do to this PR more clean, and will drop it then! I'll ping back for a review

dlorenc · 2022-06-02T20:37:33Z

Cool! Nice job here :)

Co-authored-by: Ville Aikas <[email protected]> Signed-off-by: Asra Ali <[email protected]>

asraa · 2022-06-02T20:49:21Z

cc @dlorenc @vaikas @haydentherapper @znewman01

Ready for review! Addressed some of the clean-up issues, expect more to come, including (listed in #1935):

Move usage of the TUF client to populate CheckOpts: we do this for Fulcio root, but not for the Rekor pubs. I think the cosign as a library will be a lot more clear if the TUF client was separated from the Verification library functions.
Cleanup on the Rekor pub keys alternative env vars
Minor clean-up on the GetRemoteRoot and GetEmbedded exported functions.

haydentherapper · 2022-06-02T22:45:45Z

pkg/cosign/tlog.go

-func GetRekorPubs(ctx context.Context) (map[string]RekorPubKey, error) {
+// SIGSTORE_TRUST_REKOR_API_PUBLIC_KEY - If specified, fetches the Rekor public
+// key from the Rekor server using the provided rekorClient.
+// TODO: Rename SIGSTORE_TRUST_REKOR_API_PUBLIC_KEY to be test-only or remove.


nit: I'd say remove entirely - Ultimately this will get used outside of testing, and at least with the other env var, the file is provided out of band rather than fetching the key directly from the source.

One thought here is that being able to fetch the public key from Rekor is part of the Rekor API, and whether this flag is here or not is only one part of getting used 'outside of testing'.

I think it’s fine to provide an API to fetch the public key, but there needs to be an explicit strategy around root of trust management. Ideally it’d always be TUF for Sigstore, but consumers may choose to TOFU - but they must persist the key, verification logic can’t rely on fetching the key each time.

Agree it should be removed -- but I'd like to do this in another PR because the current webhook and tests rely on it and I need to dig in separately into that.

haydentherapper · 2022-06-02T22:53:54Z

pkg/cosign/tuf/client.go

+	// singletonTUF holds a single instance of TUF that will get reused on
+	// subsequent invocations of initializeTUF
+	singletonTUF   *TUF
+	singletonTUFMu sync.Mutex


Could you use a Once to control access instead of a mutex?

We probably could. I just wasn't sure if we'd be swapping the TUF object on update, or just modify parts of it on update. I think we might just be able to modify the in-mem map during updates, but as I said wasn't sure what the level of modification was right so started with this.

We are modifying parts of it on update -- Once is feasible, I can work with reseting it during tests.

Actually, I dug in while I was attempting this change: we don't end up updating the TUF client unless we re-initialize. Since this PR uses the singleton/mutex we don't actually support long-running processes. I'm going to change to Once since it's behavior will be the same as a mutex, and call out a follow-up to support long-running process updates in the Cleanup issue (in practice this isn't that big of an issue right now).

Thanks, I think Once is cleaner since it explicitly shows that the singleton should be initialized only once.

haydentherapper · 2022-06-02T22:54:44Z

pkg/cosign/tuf/client.go

@@ -105,6 +108,12 @@ type remoteCache struct {
 	Mirror string `json:"mirror"`
 }

+func resetForTests() {
+	singletonTUFMu.Lock()


Does this need to be wrapped in a mutex? If tests aren't run in parallel the lock isn't needed. If tests are being run in parallel, then wouldn't deleting the singleton be an issue?

Since some of the tests are expecting a 'fresh' TUF setup, I added this to make sure we don't get any errors from modifying it without a lock in place.

Tests within the tuf pkg will be sequential, and we need the reset (but as you said, don't need the lock).

There is 1 test outside the TUF package that uses a custom TUF root. I'm concerned about that, that may be run in parallel. I will be refactoring that anyway to use a custom CheckOpts for the Rekor key rather than a custom TUF set up.

In the interim, I need to think how to lock that test.

OK resolution: I added a TODO for that test that uses a custom root outside this package, expect a PR this afternoon addressing that.

haydentherapper · 2022-06-02T22:55:34Z

pkg/cosign/tuf/client.go

 	return t.getRootStatus()
 }

-// Close closes the local TUF store. Should only be called once per client.
-func (t *TUF) Close() error {


To clarify, this is no longer needed because we only access the TUF DB to write the in-memory metadata on initialization if not present, or read it if present?

Yup, that's right! For longer running processes (like the webhook) there should be a way to refresh the data, but @asraa has some ideas there.

For longer running processes we are also refreshing now -- this is handled in updateClient: we Update our local in-memory metadata, then syncMetadata to the DB.

I'm a little confused why we need the mutex. Is it because there's a concern that initializeTUF will called multiple times by other processes?

It shouldn't -- I switched over to Once initialization, and reset the sync.Once for tests.

The only time it will be called multiple times potentially in parallel is because of the one test outside the TUF pkg that may run in parallel with the TUF tests.

pkg/cosign/tuf/client.go

haydentherapper · 2022-06-02T23:00:44Z

pkg/cosign/tuf/client.go

+	}
+	// Sync the in-memory local store to the on-disk cache.
+	tufDB := filepath.FromSlash(filepath.Join(rootCacheDir(), "tuf.db"))
+	diskLocal, err := tuf_leveldbstore.FileLocalStore(tufDB)


Is this API expected to be used, or are we reaching into private APIs? I just wanna make sure this is maintainable.

Yeah, FileLocalStore is meant to be used!

pkg/cosign/tuf/client.go

vaikas

Thanks @asraa for getting this wrangled into shape and @haydentherapper for the reviews!.

vaikas · 2022-06-03T05:15:38Z

pkg/cosign/tuf/client.go

 	return t.getRootStatus()
 }

-// Close closes the local TUF store. Should only be called once per client.
-func (t *TUF) Close() error {


Yup, that's right! For longer running processes (like the webhook) there should be a way to refresh the data, but @asraa has some ideas there.

vaikas · 2022-06-03T05:34:38Z

pkg/cosign/tuf/client.go

+	// singletonTUF holds a single instance of TUF that will get reused on
+	// subsequent invocations of initializeTUF
+	singletonTUF   *TUF
+	singletonTUFMu sync.Mutex


We probably could. I just wasn't sure if we'd be swapping the TUF object on update, or just modify parts of it on update. I think we might just be able to modify the in-mem map during updates, but as I said wasn't sure what the level of modification was right so started with this.

vaikas · 2022-06-03T05:35:26Z

pkg/cosign/tuf/client.go

@@ -105,6 +108,12 @@ type remoteCache struct {
 	Mirror string `json:"mirror"`
 }

+func resetForTests() {
+	singletonTUFMu.Lock()


Since some of the tests are expecting a 'fresh' TUF setup, I added this to make sure we don't get any errors from modifying it without a lock in place.

Signed-off-by: Asra Ali <[email protected]>

haydentherapper · 2022-06-03T23:09:13Z

pkg/cosign/tuf/client.go

-	singletonTUF   *TUF
-	singletonTUFMu sync.Mutex
+	singletonTUF     *TUF
+	singletonTUFOnce = new(sync.Once)


Wow, I actually didn't know that go had allocation via new.

I don't mind this approach, but I found that someone else had the same problem and made sync but with Reset - https://github.com/matryer/resync

yeah i saw this as well and ideally wanted to avoid a totally new dependency just for testing -- I'm not sure if there's a big downside to this approach though.

haydentherapper · 2022-06-03T23:10:12Z

pkg/cosign/tuf/client.go

+		if ok && !isExpiredTimestamp(trustedTimestamp) && !forceUpdate {
+			// We're golden so stash the TUF object for later use
+			singletonTUF = t
+			return


Is this returning nil?

It's returning out of the sync.Once func, but we still return singletonTUF from initializeTUF

haydentherapper · 2022-06-03T23:11:49Z

pkg/cosign/tuf/client.go

+		var err error
+		t.local, err = newLocalStore()
+		if err != nil {
+			panic(err)


Darn, is this the consequence of sync.Once? If so, this isn't viable imo, we shouldn't add a panic into the client because if a server starts depending on it, it opens up an opportunity for query of deaths. Is there a way to return an error? If not, what you had before would be fine.

+1 on not panicing.

Could do a global variable - https://stackoverflow.com/questions/42069615/too-many-arguments-to-return-error

Just ran into this same issue in fulcioroots

Using the global!

Signed-off-by: Asra Ali <[email protected]>

haydentherapper

Nice work!

znewman01 · 2022-06-06T21:03:57Z

LGTM, great work on this!

asraa added 2 commits June 2, 2022 10:46

move rekor public key fetch inside GetRekorPubs

f96f21a

Signed-off-by: Asra Ali <[email protected]>

use in-memory metadata and targets, sync to disk on start and updates

227c133

Signed-off-by: Asra Ali <[email protected]> update Signed-off-by: Asra Ali <[email protected]>

asraa force-pushed the fix-tuf-client branch from 4497372 to 227c133 Compare June 2, 2022 18:06

asraa force-pushed the fix-tuf-client branch from 94cfb96 to 078553a Compare June 2, 2022 19:06

asraa marked this pull request as ready for review June 2, 2022 20:16

Use TUF singleton.

929e797

Co-authored-by: Ville Aikas <[email protected]> Signed-off-by: Asra Ali <[email protected]>

asraa force-pushed the fix-tuf-client branch from 078553a to 929e797 Compare June 2, 2022 20:43

asraa mentioned this pull request Jun 2, 2022

Cleanup for TUF code #1935

Open

asraa changed the title ~~WIP: TUF client improvements: use in-memory store~~ tuf: improve TUF client concurrency and caching Jun 2, 2022

haydentherapper reviewed Jun 2, 2022

View reviewed changes

vaikas reviewed Jun 3, 2022

View reviewed changes

vaikas mentioned this pull request Jun 3, 2022

DNM concurrent NewFromEnv tests #1941

Closed

hayden comment, sync.Once used

77de032

Signed-off-by: Asra Ali <[email protected]>

haydentherapper requested changes Jun 3, 2022

View reviewed changes

return global error

8719e02

Signed-off-by: Asra Ali <[email protected]>

haydentherapper approved these changes Jun 6, 2022

View reviewed changes

dlorenc approved these changes Jun 7, 2022

View reviewed changes

dlorenc merged commit 6deaffb into sigstore:main Jun 7, 2022

github-actions bot added this to the v1.10.0 milestone Jun 7, 2022

This was referenced Jun 14, 2022

Lock TUF client during target loading operations #1992

Closed

Lock TUF client during target loading operations sigstore/sigstore#503

Merged

tuf: improve TUF client concurrency and caching #1953

tuf: improve TUF client concurrency and caching #1953

Conversation

asraa commented Jun 2, 2022 • edited Loading

Summary

Ticket Link

Release Note

codecov-commenter commented Jun 2, 2022 • edited Loading

Codecov Report

dlorenc commented Jun 2, 2022

asraa commented Jun 2, 2022

dlorenc commented Jun 2, 2022

asraa commented Jun 2, 2022 • 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

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

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

vaikas 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

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 left a comment

Choose a reason for hiding this comment

znewman01 commented Jun 6, 2022

asraa commented Jun 2, 2022 •

edited

Loading

codecov-commenter commented Jun 2, 2022 •

edited

Loading

asraa commented Jun 2, 2022 •

edited

Loading