diff --git a/client/client_update_test.go b/client/client_update_test.go index 18ece0f197..fd37a69197 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "reflect" + "regexp" "strings" "testing" "time" @@ -812,6 +813,36 @@ var waysToMessUpServer = []swizzleExpectations{ }}, } +var _waysToMessUpServerRoot []swizzleExpectations + +// A getter that also sets once through - we can just add these, but it's very easier to do it +// programmatically +func waysToMessUpServerRoot() []swizzleExpectations { + if _waysToMessUpServerRoot == nil { + _waysToMessUpServerRoot = waysToMessUpServer + for _, roleName := range data.BaseRoles { + _waysToMessUpServerRoot = append(_waysToMessUpServerRoot, + swizzleExpectations{ + desc: fmt.Sprintf("no %s keys", roleName), + expectErrs: []interface{}{ + &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, + swizzle: func(s *testutils.MetadataSwizzler, role string) error { + return s.MutateRoot(func(r *data.Root) { + r.Roles[roleName].KeyIDs = []string{} + }) + }}, + swizzleExpectations{ + desc: fmt.Sprintf("no %s role", roleName), + expectErrs: []interface{}{tuf.ErrNotLoaded{}}, + swizzle: func(s *testutils.MetadataSwizzler, role string) error { + return s.MutateRoot(func(r *data.Root) { delete(r.Roles, roleName) }) + }}, + ) + } + } + return _waysToMessUpServerRoot +} + // 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). @@ -820,28 +851,7 @@ func TestUpdateRootRemoteCorruptedNoLocalCache(t *testing.T) { t.Skip("skipping test in short mode") } - waysToMessUpServerRoot := waysToMessUpServer - for _, roleName := range data.BaseRoles { - waysToMessUpServerRoot = append(waysToMessUpServer, - swizzleExpectations{ - desc: fmt.Sprintf("no %s keys", roleName), - expectErrs: []interface{}{ - &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { - r.Roles[roleName].KeyIDs = []string{} - }) - }}, - swizzleExpectations{ - desc: fmt.Sprintf("no %s role", roleName), - expectErrs: []interface{}{tuf.ErrNotLoaded{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { delete(r.Roles, roleName) }) - }}, - ) - } - - for _, testData := range waysToMessUpServerRoot { + for _, testData := range waysToMessUpServerRoot() { 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 @@ -869,28 +879,7 @@ func TestUpdateRootRemoteCorruptedCanUseLocalCache(t *testing.T) { t.Skip("skipping test in short mode") } - waysToMessUpServerRoot := waysToMessUpServer - for _, roleName := range data.BaseRoles { - waysToMessUpServerRoot = append(waysToMessUpServer, - swizzleExpectations{ - desc: fmt.Sprintf("no %s keys", roleName), - expectErrs: []interface{}{ - &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { - r.Roles[roleName].KeyIDs = []string{} - }) - }}, - swizzleExpectations{ - desc: fmt.Sprintf("no %s role", roleName), - expectErrs: []interface{}{tuf.ErrNotLoaded{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { delete(r.Roles, roleName) }) - }}, - ) - } - - for _, testData := range waysToMessUpServerRoot { + for _, testData := range waysToMessUpServerRoot() { testUpdateRemoteCorruptValidChecksum(t, updateOpts{ localCache: true, forWrite: false, @@ -910,43 +899,29 @@ func TestUpdateRootRemoteCorruptedCannotUseLocalCache(t *testing.T) { if testing.Short() { t.Skip("skipping test in short mode") } + for _, testData := range waysToMessUpServerRoot() { + // TODO: Bug: currently, if any role is missing from the root, the + // update succeeds because the cached root, with good roles, was + // bootstrapped and hence the roles are loaded into the KeyDB by the + // time the update happens. So when the new root is loaded, although + // it is missing roles, the cached role from the KeyDB is used instead. + // But really, if the root is missing a role, it should be invalid and + // the update should fail instead. + isbuggy, err := regexp.MatchString("no [a-z]+ role", testData.desc) + require.NoError(t, err) - waysToMessUpServerRoot := waysToMessUpServer - for _, roleName := range data.BaseRoles { - waysToMessUpServerRoot = append(waysToMessUpServer, - swizzleExpectations{ - desc: fmt.Sprintf("no %s keys", roleName), - expectErrs: []interface{}{ - &certs.ErrValidationFail{}, signed.ErrRoleThreshold{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { - r.Roles[roleName].KeyIDs = []string{} - }) - }}, - swizzleExpectations{ - desc: fmt.Sprintf("no %s role", roleName), - expectErrs: []interface{}{tuf.ErrNotLoaded{}}, - swizzle: func(s *testutils.MetadataSwizzler, role string) error { - return s.MutateRoot(func(r *data.Root) { delete(r.Roles, roleName) }) - }}, - ) - } - - // if the timestamp role is missing, then the timestamp is broken, - // so it will revert to the prevoius timestamp - for _, testData := range waysToMessUpServerRoot { testUpdateRemoteCorruptValidChecksum(t, updateOpts{ serverHasNewData: true, localCache: true, forWrite: false, role: data.CanonicalRootRole, - }, testData, !strings.HasPrefix(testData.desc, "no timestamp role")) + }, testData, !isbuggy) testUpdateRemoteCorruptValidChecksum(t, updateOpts{ serverHasNewData: true, localCache: true, forWrite: true, role: data.CanonicalRootRole, - }, testData, !strings.HasPrefix(testData.desc, "no timestamp role")) + }, testData, !isbuggy) } } diff --git a/tuf/testutils/swizzler.go b/tuf/testutils/swizzler.go index f62ef4d947..7f501b0d8c 100644 --- a/tuf/testutils/swizzler.go +++ b/tuf/testutils/swizzler.go @@ -9,6 +9,7 @@ import ( "github.com/docker/notary/cryptoservice" "github.com/docker/notary/passphrase" "github.com/docker/notary/trustmanager" + "github.com/docker/notary/tuf" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/store" @@ -41,7 +42,11 @@ func getPubKeys(cs signed.CryptoService, s *data.Signed, role string) ([]data.Pu if err := json.Unmarshal(s.Signed, root); err != nil { return nil, err } - for _, pubKeyID := range root.Roles[data.CanonicalRootRole].KeyIDs { + rootRole, ok := root.Roles[data.CanonicalRootRole] + if !ok || rootRole == nil { + return nil, tuf.ErrNotLoaded{} + } + for _, pubKeyID := range rootRole.KeyIDs { pubKeys = append(pubKeys, root.Keys[pubKeyID]) } } else { @@ -546,6 +551,12 @@ func (m *MetadataSwizzler) MutateRoot(mutate func(*data.Root)) error { return err } + // get the original keys, in case the mutation messes with the signing keys + oldPubKeys, err := getPubKeys(m.CryptoService, signedThing, data.CanonicalRootRole) + if err != nil { + return err + } + mutate(&root) rootBytes, err := json.MarshalCanonical(root) @@ -555,8 +566,8 @@ func (m *MetadataSwizzler) MutateRoot(mutate func(*data.Root)) error { signedThing.Signed = json.RawMessage(rootBytes) pubKeys, err := getPubKeys(m.CryptoService, signedThing, data.CanonicalRootRole) - if err != nil { - return err + if err != nil || len(pubKeys) == 0 { // we have to sign it somehow - might as well use the old keys + pubKeys = oldPubKeys } metaBytes, err := serializeMetadata(m.CryptoService, signedThing, data.CanonicalRootRole, pubKeys...)