Witness command #875

endophage · 2016-07-26T06:18:51Z

Fixes #562.

cyli · 2016-07-26T20:44:25Z

tuf/builder.go


 	if err := signed.VerifyVersion(&(signedTargets.Signed.SignedCommon), minVersion); err != nil {
+		rb.invalidRoles.Targets[roleName] = signedTargets


Should this be a recoverable error in that it should be witness-able? If the version is too low, doesn't that means there's a newer version already (and hence witness could be reverting existing changes)?

cyli · 2016-07-29T22:12:52Z

client/witness.go

+func witnessTargets(repo *tuf.Repo, invalid *tuf.Repo, role string) error {
+	if r, ok := repo.Targets[role]; ok {
+		// role is already valid, mark for re-signing/updating
+		r.Dirty = true


This bumps the version too - should we have a second flag for witness that does not bump the version unless the expiry needs updating and just signs?

I'm fine with bumping the version after reading through the main-thread discussion but we should keep this consistent with the command's usage description (which mentions that publishing a new version is only for invalid roles)

Updated long form usage description.

endophage · 2016-07-29T22:19:31Z

@cyli re bumping version, I was implementing this version just to solve the immediate recovery case. Until we have actual thresholding it's kind of a moot point how we recover.

My instinct though is that because it was, in the past, a valid role that has become invalid, we should not allow the invalid version to become valid again, we should require it's published as a new version. That seems safer than allowing a version to flip flop between states, which seems like it could be abused to serve 2 different versions of a role. In fact we may want to look at adding an additional check that says "if I've seen valid version X and know it to have checksum Y, I will not accept a version X from the server with a checksum that differs from Y". Either that or require the version strictly increases (I think the current logic is >=)

cyli · 2016-07-29T22:24:16Z

@endophage Ok, that makes sense for recovering invalid roles. Should we just ignore the witness command though if the role is already valid? It seems pointless to re-sign.

endophage · 2016-07-29T22:27:01Z

I feel like there's still a use case for preemptively resigning if a role will expire soon. Like if I know I'm going on vacation but my data will expire while I'm away. I should be able to make it good in advance. Because it's an explicit user command rather than us trying to be clever about resigning I think this is the right behaviour.

cyli · 2016-07-29T22:34:10Z

In that case, would actually witnessing (e.g. for thresholds) require an extra flag to the witness command? Would it make sense for bumping the expiry and version to take a flag instead?

The word "witness" makes more sense to me to indicate that you are sort of rubber-stamping something, or agreeing with something that is already there. This behavior seems like it's just always re-generating new data, and not really "witnessing" something in the general sense of the word.

endophage · 2016-07-29T22:47:21Z

Your understanding of what this code currently does is spot on.

Because the staging/state of partially signed role for thresholds is TBD I was designing it to meet the current use case only (resigning a role that has become invalid). A full witness feature for thresholds is probably going to necessarily be online (to determine the available variations that could be witnessed), and require interactivity. In that situation, I would guess we would add a flag, as you suggested, to shortcut all that and just do the behaviour implemented in this PR (which would also be offered as an interactive option).

I'm just trying to keep the current command simple because a required flag feels wrong. The obvious question will be "what does the command do without the flag?", to which the answer would be "nothing".

riyazdf · 2016-07-29T23:53:57Z

client/tufclient.go

@@ -11,17 +12,26 @@ import (
 	"github.com/docker/notary/tuf/signed"
 )

-// Client is a usability wrapper around a raw TUF repo
-type Client struct {
+// ErrCorruptedCache - local data is incorrect


Can we delete this error? I think it's unused

cyli · 2016-08-01T19:59:53Z

cmd/notary/tuf.go

+var cmdWitnessTemplate = usageTemplate{
+	Use:   "witness [ GUN ] <role> ...",
+	Short: "Marks roles to be re-signed the next time they're published",
+	Long:  "Marks roles to be re-signed the next time they're published. If the role has no currently valid signatures, or is otherwise invalid, a new version is published. If a role has some valid signatures and is not otherwise invalid, new signatures are added without modifying the signed role data.",


Non-blocking - should we modify this comment since for now, if the role is valid, the version is bumped?

cyli · 2016-08-01T20:04:58Z

Thanks for adding this! Can we add witness gun targets/releases to the list of example valid commands here: https://github.com/docker/notary/blob/master/cmd/notary/main_test.go#L151? That way we can test the number of command line arguments.

Other than that and the version test, this LGTM!

riyazdf · 2016-08-01T20:53:34Z

cmd/notary/integration_test.go

+	require.Contains(t, output, targetName)
+	require.Contains(t, output, targetHash)
+
+	// 11. witness an invalid role and check for error on publish


this entire test flow is awesome! 👍

riyazdf · 2016-08-01T21:19:12Z

cmd/notary/integration_test.go

+	require.Contains(t, output, targetHash)
+
+	// 11. witness an invalid role and check for error on publish
+	_, err = runCommand(t, tempDir, "witness", "gun", "targets/made/up")


Can we also add a test to try to witness non-targets or delegation roles? I was thinking root, snapshot, timestamp - this should be already covered by changelist helper logic, but would be nice to have.

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

rough witness implementation Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage · 2016-08-03T22:47:15Z

@cyli @riyazdf I think the other cases you suggested should be covered now. I also made some additional updates in the application of changelists. We were inconsistent about when we returned an error or just logged an continued if we didn't recognize a given change. With the new functionality to help manage the changelist, we now more consistently error and the user will have to remove the offending change from their changelist.

cyli · 2016-08-03T23:36:24Z

LGTM! Thank you for addressing all the testing so quickly!

riyazdf · 2016-08-03T23:42:47Z

LGTM

endophage added this to the Notary 0.4 milestone Jul 26, 2016

endophage self-assigned this Jul 26, 2016

endophage force-pushed the witness branch from 5cecf3b to 439bbe8 Compare July 26, 2016 17:14

cyli reviewed Jul 26, 2016
View reviewed changes

endophage force-pushed the witness branch 2 times, most recently from 4920d34 to 78cec6c Compare July 29, 2016 00:27

endophage changed the title ~~WIP: Witness command~~ Witness command Jul 29, 2016

endophage force-pushed the witness branch from 78cec6c to 3d675b6 Compare July 29, 2016 18:33

cyli reviewed Jul 29, 2016
View reviewed changes

endophage force-pushed the witness branch from 3d675b6 to fa9c366 Compare July 29, 2016 22:21

endophage force-pushed the witness branch from fa9c366 to cd38aed Compare July 29, 2016 22:24

endophage force-pushed the witness branch from cd38aed to b9aca8b Compare July 29, 2016 22:56

riyazdf reviewed Jul 29, 2016
View reviewed changes

endophage force-pushed the witness branch from b9aca8b to b1dd399 Compare August 1, 2016 17:29

cyli reviewed Aug 1, 2016
View reviewed changes

riyazdf reviewed Aug 1, 2016
View reviewed changes

endophage force-pushed the witness branch from b1dd399 to 5310637 Compare August 1, 2016 21:16

riyazdf reviewed Aug 1, 2016
View reviewed changes

David Lawrence added 3 commits August 1, 2016 15:51

witness command

f39d53d

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

move tuf.Client beside NotaryRepository

fbdc159

rough witness implementation Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

tests for witness

3b9ffbc

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage force-pushed the witness branch from 5310637 to 3b9ffbc Compare August 3, 2016 22:45

riyazdf merged commit dc74bd3 into notaryproject:master Aug 3, 2016

riyazdf mentioned this pull request Aug 3, 2016

Implement notary witness #867

Closed

cyli mentioned this pull request Aug 5, 2016

Implement Threshold Signing mechanisms on the client/server #841

Open

endophage deleted the witness branch August 9, 2016 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Witness command #875

Witness command #875

endophage commented Jul 26, 2016 •

edited by cyli

Loading

cyli Jul 26, 2016

cyli Jul 29, 2016 •

edited

Loading

riyazdf Jul 30, 2016

endophage Aug 1, 2016

endophage commented Jul 29, 2016

cyli commented Jul 29, 2016

endophage commented Jul 29, 2016

cyli commented Jul 29, 2016

endophage commented Jul 29, 2016

riyazdf Jul 29, 2016

cyli Aug 1, 2016

cyli commented Aug 1, 2016

riyazdf Aug 1, 2016

riyazdf Aug 1, 2016

endophage commented Aug 3, 2016

cyli commented Aug 3, 2016

riyazdf commented Aug 3, 2016


		if err := signed.VerifyVersion(&(signedTargets.Signed.SignedCommon), minVersion); err != nil {
		rb.invalidRoles.Targets[roleName] = signedTargets

Witness command #875

Witness command #875

Conversation

endophage commented Jul 26, 2016 • edited by cyli Loading

cyli Jul 26, 2016

Choose a reason for hiding this comment

cyli Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

riyazdf Jul 30, 2016

Choose a reason for hiding this comment

endophage Aug 1, 2016

Choose a reason for hiding this comment

endophage commented Jul 29, 2016

cyli commented Jul 29, 2016

endophage commented Jul 29, 2016

cyli commented Jul 29, 2016

endophage commented Jul 29, 2016

riyazdf Jul 29, 2016

Choose a reason for hiding this comment

cyli Aug 1, 2016

Choose a reason for hiding this comment

cyli commented Aug 1, 2016

riyazdf Aug 1, 2016

Choose a reason for hiding this comment

riyazdf Aug 1, 2016

Choose a reason for hiding this comment

endophage commented Aug 3, 2016

cyli commented Aug 3, 2016

riyazdf commented Aug 3, 2016

endophage commented Jul 26, 2016 •

edited by cyli

Loading

cyli Jul 29, 2016 •

edited

Loading