Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired #893

haydentherapper · 2023-01-04T03:26:43Z

This changes Initialize/NewFromEnv to always check if the timestamp is expired, while still locking access to the TUF object.

Ref: https://github.com/sigstore/public-good-instance/issues/1161

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Summary

Release Note

Documentation

haydentherapper · 2023-01-04T03:28:01Z

A change in behavior to note: Previously, if you called Initialize, then NewFromEnv, the root would only be initialized once. Now, it will be twice. Additionally, Initialize is no longer thread safe with this implementation.

Lemme know if you see a better way to do this. If you want Initialize to be thread safe, I'll use a mutex.

asraa

/hold - we have had problems with thread safety with the TUF client

asraa

Could you move this

sigstore/pkg/tuf/client.go

Lines 280 to 284 in 095df6c

    
           // We may already have an up-to-date local store! Check to see if it needs to be updated. 
        
           trustedTimestamp, ok := trustedMeta["timestamp.json"] 
        
           if ok && !isExpiredTimestamp(trustedTimestamp) && !forceUpdate { 
        
           	return t, nil 
        
           }

check outside the singleton, and if it fails, refresh the singleton? (Check if there is a local cache, and if so, if the timestamp is expired. at least that wil preserve behavior and fix this issue. still, there's follow-up and it should be fixed that we need a better way to totally check the validity of the cache (signature validity) before we use the cache. otherwise we still may fail on an invalid cache during validation as we get the targets. defer to sigstore/sigstore-go...)

Without the threadsafety running cosign in parallel will break, which has happened before

EDIT: ignore the threadsafety comment, but I think the timestamp still should be moved outside the singleton DoOnce since we want to check that at each call to the TUF obj

haydentherapper · 2023-01-05T00:34:27Z

@asraa, I tried to move out all of the code before and including the timestamp expiration check outside of the singleton, but the concurrency test fails. I'm not certain what is causing this, so I thought it might be quicker to add a thread-unsafe method to use in the prober, which we need to unblock soon.

@vaikas, would you be able to take a look at this and let me know where there were thread safety issues in policy-controller?

vaikas · 2023-01-05T06:43:57Z

The problems we were seeing surfaced there, but were fixed here (@asraa and I were collaborating on the fix, so there's two PRs but only one got merged):
sigstore/cosign#1921
sigstore/cosign#1932

There was also some other investigation here:
sigstore/cosign#1941

Looks like the test for multiple access (TestConcurrentAccess) is there still in client_test.

There were also bunch of cleanup issues here:
sigstore/cosign#1935

I jotted it down in the ☝️ but just stating it here as well, I'm curious about the need for this being implemented as a singleton instead of using RW locks while operating on the underlying data structures.

haydentherapper · 2023-01-05T07:15:16Z

Thanks, I’ll take a look tomorrow!

Any opposition to what’s here? We are gonna have weekly alerts til we get a fix in, so I’d prefer to just get a quick fix in and refactor later.

vaikas · 2023-01-05T07:37:31Z

pkg/tuf/client.go

 }

+// Initialize creates a new TUF client, if one has not already been initialized,
+// and writes the cache to disk if requested. Thread safe.


this doesn't seem threadsafe from a quick glance and from the PR description.

Also probably worth while to note in the comments that this is the singleton instance. Or make it somehow clear that it does not necessairly return a new TUF client, but could return a previously created one. Reasoning being that it would then be easier to understand why this is different from InitializeAlways.

vaikas · 2023-01-05T07:57:52Z

pkg/tuf/client.go

+	return nil
+}
+
+// InitializeAlways creates a new TUF client. Repeated calls to Initialize


nit, Repeated calls to InitializeAlways?
This now allows one to have multiple clients simultaneously which I was having a bit of a rub before. I think by not forcing a singleton is nice, but the one thing that might be problematic still is the use of rootCacheDir below since now different clients will have a shared state in the form of the local cache, which is problematic. I think if you allow the caller to specify the cachedir here would be better.
And while you're at it, I think rather than having the getEmbedded live inside the function, taking in the fs.FS as an argument would be better, and would probably make the testing easier so one doesn't have to override the function (which IIRC caused some hilarity in the past as well. If not here, surely elsewhere :) ).

I think if you allow the caller to specify the cachedir here would be better.

Agree, but in practice if you used different clients then you'd need to access their roots / objs separately, but those are all hidden inside methods like

sigstore/pkg/fulcioroots/fulcioroots.go

Line 101 in 04379fd

func initRoots() ([]*x509.Certificate, []*x509.Certificate, error) {

or in methods like NewFromEnv.

(Lessons learned for a better second implementation..)

schucks :( I figure those should be hoisted to sigstore TUF client (something like GetFulcioRoots) since that's what (I think at least) the sigstore TUF client is adding on top of raw TUF, semantic meaning to various keys.

figure those should be hoisted to sigstore TUF client (something like GetFulcioRoots) since that's what (I think at least) the sigstore TUF client is adding on top of raw TUF, semantic meaning to various keys.

YES! I would have liked an API like:

GetTufObj(TufParams) -> TufObj TufObj.GetFulcio -> Fulcio ```

asraa · 2023-01-05T18:11:03Z

Idea maybe to explore: lock the singleton resets with a read/write mutex (that way it's safe to reset the singleton and block any readers from accessing the tufObj)

haydentherapper · 2023-01-10T23:44:29Z

@vaikas @asraa I'm going to be taking another look at this, lemme know if you have any other thoughts. Otherwise I'll explore moving away from a singleton to mutexes.

This changes the singleton to only initialize the client once, but on every invocation, check if the timestamp is expired. I wrapped all of this in a mutex to prevent concurrent updates (I'm not sure if this would be safe, I assume the TUF object is not thread-safe). Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper · 2023-01-13T22:27:53Z

@asraa @vaikas Reworked this, ptal

What I kept:

A singleton TUF object that's initialized once per process

Changes:

The singleton wraps the initialization of the TUF object, but not the checking of the timestamp (Asra, this was your suggestion earlier, idk why i didn't grok that)
Every invocation of Initialize or NewFromEnv will check if the timestamp is expired and download updated metadata if expired
A mutex now guards initializeTUF, which will prevent concurrent attempts to download updated metadata.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

pkg/tuf/client.go

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

asraa

Thanks! LGTM, sorry for the late review.

haydentherapper requested review from bobcallaway and asraa January 4, 2023 03:26

cpanato previously approved these changes Jan 4, 2023

View reviewed changes

asraa suggested changes Jan 4, 2023

View reviewed changes

haydentherapper dismissed cpanato’s stale review via 68a20be January 5, 2023 00:32

vaikas reviewed Jan 5, 2023

View reviewed changes

haydentherapper marked this pull request as draft January 10, 2023 23:45

haydentherapper force-pushed the tuf-update branch from 68a20be to 50c8aa4 Compare January 13, 2023 22:26

haydentherapper marked this pull request as ready for review January 13, 2023 22:27

Add concurrent test

965717d

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper changed the title ~~Allow Initialize to force TUF client recreation~~ Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired Jan 13, 2023

bobcallaway reviewed Jan 13, 2023

View reviewed changes

pkg/tuf/client.go Show resolved Hide resolved

Use singleton error

b9685d5

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

vaikas approved these changes Jan 17, 2023

View reviewed changes

bobcallaway approved these changes Jan 17, 2023

View reviewed changes

haydentherapper merged commit 8134411 into sigstore:main Jan 17, 2023

haydentherapper deleted the tuf-update branch January 17, 2023 17:15

asraa reviewed Jan 17, 2023

View reviewed changes

asraa mentioned this pull request Feb 8, 2023

cosign: TUF key expires and blocks all keyless verification fluxcd/source-controller#1004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired #893

Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired #893

haydentherapper commented Jan 4, 2023 •

edited

Loading

haydentherapper commented Jan 4, 2023

asraa left a comment

asraa left a comment •

edited

Loading

haydentherapper commented Jan 5, 2023

vaikas commented Jan 5, 2023

haydentherapper commented Jan 5, 2023

vaikas Jan 5, 2023

vaikas Jan 5, 2023

vaikas Jan 5, 2023

asraa Jan 5, 2023

vaikas Jan 5, 2023

asraa Jan 5, 2023

asraa commented Jan 5, 2023

haydentherapper commented Jan 10, 2023

haydentherapper commented Jan 13, 2023

asraa left a comment

	// We may already have an up-to-date local store! Check to see if it needs to be updated.
	trustedTimestamp, ok := trustedMeta["timestamp.json"]
	if ok && !isExpiredTimestamp(trustedTimestamp) && !forceUpdate {
	return t, nil
	}

Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired #893

Allow Initialize/NewFromEnv to reinitialize metadata if timestamp expired #893

Conversation

haydentherapper commented Jan 4, 2023 • edited Loading

Summary

Release Note

Documentation

haydentherapper commented Jan 4, 2023

asraa left a comment

Choose a reason for hiding this comment

asraa left a comment • edited Loading

Choose a reason for hiding this comment

haydentherapper commented Jan 5, 2023

vaikas commented Jan 5, 2023

haydentherapper commented Jan 5, 2023

vaikas Jan 5, 2023

Choose a reason for hiding this comment

vaikas Jan 5, 2023

Choose a reason for hiding this comment

vaikas Jan 5, 2023

Choose a reason for hiding this comment

asraa Jan 5, 2023

Choose a reason for hiding this comment

vaikas Jan 5, 2023

Choose a reason for hiding this comment

asraa Jan 5, 2023

Choose a reason for hiding this comment

asraa commented Jan 5, 2023

haydentherapper commented Jan 10, 2023

haydentherapper commented Jan 13, 2023

asraa left a comment

Choose a reason for hiding this comment

haydentherapper commented Jan 4, 2023 •

edited

Loading

asraa left a comment •

edited

Loading