From b82863152fc7580017bb4bd2935ad30456c84546 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Wed, 20 Jul 2016 23:26:43 -0700 Subject: [PATCH 1/2] Ensure what we do not clobber local key id data for nested delegations on list Signed-off-by: Riyaz Faizullabhoy --- client/delegations.go | 17 ++++++----- cmd/notary/integration_test.go | 52 ++++++++++++++++++++++++++++++++++ cmd/notary/prettyprint.go | 4 +-- cmd/notary/prettyprint_test.go | 4 +-- 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/client/delegations.go b/client/delegations.go index 5fbee5af2..2c972a824 100644 --- a/client/delegations.go +++ b/client/delegations.go @@ -238,7 +238,7 @@ func newDeleteDelegationChange(name string, content []byte) *changelist.TUFChang // GetDelegationRoles returns the keys and roles of the repository's delegations // Also converts key IDs to canonical key IDs to keep consistent with signing prompts -func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) { +func (r *NotaryRepository) GetDelegationRoles() ([]data.Role, error) { // Update state of the repo to latest if err := r.Update(false); err != nil { return nil, err @@ -251,7 +251,7 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) { } // make a copy for traversing nested delegations - allDelegations := []*data.Role{} + allDelegations := []data.Role{} // Define a visitor function to populate the delegations list and translate their key IDs to canonical IDs delegationCanonicalListVisitor := func(tgt *data.SignedTargets, validRole data.DelegationRole) interface{} { @@ -271,20 +271,23 @@ func (r *NotaryRepository) GetDelegationRoles() ([]*data.Role, error) { return allDelegations, nil } -func translateDelegationsToCanonicalIDs(delegationInfo data.Delegations) ([]*data.Role, error) { - canonicalDelegations := make([]*data.Role, len(delegationInfo.Roles)) - copy(canonicalDelegations, delegationInfo.Roles) +func translateDelegationsToCanonicalIDs(delegationInfo data.Delegations) ([]data.Role, error) { + canonicalDelegations := make([]data.Role, len(delegationInfo.Roles)) + // Do a copy by value to ensure local delegation metadata is untouched + for idx, origRole := range delegationInfo.Roles { + canonicalDelegations[idx] = *origRole + } delegationKeys := delegationInfo.Keys for i, delegation := range canonicalDelegations { canonicalKeyIDs := []string{} for _, keyID := range delegation.KeyIDs { pubKey, ok := delegationKeys[keyID] if !ok { - return nil, fmt.Errorf("Could not translate canonical key IDs for %s", delegation.Name) + return []data.Role{}, fmt.Errorf("Could not translate canonical key IDs for %s", delegation.Name) } canonicalKeyID, err := utils.CanonicalKeyID(pubKey) if err != nil { - return nil, fmt.Errorf("Could not translate canonical key IDs for %s: %v", delegation.Name, err) + return []data.Role{}, fmt.Errorf("Could not translate canonical key IDs for %s: %v", delegation.Name, err) } canonicalKeyIDs = append(canonicalKeyIDs, canonicalKeyID) } diff --git a/cmd/notary/integration_test.go b/cmd/notary/integration_test.go index 3b6761b84..e5931d043 100644 --- a/cmd/notary/integration_test.go +++ b/cmd/notary/integration_test.go @@ -946,6 +946,58 @@ func TestClientDelegationsPublishing(t *testing.T) { output, err = runCommand(t, tempDir, "-s", server.URL, "list", "gun", "--roles", "targets/releases") require.NoError(t, err) require.Contains(t, output, "targets/releases") + + // Setup another certificate + tempFile2, err := ioutil.TempFile("", "pemfile2") + require.NoError(t, err) + + privKey, err = utils.GenerateECDSAKey(rand.Reader) + startTime = time.Now() + endTime = startTime.AddDate(10, 0, 0) + cert, err = cryptoservice.GenerateCertificate(privKey, "gun", startTime, endTime) + require.NoError(t, err) + + _, err = tempFile2.Write(utils.CertToPEM(cert)) + require.NoError(t, err) + require.NoError(t, err) + tempFile2.Close() + defer os.Remove(tempFile2.Name()) + + rawPubBytes2, _ := ioutil.ReadFile(tempFile2.Name()) + parsedPubKey2, _ := utils.ParsePEMPublicKey(rawPubBytes2) + keyID2, err := utils.CanonicalKeyID(parsedPubKey2) + require.NoError(t, err) + + // add a nested delegation under this releases role + output, err = runCommand(t, tempDir, "delegation", "add", "gun", "targets/releases/nested", tempFile2.Name(), "--paths", "nested/path") + require.NoError(t, err) + require.Contains(t, output, "Addition of delegation role") + require.Contains(t, output, keyID2) + + // publish repo + _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") + require.NoError(t, err) + + // list delegations - we should see two roles + output, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "list", "gun") + require.NoError(t, err) + require.Contains(t, output, "targets/releases") + require.Contains(t, output, "targets/releases/nested") + require.Contains(t, output, canonicalKeyID) + require.Contains(t, output, keyID2) + require.Contains(t, output, "nested/path") + require.Contains(t, output, "\"\"") + require.Contains(t, output, "") + + // remove by force to delete the nested delegation entirely + output, err = runCommand(t, tempDir, "delegation", "remove", "gun", "targets/releases/nested", "-y") + require.NoError(t, err) + require.Contains(t, output, "Forced removal (including all keys and paths) of delegation role") + + // publish repo + _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") + require.NoError(t, err) + } // Splits a string into lines, and returns any lines that are not empty ( diff --git a/cmd/notary/prettyprint.go b/cmd/notary/prettyprint.go index 5fcdbf027..51201870e 100644 --- a/cmd/notary/prettyprint.go +++ b/cmd/notary/prettyprint.go @@ -140,7 +140,7 @@ func (t targetsSorter) Less(i, j int) bool { // --- pretty printing roles --- -type roleSorter []*data.Role +type roleSorter []data.Role func (r roleSorter) Len() int { return len(r) } func (r roleSorter) Swap(i, j int) { r[i], r[j] = r[j], r[i] } @@ -173,7 +173,7 @@ func prettyPrintTargets(ts []*client.TargetWithRole, writer io.Writer) { } // Pretty-prints the list of provided Roles -func prettyPrintRoles(rs []*data.Role, writer io.Writer, roleType string) { +func prettyPrintRoles(rs []data.Role, writer io.Writer, roleType string) { if len(rs) == 0 { writer.Write([]byte(fmt.Sprintf("\nNo %s present in this repository.\n\n", roleType))) return diff --git a/cmd/notary/prettyprint_test.go b/cmd/notary/prettyprint_test.go index 9eec0c64c..10ed6407c 100644 --- a/cmd/notary/prettyprint_test.go +++ b/cmd/notary/prettyprint_test.go @@ -205,7 +205,7 @@ func TestPrettyPrintSortedTargets(t *testing.T) { // are no roles. func TestPrettyPrintZeroRoles(t *testing.T) { var b bytes.Buffer - prettyPrintRoles([]*data.Role{}, &b, "delegations") + prettyPrintRoles([]data.Role{}, &b, "delegations") text, err := ioutil.ReadAll(&b) require.NoError(t, err) @@ -218,7 +218,7 @@ func TestPrettyPrintZeroRoles(t *testing.T) { func TestPrettyPrintSortedRoles(t *testing.T) { var err error - unsorted := []*data.Role{ + unsorted := []data.Role{ {Name: "targets/zebra", Paths: []string{"stripes", "black", "white"}, RootRole: data.RootRole{KeyIDs: []string{"101"}, Threshold: 1}}, {Name: "targets/aardvark/unicorn/pony", Paths: []string{"rainbows"}, RootRole: data.RootRole{KeyIDs: []string{"135"}, Threshold: 1}}, {Name: "targets/bee", Paths: []string{"honey"}, RootRole: data.RootRole{KeyIDs: []string{"246"}, Threshold: 1}}, From a7321282a3e19c8a29a52d9f0db14df30fe7f5b8 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Fri, 22 Jul 2016 11:13:06 -0700 Subject: [PATCH 2/2] Add to unit test to ensure delegation roles are respected and unchanged when listing and converting to canonical ID Signed-off-by: Riyaz Faizullabhoy --- client/client_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/client/client_test.go b/client/client_test.go index f45a910f8..e3727e8a2 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2325,6 +2325,25 @@ func TestPublishTargetsDelegationSuccessNeedsToDownloadRoles(t *testing.T) { // delegation parents all get signed ownerRec.requireAsked(t, []string{data.CanonicalTargetsRole, "targets/a"}) + // assert both delegation roles appear to the other repo in a call to GetDelegationRoles + delgRoleList, err := delgRepo.GetDelegationRoles() + require.NoError(t, err) + require.Len(t, delgRoleList, 2) + // The walk is a pre-order so we can enforce ordering. Also check that canonical key IDs are reported from this walk + require.Equal(t, delgRoleList[0].Name, "targets/a") + require.NotContains(t, delgRoleList[0].KeyIDs, ownerRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs) + canonicalAKeyID, err := utils.CanonicalKeyID(aKey) + require.NoError(t, err) + require.Contains(t, delgRoleList[0].KeyIDs, canonicalAKeyID) + require.Equal(t, delgRoleList[1].Name, "targets/a/b") + require.NotContains(t, delgRoleList[1].KeyIDs, ownerRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs) + canonicalBKeyID, err := utils.CanonicalKeyID(bKey) + require.NoError(t, err) + require.Contains(t, delgRoleList[1].KeyIDs, canonicalBKeyID) + // assert that the key ID data didn't somehow change between the two repos, since we translated to canonical key IDs + require.Equal(t, ownerRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs, delgRepo.tufRepo.Targets[data.CanonicalTargetsRole].Signed.Delegations.Roles[0].KeyIDs) + require.Equal(t, ownerRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs, delgRepo.tufRepo.Targets["targets/a"].Signed.Delegations.Roles[0].KeyIDs) + // delegated repo now publishes to delegated roles, but it will need // to download those roles first, since it doesn't know about them requirePublishToRolesSucceeds(t, delgRepo, []string{"targets/a/b"},