From 0bec06eb9b31adb32e30bf68968d7645606084ef Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 16 Dec 2015 00:14:09 -0800 Subject: [PATCH] RemoveTarget now takes an optional variadic list of roles to remove from. If none are provided, it defaults to the targets role, as before. Signed-off-by: Ying Li --- client/client.go | 101 +++++++++-------- client/client_test.go | 248 ++++++++++++++++++++++++++++++++---------- 2 files changed, 241 insertions(+), 108 deletions(-) diff --git a/client/client.go b/client/client.go index 6eaaa14d9c..2f83abec9e 100644 --- a/client/client.go +++ b/client/client.go @@ -259,9 +259,48 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st return r.saveMetadata(serverManagesSnapshot) } -// AddTarget adds a new target to the repository to the given roles, forcing a -// timestamps check from TUF. If roles are unspecified, AddTarget the default -// role is "target" +// adds a TUF Change template to the given roles +func addChange(cl *changelist.FileChangelist, c changelist.Change, + roles ...string) error { + if len(roles) == 0 { + roles = []string{data.CanonicalTargetsRole} + } + + var changes []changelist.Change + for _, role := range roles { + role = strings.ToLower(role) + + if !data.ValidRole(role) { + return data.ErrInvalidRole{Role: role} + } + + if _, ok := data.ValidRoles[role]; ok && role != data.CanonicalTargetsRole { + return data.ErrInvalidRole{ + Role: role, + Reason: "cannot add targets to this role", + } + } + + changes = append(changes, changelist.NewTufChange( + c.Action(), + role, + c.Type(), + c.Path(), + c.Content(), + )) + } + + for _, c := range changes { + if err := cl.Add(c); err != nil { + return err + } + } + return nil +} + +// AddTarget creates new changelist entries to add a target to the given roles +// in the repository when the changelist gets appied at publish time. +// If roles are unspecified, the default role is "target". func (r *NotaryRepository) AddTarget(target *Target, roles ...string) error { cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) if err != nil { @@ -276,58 +315,24 @@ func (r *NotaryRepository) AddTarget(target *Target, roles ...string) error { return err } - if len(roles) == 0 { - roles = []string{data.CanonicalTargetsRole} - } - - var changes []*changelist.TufChange - for _, role := range roles { - role = strings.ToLower(role) - - if !data.ValidRole(role) { - return data.ErrInvalidRole{Role: role} - } - - _, ok := data.ValidRoles[role] - if ok && role != data.CanonicalTargetsRole { - return data.ErrInvalidRole{ - Role: role, - Reason: "cannot add targets to this role", - } - } - - changes = append(changes, changelist.NewTufChange( - changelist.ActionCreate, - role, - changelist.TypeTargetsTarget, - target.Name, - metaJSON, - )) - } - - for _, c := range changes { - err = cl.Add(c) - if err != nil { - return err - } - } - return nil + template := changelist.NewTufChange( + changelist.ActionCreate, "", changelist.TypeTargetsTarget, + target.Name, metaJSON) + return addChange(cl, template, roles...) } -// RemoveTarget creates a new changelist entry to remove a target from the repository -// when the changelist gets applied at publish time -func (r *NotaryRepository) RemoveTarget(targetName string) error { +// RemoveTarget creates new changelist entries to remove a target from the given +// roles in the repository when the changelist gets applied at publish time. +// If roles are unspecified, the default role is "target". +func (r *NotaryRepository) RemoveTarget(targetName string, roles ...string) error { cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) if err != nil { return err } logrus.Debugf("Removing target \"%s\"", targetName) - c := changelist.NewTufChange(changelist.ActionDelete, changelist.ScopeTargets, "target", targetName, nil) - err = cl.Add(c) - if err != nil { - return err - } - return nil + template := changelist.NewTufChange(changelist.ActionDelete, "", + changelist.TypeTargetsTarget, targetName, nil) + return addChange(cl, template, roles...) } // ListTargets lists all targets for the current repository diff --git a/client/client_test.go b/client/client_test.go index 8c249fbf67..f408425000 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -427,17 +427,6 @@ func testInitRepoPasswordInvalid(t *testing.T, rootType string) { assert.EqualError(t, err, trustmanager.ErrPasswordInvalid{}.Error()) } -// TestAddTargetToTargetRoleByDefault adds a target without specifying a role -// to a repo without delegations. Confirms that the changelist is created -// correctly, for the targets scope. -// We test this with both an RSA and ECDSA root key -func TestAddTargetToTargetRoleByDefault(t *testing.T) { - testAddTargetToTargetRoleByDefault(t, data.ECDSAKey) - if !testing.Short() { - testAddTargetToTargetRoleByDefault(t, data.RSAKey) - } -} - func addTarget(t *testing.T, repo *NotaryRepository, targetName, targetFile string, roles ...string) *Target { target, err := NewTarget(targetName, targetFile) @@ -454,7 +443,10 @@ func getChanges(t *testing.T, repo *NotaryRepository) []changelist.Change { return changeList.List() } -func testAddTargetToTargetRoleByDefault(t *testing.T, rootType string) { +// TestAddTargetToTargetRoleByDefault adds a target without specifying a role +// to a repo without delegations. Confirms that the changelist is created +// correctly, for the targets scope. +func TestAddTargetToTargetRoleByDefault(t *testing.T) { // Temporary directory where test files will be created tempBaseDir, err := ioutil.TempDir("", "notary-test-") defer os.RemoveAll(tempBaseDir) @@ -466,45 +458,58 @@ func testAddTargetToTargetRoleByDefault(t *testing.T, rootType string) { ts, _, _ := simpleTestServer(t) defer ts.Close() - repo, _ := initializeRepo(t, rootType, tempBaseDir, gun, ts.URL, false) + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) - // tests need to manually boostrap timestamp as client doesn't generate it - err = repo.tufRepo.InitTimestamp() - assert.NoError(t, err, "error creating repository: %s", err) - - testAddTargetToRepo(t, repo, nil, []string{data.CanonicalTargetsRole}) + testAddOrDeleteTarget(t, repo, changelist.ActionCreate, nil, + []string{data.CanonicalTargetsRole}) } -// Tests that adding a target to a repo, with the given roles, adds it to the -// expected scopes -func testAddTargetToRepo(t *testing.T, repo *NotaryRepository, - addToRoles []string, expectedScopes []string) { +// Tests that adding a target to a repo or deleting a target from a repo, +// with the given roles, makes a change to the expected scopes +func testAddOrDeleteTarget(t *testing.T, repo *NotaryRepository, action string, + rolesToChange []string, expectedScopes []string) { assert.Len(t, getChanges(t, repo), 0, "should start with zero changes") - // Add fixtures/intermediate-ca.crt as a target. There's no particular - // reason for using this file except that it happens to be available as - // a fixture. - addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt", addToRoles...) + if action == changelist.ActionCreate { + // Add fixtures/intermediate-ca.crt as a target. There's no particular + // reason for using this file except that it happens to be available as + // a fixture. + addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt", rolesToChange...) + } else { + err := repo.RemoveTarget("latest", rolesToChange...) + assert.NoError(t, err, "error removing target") + } + changes := getChanges(t, repo) assert.Len(t, changes, len(expectedScopes), "wrong number of changes files found") foundScopes := make(map[string]bool) for _, c := range changes { // there is only one - assert.EqualValues(t, changelist.ActionCreate, c.Action()) + assert.EqualValues(t, action, c.Action()) foundScopes[c.Scope()] = true assert.Equal(t, "target", c.Type()) assert.Equal(t, "latest", c.Path()) - assert.NotEmpty(t, c.Content()) + if action == changelist.ActionCreate { + assert.NotEmpty(t, c.Content()) + } else { + assert.Empty(t, c.Content()) + } } assert.Len(t, foundScopes, len(expectedScopes)) for _, expectedScope := range expectedScopes { _, ok := foundScopes[expectedScope] - assert.True(t, ok, "Target was not added to %s", expectedScope) + assert.True(t, ok, "Target was not added/removed from %s", expectedScope) + } + + // add/delete a second time + if action == changelist.ActionCreate { + addTarget(t, repo, "current", "../fixtures/intermediate-ca.crt", rolesToChange...) + } else { + err := repo.RemoveTarget("current", rolesToChange...) + assert.NoError(t, err, "error removing target") } - // Create a second target - addTarget(t, repo, "current", "../fixtures/intermediate-ca.crt", addToRoles...) changes = getChanges(t, repo) assert.Len(t, changes, 2*len(expectedScopes), "wrong number of changelist files found") @@ -513,11 +518,15 @@ func testAddTargetToRepo(t *testing.T, repo *NotaryRepository, foundScopes = make(map[string]bool) for _, c := range changes { if c.Path() != "latest" { - assert.EqualValues(t, changelist.ActionCreate, c.Action()) + assert.EqualValues(t, action, c.Action()) foundScopes[c.Scope()] = true assert.Equal(t, "target", c.Type()) assert.Equal(t, "current", c.Path()) - assert.NotEmpty(t, c.Content()) + if action == changelist.ActionCreate { + assert.NotEmpty(t, c.Content()) + } else { + assert.Empty(t, c.Content()) + } newFileFound = true } @@ -526,22 +535,14 @@ func testAddTargetToRepo(t *testing.T, repo *NotaryRepository, assert.Len(t, foundScopes, len(expectedScopes)) for _, expectedScope := range expectedScopes { _, ok := foundScopes[expectedScope] - assert.True(t, ok, "Target was not added to %s", expectedScope) + assert.True(t, ok, "Target was not added/removed from %s", expectedScope) } } // TestAddTargetToSpecifiedValidRoles adds a target to the specified roles. // Confirms that the changelist is created correctly, one for each of the // the specified roles as scopes. -// We test this with both an RSA and ECDSA root key func TestAddTargetToSpecifiedValidRoles(t *testing.T) { - testAddTargetToSpecifiedRoles(t, data.ECDSAKey) - if !testing.Short() { - testAddTargetToSpecifiedRoles(t, data.RSAKey) - } -} - -func testAddTargetToSpecifiedRoles(t *testing.T, keyType string) { // Temporary directory where test files will be created tempBaseDir, err := ioutil.TempDir("", "notary-test-") defer os.RemoveAll(tempBaseDir) @@ -553,21 +554,10 @@ func testAddTargetToSpecifiedRoles(t *testing.T, keyType string) { ts, _, _ := simpleTestServer(t) defer ts.Close() - repo, _ := initializeRepo(t, keyType, tempBaseDir, gun, ts.URL, false) + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) - // tests need to manually boostrap timestamp as client doesn't generate it - err = repo.tufRepo.InitTimestamp() - assert.NoError(t, err, "error creating repository: %s", err) - - // add a delegated role roleName := filepath.Join(data.CanonicalTargetsRole, "a") - delegationKey, err := repo.CryptoService.Create(roleName, keyType) - assert.NoError(t, err) - newRole, err := data.NewRole(roleName, 1, nil, nil, nil) - assert.NoError(t, err) - repo.tufRepo.UpdateDelegations(newRole, []data.PublicKey{delegationKey}) - - testAddTargetToRepo(t, repo, + testAddOrDeleteTarget(t, repo, changelist.ActionCreate, []string{ strings.ToUpper(data.CanonicalTargetsRole), strings.ToUpper(roleName), @@ -617,6 +607,7 @@ func TestAddTargetToSpecifiedInvalidRoles(t *testing.T) { func TestAddTargetErrorWritingChanges(t *testing.T) { // Temporary directory where test files will be created tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.RemoveAll(tempBaseDir) assert.NoError(t, err, "failed to create a temporary directory: %s", err) gun := "docker.com/notary" @@ -629,15 +620,152 @@ func TestAddTargetErrorWritingChanges(t *testing.T) { target, err := NewTarget("latest", "../fixtures/intermediate-ca.crt") assert.NoError(t, err, "error creating target") - os.RemoveAll(tempBaseDir) - // ioutil.WriteFile seems to create all the directories necessary, so make - // it impossible to do so - ioutil.WriteFile(tempBaseDir, []byte("hi"), 0644) - defer os.Remove(tempBaseDir) + // first, make the actual changefile unwritable by making the changelist + // directory unwritable + changelistPath := filepath.Join(repo.tufRepoPath, "changelist") + err = os.MkdirAll(changelistPath, 0744) + assert.NoError(t, err, "could not create changelist dir") + err = os.Chmod(changelistPath, 0600) + assert.NoError(t, err, "could not change permission of changelist dir") err = repo.AddTarget(target, data.CanonicalTargetsRole) assert.Error(t, err, "Expected an error writing the change") assert.IsType(t, &os.PathError{}, err) + + // then break prevent the changlist directory from being able to be created + err = os.Chmod(changelistPath, 0744) + assert.NoError(t, err, "could not change permission of temp dir") + err = os.RemoveAll(changelistPath) + assert.NoError(t, err, "could not remove changelist dir") + // creating a changelist file so the directory can't be created + err = ioutil.WriteFile(changelistPath, []byte("hi"), 0644) + assert.NoError(t, err, "could not write temporary file") + + err = repo.AddTarget(target, data.CanonicalTargetsRole) + assert.Error(t, err, "Expected an error writing the change") + assert.IsType(t, &os.PathError{}, err) +} + +// TestRemoveTargetToTargetRoleByDefault removes a target without specifying a +// role from a repo. Confirms that the changelist is created correctly for +// the targets scope. +func TestRemoveTargetToTargetRoleByDefault(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.RemoveAll(tempBaseDir) + + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + + gun := "docker.com/notary" + + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) + + testAddOrDeleteTarget(t, repo, changelist.ActionDelete, nil, + []string{data.CanonicalTargetsRole}) +} + +// TestRemoveTargetFromSpecifiedValidRoles removes a target from the specified +// roles. Confirms that the changelist is created correctly, one for each of +// the the specified roles as scopes. +func TestRemoveTargetFromSpecifiedValidRoles(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.RemoveAll(tempBaseDir) + + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + + gun := "docker.com/notary" + + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) + + roleName := filepath.Join(data.CanonicalTargetsRole, "a") + testAddOrDeleteTarget(t, repo, changelist.ActionDelete, + []string{ + strings.ToUpper(data.CanonicalTargetsRole), + strings.ToUpper(roleName), + }, + []string{data.CanonicalTargetsRole, roleName}) +} + +// TestRemoveTargetFromSpecifiedInvalidRoles expects errors to be returned if +// removing a target to an invalid role. If any of the roles are invalid, +// no targets are removed from any roles. +func TestRemoveTargetToSpecifiedInvalidRoles(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + defer os.RemoveAll(tempBaseDir) + + gun := "docker.com/notary" + + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) + + invalidRoles := []string{ + data.CanonicalRootRole, + data.CanonicalSnapshotRole, + data.CanonicalTimestampRole, + "target/otherrole", + "otherrole", + } + + for _, invalidRole := range invalidRoles { + err = repo.RemoveTarget(data.CanonicalTargetsRole, invalidRole) + assert.Error(t, err, "Expected an ErrInvalidRole error") + assert.IsType(t, data.ErrInvalidRole{}, err) + + changes := getChanges(t, repo) + assert.Len(t, changes, 0) + } +} + +// TestRemoveTargetErrorWritingChanges expects errors writing a change to file +// to be propagated. +func TestRemoveTargetErrorWritingChanges(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.Remove(tempBaseDir) + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + + gun := "docker.com/notary" + + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) + + // first, make the actual changefile unwritable by making the changelist + // directory unwritable + changelistPath := filepath.Join(repo.tufRepoPath, "changelist") + err = os.MkdirAll(changelistPath, 0744) + assert.NoError(t, err, "could not create changelist dir") + err = os.Chmod(changelistPath, 0600) + assert.NoError(t, err, "could not change permission of changelist dir") + + err = repo.RemoveTarget(data.CanonicalTargetsRole) + assert.Error(t, err, "Expected an error writing the change") + assert.IsType(t, &os.PathError{}, err) + + // then break prevent the changlist directory from being able to be created + err = os.Chmod(changelistPath, 0744) + assert.NoError(t, err, "could not change permission of temp dir") + err = os.RemoveAll(changelistPath) + assert.NoError(t, err, "could not remove changelist dir") + // creating a changelist file so the directory can't be created + err = ioutil.WriteFile(changelistPath, []byte("hi"), 0644) + assert.NoError(t, err, "could not write temporary file") + + err = repo.RemoveTarget(data.CanonicalTargetsRole) + assert.Error(t, err, "Expected an error writing the change") + assert.IsType(t, &os.PathError{}, err) } // TestListTarget fakes serving signed metadata files over the test's