From a969db7a133ddaeaf5b0564c96e986d40421201a Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 21 Jan 2016 09:48:07 -0800 Subject: [PATCH 1/7] Add swizzler method to just change the checksum by adding a space. Signed-off-by: Ying Li --- tuf/testutils/swizzler.go | 12 ++++++++++++ tuf/testutils/swizzler_test.go | 35 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/tuf/testutils/swizzler.go b/tuf/testutils/swizzler.go index 8807196ab8..b08db639a4 100644 --- a/tuf/testutils/swizzler.go +++ b/tuf/testutils/swizzler.go @@ -123,6 +123,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, maxSize) + 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 { diff --git a/tuf/testutils/swizzler_test.go b/tuf/testutils/swizzler_test.go index f730f29d12..d60cb06cab 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, maxSize) + 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) { From f8a0e46b6c6cac68497269354ee22a8bee5a5bdd Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 21 Jan 2016 12:08:28 -0800 Subject: [PATCH 2/7] Add test for when any downloaded metadata has an invalid checksum compared to snapshot or timestamp. Signed-off-by: Ying Li Conflicts: client/client_update_test.go --- client/client_update_test.go | 122 +++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/client/client_update_test.go b/client/client_update_test.go index b7f509496d..dc3aa15cb3 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/docker/notary/passphrase" + "github.com/docker/notary/tuf/client" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/store" "github.com/docker/notary/tuf/testutils" @@ -606,3 +607,124 @@ 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. Having extra spaces doesn't prevent it from validating 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) have an extra space, 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. Having +// extra spaces doesn't prevent it from validating 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) have an extra space, 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 add space 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) + } +} From bb5f9cc170bc1467343503570b7a1db215336073 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 21 Jan 2016 14:05:31 -0800 Subject: [PATCH 3/7] Update swizzler so that if messing up the root file, we can still get the pub keys and sign. Signed-off-by: Ying Li --- tuf/testutils/swizzler.go | 104 ++++++++++++++++++++++----------- tuf/testutils/swizzler_test.go | 5 +- 2 files changed, 73 insertions(+), 36 deletions(-) diff --git a/tuf/testutils/swizzler.go b/tuf/testutils/swizzler.go index b08db639a4..f037ad07a0 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 { @@ -160,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 @@ -175,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 } @@ -206,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 } @@ -291,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 } @@ -319,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 } @@ -370,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 } @@ -403,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 } @@ -450,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 } @@ -490,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 d60cb06cab..ded7096d4d 100644 --- a/tuf/testutils/swizzler_test.go +++ b/tuf/testutils/swizzler_test.go @@ -159,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)) @@ -227,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)) } From befd30e9a4e2d3db0fed29f91c396c474ecfd23a Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 21 Jan 2016 16:51:00 -0800 Subject: [PATCH 4/7] Add tests for updating if server has metadata corruption such that the checksum was valid. Signed-off-by: Ying Li --- client/client_update_test.go | 243 +++++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) diff --git a/client/client_update_test.go b/client/client_update_test.go index dc3aa15cb3..53d5db748b 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -2,16 +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" @@ -728,3 +733,241 @@ func testUpdateRemoteFileChecksumWrong(t *testing.T, opts updateOpts, errExpecte 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) --- + +type swizzleServer func(*testutils.MetadataSwizzler, string) error + +type swizzleExpectations struct { + desc string + swizzle swizzleServer + expectErrs []interface{} +} + +// 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, -2) + }}, + + {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) { + for _, testData := range waysToMessUpServer { + if testData.desc == "insufficient signatures" { + // TODO: fix bug where we don't check for enough signatures to meet the + // threshold when bootstrapping the root + 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) { + 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) { + 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) { + for _, role := range append(data.BaseRoles, "targets/a", "targets/a/b") { + if role == data.CanonicalRootRole { + continue + } + // TODO: this is a bug right now where timestamp will not fall back if + // there is any error other than JSON unmarshalling + if role == data.CanonicalTimestampRole { + 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. +func TestUpdateNonRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { + for _, role := range []string{"snapshot"} { //append(data.BaseRoles, "targets/a", "targets/a/b") { + if role == data.CanonicalRootRole { + continue + } + // TODO: this is a bug right now where timestamp will not fall back if + // there is any error other than JSON unmarshalling + if role == data.CanonicalTimestampRole { + continue + } + for _, testData := range waysToMessUpServer { + testUpdateRemoteCorruptValidChecksum(t, updateOpts{ + serverHasNewData: true, + localCache: true, + role: role, + }, testData, role != data.CanonicalTimestampRole) + } + } +} + +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) + } +} From 237561a2a9e426ff3e310d2b34c0a7dc8148638f Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 22 Jan 2016 11:41:06 -0800 Subject: [PATCH 5/7] Fixed timestamp downloading so if verification fails, we fall back to cached. Signed-off-by: Ying Li Conflicts: tuf/client/client.go --- client/client_update_test.go | 23 ++++----- tuf/client/client.go | 52 ++++++++++--------- tuf/client/client_test.go | 93 ++++++++++++++++++++++++++++++++++ tuf/testutils/swizzler.go | 2 +- tuf/testutils/swizzler_test.go | 2 +- 5 files changed, 131 insertions(+), 41 deletions(-) diff --git a/client/client_update_test.go b/client/client_update_test.go index 53d5db748b..2d0b4c65ba 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -876,11 +876,6 @@ func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) { if role == data.CanonicalRootRole { continue } - // TODO: this is a bug right now where timestamp will not fall back if - // there is any error other than JSON unmarshalling - if role == data.CanonicalTimestampRole { - continue - } for _, testData := range waysToMessUpServer { testUpdateRemoteCorruptValidChecksum(t, updateOpts{ localCache: true, @@ -893,23 +888,24 @@ func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) { // 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. +// not have to redownload anything (usually) func TestUpdateNonRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { - for _, role := range []string{"snapshot"} { //append(data.BaseRoles, "targets/a", "targets/a/b") { + for _, role := range append(data.BaseRoles, "targets/a", "targets/a/b") { if role == data.CanonicalRootRole { continue } - // TODO: this is a bug right now where timestamp will not fall back if - // there is any error other than JSON unmarshalling - if role == data.CanonicalTimestampRole { - 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, role != data.CanonicalTimestampRole) + }, testData, shouldFail) } } } @@ -951,7 +947,6 @@ func testUpdateRemoteCorruptValidChecksum(t *testing.T, opts updateOpts, expt sw serverSwizzler.UpdateTimestampHash() } } - _, err := repo.Update(opts.forWrite) if shouldErr { require.Error(t, err, "expected failure updating when %s", msg) 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..420f1aae96 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: []byte{}}, 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 f037ad07a0..a594871b97 100644 --- a/tuf/testutils/swizzler.go +++ b/tuf/testutils/swizzler.go @@ -128,7 +128,7 @@ func (m *MetadataSwizzler) SetInvalidJSON(role string) error { // 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, maxSize) + metaBytes, err := m.MetadataCache.GetMeta(role, -1) if err != nil { return err } diff --git a/tuf/testutils/swizzler_test.go b/tuf/testutils/swizzler_test.go index ded7096d4d..0e779082fb 100644 --- a/tuf/testutils/swizzler_test.go +++ b/tuf/testutils/swizzler_test.go @@ -107,7 +107,7 @@ func TestSwizzlerAddExtraSpace(t *testing.T) { require.NoError(t, json.Unmarshal(origMeta[data.CanonicalSnapshotRole], snapshot)) for role, metaBytes := range origMeta { - newMeta, err := f.MetadataCache.GetMeta(role, maxSize) + newMeta, err := f.MetadataCache.GetMeta(role, -1) require.NoError(t, err) if role != data.CanonicalTargetsRole { From 1baf3c781cee7169b933e9e920d0bb890cd6f0f5 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 22 Jan 2016 14:41:17 -0800 Subject: [PATCH 6/7] Add test that update fails if the local root is corrupt AND the remote root is corrupt. Signed-off-by: Ying Li Conflicts: client/client_update_test.go --- client/client_update_test.go | 169 ++++++++++++++++++++++++++++------- tuf/client/client_test.go | 2 +- 2 files changed, 137 insertions(+), 34 deletions(-) diff --git a/client/client_update_test.go b/client/client_update_test.go index 2d0b4c65ba..6c4d68f336 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -178,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 @@ -223,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 { @@ -273,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 @@ -456,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 @@ -474,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. @@ -494,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 @@ -519,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 @@ -536,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...) { @@ -556,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 @@ -737,14 +761,6 @@ func testUpdateRemoteFileChecksumWrong(t *testing.T, opts updateOpts, errExpecte // --- these tests below assume the checksums are correct (since the server can sign snapshots and // timestamps, so can be malicious) --- -type swizzleServer func(*testutils.MetadataSwizzler, string) error - -type swizzleExpectations struct { - desc string - swizzle swizzleServer - expectErrs []interface{} -} - // this does not include delete, which is tested separately so we can try to get // 404s and 503s var waysToMessUpServer = []swizzleExpectations{ @@ -781,7 +797,7 @@ var waysToMessUpServer = []swizzleExpectations{ {desc: "lower metadata version", expectErrs: []interface{}{ &certs.ErrValidationFail{}, signed.ErrLowVersion{}}, swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.OffsetMetadataVersion(role, -2) + return s.OffsetMetadataVersion(role, -3) }}, {desc: "insufficient signatures", expectErrs: []interface{}{ @@ -795,10 +811,13 @@ var waysToMessUpServer = []swizzleExpectations{ // 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" { - // TODO: fix bug where we don't check for enough signatures to meet the - // threshold when bootstrapping the root + // TODO: bug right now if we download the root during the bootstrap phase, + // we don't check for enough signatures to meet the threshold continue } @@ -835,6 +854,9 @@ func TestUpdateRootRemoteCorruptedCanUseLocalCache(t *testing.T) { // 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, @@ -854,6 +876,9 @@ func TestUpdateRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { // 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 @@ -872,6 +897,9 @@ func TestUpdateNonRootRemoteCorruptedNoLocalCache(t *testing.T) { // 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 @@ -890,6 +918,9 @@ func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) { // 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 @@ -966,3 +997,75 @@ func testUpdateRemoteCorruptValidChecksum(t *testing.T, opts updateOpts, expt sw 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" { + // TODO: bug right now if we download the root during the bootstrap phase, + // we don't check for enough signatures to meet the threshold + 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_test.go b/tuf/client/client_test.go index 420f1aae96..cf4fd81d80 100644 --- a/tuf/client/client_test.go +++ b/tuf/client/client_test.go @@ -842,7 +842,7 @@ 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: []byte{}}, nil) + remoteStorage := store.NewMemoryStore(map[string][]byte{data.CanonicalTimestampRole: {}}, nil) client := NewClient(repo, remoteStorage, kdb, localStorage) err = client.downloadTimestamp() From 5a39366f7522779012788d0dd0f7337324a754fe Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 25 Jan 2016 10:16:58 -0800 Subject: [PATCH 7/7] Clarify comments w.r.t. having an 'extra space' as being corrupted in transit. Also, we are not sure if we want to support thresholds, so make sure the comments reflect that. Signed-off-by: Ying Li --- client/client_update_test.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/client/client_update_test.go b/client/client_update_test.go index 6c4d68f336..a9ac2b480c 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -638,11 +638,12 @@ 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. Having extra spaces doesn't prevent it from validating during bootstrap, +// 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) have an extra space, they -// will also fail during update with the same error. +// 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{ @@ -686,13 +687,14 @@ func TestUpdateRemoteChecksumWrongCanUseLocalCache(t *testing.T) { } } -// 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. Having -// extra spaces doesn't prevent it from validating during bootstrap, +// 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) have an extra space, they -// will also fail during update with the same error. +// 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{ @@ -730,7 +732,7 @@ func testUpdateRemoteFileChecksumWrong(t *testing.T, opts updateOpts, errExpecte bumpVersions(t, serverSwizzler, 1) } - require.NoError(t, serverSwizzler.AddExtraSpace(opts.role), "failed to add space to %s", opts.role) + require.NoError(t, serverSwizzler.AddExtraSpace(opts.role), "failed to checksum-corrupt to %s", opts.role) _, err := repo.Update(opts.forWrite) if !errExpected { @@ -816,8 +818,9 @@ func TestUpdateRootRemoteCorruptedNoLocalCache(t *testing.T) { } for _, testData := range waysToMessUpServer { if testData.desc == "insufficient signatures" { - // TODO: bug right now if we download the root during the bootstrap phase, - // we don't check for enough signatures to meet the threshold + // 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 } @@ -1013,8 +1016,9 @@ func TestUpdateLocalAndRemoteRootCorrupt(t *testing.T) { continue } if serverExpt.desc == "insufficient signatures" { - // TODO: bug right now if we download the root during the bootstrap phase, - // we don't check for enough signatures to meet the threshold + // 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)