From 94cfb961d7432e78c2b95549831bdf7dea957813 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Thu, 2 Jun 2022 13:36:32 -0500 Subject: [PATCH] Use TUF singleton. Co-authored-by: Ville Aikas Signed-off-by: Asra Ali --- .../cli/fulcio/fulcioroots/fulcioroots.go | 1 - .../cli/fulcio/fulcioverifier/ctl/verify.go | 1 - pkg/cosign/tlog.go | 1 - pkg/cosign/tuf/client.go | 43 ++++++++++------- pkg/cosign/tuf/client_test.go | 47 ++++++++++++++----- 5 files changed, 60 insertions(+), 33 deletions(-) diff --git a/cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go b/cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go index cba25ffe376a..29989321bbbb 100644 --- a/cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go +++ b/cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go @@ -116,7 +116,6 @@ func initRoots() (*x509.CertPool, *x509.CertPool, error) { if err != nil { return nil, nil, fmt.Errorf("initializing tuf: %w", err) } - defer tufClient.Close() // Retrieve from the embedded or cached TUF root. If expired, a network // call is made to update the root. targets, err := tufClient.GetTargetsByMeta(tuf.Fulcio, []string{fulcioTargetStr, fulcioV1TargetStr}) diff --git a/cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go b/cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go index 4031d6ae87ad..641088f375c0 100644 --- a/cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go +++ b/cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go @@ -81,7 +81,6 @@ func VerifySCT(ctx context.Context, certPEM, chainPEM, rawSCT []byte) error { if err != nil { return err } - defer tufClient.Close() targets, err := tufClient.GetTargetsByMeta(tuf.CTFE, []string{ctPublicKeyStr}) if err != nil { diff --git a/pkg/cosign/tlog.go b/pkg/cosign/tlog.go index c82370a9c4f7..8d31cea8d89b 100644 --- a/pkg/cosign/tlog.go +++ b/pkg/cosign/tlog.go @@ -109,7 +109,6 @@ func GetRekorPubs(ctx context.Context, rekorClient *client.Rekor) (map[string]Re if err != nil { return nil, err } - defer tufClient.Close() targets, err := tufClient.GetTargetsByMeta(tuf.Rekor, []string{rekorTargetStr}) if err != nil { return nil, err diff --git a/pkg/cosign/tuf/client.go b/pkg/cosign/tuf/client.go index 85ecef8ff38d..922cbe55ec05 100644 --- a/pkg/cosign/tuf/client.go +++ b/pkg/cosign/tuf/client.go @@ -32,6 +32,7 @@ import ( "runtime" "strconv" "strings" + "sync" "time" "github.com/theupdateframework/go-tuf/client" @@ -46,8 +47,14 @@ const ( SigstoreNoCache = "SIGSTORE_NO_CACHE" ) -// Global in-memory targets to avoid re-downloading when there is no local cache. -var memoryTargets = map[string][]byte{} +var ( + // singletonTUF holds a single instance of TUF that will get reused on + // subsequent invocations of initializeTUF + singletonTUF *TUF + singletonTUFMu sync.Mutex + // Global in-memory targets to avoid re-downloading when there is no local cache. + memoryTargets = map[string][]byte{} +) var GetRemoteRoot = func() string { return DefaultRemoteRoot @@ -103,6 +110,12 @@ type remoteCache struct { Mirror string `json:"mirror"` } +func resetForTests() { + singletonTUFMu.Lock() + singletonTUF = nil + singletonTUFMu.Unlock() +} + func getExpiration(metadata []byte) (*time.Time, error) { s := &data.Signed{} if err := json.Unmarshal(metadata, s); err != nil { @@ -211,15 +224,9 @@ func GetRootStatus(ctx context.Context) (*RootStatus, error) { if err != nil { return nil, err } - defer t.Close() return t.getRootStatus() } -// Close closes the local TUF store. Should only be called once per client. -func (t *TUF) Close() error { - return t.local.Close() -} - // initializeTUF creates a TUF client using the following params: // * embed: indicates using the embedded metadata and in-memory file updates. // When this is false, this uses a filesystem cache. @@ -231,6 +238,12 @@ func (t *TUF) Close() error { // * forceUpdate: indicates checking the remote for an update, even when the local // timestamp.json is up to date. func initializeTUF(ctx context.Context, mirror string, root []byte, embedded fs.FS, forceUpdate bool) (*TUF, error) { + singletonTUFMu.Lock() + defer singletonTUFMu.Unlock() + if singletonTUF != nil { + return singletonTUF, nil + } + t := &TUF{ mirror: mirror, embedded: embedded, @@ -245,7 +258,6 @@ func initializeTUF(ctx context.Context, mirror string, root []byte, embedded fs. t.remote, err = remoteFromMirror(ctx, t.mirror) if err != nil { - t.Close() return nil, err } @@ -253,7 +265,6 @@ func initializeTUF(ctx context.Context, mirror string, root []byte, embedded fs. trustedMeta, err := t.local.GetMeta() if err != nil { - t.Close() return nil, fmt.Errorf("getting trusted meta: %w", err) } @@ -262,28 +273,29 @@ func initializeTUF(ctx context.Context, mirror string, root []byte, embedded fs. if root == nil { root, err = getRoot(trustedMeta, t.embedded) if err != nil { - t.Close() return nil, fmt.Errorf("getting trusted root: %w", err) } } if err := t.client.InitLocal(root); err != nil { - t.Close() return nil, fmt.Errorf("unable to initialize client, local cache may be corrupt: %w", err) } // 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 { + // We're golden so stash the TUF object for later use + singletonTUF = t return t, nil } // Update if local is not populated or out of date. if err := t.updateMetadataAndDownloadTargets(); err != nil { - t.Close() return nil, fmt.Errorf("updating local metadata and targets: %w", err) } + // We're golden so stash the TUF object for later use + singletonTUF = t return t, err } @@ -304,11 +316,9 @@ func NewFromEnv(ctx context.Context) (*TUF, error) { func Initialize(ctx context.Context, mirror string, root []byte) error { // Initialize the client. Force an update. - t, err := initializeTUF(ctx, mirror, root, GetEmbedded(), true) - if err != nil { + if _, err := initializeTUF(ctx, mirror, root, GetEmbedded(), true); err != nil { return err } - t.Close() // Store the remote for later if we are caching. if !noCache() { @@ -565,7 +575,6 @@ func syncLocalMeta(from client.LocalStore, to client.LocalStore) error { return fmt.Errorf("syncing local store for metadata %s", k) } } - // We're done with this now, so close it. return nil } diff --git a/pkg/cosign/tuf/client_test.go b/pkg/cosign/tuf/client_test.go index e7ebb68faa88..6f5159b04c1c 100644 --- a/pkg/cosign/tuf/client_test.go +++ b/pkg/cosign/tuf/client_test.go @@ -29,6 +29,7 @@ import ( "reflect" "sort" "strings" + "sync" "testing" "testing/fstest" "time" @@ -61,7 +62,7 @@ func TestNewFromEnv(t *testing.T) { } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() // Now try with expired targets forceExpiration(t, true) @@ -70,7 +71,7 @@ func TestNewFromEnv(t *testing.T) { t.Fatal(err) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() if err := Initialize(ctx, DefaultRemoteRoot, nil); err != nil { t.Error() @@ -85,7 +86,7 @@ func TestNewFromEnv(t *testing.T) { t.Fatal(err) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() } func TestNoCache(t *testing.T) { @@ -101,7 +102,7 @@ func TestNoCache(t *testing.T) { t.Fatal(err) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() // Force expiration so we have some content to download forceExpiration(t, true) @@ -111,12 +112,13 @@ func TestNoCache(t *testing.T) { t.Fatal(err) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() // No filesystem writes when using SIGSTORE_NO_CACHE. if l := dirLen(t, td); l != 0 { t.Errorf("expected no filesystem writes, got %d entries", l) } + resetForTests() } func TestCache(t *testing.T) { @@ -137,7 +139,7 @@ func TestCache(t *testing.T) { t.Fatal(err) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() cachedDirLen := dirLen(t, td) if cachedDirLen == 0 { t.Errorf("expected filesystem writes, got %d entries", cachedDirLen) @@ -145,11 +147,11 @@ func TestCache(t *testing.T) { // Nothing should get downloaded if everything is up to date. forceExpiration(t, false) - tuf, err = NewFromEnv(ctx) + _, err = NewFromEnv(ctx) if err != nil { t.Fatal(err) } - tuf.Close() + resetForTests() if l := dirLen(t, td); cachedDirLen != l { t.Errorf("expected no filesystem writes, got %d entries", l-cachedDirLen) @@ -166,7 +168,7 @@ func TestCache(t *testing.T) { t.Errorf("expected filesystem writes, got %d entries", l) } checkTargetsAndMeta(t, tuf) - tuf.Close() + resetForTests() } func TestCustomRoot(t *testing.T) { @@ -205,7 +207,7 @@ func TestCustomRoot(t *testing.T) { if b, err := tufObj.GetTarget("foo.txt"); err != nil || !bytes.Equal(b, []byte("foo")) { t.Fatal(err) } - tufObj.Close() + resetForTests() // Force expiration on the first timestamp and internal go-tuf verification. forceExpirationVersion(t, 1) @@ -231,7 +233,7 @@ func TestCustomRoot(t *testing.T) { if b, err := tufObj.GetTarget("foo.txt"); err != nil || !bytes.Equal(b, []byte("foo1")) { t.Fatal(err) } - tufObj.Close() + resetForTests() } func TestGetTargetsByMeta(t *testing.T) { @@ -266,7 +268,7 @@ func TestGetTargetsByMeta(t *testing.T) { if err != nil { t.Fatal(err) } - defer tufObj.Close() + defer resetForTests() // Fetch a target with no custom metadata. targets, err := tufObj.GetTargetsByMeta(UnknownUsage, []string{"fooNoCustom.txt"}) if err != nil { @@ -402,7 +404,7 @@ func TestUpdatedTargetNamesEmbedded(t *testing.T) { if err != nil { t.Fatal(err) } - defer tufObj.Close() + defer resetForTests() // Try to retrieve the newly added target. targets, err := tufObj.GetTargetsByMeta(Fulcio, []string{"fooNoCustom.txt"}) @@ -622,3 +624,22 @@ func updateTufRepo(t *testing.T, td string, r *tuf.Repo, targetData string) { t.Error(err) } } +func TestConcurrentAccess(t *testing.T) { + var wg sync.WaitGroup + + for i := 0; i < 20; i++ { + wg.Add(1) + go func() { + defer wg.Done() + tufObj, err := NewFromEnv(context.Background()) + if err != nil { + t.Errorf("Failed to construct NewFromEnv: %s", err) + } + if tufObj == nil { + t.Error("Got back nil tufObj") + } + time.Sleep(1 * time.Second) + }() + } + wg.Wait() +}