From 448082f333f0b4d7774c4085927e1206459fd2a0 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 5 Jun 2018 16:04:41 -0700 Subject: [PATCH 1/4] Fix docker trust signer removal Signed-off-by: Nassim 'Nass' Eddequiouaq --- cli/command/trust/signer_remove.go | 35 +++++++++++++++---------- cli/command/trust/signer_remove_test.go | 4 +-- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index 27774632bf..a8b697c242 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -43,11 +43,13 @@ func removeSigner(cli command.Cli, options signerRemoveOptions) error { var errRepos []string for _, repo := range options.repos { fmt.Fprintf(cli.Out(), "Removing signer \"%s\" from %s...\n", options.signer, repo) - if err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { + if didRemove, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { fmt.Fprintln(cli.Err(), err.Error()+"\n") errRepos = append(errRepos, repo) } else { - fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) + if didRemove { + fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) + } } } if len(errRepos) > 0 { @@ -78,24 +80,24 @@ func isLastSignerForReleases(roleWithSig data.Role, allRoles []client.RoleWithSi return counter < releasesRoleWithSigs.Threshold, nil } -func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) error { +func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) (bool, error) { ctx := context.Background() imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, image.AuthResolver(cli), repoName) if err != nil { - return err + return false, err } signerDelegation := data.RoleName("targets/" + signerName) if signerDelegation == releasesRoleTUFName { - return fmt.Errorf("releases is a reserved keyword and cannot be removed") + return false, fmt.Errorf("releases is a reserved keyword and cannot be removed") } notaryRepo, err := cli.NotaryClient(imgRefAndAuth, trust.ActionsPushAndPull) if err != nil { - return trust.NotaryError(imgRefAndAuth.Reference().Name(), err) + return false, trust.NotaryError(imgRefAndAuth.Reference().Name(), err) } delegationRoles, err := notaryRepo.GetDelegationRoles() if err != nil { - return errors.Wrapf(err, "error retrieving signers for %s", repoName) + return false, errors.Wrapf(err, "error retrieving signers for %s", repoName) } var role data.Role for _, delRole := range delegationRoles { @@ -105,11 +107,11 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b } } if role.Name == "" { - return fmt.Errorf("No signer %s for repository %s", signerName, repoName) + return false, fmt.Errorf("No signer %s for repository %s", signerName, repoName) } allRoles, err := notaryRepo.ListRoles() if err != nil { - return err + return false, err } if ok, err := isLastSignerForReleases(role, allRoles); ok && !forceYes { removeSigner := command.PromptForConfirmation(os.Stdin, cli.Out(), fmt.Sprintf("The signer \"%s\" signed the last released version of %s. "+ @@ -120,16 +122,21 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b if !removeSigner { fmt.Fprintf(cli.Out(), "\nAborting action.\n") - return nil + return false, nil } } else if err != nil { - return err + return false, err } if err = notaryRepo.RemoveDelegationKeys(releasesRoleTUFName, role.KeyIDs); err != nil { - return err + return false, err } if err = notaryRepo.RemoveDelegationRole(signerDelegation); err != nil { - return err + return false, err } - return notaryRepo.Publish() + + if err = notaryRepo.Publish(); err != nil { + return false, err + } + + return true, nil } diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go index 4f11634be2..f2ca1e7af8 100644 --- a/cli/command/trust/signer_remove_test.go +++ b/cli/command/trust/signer_remove_test.go @@ -71,9 +71,9 @@ func TestTrustSignerRemoveErrors(t *testing.T) { func TestRemoveSingleSigner(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository) - err := removeSingleSigner(cli, "signed-repo", "test", true) + _, err := removeSingleSigner(cli, "signed-repo", "test", true) assert.Error(t, err, "No signer test for repository signed-repo") - err = removeSingleSigner(cli, "signed-repo", "releases", true) + _, err = removeSingleSigner(cli, "signed-repo", "releases", true) assert.Error(t, err, "releases is a reserved keyword and cannot be removed") } From 5ebb7a65ab3c7e6b40c1751c36fe342186e6318a Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 5 Jun 2018 22:17:32 -0700 Subject: [PATCH 2/4] Fix tests and nit Signed-off-by: Nassim 'Nass' Eddequiouaq --- cli/command/trust/signer_remove.go | 6 ++---- cli/command/trust/signer_remove_test.go | 5 ++++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index a8b697c242..f945a8e111 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -46,10 +46,8 @@ func removeSigner(cli command.Cli, options signerRemoveOptions) error { if didRemove, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { fmt.Fprintln(cli.Err(), err.Error()+"\n") errRepos = append(errRepos, repo) - } else { - if didRemove { - fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) - } + } else if didRemove { + fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) } } if len(errRepos) > 0 { diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go index f2ca1e7af8..27a622bd5b 100644 --- a/cli/command/trust/signer_remove_test.go +++ b/cli/command/trust/signer_remove_test.go @@ -71,10 +71,13 @@ func TestTrustSignerRemoveErrors(t *testing.T) { func TestRemoveSingleSigner(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository) - _, err := removeSingleSigner(cli, "signed-repo", "test", true) + didRemove, err := removeSingleSigner(cli, "signed-repo", "test", true) assert.Error(t, err, "No signer test for repository signed-repo") + assert.Equal(t, didRemove, false, "No signer should be removed") + _, err = removeSingleSigner(cli, "signed-repo", "releases", true) assert.Error(t, err, "releases is a reserved keyword and cannot be removed") + assert.Equal(t, didRemove, false, "No signer should be removed") } func TestRemoveMultipleSigners(t *testing.T) { From 2b3361cc1ad45c14a413a62b5dbfd3b8a8608357 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Fri, 8 Jun 2018 09:21:36 -0700 Subject: [PATCH 3/4] Move the successful removal print Signed-off-by: Nassim 'Nass' Eddequiouaq --- cli/command/trust/signer_remove.go | 8 +++++--- cli/command/trust/signer_remove_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index f945a8e111..5bd586c536 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -43,11 +43,9 @@ func removeSigner(cli command.Cli, options signerRemoveOptions) error { var errRepos []string for _, repo := range options.repos { fmt.Fprintf(cli.Out(), "Removing signer \"%s\" from %s...\n", options.signer, repo) - if didRemove, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { + if _, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { fmt.Fprintln(cli.Err(), err.Error()+"\n") errRepos = append(errRepos, repo) - } else if didRemove { - fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) } } if len(errRepos) > 0 { @@ -78,6 +76,8 @@ func isLastSignerForReleases(roleWithSig data.Role, allRoles []client.RoleWithSi return counter < releasesRoleWithSigs.Threshold, nil } +// removeSingleSigner returns whether the signer has been removed during this operation and an error +// Note: the signer not being removed doesn't necessarily raise an error (eg. User saying "No" to the confirmation prompt) func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) (bool, error) { ctx := context.Background() imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, image.AuthResolver(cli), repoName) @@ -136,5 +136,7 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b return false, err } + fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", signerName, repoName) + return true, nil } diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go index 27a622bd5b..ac81c1a676 100644 --- a/cli/command/trust/signer_remove_test.go +++ b/cli/command/trust/signer_remove_test.go @@ -71,13 +71,13 @@ func TestTrustSignerRemoveErrors(t *testing.T) { func TestRemoveSingleSigner(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository) - didRemove, err := removeSingleSigner(cli, "signed-repo", "test", true) + removed, err := removeSingleSigner(cli, "signed-repo", "test", true) assert.Error(t, err, "No signer test for repository signed-repo") - assert.Equal(t, didRemove, false, "No signer should be removed") + assert.Equal(t, removed, false, "No signer should be removed") - _, err = removeSingleSigner(cli, "signed-repo", "releases", true) + removed, err = removeSingleSigner(cli, "signed-repo", "releases", true) assert.Error(t, err, "releases is a reserved keyword and cannot be removed") - assert.Equal(t, didRemove, false, "No signer should be removed") + assert.Equal(t, removed, false, "No signer should be removed") } func TestRemoveMultipleSigners(t *testing.T) { From 92c39dd0abee2431bddb93aebfc2b4e42dba5825 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Sat, 9 Jun 2018 08:14:34 -0700 Subject: [PATCH 4/4] Fix removeSingleSigner description Signed-off-by: Nassim 'Nass' Eddequiouaq --- cli/command/trust/signer_remove.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index 5bd586c536..d4e8eec42b 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -76,8 +76,8 @@ func isLastSignerForReleases(roleWithSig data.Role, allRoles []client.RoleWithSi return counter < releasesRoleWithSigs.Threshold, nil } -// removeSingleSigner returns whether the signer has been removed during this operation and an error -// Note: the signer not being removed doesn't necessarily raise an error (eg. User saying "No" to the confirmation prompt) +// removeSingleSigner attempts to remove a single signer and returns whether signer removal happened. +// The signer not being removed doesn't necessarily raise an error e.g. user choosing "No" when prompted for confirmation. func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) (bool, error) { ctx := context.Background() imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, image.AuthResolver(cli), repoName)