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.

Sign up for GitHub

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

Jump to bottom

Change the CLI to be able to rotate server managed keys #571

Merged
merged 7 commits into from
Mar 16, 2016
Merged

Change the CLI to be able to rotate server managed keys #571

merged 7 commits into from
Mar 16, 2016

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Feb 15, 2016

This includes some minor changes from #553.

Fixes #554

@cyli cyli added this to the notary/0.2 milestone Feb 15, 2016
riyazdf
riyazdf reviewed Feb 16, 2016
View reviewed changes
client/client.go
}

// ErrInvalidLocalRole is returned when the client wants to manage
// an unsupported key type
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: I'd change an unsupported key type to a key type that is not permitted so that it's consistent with the err message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@riyazdf
Copy link
Contributor

riyazdf commented Feb 16, 2016

LGTM after comments!

@cyli cyli force-pushed the cli-server-rotate branch from 544c61e to e61fc7f Compare February 16, 2016 19:57
diogomonica
diogomonica reviewed Feb 17, 2016
View reviewed changes
client/client_test.go
@@ -2392,16 +2392,24 @@ func TestRotateKeyInvalidRole(t *testing.T) {
repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false)
defer os.RemoveAll(repo.baseDir)

// the equivalent of: (root, true), (root, false), (timestamp, true),
// (timestamp, false), (targets, true)
// the equivalent of: remotely remotely rotating the root key
Copy link
Contributor

Choose a reason for hiding this comment

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

two "remotely"'s

@cyli cyli force-pushed the cli-server-rotate branch from 90db14e to 815981f Compare February 17, 2016 20:58
@riyazdf riyazdf modified the milestones: notary/0.2, notary/0.3 Feb 19, 2016
riyazdf
riyazdf reviewed Mar 1, 2016
View reviewed changes
cmd/notary/keys.go
}
err = nRepo.RotateKey(rotateKeyRole, k.rotateKeyServerManaged)
if err == nil && k.rotateKeyServerManaged {
err = nRepo.Publish()
Copy link
Contributor

Choose a reason for hiding this comment

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

One thought that we were discussing: is it possible for publish only the key rotation change, instead of all staged changes? I'm not sure of the best way to go about doing this without making users published all staged changes prior to this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd need to break up Publish into a few component steps but that wouldn't be a bad thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move the call to publish to nRepo.RotateKey - I was thinking about adding a private publish(changelist) command. RotateKey can generate an in-memory change file that gets published using that private method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we were thinking about breaking Publish up into several publicly exposed component functions such as (for example) LoadOrUpdate, ApplyChanges, and UploadChanges or something. Then have the CLI call all those in sequence.

It might make more sense to have the CLI publish rather than have RotateKey do it. This would be a much bigger change than the private publish function described in the previous comment, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slack discussion: publish(changelist) seems like the way to go

@cyli cyli force-pushed the cli-server-rotate branch 5 times, most recently from 684af0b to da68512 Compare March 8, 2016 00:33
endophage
endophage reviewed Mar 11, 2016
View reviewed changes
client/client.go
return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
switch {
// We currently support locally or remotely managing snapshot keys...
case role == data.CanonicalSnapshotRole:
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 add an explicit break just to be clear that this case is intentionally empty.

@endophage
Copy link
Contributor

I don't have a strong opinion on collapsing the case/if statements.

LGTM

@cyli cyli force-pushed the cli-server-rotate branch 2 times, most recently from 0564e5e to 3c1fca2 Compare March 12, 2016 02:17
@endophage
Copy link
Contributor

Rebase and we can merge this :-)

riyazdf
riyazdf reviewed Mar 16, 2016
View reviewed changes
docs/advanced_usage.md
@@ -90,11 +90,13 @@ subsection.
In case of potential compromise, notary provides a CLI command for rotating keys. Currently, you can use the `notary key rotate` command to rotate the targets or snapshot keys.

While the snapshot key is managed by the notary client by default, use the `notary key
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It seems like I missed this documentation piece -- it might be nice to also mention the Docker 1.11 default behavior for managing snapshot keys on the server here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like your documentation appears in the next paragraph. :) This just needed a rebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 awesome

cyli added 7 commits March 15, 2016 18:17
@cyli
Update the CLI and client to no longer reject remote timestamp rotati…
07b9f50 
…ons.

Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Change the CLI for rotate key to require a role type
e3716f0 
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Update comments, and publish in the CLI after remote key rotation
b6c4840 
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Clean up yubikeys between each cmd/notary/keys_test.go test
4e5e2f3 
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Publish only the key rotation changes after a remote key rotation
fa5edc4 
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Update advanced usage documentation
baaa703 
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
Make all key rotations publish immediately, not just remote key rotat…
44cccbb 
…ions

Signed-off-by: Ying Li <ying.li@docker.com>
@cyli cyli force-pushed the cli-server-rotate branch from 3c1fca2 to 44cccbb Compare March 16, 2016 01:35
riyazdf added a commit that referenced this pull request Mar 16, 2016
@riyazdf
Merge pull request #571 from docker/cli-server-rotate
d567198 
Change the CLI to be able to rotate server managed keys
@riyazdf riyazdf merged commit d567198 into master Mar 16, 2016
@riyazdf riyazdf deleted the cli-server-rotate branch March 16, 2016 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Notary 0.3
Development

Successfully merging this pull request may close these issues.
5 participants
@cyli @riyazdf @endophage @diogomonica @GordonTheTurtle