Skip to content
This repository was archived by the owner on Jul 31, 2025. It is now read-only.

Conversation

@riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Jul 29, 2016

Stacked on #880 to make use of the key marking functionality

Implements remote key rotation for snapshot and timestamp keys via notary key rotate <GUN> <ROLE> -r.

In case this operation is interrupted, we will attempt to sign in any "pending keys" in the database on the next remote rotation -- each role/gun pair can only have one pending key.

This PR also removes the timestamp_keys table in the notary server database, and instead parses out public keys from the TUF metadata in the tuf_files table. If TUF metadata does not yet exist for a repo, we attempt to create new remote keys (using the same "pending" logic described above to prevent key explosion).

Closes #846, #847, #752
Supersedes #553, #880

@riyazdf riyazdf force-pushed the rotate-key-endpoint branch from c5b56b7 to f9355ab Compare July 29, 2016 23:19
@riyazdf riyazdf added this to the Notary 0.4 milestone Jul 29, 2016
@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 8 times, most recently from 7c1b1a9 to 4da73c6 Compare August 1, 2016 22:52
@riyazdf riyazdf changed the title WIP: Rotate remote key endpoint Rotate remote key endpoint Aug 1, 2016
@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 2 times, most recently from 1a8ca83 to 4bf82bd Compare August 2, 2016 17:00
@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 3 times, most recently from 683946c to 00e92ea Compare August 2, 2016 22:17
@@ -0,0 +1 @@
DROP TABLE `timestamp_keys`; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

\o/

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 - this is probably a bad idea from a historical point of view, but wondering if it's better to also remove the table from 0001 and add a IF EXISTS to this drop table command, that way it doesn't have to be created and then immediately dropped.

https://dev.mysql.com/doc/refman/5.7/en/drop-table.html to this if there is no error, and modify

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'd be more than happy to add a IF EXISTS to this drop table but I'm not sure if we should edit existing migrations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm not sure either. I guess you really only run migrations once, so creating and then destroying a table isn't an issue.

@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 4 times, most recently from 158c542 to 2223531 Compare August 3, 2016 01:33
@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 3 times, most recently from 461e8ea to bde7dc7 Compare August 9, 2016 02:29
crypto = signed.NewEd25519()
k3, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
require.Nil(t, err, "Expected nil error")
require.NotEqual(t, k, k3, "Did not receive same key when attempting to recreate.")
Copy link
Contributor

@cyli cyli Aug 9, 2016

Choose a reason for hiding this comment

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

Non-blocking nitpick: should this be "Received same key when..."?

Similarly with the snapshot test.

@cyli
Copy link
Contributor

cyli commented Aug 9, 2016

Thanks for all your work on this! This LGTM again aside from some minor nits on naming!

@riyazdf riyazdf force-pushed the rotate-key-endpoint branch 2 times, most recently from 58f9de4 to 947943c Compare August 9, 2016 17:11
@endophage
Copy link
Contributor

LGTM. Excited to merge this! @riyazdf can you rebase just so we can get a final test run to make sure everything is good, then we can merge if it's all green :-D

cyli and others added 15 commits August 9, 2016 16:26
Signed-off-by: Ying Li <ying.li@docker.com>
… Get RPC calls

to also return the role in order to conform with the default cryptoservice behavior

Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Ying Li <ying.li@docker.com>
…ned by mark as active

Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
extra keys until we used a pending one

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
drop mysql table

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
…s implement cryptoservice

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
…eanup, add tests

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
@riyazdf riyazdf force-pushed the rotate-key-endpoint branch from 947943c to e29b1ae Compare August 9, 2016 23:26
@riyazdf
Copy link
Contributor Author

riyazdf commented Aug 9, 2016

@endophage sounds good, just rebased!

@cyli
Copy link
Contributor

cyli commented Aug 10, 2016

Yay all green!

@cyli cyli merged commit b4d4ab6 into master Aug 10, 2016
@cyli cyli deleted the rotate-key-endpoint branch August 10, 2016 00:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants