Delegation Support #27

murphybytes · 2017-06-30T15:36:14Z

WIP This PR adds delegation support per the TUF spec. Note that currently the save and backup file handling only supports the fixed roles root, timestamp, snapshot, and targets. targets will now need handle a tree path structure so I'm in the process of handling that. I'm also in the process of adding more tests, and test scenarios.

groob

I made a few comments regarding style/structure of the PR.

I'll make a separate review to address correctness but I have to set some time to assess how to traverse the target delegates graph first.

groob · 2017-06-30T15:39:28Z

tuf/local_repo.go

+	}
+	defer f.Close()
+	var result Targets
+	err = json.NewDecoder(f).Decode(&result)


nit, but if there's a single return and that return is an error, you should do the error handling on the same line.

if err := ... ; err != nil {

groob · 2017-06-30T15:40:44Z

tuf/delegation_test.go

+}
+
+func TestMockReader(t *testing.T) {
+	rdr := mockLocalRepoReader{"test/delegation/0/"}


wrap these in a table with a subtest.

groob · 2017-06-30T15:41:53Z

tuf/delegation_test.go

+		{"targets/bar", 3},
+	}
+	for _, tc := range tt {
+		targ, ok := root.targetLookup[tc.role]


use t.Run inside the loop to create subtests for each case. It helps when the test fails because you know which of the cases is causing the error.

groob · 2017-06-30T15:43:49Z

tuf/delegation_test.go

+	}
+}
+
+func setupValidationTest(testRoot string) (*Root, *Snapshot, *RootTarget, error) {


when creating a test helper, you can add *testing.T as a required argument, and then call t.Fatal for error handling.

setupValidationTest(t *testing.T, testRoot string) (*Root, *Snapshot, *RootTarget) { // do some setup require.Nil(t, err) }

groob · 2017-06-30T15:44:45Z

tuf/delegation_test.go

+		w.Write(buff)
+	}))
+	defer svr.Close()
+	rootRole, snapshotRole, rootTarget, err := setupValidationTest(testRootPath)


see my comment about passing testing.T. It would remove the need for the following 4 lines of test setup.

groob · 2017-06-30T15:50:49Z

tuf/remote_repo.go

+	if !ok {
+		return nil
+	}
+	// 4.3. **Check for a rollback attack.** The version number of the previous


groob · 2017-06-30T15:57:34Z

tuf/tuf.go

+	errLengthIncorrect        = errors.New("file length incorrect")
+	errNoSuchTarget           = errors.New("no such target")
+	errNotFound               = errors.New("remote resource does not exist")
+	errMaxDelegationsExceeded = errors.New("too many delegations")


how is this different from line 30?

groob · 2017-06-30T15:57:49Z

tuf/tuf.go

+	errMaxDelegationsExceeded = errors.New("too many delegations")
+	errTargetSeen             = errors.New("target already seen in tree")
+	errFailedIntegrityCheck   = errors.New("target file fails integrity check")
+	errTooManyDelegates       = errors.Errorf("number of delegates exceeds max %d", maxDelegationCount)


Is max delegationCount in the spec?

groob · 2017-06-30T16:03:46Z

tuf/roles.go

+	return newFim
+}
+
+func (f FileIntegrityMeta) equal(fim *FileIntegrityMeta) bool {


Why is the first argument a value but the second a pointer?

I pass the receiver by value to indicate that it won't be modified. clone creates a new structure so imho it should follow customary Go constructor semantics which usually involve making something and returning a pointer to the new thing.

the comment is about equal not clone.

groob · 2017-06-30T16:07:11Z

tuf/client.go

@@ -116,7 +116,7 @@ func NewClient(settings *Settings, opts ...Option) (*Client, error) {
 // Note that we expect that we do not use consistent snapshots and delegations are
 // not supported because for our purposes, both are unnecessary.
 // See https://github.com/theupdateframework/tuf/blob/904fa9b8df8ab8c632a210a2b05fd741e366788a/docs/tuf-spec.txt
-func (c *Client) Update() (files map[string]FileIntegrityMeta, latest bool, err error) {
+func (c *Client) Update() (files fimMap, latest bool, err error) {


Don't return a private type in a public method. It makes it impossible to pass files to a function outside this package.

Good catch, thank you.

Closes the download target file descriptor before calling the notification handler callback. If the file is still open, operations like fork/exec are not permitted.

groob

nits and a few questions.

Still need to review the actual delegation feature at a later time.

groob · 2017-07-05T13:46:09Z

tuf/client.go

-
-	client          *http.Client
-	maxResponseSize int64
+	klock               clock.Clock


change the field to clock

groob · 2017-07-05T13:48:46Z

tuf/client.go

 //
 // Update gets the current metadata from the notary repository and performs
 // requisite checks and validations as specified in the TUF spec section 5.1 'The Client Application'.
 // Note that we expect that we do not use consistent snapshots and delegations are
 // not supported because for our purposes, both are unnecessary.
 // See https://github.com/theupdateframework/tuf/blob/904fa9b8df8ab8c632a210a2b05fd741e366788a/docs/tuf-spec.txt
-func (c *Client) Update() (files map[string]FileIntegrityMeta, latest bool, err error) {
-	latest, err = c.manager.refresh()
+func (c *Client) Update() ([]string, error) {


Mind clarifying why it's a []string now? Why is it a change for the better. How would the caller access hashes etc?

The purpose of the caller accessing hashes was to detect changes. This change detection process now happens upstream in the processing flow at the end of refreshTargets, where it was happening anyway. In this way you don't have to try to detect changes in two places. Also the change detection process is more straight forward and removes the need to maintain state in the monitor loop. In addition, the earlier way of doing it was broken. The behavior we wanted for monitoring, is that when it starts you want to pull down changes immediately. This was broken because the initial comparison hash was empty and so you have to go through a full refresh cycle to see if any changes happen.

groob · 2017-07-05T13:50:42Z

tuf/client.go

 	if client.watchedTarget != "" {
 		go client.monitorTarget()
 	}
 	return &client, nil
 }

 // Update updates the local TUF metadata from a remote repository. If the update is successful,
-// a dictionary of available files will be returned.
+// a list of files that have changed will be returned.


a list of files that have changed suggests that if someone else calls 'Update' I will not see the local files, so I might get a empty slice of changes, even though actual changes happened. Is that correct?

A bit confused about the rationale.

Given the set of all targets in your respository, you will get a subset of targets that have changed. So if I understand your question, the answer is no, when changes to a target happens, that target will always be included in the list. There is one exception to this rule. If you have two or more delegates, each having the same target, one delegate will have higher precedence than the other. In this case we only detect changes in the highest precedence target. This behavior is due to TUF specifying a preorder depth first search of the delegate tree, cataloguing the first target it encounters and discarding equal targets as it continues to visit each target in the traversal.

My question is more about Update being invoked 2 times in a row by the same caller vs being invoked by two different callers independently. Who gets the slice? both?

I see your point. In the case of two or more independent callers, this would not work. The first update call will get changes, the second will not. Back to the drawing board.

groob · 2017-07-05T13:51:53Z

tuf/client.go

 		return errors.Wrap(err, "downloading target")
 	}
 	return nil
 }

 func (c *Client) monitorTarget() {
-	var hash string
-	ticker := time.NewTicker(c.checkFrequency).C
+	ticker := c.klock.NewTicker(c.checkFrequency)


call .Chan() here, and keep ticker a channel.

groob · 2017-07-05T13:53:21Z

tuf/client.go

-	if old == "" || old == new {
-		return
+func hasTarget(target string, changes []string) bool {
+	if changes != nil {


There's no reason to check if changes is nil. Drop the conditional and just use the for loop.

groob · 2017-07-05T14:11:15Z

tuf/persistence.go

+		return errors.Wrap(err, "marshalling role")
+	}
+	rolePath := filepath.Join(tufRoot, fileName)
+	f, err := os.OpenFile(rolePath, os.O_CREATE|os.O_WRONLY, 0644)


If buff is not a reader you're better off using ioutil.WriteFile I think.

groob · 2017-07-05T14:12:42Z

tuf/delegation_test.go

+
+	rrs := notaryTargetFetcherSettings{
+		gun: testRootPath,
+		client: &http.Client{


there's a helper so you don't have to create this struct each time.

groob · 2017-07-05T14:14:44Z

tuf/delegation_test.go

+func TestEndToEnd(t *testing.T) {
+	testTime, _ := time.Parse(time.UnixDate, "Sat Jul 1 18:00:00 CST 2017")
+	mockClock := clock.NewMockClock(testTime)
+	client := &http.Client{


use existing helper for insecure http client.

groob · 2017-07-05T14:20:14Z

tuf/delegation_test.go

+				MirrorURL:     mirrorServer.URL,
+				GUN:           testGUN,
+			}
+			tc.testCase(t, &settings, client, stagingDir, mockClock)


IMO the 30+ lines of setup really hurt the readability of this test. You could move all that code to a helper like

settings, cleanup := setupEndToEndTest(t, args,...) defer cleanup()

groob · 2017-07-05T14:23:05Z

tuf/client.go

 	for {
-		files, _, err := c.Update()
+		changes, err := c.Update()


Noted in another comment, but this assumes Update is only ever called from inside the for loop.

Update can be called by anyone at any time, so the next time the timer ticks changes will be empty.

groob

a few more nits, but this LGTM overall.

groob · 2017-07-06T13:36:53Z

tuf/client.go

 		if err != nil {
 			c.notificationHandler("", err)
 		}
-		if hasTarget(c.watchedTarget, changes) {
-			c.download()
+		new, ok := files[c.watchedTarget]


New is a go keyword. Let's try to avoid using it as a variable.

groob · 2017-07-06T13:43:43Z

tuf/persistence.go

 func restoreTUFRepo(tufRoot, tag string) error {
-	err := checkForDirectoryPresence(tufRoot)
-	if err != nil {
+	var err error


Why declare var err error? You only need to create the err in the if scope each time.

if err := ...
That is common across most Go codebases and is also the pattern shown in Effective Go.
Am I missing a specific reason to do that?

I don't know why I was thinking I needed these, I don't. I'll change before I merge.

groob · 2017-07-06T13:47:07Z

tuf/persistence.go

+// Restores local TUF repository finding all backup files with a matching tag
+// and copying them to normal TUF files.
+func restoreTUFRepo(tufRoot, tag string) error {
+	var err error


remove the var declaration

groob · 2017-07-06T13:47:17Z

tuf/persistence.go

+
+// Remove backups files that are older than the time duration specified by age.
+func removeAgedBackups(tufRoot string, age time.Duration) error {
+	var err error


remove the var declaration

groob · 2017-07-06T13:48:33Z

tuf/remote_repo.go

+	rootRole        *Root
+	snapshotRole    *Snapshot
+	localRootTarget *RootTarget
+	klock           clock.Clock


rename klock to clock

groob · 2017-07-06T13:52:13Z

tuf/repo.go

+	for _, role := range target.Signed.Delegations.Roles {
+		err = getDelegatedTarget(rdr, root, role.Name)
+		// prevent cycles
+		if err != nil && err == errTargetSeen {


there's no possible case where err == nil && err == errTargetSeen. Can we drop the first part of the conditional?

That's true, but that's not what the code on 180 is.

What do you mean by "not what the code on 180 is"?

It's if err != nil && err == errTargetSeen look at the first condition.

My comment says that if you shorten that to if err == errTargetSeen you should get the same outcome.

Oh duh! You're right.

groob · 2017-07-06T13:53:20Z

tuf/repo.go

+	return &root, nil
+}
+
+func getDelegatedTarget(rdr roleFetcher, root *RootTarget, roleName string) error {


rdr should be f or fetcher

groob · 2017-07-06T13:55:21Z

tuf/repo.go

+	}
+	root.append(string(roleTargets), targ)
+
+	for _, delegation := range root.Signed.Delegations.Roles {


Is it possible for any part of the root.Signed.Delegations.Roles path to be nil and cause a panic?

No, all the child structs are values, not pointers. I suppose it's possible for the composition struct *Target could be nil but in that case it would panic higher in the call stack.

groob · 2017-07-06T13:56:46Z

tuf/client.go

 	for {
+		// Get the state of our current files from the local repo
+		if old == nil {


old will only be nil on the first iteration. Can it be created outside the loop?

murphybytes added 3 commits June 29, 2017 13:45

Got tree building logic working

010de74

tests green

1bfce91

wrapped up delagation verification logic

2d8f94a

groob suggested changes Jun 30, 2017

View reviewed changes

murphybytes and others added 14 commits June 30, 2017 12:24

Added doco and renamed things for clarification

0e34641

more and better names

25a7f39

close staging path file descriptor before executing callback (#28)

8ff7142

Closes the download target file descriptor before calling the notification handler callback. If the file is still open, operations like fork/exec are not permitted.

Got tree building logic working

a88b06f

tests green

5f4c30c

wrapped up delagation verification logic

8457049

Added doco and renamed things for clarification

1010505

more and better names

c3b778c

check in working persistence before client refactor

c8c25bf

Refactored repo save to support delegates

c0b9c55

fixed merge conflicts

fdc9ff1

removed some extraneous debug code

78816ab

checkin before refactor update

df9304e

finished delegation tests

56bfbf6

murphybytes requested review from zwass and marpaia July 3, 2017 21:46

groob reviewed Jul 5, 2017

View reviewed changes

Code review changes

b51d076

groob approved these changes Jul 6, 2017

View reviewed changes

More code review changes

f1e1d8c

murphybytes merged commit 82e35ed into kolide:master Jul 6, 2017

murphybytes deleted the delegation branch July 6, 2017 15:07

groob mentioned this pull request Aug 4, 2017

autoupdate: updates in a loop with latest updater/tuf version kolide/launcher#66

Closed

Delegation Support #27

Delegation Support #27

Conversation

murphybytes commented Jun 30, 2017

groob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment