diff --git a/client/client_update_test.go b/client/client_update_test.go index b7f509496d..a9ac2b480c 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -2,15 +2,21 @@ package client import ( "bytes" + "encoding/json" "fmt" "io/ioutil" "net/http" "net/http/httptest" "os" + "reflect" "testing" + "time" + "github.com/docker/notary/certs" "github.com/docker/notary/passphrase" + "github.com/docker/notary/tuf/client" "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/store" "github.com/docker/notary/tuf/testutils" "github.com/gorilla/mux" @@ -172,26 +178,30 @@ func TestUpdateSucceedsEvenIfCannotWriteExistingRepo(t *testing.T) { } } -type messUpMetadata func(role string) error +type swizzleFunc func(*testutils.MetadataSwizzler, string) error +type swizzleExpectations struct { + desc string + swizzle swizzleFunc + expectErrs []interface{} +} -func waysToMessUpLocalMetadata(repoSwizzler *testutils.MetadataSwizzler) map[string]messUpMetadata { - return map[string]messUpMetadata{ - // for instance if the metadata got truncated or otherwise block corrupted - "invalid JSON": repoSwizzler.SetInvalidJSON, - // if the metadata was accidentally deleted - "missing metadata": repoSwizzler.RemoveMetadata, - // if the signature was invalid - maybe the user tried to modify something manually - // that they forgot (add a key, or something) - "signed with right key but wrong hash": repoSwizzler.InvalidateMetadataSignatures, - // if the user copied the wrong root.json over it by accident or something - "signed with wrong key": repoSwizzler.SignMetadataWithInvalidKey, - // self explanatory - "expired": repoSwizzler.ExpireMetadata, +var waysToMessUpLocalMetadata = []swizzleExpectations{ + // for instance if the metadata got truncated or otherwise block corrupted + {desc: "invalid JSON", swizzle: (*testutils.MetadataSwizzler).SetInvalidJSON}, + // if the metadata was accidentally deleted + {desc: "missing metadata", swizzle: (*testutils.MetadataSwizzler).RemoveMetadata}, + // if the signature was invalid - maybe the user tried to modify something manually + // that they forgot (add a key, or something) + {desc: "signed with right key but wrong hash", + swizzle: (*testutils.MetadataSwizzler).InvalidateMetadataSignatures}, + // if the user copied the wrong root.json over it by accident or something + {desc: "signed with wrong key", swizzle: (*testutils.MetadataSwizzler).SignMetadataWithInvalidKey}, + // self explanatory + {desc: "expired metadata", swizzle: (*testutils.MetadataSwizzler).ExpireMetadata}, - // Not trying any of the other repoSwizzler methods, because those involve modifying - // and re-serializing, and that means a user has the root and other keys and was trying to - // actively sabotage and break their own local repo (particularly the root.json) - } + // Not trying any of the other repoSwizzler methods, because those involve modifying + // and re-serializing, and that means a user has the root and other keys and was trying to + // actively sabotage and break their own local repo (particularly the root.json) } // If a repo has corrupt metadata (in that the hash doesn't match the snapshot) or @@ -217,9 +227,10 @@ func TestUpdateReplacesCorruptOrMissingMetadata(t *testing.T) { repoSwizzler.MetadataCache = repo.fileStore for _, role := range repoSwizzler.Roles { - for text, messItUp := range waysToMessUpLocalMetadata(repoSwizzler) { + for _, expt := range waysToMessUpLocalMetadata { + text, messItUp := expt.desc, expt.swizzle for _, forWrite := range []bool{true, false} { - require.NoError(t, messItUp(role), "could not fuzz %s (%s)", role, text) + require.NoError(t, messItUp(repoSwizzler, role), "could not fuzz %s (%s)", role, text) _, err := repo.Update(forWrite) require.NoError(t, err) for r, expected := range serverMeta { @@ -267,9 +278,10 @@ func TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign(t *testing.T) { Roles: serverSwizzler.Roles, } - for text, messItUp := range waysToMessUpLocalMetadata(repoSwizzler) { + for _, expt := range waysToMessUpLocalMetadata { + text, messItUp := expt.desc, expt.swizzle for _, forWrite := range []bool{true, false} { - require.NoError(t, messItUp(data.CanonicalRootRole), "could not fuzz root (%s)", text) + require.NoError(t, messItUp(repoSwizzler, data.CanonicalRootRole), "could not fuzz root (%s)", text) messedUpMeta, err := repo.fileStore.GetMeta(data.CanonicalRootRole, -1) if _, ok := err.(store.ErrMetaNotFound); ok { // one of the ways to mess up is to delete metadata @@ -450,6 +462,9 @@ func TestUpdateRemoteRoot50XCannotUseLocalCache(t *testing.T) { // but is missing other data, then we propagate the ErrMetaNotFound. Skipping // force check, because that only matters for root. func TestUpdateNonRootRemoteMissingMetadataNoLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { if role == data.CanonicalRootRole { continue @@ -468,6 +483,9 @@ func TestUpdateNonRootRemoteMissingMetadataNoLocalCache(t *testing.T) { // nothing needs to be downloaded. // Skipping force check, because that only matters for root. func TestUpdateNonRootRemoteMissingMetadataCanUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } // really we can delete everything at once except for the timestamp, but // it's better to check one by one in case we change the download code // somewhat. @@ -488,6 +506,9 @@ func TestUpdateNonRootRemoteMissingMetadataCanUseLocalCache(t *testing.T) { // use the local cache so if the server is missing any metadata we cannot update. // Skipping force check, because that only matters for root. func TestUpdateNonRootRemoteMissingMetadataCannotUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { if role == data.CanonicalRootRole { continue @@ -513,6 +534,9 @@ func TestUpdateNonRootRemoteMissingMetadataCannotUseLocalCache(t *testing.T) { // If there is no local cache, we just update. If the server 50X's when getting // metadata, we propagate ErrServerUnavailable. func TestUpdateNonRootRemote50XNoLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { if role == data.CanonicalRootRole { continue @@ -530,6 +554,9 @@ func TestUpdateNonRootRemote50XNoLocalCache(t *testing.T) { // If the timestamp is present, but the same, we already have all the data, so // nothing needs to be downloaded. func TestUpdateNonRootRemote50XCanUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } // actually everything can error at once, but it's better to check one by // one in case we change the download code somewhat. for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { @@ -550,6 +577,9 @@ func TestUpdateNonRootRemote50XCanUseLocalCache(t *testing.T) { // This happens whether or not we force a remote check (because that's on the // root) func TestUpdateNonRootRemote50XCannotUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { if role == data.CanonicalRootRole { continue @@ -606,3 +636,440 @@ func testUpdateRemoteNon200Error(t *testing.T, opts updateOpts, errExpected inte } } } + +// If there's no local cache, we go immediately to check the remote server for +// root. If the root is corrupted in transit in such a way that the signature is +// wrong, but it is correct in all other ways, then it validates during bootstrap, +// but it will fail validation during update. So it will fail with or without +// a force check (update for write). If any of the other roles (except +// timestamp, because there is no checksum for that) are corrupted in the same +// way, they will also fail during update with the same error. +func TestUpdateRemoteChecksumWrongNoLocalCache(t *testing.T) { + for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: false, + localCache: false, + forWrite: false, + role: role, + }, role != data.CanonicalTimestampRole) // timestamp role should not fail + + if role == data.CanonicalRootRole { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: false, + localCache: false, + forWrite: true, + role: role, + }, true) + } + } +} + +// If there's is a local cache, and the remote server has the same data (except +// corrupted), then we can just use our local cache. So update succeeds ( +// with or without a force check (update for write)) +func TestUpdateRemoteChecksumWrongCanUseLocalCache(t *testing.T) { + for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: false, + localCache: true, + forWrite: false, + role: role, + }, false) + + if role == data.CanonicalRootRole { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: false, + localCache: true, + forWrite: true, + role: role, + }, false) + } + } +} + +// If there's is a local cache, but the remote server has new data (some +// corrupted), we go immediately to check the remote server for root. If the +// root is corrupted in transit in such a way that the signature is wrong, but +// it is correct in all other ways, it from validates during bootstrap, +// but it will fail validation during update. So it will fail with or without +// a force check (update for write). If any of the other roles (except +// timestamp, because there is no checksum for that) is corrupted in the same +// way, they will also fail during update with the same error. +func TestUpdateRemoteChecksumWrongCannotUseLocalCache(t *testing.T) { + for _, role := range append(data.BaseRoles, delegationsWithNonEmptyMetadata...) { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: true, + localCache: true, + forWrite: false, + role: role, + }, role != data.CanonicalTimestampRole) // timestamp role should not fail + + if role == data.CanonicalRootRole { + testUpdateRemoteFileChecksumWrong(t, updateOpts{ + serverHasNewData: true, + localCache: true, + forWrite: true, + role: role, + }, true) + } + } +} + +func testUpdateRemoteFileChecksumWrong(t *testing.T, opts updateOpts, errExpected bool) { + _, serverSwizzler := newServerSwizzler(t) + ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound) + defer ts.Close() + + repo := newBlankRepo(t, ts.URL) + defer os.RemoveAll(repo.baseDir) + + if opts.localCache { + _, err := repo.Update(false) // acquire local cache + require.NoError(t, err) + } + + if opts.serverHasNewData { + bumpVersions(t, serverSwizzler, 1) + } + + require.NoError(t, serverSwizzler.AddExtraSpace(opts.role), "failed to checksum-corrupt to %s", opts.role) + + _, err := repo.Update(opts.forWrite) + if !errExpected { + require.NoError(t, err, "expected no failure updating when %s has the wrong checksum (forWrite: %v)", + opts.role, opts.forWrite) + } else { + require.Error(t, err, "expected failure updating when %s has the wrong checksum (forWrite: %v)", + opts.role, opts.forWrite) + + // it could be ErrMaliciousServer (if the server sent the metadata with a content length) + // or a checksum error (if the server didn't set content length because transfer-encoding + // was specified). For the timestamp, which is really short, it should be the content-length. + + var rightError bool + if opts.role == data.CanonicalTimestampRole { + _, rightError = err.(store.ErrMaliciousServer) + } else { + _, isErrChecksum := err.(client.ErrChecksumMismatch) + _, isErrMaliciousServer := err.(store.ErrMaliciousServer) + rightError = isErrChecksum || isErrMaliciousServer + } + require.True(t, rightError, err, + "wrong update error (%v) when %s has the wrong checksum (forWrite: %v)", + err, opts.role, opts.forWrite) + } +} + +// --- these tests below assume the checksums are correct (since the server can sign snapshots and +// timestamps, so can be malicious) --- + +// this does not include delete, which is tested separately so we can try to get +// 404s and 503s +var waysToMessUpServer = []swizzleExpectations{ + {desc: "invalid JSON", expectErrs: []interface{}{&json.SyntaxError{}}, + swizzle: (*testutils.MetadataSwizzler).SetInvalidJSON}, + + {desc: "an invalid Signed", expectErrs: []interface{}{&json.UnmarshalTypeError{}}, + swizzle: (*testutils.MetadataSwizzler).SetInvalidSigned}, + + {desc: "an invalid SignedMeta", + // it depends which field gets unmarshalled first + expectErrs: []interface{}{&json.UnmarshalTypeError{}, &time.ParseError{}}, + swizzle: (*testutils.MetadataSwizzler).SetInvalidSignedMeta}, + + // for the errors below, when we bootstrap root, we get cert.ErrValidationFail failures + // for everything else, the errors come from tuf/signed + + {desc: "invalid SignedMeta Type", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrWrongType}, + swizzle: (*testutils.MetadataSwizzler).SetInvalidMetadataType}, + + {desc: "invalid signatures", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, + swizzle: (*testutils.MetadataSwizzler).InvalidateMetadataSignatures}, + + {desc: "meta signed by wrong key", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, + swizzle: (*testutils.MetadataSwizzler).SignMetadataWithInvalidKey}, + + {desc: "expired metadata", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrExpired{}}, + swizzle: (*testutils.MetadataSwizzler).ExpireMetadata}, + + {desc: "lower metadata version", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrLowVersion{}}, + swizzle: func(s *testutils.MetadataSwizzler, role string) error { + return s.OffsetMetadataVersion(role, -3) + }}, + + {desc: "insufficient signatures", expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, + swizzle: func(s *testutils.MetadataSwizzler, role string) error { + return s.SetThreshold(role, 2) + }}, +} + +// If there's no local cache, we go immediately to check the remote server for +// root, and if it invalid (corrupted), we cannot update. This happens +// with and without a force check (update for write). +func TestUpdateRootRemoteCorruptedNoLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, testData := range waysToMessUpServer { + if testData.desc == "insufficient signatures" { + // Currently if we download the root during the bootstrap phase, + // we don't check for enough signatures to meet the threshold. We + // are also not entirely sure if we want to support threshold. + continue + } + + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + forWrite: false, + role: data.CanonicalRootRole, + }, testData, true) + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + forWrite: true, + role: data.CanonicalRootRole, + }, testData, true) + } +} + +// Having a local cache, if the server has the same data (timestamp has not changed), +// should succeed in all cases if whether forWrite (force check) is true or not +// because the fact that the timestamp hasn't changed should mean that we don't +// have to re-download the root. +func TestUpdateRootRemoteCorruptedCanUseLocalCache(t *testing.T) { + for _, testData := range waysToMessUpServer { + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + localCache: true, + forWrite: false, + role: data.CanonicalRootRole, + }, testData, false) + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + localCache: true, + forWrite: true, + role: data.CanonicalRootRole, + }, testData, false) + } +} + +// Having a local cache, if the server has new same data should fail in all cases +// because the metadata is re-downloaded. +func TestUpdateRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, testData := range waysToMessUpServer { + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + serverHasNewData: true, + localCache: true, + forWrite: false, + role: data.CanonicalRootRole, + }, testData, true) + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + serverHasNewData: true, + localCache: true, + forWrite: true, + role: data.CanonicalRootRole, + }, testData, true) + } +} + +// If there's no local cache, we just download from the server and if anything +// is corrupt, we cannot update. +func TestUpdateNonRootRemoteCorruptedNoLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, role := range append(data.BaseRoles, "targets/a", "targets/a/b") { + if role == data.CanonicalRootRole { + continue + } + for _, testData := range waysToMessUpServer { + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + role: role, + }, testData, true) + } + } +} + +// Having a local cache, if the server has the same data (timestamp has not changed), +// should succeed in all cases if whether forWrite (force check) is true or not. +// If the timestamp is fine, it hasn't changed and we don't have to download +// anything. If it's broken, we used the cached timestamp and again download +// nothing. +func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, role := range append(data.BaseRoles, "targets/a", "targets/a/b") { + if role == data.CanonicalRootRole { + continue + } + for _, testData := range waysToMessUpServer { + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + localCache: true, + role: role, + }, testData, false) + } + } +} + +// Having a local cache, if the server has new same data should fail in all cases +// (except if we modify the timestamp) because the metadata is re-downloaded. +// In the case of the timestamp, we'd default to our cached timestamp, and +// not have to redownload anything (usually) +func TestUpdateNonRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, role := range append(data.BaseRoles, "targets/a", "targets/a/b") { + if role == data.CanonicalRootRole { + continue + } + for _, testData := range waysToMessUpServer { + // in general the cached timsestamp will always succeed, but if the threshold has been + // increased, it fails because when we download the new timestamp, it validates as per our + // previous root. But the root hash doesn't match. So we download a new root and + // try the update again. In this case, both the old and new timestamps won't have enough + // signatures. + shouldFail := role != data.CanonicalTimestampRole || testData.desc == "insufficient signatures" + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + serverHasNewData: true, + localCache: true, + role: role, + }, testData, shouldFail) + } + } +} + +func testUpdateRemoteCorruptValidChecksum(t *testing.T, opts updateOpts, expt swizzleExpectations, shouldErr bool) { + _, serverSwizzler := newServerSwizzler(t) + ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound) + defer ts.Close() + + repo := newBlankRepo(t, ts.URL) + defer os.RemoveAll(repo.baseDir) + + if opts.localCache { + _, err := repo.Update(false) + require.NoError(t, err) + } + + if opts.serverHasNewData { + bumpVersions(t, serverSwizzler, 1) + } + + msg := fmt.Sprintf("swizzling %s to return: %v (forWrite: %v)", opts.role, expt.desc, opts.forWrite) + + require.NoError(t, expt.swizzle(serverSwizzler, opts.role), + "failed %s", msg) + + // update the snapshot and timestamp hashes to make sure it's not an involuntary checksum failure + // unless we want the server to not actually have any new data + if !opts.localCache || opts.serverHasNewData { + // we don't want to sign if we are trying to swizzle one of these roles to + // have a different signature - updating hashes would be pointless (because + // nothing else has changed) and would just overwrite the signature. + isSignatureSwizzle := expt.desc == "invalid signatures" || expt.desc == "meta signed by wrong key" + // just try to do these - if they fail (probably because they've been swizzled), that's fine + if opts.role != data.CanonicalSnapshotRole || !isSignatureSwizzle { + serverSwizzler.UpdateSnapshotHashes() + } + if opts.role != data.CanonicalTimestampRole || !isSignatureSwizzle { + serverSwizzler.UpdateTimestampHash() + } + } + _, err := repo.Update(opts.forWrite) + if shouldErr { + require.Error(t, err, "expected failure updating when %s", msg) + + errType := reflect.TypeOf(err) + isExpectedType := false + var expectedTypes []string + for _, expectErr := range expt.expectErrs { + expectedType := reflect.TypeOf(expectErr) + isExpectedType = isExpectedType || errType == expectedType + expectedTypes = append(expectedTypes, expectedType.String()) + } + require.True(t, isExpectedType, "expected one of %v when %s: got %s", + expectedTypes, msg, errType) + + } else { + require.NoError(t, err, "expected no failure updating when %s", msg) + } +} + +// If the local root is corrupt, and the remote root is corrupt, we should fail +// to update. Note - this one is really slow. +func TestUpdateLocalAndRemoteRootCorrupt(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + for _, localExpt := range waysToMessUpLocalMetadata { + for _, serverExpt := range waysToMessUpServer { + if localExpt.desc == "expired metadata" && serverExpt.desc == "lower metadata version" { + // TODO: bug right now where if the local metadata is invalid, we just download a + // new version - we verify the signatures and everything, but don't check the version + // against the previous if we can + continue + } + if serverExpt.desc == "insufficient signatures" { + // Currently if we download the root during the bootstrap phase, + // we don't check for enough signatures to meet the threshold. + // We are also not sure if we want to support thresholds. + continue + } + testUpdateLocalAndRemoteRootCorrupt(t, true, localExpt, serverExpt) + testUpdateLocalAndRemoteRootCorrupt(t, false, localExpt, serverExpt) + } + } +} + +func testUpdateLocalAndRemoteRootCorrupt(t *testing.T, forWrite bool, localExpt, serverExpt swizzleExpectations) { + _, serverSwizzler := newServerSwizzler(t) + ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound) + defer ts.Close() + + repo := newBlankRepo(t, ts.URL) + defer os.RemoveAll(repo.baseDir) + + // get local cache + _, err := repo.Update(false) + require.NoError(t, err) + repoSwizzler := &testutils.MetadataSwizzler{ + Gun: serverSwizzler.Gun, + MetadataCache: repo.fileStore, + CryptoService: serverSwizzler.CryptoService, + Roles: serverSwizzler.Roles, + } + + bumpVersions(t, serverSwizzler, 1) + + require.NoError(t, localExpt.swizzle(repoSwizzler, data.CanonicalRootRole), + "failed to swizzle local root to %s", localExpt.desc) + require.NoError(t, serverExpt.swizzle(serverSwizzler, data.CanonicalRootRole), + "failed to swizzle remote root to %s", serverExpt.desc) + + // update the hashes on both + require.NoError(t, serverSwizzler.UpdateSnapshotHashes()) + require.NoError(t, serverSwizzler.UpdateTimestampHash()) + + msg := fmt.Sprintf("swizzling root locally to return <%v> and remotely to return: <%v> (forWrite: %v)", + localExpt.desc, serverExpt.desc, forWrite) + + _, err = repo.Update(forWrite) + require.Error(t, err, "expected failure updating when %s", msg) + + errType := reflect.TypeOf(err) + isExpectedType := false + var expectedTypes []string + for _, expectErr := range serverExpt.expectErrs { + expectedType := reflect.TypeOf(expectErr) + isExpectedType = isExpectedType || errType == expectedType + expectedTypes = append(expectedTypes, expectedType.String()) + } + require.True(t, isExpectedType, "expected one of %v when %s: got %s", + expectedTypes, msg, errType) +} diff --git a/tuf/client/client.go b/tuf/client/client.go index 8bed27be25..786ea37599 100644 --- a/tuf/client/client.go +++ b/tuf/client/client.go @@ -249,9 +249,9 @@ func (c *Client) downloadTimestamp() error { // we're interacting with the repo. This will result in the // version being 0 var ( - saveToCache bool - old *data.Signed - version = 0 + old *data.Signed + ts *data.SignedTimestamp + version = 0 ) cachedTS, err := c.cache.GetMeta(role, notary.MaxTimestampSize) if err == nil { @@ -268,37 +268,39 @@ func (c *Client) downloadTimestamp() error { // unlike root, targets and snapshot, always try and download timestamps // from remote, only using the cache one if we couldn't reach remote. raw, s, err := c.downloadSigned(role, notary.MaxTimestampSize, nil) - if err != nil || len(raw) == 0 { - if old == nil { - if err == nil { - // couldn't retrieve data from server and don't have valid - // data in cache. - return store.ErrMetaNotFound{Resource: data.CanonicalTimestampRole} - } - return err + if err == nil { + ts, err = c.verifyTimestamp(s, version, c.keysDB) + if err == nil { + logrus.Debug("successfully verified downloaded timestamp") + c.cache.SetMeta(role, raw) + c.local.SetTimestamp(ts) + return nil } - logrus.Debug(err.Error()) - logrus.Warn("Error while downloading remote metadata, using cached timestamp - this might not be the latest version available remotely") - s = old - } else { - saveToCache = true } - err = signed.Verify(s, role, version, c.keysDB) - if err != nil { - return err - } - logrus.Debug("successfully verified timestamp") - if saveToCache { - c.cache.SetMeta(role, raw) - } - ts, err := data.TimestampFromSigned(s) + if old == nil { + // couldn't retrieve valid data from server and don't have unmarshallable data in cache. + logrus.Debug("no cached timestamp available") + return err + } + logrus.Debug(err.Error()) + logrus.Warn("Error while downloading remote metadata, using cached timestamp - this might not be the latest version available remotely") + ts, err = c.verifyTimestamp(old, version, c.keysDB) if err != nil { return err } + logrus.Debug("successfully verified cached timestamp") c.local.SetTimestamp(ts) return nil } +// verifies that a timestamp is valid, and returned the SignedTimestamp object to add to the tuf repo +func (c *Client) verifyTimestamp(s *data.Signed, minVersion int, kdb *keys.KeyDB) (*data.SignedTimestamp, error) { + if err := signed.Verify(s, data.CanonicalTimestampRole, minVersion, kdb); err != nil { + return nil, err + } + return data.TimestampFromSigned(s) +} + // downloadSnapshot is responsible for downloading the snapshot.json func (c *Client) downloadSnapshot() error { logrus.Debug("Downloading Snapshot...") diff --git a/tuf/client/client_test.go b/tuf/client/client_test.go index 1c0da9c7ab..cf4fd81d80 100644 --- a/tuf/client/client_test.go +++ b/tuf/client/client_test.go @@ -819,3 +819,96 @@ func TestTargetMeta(t *testing.T) { assert.Equal(t, &f, fileMeta) assert.Equal(t, "targets/level1/a/i", role) } + +// If there is no local cache and also no remote timestamp, downloading the timestamp +// fails with a store.ErrMetaNotFound +func TestDownloadTimestampNoTimestamps(t *testing.T) { + kdb, repo, _, err := testutils.EmptyRepo("docker.com/notary") + assert.NoError(t, err) + localStorage := store.NewMemoryStore(nil, nil) + remoteStorage := store.NewMemoryStore(nil, nil) + client := NewClient(repo, remoteStorage, kdb, localStorage) + + err = client.downloadTimestamp() + assert.Error(t, err) + notFoundErr, ok := err.(store.ErrMetaNotFound) + assert.True(t, ok) + assert.Equal(t, data.CanonicalTimestampRole, notFoundErr.Resource) +} + +// If there is no local cache and the remote timestamp is empty, downloading the timestamp +// fails with a store.ErrMetaNotFound +func TestDownloadTimestampNoLocalTimestampRemoteTimestampEmpty(t *testing.T) { + kdb, repo, _, err := testutils.EmptyRepo("docker.com/notary") + assert.NoError(t, err) + localStorage := store.NewMemoryStore(nil, nil) + remoteStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: {}}, nil) + client := NewClient(repo, remoteStorage, kdb, localStorage) + + err = client.downloadTimestamp() + assert.Error(t, err) + assert.IsType(t, &json.SyntaxError{}, err) +} + +// If there is no local cache and the remote timestamp is invalid, downloading the timestamp +// fails with a store.ErrMetaNotFound +func TestDownloadTimestampNoLocalTimestampRemoteTimestampInvalid(t *testing.T) { + kdb, repo, _, err := testutils.EmptyRepo("docker.com/notary") + assert.NoError(t, err) + localStorage := store.NewMemoryStore(nil, nil) + + // add a timestamp to the remote cache + tsSigned, err := repo.SignTimestamp(data.DefaultExpires("timestamp")) + assert.NoError(t, err) + tsSigned.Signatures[0].Signature = []byte("12345") // invalidate the signature + ts, err := json.Marshal(tsSigned) + assert.NoError(t, err) + remoteStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: ts}, nil) + + client := NewClient(repo, remoteStorage, kdb, localStorage) + err = client.downloadTimestamp() + assert.Error(t, err) + assert.IsType(t, signed.ErrRoleThreshold{}, err) +} + +// If there is is a local cache and no remote timestamp, we fall back on the cached timestamp +func TestDownloadTimestampLocalTimestampNoRemoteTimestamp(t *testing.T) { + kdb, repo, _, err := testutils.EmptyRepo("docker.com/notary") + assert.NoError(t, err) + + // add a timestamp to the local cache + tsSigned, err := repo.SignTimestamp(data.DefaultExpires("timestamp")) + assert.NoError(t, err) + ts, err := json.Marshal(tsSigned) + assert.NoError(t, err) + localStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: ts}, nil) + + remoteStorage := store.NewMemoryStore(nil, nil) + client := NewClient(repo, remoteStorage, kdb, localStorage) + + err = client.downloadTimestamp() + assert.NoError(t, err) +} + +// If there is is a local cache and the remote timestamp is invalid, we fall back on the cached timestamp +func TestDownloadTimestampLocalTimestampInvalidRemoteTimestamp(t *testing.T) { + kdb, repo, _, err := testutils.EmptyRepo("docker.com/notary") + assert.NoError(t, err) + + // add a timestamp to the local cache + tsSigned, err := repo.SignTimestamp(data.DefaultExpires("timestamp")) + assert.NoError(t, err) + ts, err := json.Marshal(tsSigned) + assert.NoError(t, err) + localStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: ts}, nil) + + // add a timestamp to the remote cache + tsSigned.Signatures[0].Signature = []byte("12345") // invalidate the signature + ts, err = json.Marshal(tsSigned) + assert.NoError(t, err) + remoteStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: ts}, nil) + + client := NewClient(repo, remoteStorage, kdb, localStorage) + err = client.downloadTimestamp() + assert.NoError(t, err) +} diff --git a/tuf/testutils/swizzler.go b/tuf/testutils/swizzler.go index 8807196ab8..a594871b97 100644 --- a/tuf/testutils/swizzler.go +++ b/tuf/testutils/swizzler.go @@ -32,21 +32,9 @@ type MetadataSwizzler struct { Roles []string // list of Roles in the metadataStore } -// signs the new metadata, replacing whatever signature was there -func serializeMetadata(cs signed.CryptoService, s *data.Signed, role string, - pubKeys ...data.PublicKey) ([]byte, error) { - - // delete the existing signatures - s.Signatures = []data.Signature{} - - if len(pubKeys) > 0 { - if err := signed.Sign(cs, s, pubKeys...); err != nil { - if _, ok := err.(signed.ErrNoKeys); ok { - return nil, ErrNoKeyForRole{Role: role} - } - return nil, err - } - } else if role == data.CanonicalRootRole { +func getPubKeys(cs signed.CryptoService, s *data.Signed, role string) ([]data.PublicKey, error) { + var pubKeys []data.PublicKey + if role == data.CanonicalRootRole { // if this is root metadata, we have to get the keys from the root because they // are certs root := &data.Root{} @@ -54,24 +42,37 @@ func serializeMetadata(cs signed.CryptoService, s *data.Signed, role string, return nil, err } for _, pubKeyID := range root.Roles[data.CanonicalRootRole].KeyIDs { - if err := signed.Sign(cs, s, root.Keys[pubKeyID]); err != nil { - if _, ok := err.(signed.ErrNoKeys); ok { - return nil, ErrNoKeyForRole{Role: role} - } - return nil, err - } + pubKeys = append(pubKeys, root.Keys[pubKeyID]) } } else { pubKeyIDs := cs.ListKeys(role) - if len(pubKeyIDs) < 1 { - return nil, ErrNoKeyForRole{role} - } for _, pubKeyID := range pubKeyIDs { - if err := signed.Sign(cs, s, cs.GetKey(pubKeyID)); err != nil { - return nil, err + pubKey := cs.GetKey(pubKeyID) + if pubKey != nil { + pubKeys = append(pubKeys, pubKey) } } } + return pubKeys, nil +} + +// signs the new metadata, replacing whatever signature was there +func serializeMetadata(cs signed.CryptoService, s *data.Signed, role string, + pubKeys ...data.PublicKey) ([]byte, error) { + + // delete the existing signatures + s.Signatures = []data.Signature{} + + if len(pubKeys) < 1 { + return nil, ErrNoKeyForRole{role} + } + + if err := signed.Sign(cs, s, pubKeys...); err != nil { + if _, ok := err.(signed.ErrNoKeys); ok { + return nil, ErrNoKeyForRole{Role: role} + } + return nil, err + } metaBytes, err := json.Marshal(s) if err != nil { @@ -123,6 +124,18 @@ func (m *MetadataSwizzler) SetInvalidJSON(role string) error { return m.MetadataCache.SetMeta(role, metaBytes[5:]) } +// AddExtraSpace adds an extra space to the beginning and end of the serialized +// JSON bytes, which should not affect serialization, but will change the checksum +// of the file. +func (m *MetadataSwizzler) AddExtraSpace(role string) error { + metaBytes, err := m.MetadataCache.GetMeta(role, -1) + if err != nil { + return err + } + newBytes := append(append([]byte{' '}, metaBytes...), ' ') + return m.MetadataCache.SetMeta(role, newBytes) +} + // SetInvalidSigned corrupts the metadata into something that is valid JSON, // but not unmarshallable into signed JSON func (m *MetadataSwizzler) SetInvalidSigned(role string) error { @@ -148,6 +161,11 @@ func (m *MetadataSwizzler) SetInvalidSignedMeta(role string) error { return err } + pubKeys, err := getPubKeys(m.CryptoService, signedThing, role) + if err != nil { + return err + } + var unmarshalled map[string]interface{} if err := json.Unmarshal(signedThing.Signed, &unmarshalled); err != nil { return err @@ -163,7 +181,7 @@ func (m *MetadataSwizzler) SetInvalidSignedMeta(role string) error { } signedThing.Signed = json.RawMessage(metaBytes) - metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role) + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role, pubKeys...) if err != nil { return err } @@ -194,7 +212,11 @@ func (m *MetadataSwizzler) SetInvalidMetadataType(role string) error { } signedThing.Signed = json.RawMessage(metaBytes) - metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role) + pubKeys, err := getPubKeys(m.CryptoService, signedThing, role) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role, pubKeys...) + } + if err != nil { return err } @@ -279,7 +301,11 @@ func (m *MetadataSwizzler) OffsetMetadataVersion(role string, offset int) error } signedThing.Signed = json.RawMessage(metaBytes) - metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role) + pubKeys, err := getPubKeys(m.CryptoService, signedThing, role) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role, pubKeys...) + } + if err != nil { return err } @@ -307,7 +333,11 @@ func (m *MetadataSwizzler) ExpireMetadata(role string) error { } signedThing.Signed = json.RawMessage(metaBytes) - metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role) + pubKeys, err := getPubKeys(m.CryptoService, signedThing, role) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, role, pubKeys...) + } + if err != nil { return err } @@ -358,7 +388,12 @@ func (m *MetadataSwizzler) SetThreshold(role string, newThreshold int) error { } } - metaBytes, err := serializeMetadata(m.CryptoService, signedThing, roleSpecifier) + var metaBytes []byte + pubKeys, err := getPubKeys(m.CryptoService, signedThing, roleSpecifier) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, roleSpecifier, pubKeys...) + } + if err != nil { return err } @@ -391,7 +426,12 @@ func (m *MetadataSwizzler) ChangeRootKey() error { return err } - metaBytes, err := serializeMetadata(m.CryptoService, signedThing, data.CanonicalRootRole) + var metaBytes []byte + pubKeys, err := getPubKeys(m.CryptoService, signedThing, data.CanonicalRootRole) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, signedThing, data.CanonicalRootRole, pubKeys...) + } + if err != nil { return err } @@ -438,7 +478,11 @@ func (m *MetadataSwizzler) UpdateSnapshotHashes(roles ...string) error { if snapshotSigned, err = snapshot.ToSigned(); err != nil { return err } - metaBytes, err = serializeMetadata(m.CryptoService, snapshotSigned, data.CanonicalSnapshotRole) + pubKeys, err := getPubKeys(m.CryptoService, snapshotSigned, data.CanonicalSnapshotRole) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, snapshotSigned, data.CanonicalSnapshotRole, pubKeys...) + } + if err != nil { return err } @@ -478,7 +522,11 @@ func (m *MetadataSwizzler) UpdateTimestampHash() error { if err != nil { return err } - metaBytes, err = serializeMetadata(m.CryptoService, timestampSigned, data.CanonicalTimestampRole) + pubKeys, err := getPubKeys(m.CryptoService, timestampSigned, data.CanonicalTimestampRole) + if err == nil { + metaBytes, err = serializeMetadata(m.CryptoService, timestampSigned, data.CanonicalTimestampRole, pubKeys...) + } + if err != nil { return err } diff --git a/tuf/testutils/swizzler_test.go b/tuf/testutils/swizzler_test.go index f730f29d12..0e779082fb 100644 --- a/tuf/testutils/swizzler_test.go +++ b/tuf/testutils/swizzler_test.go @@ -5,6 +5,7 @@ package testutils import ( "bytes" + "crypto/sha256" "encoding/json" "reflect" "testing" @@ -95,6 +96,40 @@ func TestSwizzlerSetInvalidJSON(t *testing.T) { } } +// This adds a single byte of whitespace to the metadata file, so it should be parsed +// and deserialized the same way, but checksums against snapshot/timestamp may fail +func TestSwizzlerAddExtraSpace(t *testing.T) { + f, origMeta := createNewSwizzler(t) + + f.AddExtraSpace(data.CanonicalTargetsRole) + + snapshot := &data.SignedSnapshot{} + require.NoError(t, json.Unmarshal(origMeta[data.CanonicalSnapshotRole], snapshot)) + + for role, metaBytes := range origMeta { + newMeta, err := f.MetadataCache.GetMeta(role, -1) + require.NoError(t, err) + + if role != data.CanonicalTargetsRole { + require.True(t, bytes.Equal(metaBytes, newMeta), "bytes have changed for role %s", role) + } else { + require.False(t, bytes.Equal(metaBytes, newMeta)) + require.True(t, bytes.Equal(metaBytes, newMeta[1:len(metaBytes)+1])) + require.Equal(t, byte(' '), newMeta[0]) + require.Equal(t, byte(' '), newMeta[len(newMeta)-1]) + + // make sure the hash is not the same as the hash in snapshot + newHash := sha256.Sum256(newMeta) + require.False(t, bytes.Equal( + snapshot.Signed.Meta[data.CanonicalTargetsRole].Hashes["sha256"], + newHash[:])) + require.NotEqual(t, + snapshot.Signed.Meta[data.CanonicalTargetsRole].Length, + len(newMeta)) + } + } +} + // This modifies metdata so that it is unmarshallable as JSON, but cannot be // unmarshalled as a Signed object func TestSwizzlerSetInvalidSigned(t *testing.T) { @@ -124,13 +159,13 @@ func TestSwizzlerSetInvalidSigned(t *testing.T) { func TestSwizzlerSetInvalidSignedMeta(t *testing.T) { f, origMeta := createNewSwizzler(t) - f.SetInvalidSignedMeta(data.CanonicalTargetsRole) + f.SetInvalidSignedMeta(data.CanonicalRootRole) for role, metaBytes := range origMeta { newMeta, err := f.MetadataCache.GetMeta(role, -1) require.NoError(t, err) - if role != data.CanonicalTargetsRole { + if role != data.CanonicalRootRole { require.True(t, bytes.Equal(metaBytes, newMeta), "bytes have changed for role %s", role) } else { require.False(t, bytes.Equal(metaBytes, newMeta)) @@ -192,6 +227,7 @@ func TestSwizzlerInvalidateMetadataSignatures(t *testing.T) { require.Equal(t, origSigned.Signatures[i].KeyID, newSigned.Signatures[i].KeyID) require.Equal(t, origSigned.Signatures[i].Method, newSigned.Signatures[i].Method) require.NotEqual(t, origSigned.Signatures[i].Signature, newSigned.Signatures[i].Signature) + require.Equal(t, []byte("invalid signature"), newSigned.Signatures[i].Signature) } require.True(t, bytes.Equal(origSigned.Signed, newSigned.Signed)) }