From 0e5cf24f729b3be140f4c73f218b40682289f01d Mon Sep 17 00:00:00 2001 From: James Carnegie Date: Wed, 5 Nov 2025 15:09:59 +0000 Subject: [PATCH 1/2] test: add failing tests for multiple Refresh() calls Add test cases that demonstrate the current limitation where Refresh() can only be called once during an Updater's lifetime. - TestMultipleRefreshCalls: Tests calling Refresh() multiple times with metadata updates between calls - TestMultipleRefreshCallsNoChanges: Tests calling Refresh() multiple times when no changes are available These tests currently fail with "cannot update timestamp after snapshot" errors due to TrustedMetadata state machine restrictions. Related to #593 Signed-off-by: James Carnegie --- .../updater/updater_top_level_update_test.go | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/metadata/updater/updater_top_level_update_test.go b/metadata/updater/updater_top_level_update_test.go index 214105b4..6930d13f 100644 --- a/metadata/updater/updater_top_level_update_test.go +++ b/metadata/updater/updater_top_level_update_test.go @@ -1081,3 +1081,81 @@ func TestTimestampEqVersionsCheck(t *testing.T) { assert.NoError(t, err) assert.Equal(t, initialTimestampMetadataVer, timestamp.Signed.Meta["snapshot.json"].Version) } + +func TestMultipleRefreshCalls(t *testing.T) { + // Test that Refresh() can be called multiple times on the same Updater instance + // and that it successfully updates metadata when changes are available. + + err := loadOrResetTrustedRootMetadata() + assert.NoError(t, err) + + updaterConfig, err := loadUpdaterConfig() + assert.NoError(t, err) + + // Create an updater and perform first refresh + updater := initUpdater(updaterConfig) + err = updater.Refresh() + assert.NoError(t, err) + + // Verify initial versions + assertVersionEquals(t, metadata.TIMESTAMP, 1) + assertVersionEquals(t, metadata.SNAPSHOT, 1) + assertVersionEquals(t, metadata.TARGETS, 1) + + // Update metadata on the repository + simulator.Sim.MDTargets.Signed.Version += 1 + simulator.Sim.UpdateSnapshot() + + // Call Refresh() again on the same updater instance + err = updater.Refresh() + assert.NoError(t, err) + + // Verify that metadata was updated + assertVersionEquals(t, metadata.TIMESTAMP, 2) + assertVersionEquals(t, metadata.SNAPSHOT, 2) + assertVersionEquals(t, metadata.TARGETS, 2) + + // Update metadata again + simulator.Sim.MDTargets.Signed.Version += 1 + simulator.Sim.UpdateSnapshot() + + // Call Refresh() a third time + err = updater.Refresh() + assert.NoError(t, err) + + // Verify that metadata was updated again + assertVersionEquals(t, metadata.TIMESTAMP, 3) + assertVersionEquals(t, metadata.SNAPSHOT, 3) + assertVersionEquals(t, metadata.TARGETS, 3) +} + +func TestMultipleRefreshCallsNoChanges(t *testing.T) { + // Test that Refresh() can be called multiple times even when there are no changes + // and returns nil (not an error) when everything is already up-to-date. + + err := loadOrResetTrustedRootMetadata() + assert.NoError(t, err) + + updaterConfig, err := loadUpdaterConfig() + assert.NoError(t, err) + + // Create an updater and perform first refresh + updater := initUpdater(updaterConfig) + err = updater.Refresh() + assert.NoError(t, err) + + // Verify initial versions + assertVersionEquals(t, metadata.TIMESTAMP, 1) + assertVersionEquals(t, metadata.SNAPSHOT, 1) + assertVersionEquals(t, metadata.TARGETS, 1) + + // Call Refresh() again without any changes + // Should return nil (no error) since everything is up-to-date + err = updater.Refresh() + assert.NoError(t, err) + + // Verify versions haven't changed + assertVersionEquals(t, metadata.TIMESTAMP, 1) + assertVersionEquals(t, metadata.SNAPSHOT, 1) + assertVersionEquals(t, metadata.TARGETS, 1) +} From 12ae6337f5ce46a3c76be59746314d8639f5a562 Mon Sep 17 00:00:00 2001 From: James Carnegie Date: Wed, 5 Nov 2025 15:10:33 +0000 Subject: [PATCH 2/2] feat: enable Refresh() to be called multiple times Allow Refresh() to be called multiple times during the lifetime of an Updater by removing workflow guards that prevented re-updating metadata after dependent metadata was loaded. The Update* methods in TrustedMetadata already handle atomic updates, version checking, and rollback protection. The guards were only needed to enforce workflow order within a single refresh cycle, but prevented multiple refresh cycles. This enables long-running processes to periodically refresh metadata without creating new Updater instances. Updates are atomic and thread-safe - metadata is never cleared, only replaced after validation. Fixes #593 Signed-off-by: James Carnegie --- metadata/trustedmetadata/trustedmetadata.go | 9 ------ .../trustedmetadata/trustedmetadata_test.go | 15 +++++---- metadata/updater/updater.go | 31 ++++++++++++------- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/metadata/trustedmetadata/trustedmetadata.go b/metadata/trustedmetadata/trustedmetadata.go index 0726f31f..2b605a2b 100644 --- a/metadata/trustedmetadata/trustedmetadata.go +++ b/metadata/trustedmetadata/trustedmetadata.go @@ -57,9 +57,6 @@ func New(rootData []byte) (*TrustedMetadata, error) { func (trusted *TrustedMetadata) UpdateRoot(rootData []byte) (*metadata.Metadata[metadata.RootType], error) { log := metadata.GetLogger() - if trusted.Timestamp != nil { - return nil, &metadata.ErrRuntime{Msg: "cannot update root after timestamp"} - } log.Info("Updating root") // generate root metadata newRoot, err := metadata.Root().FromBytes(rootData) @@ -99,9 +96,6 @@ func (trusted *TrustedMetadata) UpdateRoot(rootData []byte) (*metadata.Metadata[ func (trusted *TrustedMetadata) UpdateTimestamp(timestampData []byte) (*metadata.Metadata[metadata.TimestampType], error) { log := metadata.GetLogger() - if trusted.Snapshot != nil { - return nil, &metadata.ErrRuntime{Msg: "cannot update timestamp after snapshot"} - } // client workflow 5.3.10: Make sure final root is not expired. if trusted.Root.Signed.IsExpired(trusted.RefTime) { // no need to check for 5.3.11 (fast forward attack recovery): @@ -178,9 +172,6 @@ func (trusted *TrustedMetadata) UpdateSnapshot(snapshotData []byte, isTrusted bo if trusted.Timestamp == nil { return nil, &metadata.ErrRuntime{Msg: "cannot update snapshot before timestamp"} } - if trusted.Targets[metadata.TARGETS] != nil { - return nil, &metadata.ErrRuntime{Msg: "cannot update snapshot after targets"} - } log.Info("Updating snapshot") // snapshot cannot be loaded if final timestamp is expired diff --git a/metadata/trustedmetadata/trustedmetadata_test.go b/metadata/trustedmetadata/trustedmetadata_test.go index 86afdddd..a538fa94 100644 --- a/metadata/trustedmetadata/trustedmetadata_test.go +++ b/metadata/trustedmetadata/trustedmetadata_test.go @@ -227,9 +227,10 @@ func TestOutOfOrderOps(t *testing.T) { _, err = trustedSet.UpdateTimestamp(allRoles[metadata.TIMESTAMP]) assert.NoError(t, err) - // Update root after timestamp + // Update root after timestamp is now allowed (for multiple Refresh() calls) + // but will fail due to version number check (expects v2, has v1) _, err = trustedSet.UpdateRoot(allRoles[metadata.ROOT]) - assert.ErrorIs(t, err, &metadata.ErrRuntime{Msg: "cannot update root after timestamp"}) + assert.ErrorIs(t, err, &metadata.ErrBadVersionNumber{Msg: "bad version number, expected 2, got 1"}) // Update targets before snapshot _, err = trustedSet.UpdateTargets(allRoles[metadata.TARGETS]) @@ -238,9 +239,10 @@ func TestOutOfOrderOps(t *testing.T) { _, err = trustedSet.UpdateSnapshot(allRoles[metadata.SNAPSHOT], false) assert.NoError(t, err) - // Update timestamp after snapshot + // Update timestamp after snapshot is now allowed (for multiple Refresh() calls) + // and will succeed since versions are equal _, err = trustedSet.UpdateTimestamp(allRoles[metadata.TIMESTAMP]) - assert.ErrorIs(t, err, &metadata.ErrRuntime{Msg: "cannot update timestamp after snapshot"}) + assert.ErrorIs(t, err, &metadata.ErrEqualVersionNumber{Msg: "new timestamp version 1 equals the old one 1"}) // Update delegated targets before targets _, err = trustedSet.UpdateDelegatedTargets(allRoles["role1"], "role1", metadata.TARGETS) @@ -249,9 +251,10 @@ func TestOutOfOrderOps(t *testing.T) { _, err = trustedSet.UpdateTargets(allRoles[metadata.TARGETS]) assert.NoError(t, err) - // Update snapshot after sucessful targets update + // Update snapshot after targets is now allowed (for multiple Refresh() calls) + // and will succeed since versions are equal _, err = trustedSet.UpdateSnapshot(allRoles[metadata.SNAPSHOT], false) - assert.ErrorIs(t, err, &metadata.ErrRuntime{Msg: "cannot update snapshot after targets"}) + assert.NoError(t, err) _, err = trustedSet.UpdateDelegatedTargets(allRoles["role1"], "role1", metadata.TARGETS) assert.NoError(t, err) diff --git a/metadata/updater/updater.go b/metadata/updater/updater.go index bd533b63..5d9310d8 100644 --- a/metadata/updater/updater.go +++ b/metadata/updater/updater.go @@ -102,7 +102,9 @@ func New(config *config.UpdaterConfig) (*Updater, error) { // Downloads, verifies, and loads metadata for the top-level roles in the // specified order (root -> timestamp -> snapshot -> targets) implementing // all the checks required in the TUF client workflow. -// A Refresh() can be done only once during the lifetime of an Updater. +// Refresh() can be called multiple times during the lifetime of an Updater +// to ensure that the metadata is up-to-date. Each call will reload the +// timestamp, snapshot, and targets metadata while preserving the root metadata. // If Refresh() has not been explicitly called before the first // GetTargetInfo() call, it will be done implicitly at that time. // The metadata for delegated roles is not updated by Refresh(): @@ -135,6 +137,9 @@ func (update *Updater) onlineRefresh() error { if err != nil { return err } + // Remove the targets entry to allow re-loading during multiple Refresh() calls + // while still preventing redundant loads during GetTargetInfo() + delete(update.trusted.Targets, metadata.TARGETS) _, err = update.loadTargets(metadata.TARGETS, metadata.ROOT) if err != nil { return err @@ -313,12 +318,16 @@ func (update *Updater) loadTimestamp() error { if errors.Is(err, &metadata.ErrRepository{}) { // local timestamp is not valid, proceed downloading from remote; note that this error type includes several other subset errors log.Info("Local timestamp is not valid") + } else if errors.Is(err, &metadata.ErrEqualVersionNumber{}) { + // local timestamp version equals current trusted version, proceed to check remote for updates + log.Info("Local timestamp version equals trusted version") } else { // another error return err } + } else { + log.Info("Local timestamp is valid") } - log.Info("Local timestamp is valid") // all okay, local timestamp exists and it is valid, nevertheless proceed with downloading from remote } // load from remote (whether local load succeeded or not) @@ -368,12 +377,12 @@ func (update *Updater) loadSnapshot() error { } } else { // this means snapshot verification/loading succeeded - log.Info("Local snapshot is valid: not downloading new one") - return nil + log.Info("Local snapshot is valid") + // Continue to check remote for potential updates } } - // local snapshot does not exist or is invalid, update from remote - log.Info("Failed to load local snapshot") + // check remote for updates (whether local load succeeded or not) + log.Info("Checking remote for snapshot updates") if update.trusted.Timestamp == nil { return fmt.Errorf("trusted timestamp not set") } @@ -422,7 +431,7 @@ func (update *Updater) loadTargets(roleName, parentName string) (*metadata.Metad log.Info("Local role does not exist", "role", roleName) } else { // successfully read a local targets metadata, so let's try to verify and load it to the trusted metadata set - delegatedTargets, err := update.trusted.UpdateDelegatedTargets(data, roleName, parentName) + _, err := update.trusted.UpdateDelegatedTargets(data, roleName, parentName) if err != nil { // this means targets verification/loading failed if errors.Is(err, &metadata.ErrRepository{}) { @@ -434,12 +443,12 @@ func (update *Updater) loadTargets(roleName, parentName string) (*metadata.Metad } } else { // this means targets verification/loading succeeded - log.Info("Local role is valid: not downloading new one", "role", roleName) - return delegatedTargets, nil + log.Info("Local role is valid", "role", roleName) + // Continue to check remote for potential updates } } - // local "roleName" does not exist or is invalid, update from remote - log.Info("Failed to load local role", "role", roleName) + // check remote for updates (whether local load succeeded or not) + log.Info("Checking remote for role updates", "role", roleName) if update.trusted.Snapshot == nil { return nil, fmt.Errorf("trusted snapshot not set") }