#806 Part 3: Reimplement import export #825

endophage · 2016-07-11T17:38:15Z

Depends on #808

~~Still need to write some tests for the new cmd/notary/keys.go code. Not sure my parsing of CLI flags is correct.~~

N.B. import will no longer import to a yubikey. More discussion required but I think this needs to be more explicit around when we import to a yubikey, maybe even a separate command, because it hits the key management APIs at a different level. Functionality has been brought up to parity :-)

This PR contains parts 1 and 2 which have been closed in favour of merging this PR alone (means only this PR has to be rebased, not all 3)

riyazdf · 2016-07-11T23:17:03Z

cmd/notary/keys.go

+	if k.outFile == "" {
+		out = cmd.Out()
+	} else {
+		f, err := os.OpenFile(k.outFile, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, notary.PrivKeyPerms)


nit: might be worth explaining the bitwise or over these flags to explain that we're creating a new file in write mode, and possibly truncating

Actually I'm happy with the usage comment, which explains this at a higher level

riyazdf · 2016-07-12T00:19:39Z

cmd/notary/main_test.go

@@ -159,10 +159,6 @@ var exampleValidCommands = []string{
 	"key list",
 	"key rotate repo snapshot",
 	"key generate rsa",
-	"key backup tempfile.zip",
-	"key export e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 backup.pem",


I think it'd be good to bring back the key import and key export commands here. This just tests for invalid number of arguments, if I remember correctly.

riyazdf · 2016-07-12T01:01:04Z

utils/keys.go

+	for block, rest := pem.Decode(data); block != nil; block, rest = pem.Decode(rest) {
+		loc, ok := block.Headers["path"]
+		if !ok || loc == "" {
+			continue // don't know where to copy this key. Skip it.


might be worth adding a debug log here in case users get confused why non-notary exported keys can't be imported due to the missing "path" header

riyazdf · 2016-07-12T01:28:40Z

this cleanup is awesome, thank you for tackling it!

I think it'd be good to add some of the following tests around notary repositories:

Exporting keys from an initialized notary repository on disk
Importing over existing notary repository keys when paths collide with pre-existing key files
Exporting keys from a published notary repository, importing the same keys into a blank filesystem and successfully publishing to the same notary repository

Also we should open an issue for tracking the delegation "import key" flow - we'll still want a way to easily import a key fresh from openssl (without PEM headers) into a delegation role

cyli · 2016-07-12T23:54:13Z

storage/filestore.go

+
+// NewPrivateSimpleFileStore is a wrapper to create an owner readable/writeable
+// _only_ filestore
+func NewPrivateSimpleFileStore(baseDir, fileExt string) (*FilesystemStore, error) {


With the modification of import/export to use NewPrivateKeyFileStorage, I think this constructor is no longer used anywhere else except filestore_test.go, where I don't think permissions are getting checked (we could use one of the other constructors?).

HuKeping · 2016-07-14T03:52:55Z

Tons of code changed, I think it's a good choice to just wait it to be merged for myself 😄

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

cyli · 2016-07-18T17:12:03Z

cmd/notary/keys.go

+var cmdKeyImportTemplate = usageTemplate{
+	Use:   "import pemfile [ pemfile ... ]",
+	Short: "Imports all keys from all provided .pem files",
+	Long:  "Imports all keys from all provided .pem files by reading each PEM block from the file and writing that block to a unique object in the local keystore",


Do we want to mention that hardware storage is preferred in the long usage description?

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

riyazdf · 2016-07-19T16:56:34Z

I did a quick export - remove keys - re-import - publish test and it worked great 👍

LGTM!! Thank you for all of your hard work on this PR and the parts that preceded it :)

If you don't mind, I'd like to file a couple of follow-up issues: importing delegation keys without the path PEM header (notary delegation import?), and more end-to-end integration tests around exporting/importing.

cyli · 2016-07-19T17:01:14Z

Thank you for all your work on this refactor! LGTM!

endophage · 2016-07-19T17:26:58Z

@riyazdf goal is to make the path header irrelevant through flattening of the key store

HuKeping · 2016-07-27T11:03:57Z

Hi, is there any plan to add new docs for this, I find that I can no longer use the command like

notary key import delegation.key --role=targets/delegation_1

GordonTheTurtle added status/0-triage group/distribution labels Jul 11, 2016

endophage force-pushed the reimplement_import_export branch 2 times, most recently from 2e34719 to 81e4114 Compare July 11, 2016 21:04

endophage changed the title ~~WIP: #806 Part 3: Reimplement import export~~ #806 Part 3: Reimplement import export Jul 11, 2016

endophage mentioned this pull request Jul 11, 2016

Cleanup import-export and key architecture #806

Closed

3 tasks

endophage force-pushed the reimplement_import_export branch from 81e4114 to 1c967ee Compare July 11, 2016 23:01

riyazdf reviewed Jul 11, 2016
View reviewed changes

endophage force-pushed the reimplement_import_export branch from 1c967ee to 86b916a Compare July 11, 2016 23:34

riyazdf reviewed Jul 12, 2016
View reviewed changes

endophage force-pushed the reimplement_import_export branch from 86b916a to b4bbd1c Compare July 12, 2016 00:54

riyazdf reviewed Jul 12, 2016
View reviewed changes

endophage force-pushed the reimplement_import_export branch 2 times, most recently from e82dcf3 to ce5df78 Compare July 12, 2016 17:45

endophage mentioned this pull request Jul 12, 2016

#806 Part 2: Cleanup KeyStore Implementations #808

Closed

endophage force-pushed the reimplement_import_export branch 2 times, most recently from 1f99671 to 0c04e1d Compare July 12, 2016 22:18

cyli reviewed Jul 12, 2016
View reviewed changes

endophage force-pushed the reimplement_import_export branch 3 times, most recently from b21de94 to e710460 Compare July 13, 2016 20:31

endophage force-pushed the reimplement_import_export branch 8 times, most recently from 1919f33 to 1a1845c Compare July 14, 2016 00:34

endophage force-pushed the reimplement_import_export branch from 1a1845c to 6a650a6 Compare July 14, 2016 17:55

endophage removed group/distribution labels Jul 14, 2016

endophage and others added 2 commits July 18, 2016 10:09

tear out import/export

7a154ed

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

unify KeyMemoryStore and KeyFileStore in trustmanager

635b90e

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

cyli reviewed Jul 18, 2016
View reviewed changes

endophage force-pushed the reimplement_import_export branch from 6a650a6 to 7f60c41 Compare July 18, 2016 18:56

cyli mentioned this pull request Jul 18, 2016

Expand testclient to test delegations and root rotations #720

Merged

endophage force-pushed the reimplement_import_export branch 2 times, most recently from 269f314 to b861f8c Compare July 18, 2016 23:44

re-implement import/export

dff7044

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

endophage force-pushed the reimplement_import_export branch from b861f8c to dff7044 Compare July 18, 2016 23:48

endophage merged commit 14bed4f into notaryproject:master Jul 19, 2016

endophage deleted the reimplement_import_export branch July 19, 2016 17:44

cyli mentioned this pull request Jul 28, 2016

adding --role flag to key import #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#806 Part 3: Reimplement import export #825

#806 Part 3: Reimplement import export #825

endophage commented Jul 11, 2016 •

edited

Loading

riyazdf Jul 11, 2016

riyazdf Jul 11, 2016

riyazdf Jul 12, 2016

riyazdf Jul 12, 2016

riyazdf commented Jul 12, 2016 •

edited

Loading

cyli Jul 12, 2016

HuKeping commented Jul 14, 2016

cyli Jul 18, 2016

riyazdf commented Jul 19, 2016

cyli commented Jul 19, 2016

endophage commented Jul 19, 2016

HuKeping commented Jul 27, 2016 •

edited

Loading

#806 Part 3: Reimplement import export #825

#806 Part 3: Reimplement import export #825

Conversation

endophage commented Jul 11, 2016 • edited Loading

riyazdf Jul 11, 2016

Choose a reason for hiding this comment

riyazdf Jul 11, 2016

Choose a reason for hiding this comment

riyazdf Jul 12, 2016

Choose a reason for hiding this comment

riyazdf Jul 12, 2016

Choose a reason for hiding this comment

riyazdf commented Jul 12, 2016 • edited Loading

cyli Jul 12, 2016

Choose a reason for hiding this comment

HuKeping commented Jul 14, 2016

cyli Jul 18, 2016

Choose a reason for hiding this comment

riyazdf commented Jul 19, 2016

cyli commented Jul 19, 2016

endophage commented Jul 19, 2016

HuKeping commented Jul 27, 2016 • edited Loading

endophage commented Jul 11, 2016 •

edited

Loading

riyazdf commented Jul 12, 2016 •

edited

Loading

HuKeping commented Jul 27, 2016 •

edited

Loading