From 3b3c24b9006e6d3e7145f3bec5c74fe83992917c Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 24 Jul 2016 14:47:53 -0700 Subject: [PATCH] addressing comments in PR Signed-off-by: David Lawrence (github: endophage) --- client/helpers.go | 2 +- cmd/notary/delegations.go | 8 +++--- cmd/notary/integration_test.go | 48 ++++++++++++++++++++++++++++++++++ docs/advanced_usage.md | 4 +-- tuf/data/roles.go | 12 ++++++--- tuf/data/roles_test.go | 4 +++ tuf/tuf.go | 32 +++++++++-------------- tuf/tuf_test.go | 4 +++ 8 files changed, 84 insertions(+), 30 deletions(-) diff --git a/client/helpers.go b/client/helpers.go index 7f012a0e9c..f188c3a7a6 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -40,7 +40,7 @@ func applyChangelist(repo *tuf.Repo, cl changelist.Changelist) error { if err != nil { return err } - isDel := data.IsDelegation(c.Scope()) + isDel := data.IsDelegation(c.Scope()) || data.IsWildDelegation(c.Scope()) switch { case c.Scope() == changelist.ScopeTargets || isDel: err = applyTargetsChange(repo, c) diff --git a/cmd/notary/delegations.go b/cmd/notary/delegations.go index 7514cb18d8..1e7fa73b53 100644 --- a/cmd/notary/delegations.go +++ b/cmd/notary/delegations.go @@ -34,8 +34,8 @@ var cmdDelegationRemoveTemplate = usageTemplate{ var cmdDelegationPurgeKeysTemplate = usageTemplate{ Use: "purge [ GUN ]", - Short: "Remove KeyID(s) from all delegations it is found in.", - Long: "Remove KeyID(s) from all delegations it is found in, for which the signing keys are available. Warnings will be printed for delegations that cannot be updated.", + Short: "Remove KeyID(s) from all delegation roles in the given GUN.", + Long: "Remove KeyID(s) from all delegation roles in the given GUN, for which the signing keys are available. Warnings will be printed for delegations that cannot be updated.", } var cmdDelegationAddTemplate = usageTemplate{ @@ -59,7 +59,7 @@ func (d *delegationCommander) GetCommand() *cobra.Command { cmd.AddCommand(cmdDelegationListTemplate.ToCommand(d.delegationsList)) cmdPurgeDelgKeys := cmdDelegationPurgeKeysTemplate.ToCommand(d.delegationPurgeKeys) - cmdPurgeDelgKeys.Flags().StringSliceVar(&d.keyIDs, "key", nil, "Delegation keys to be removed from the GUN") + cmdPurgeDelgKeys.Flags().StringSliceVar(&d.keyIDs, "key", nil, "Delegation key IDs to be removed from the GUN") cmd.AddCommand(cmdPurgeDelgKeys) cmdRemDelg := cmdDelegationRemoveTemplate.ToCommand(d.delegationRemove) @@ -112,7 +112,7 @@ func (d *delegationCommander) delegationPurgeKeys(cmd *cobra.Command, args []str err = nRepo.RemoveDelegationKeys("targets/*", d.keyIDs) if err != nil { - return fmt.Errorf("failed to remove delegation: %v", err) + return fmt.Errorf("failed to remove keys from delegations: %v", err) } fmt.Printf( "Removal of the following keys from all delegations in %s staged for next publish:\n\t- %s\n", diff --git a/cmd/notary/integration_test.go b/cmd/notary/integration_test.go index e9e99522d8..a69773b24f 100644 --- a/cmd/notary/integration_test.go +++ b/cmd/notary/integration_test.go @@ -1292,3 +1292,51 @@ func TestMain(m *testing.M) { } os.Exit(m.Run()) } + +func TestPurge(t *testing.T) { + setUp(t) + + tempDir := tempDirWithConfig(t, "{}") + defer os.RemoveAll(tempDir) + + server := setupServer() + defer server.Close() + + startTime := time.Now() + endTime := startTime.AddDate(10, 0, 0) + + // Setup certificates + tempFile, err := ioutil.TempFile("", "pemfile") + require.NoError(t, err) + privKey, err := utils.GenerateECDSAKey(rand.Reader) + cert, err := cryptoservice.GenerateCertificate(privKey, "gun", startTime, endTime) + require.NoError(t, err) + _, err = tempFile.Write(utils.CertToPEM(cert)) + require.NoError(t, err) + tempFile.Close() + defer os.Remove(tempFile.Name()) + rawPubBytes, _ := ioutil.ReadFile(tempFile.Name()) + parsedPubKey, _ := utils.ParsePEMPublicKey(rawPubBytes) + keyID, err := utils.CanonicalKeyID(parsedPubKey) + require.NoError(t, err) + + delgName := "targets/delegation" + + _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun") + require.NoError(t, err) + + _, err = runCommand(t, tempDir, "delegation", "add", "gun", delgName, tempFile.Name(), "--all-paths") + require.NoError(t, err) + + _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") + require.NoError(t, err) + + // remove targets key and ensure we fail to publish + os.RemoveAll(filepath.Join(tempDir, notary.PrivDir, notary.NonRootKeysSubdir)) + + _, err = runCommand(t, tempDir, "delegation", "purge", "gun", "--key", keyID) + require.NoError(t, err) + + _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") + require.Error(t, err) +} diff --git a/docs/advanced_usage.md b/docs/advanced_usage.md index 6c76bedc03..a3e1de39b2 100644 --- a/docs/advanced_usage.md +++ b/docs/advanced_usage.md @@ -188,8 +188,8 @@ Forced removal (including all keys and paths) of delegation role targets/release You can remove individual keys and/or paths by passing keys as arguments, and/or paths under the `--paths` flag. Use `--all-paths` to clear all paths for this -role. If you specify all key IDs currently in the delegation role, you will -delete the role entirely. +role. If you specify all key IDs currently in the delegation role, you will be left +with a role that is unusable as it will not have enough valid signatures. To add targets to a specified delegation role, we can use the `notary add` command with the `--roles` flag. diff --git a/tuf/data/roles.go b/tuf/data/roles.go index b3123b65a4..282377827a 100644 --- a/tuf/data/roles.go +++ b/tuf/data/roles.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/Sirupsen/logrus" - "path/filepath" ) // Canonical base role names @@ -102,9 +101,14 @@ func IsBaseRole(role string) bool { // The wildcard may only appear as the final part of the delegation and must // be a whole segment, i.e. targets/foo* is not a valid wildcard delegation. func IsWildDelegation(role string) bool { - base := filepath.Dir(role) - splat := filepath.Base(role) - return splat == "*" && (IsDelegation(base) || base == CanonicalTargetsRole) + if path.Clean(role) != role { + return false + } + base := path.Dir(role) + if !(IsDelegation(base) || base == CanonicalTargetsRole) { + return false + } + return role[len(role)-2:] == "/*" } // BaseRole is an internal representation of a root/targets/snapshot/timestamp role, with its public keys included diff --git a/tuf/data/roles_test.go b/tuf/data/roles_test.go index e697ae0e31..14dc38ac95 100644 --- a/tuf/data/roles_test.go +++ b/tuf/data/roles_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "fmt" "github.com/stretchr/testify/require" ) @@ -153,6 +154,9 @@ func TestIsWildDelegation(t *testing.T) { path.Join(CanonicalTimestampRole, "*"): f, path.Join(CanonicalTargetsRole, "*", "foo"): f, path.Join(CanonicalTargetsRole, "*", "*"): f, + fmt.Sprintf("%s//*", CanonicalTargetsRole): f, + fmt.Sprintf("%s/*//", CanonicalTargetsRole): f, + fmt.Sprintf("%s/*/", CanonicalTargetsRole): f, // true checks path.Join(CanonicalTargetsRole, "*"): tr, diff --git a/tuf/tuf.go b/tuf/tuf.go index 9711c3fa6a..34621b37ff 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -325,8 +325,7 @@ func delegationUpdateVisitor(roleName string, addKeys data.KeyList, removeKeys, break } } - // We didn't find the role earlier, so create it only if we have keys to add - // that are also sufficient to meet the threshold. + // We didn't find the role earlier, so create it. if addKeys == nil { addKeys = data.KeyList{} // initialize to empty list if necessary so calling .IDs() below won't panic } @@ -416,13 +415,9 @@ func (tr *Repo) PurgeDelegationKeys(role string, removeKeys []string) error { purgeKeys := func(tgt *data.SignedTargets, validRole data.DelegationRole) interface{} { var ( - deleteCandidates []string - err error - canSign bool - needsUpdateCantSign bool + deleteCandidates []string + err error ) - err = tr.VerifyCanSign(validRole.Name) - canSign = err == nil for id, key := range tgt.Signed.Delegations.Keys { var ( canonID string @@ -431,21 +426,20 @@ func (tr *Repo) PurgeDelegationKeys(role string, removeKeys []string) error { if canonID, ok = tufIDToCanon[id]; !ok { canonID, err = utils.CanonicalKeyID(key) if err != nil { - // TODO: should we return here or just log? return err } tufIDToCanon[id] = canonID } if _, ok := removeIDs[canonID]; ok { - if !canSign { - needsUpdateCantSign = true - break - } - delete(tgt.Signed.Delegations.Keys, id) deleteCandidates = append(deleteCandidates, id) } } - if needsUpdateCantSign { + if len(deleteCandidates) == 0 { + // none of the interesting keys were present. We're done with this role + return nil + } + // now we know there are changes, check if we'll be able to sign them in + if err := tr.VerifyCanSign(validRole.Name); err != nil { logrus.Warnf( "role %s contains keys being purged but you do not have the necessary keys present to sign it; keys will not be purged from %s or its immediate children", validRole.Name, @@ -453,11 +447,11 @@ func (tr *Repo) PurgeDelegationKeys(role string, removeKeys []string) error { ) return nil } - if len(deleteCandidates) == 0 { - // none of the interesting keys were present - return nil + // we know we can sign in the changes, delete the keys + for _, id := range deleteCandidates { + delete(tgt.Signed.Delegations.Keys, id) } - // delete candidate keys from all roles. Keep only roles that still have keys + // delete candidate keys from all roles. for _, role := range tgt.Signed.Delegations.Roles { role.RemoveKeys(deleteCandidates) if len(role.KeyIDs) < role.Threshold { diff --git a/tuf/tuf_test.go b/tuf/tuf_test.go index 681b9b3c2f..c40571ed7e 100644 --- a/tuf/tuf_test.go +++ b/tuf/tuf_test.go @@ -227,6 +227,10 @@ func TestPurgeDelegationsKeyFromTop(t *testing.T) { role, err = repo.GetDelegationRole(carrot) require.NoError(t, err) require.Len(t, role.Keys, 0) + + // we know id1 was successfully purged, try purging again and make sure it doesn't error + err = repo.PurgeDelegationKeys(targetsWild, []string{id1}) + require.NoError(t, err) } func TestPurgeDelegationsKeyFromDeep(t *testing.T) {