Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Witness command #875

Merged
merged 3 commits into from
Aug 3, 2016
Merged

Witness command #875

merged 3 commits into from
Aug 3, 2016

Conversation

endophage
Copy link
Contributor

@endophage endophage commented Jul 26, 2016

Fixes #562.

@endophage endophage added this to the Notary 0.4 milestone Jul 26, 2016
@endophage endophage self-assigned this Jul 26, 2016

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

@endophage endophage force-pushed the witness branch 2 times, most recently from 4920d34 to 78cec6c Compare July 29, 2016 00:27
@endophage endophage changed the title WIP: Witness command Witness command Jul 29, 2016
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
Copy link
Contributor

@cyli cyli Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated long form usage description.

@endophage
Copy link
Contributor Author

@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
Copy link
Contributor

cyli commented Jul 29, 2016

@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
Copy link
Contributor Author

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
Copy link
Contributor

cyli commented Jul 29, 2016

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
Copy link
Contributor Author

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".

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cyli
Copy link
Contributor

cyli commented Aug 1, 2016

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!

require.Contains(t, output, targetName)
require.Contains(t, output, targetHash)

// 11. witness an invalid role and check for error on publish
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this entire test flow is awesome! 👍

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

David Lawrence added 3 commits August 1, 2016 15:51
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
rough witness implementation
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
@endophage
Copy link
Contributor Author

@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
Copy link
Contributor

cyli commented Aug 3, 2016

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

@riyazdf
Copy link
Contributor

riyazdf commented Aug 3, 2016

LGTM

@riyazdf riyazdf merged commit dc74bd3 into notaryproject:master Aug 3, 2016
@riyazdf riyazdf mentioned this pull request Aug 3, 2016
@endophage endophage deleted the witness branch August 9, 2016 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants