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

#806 Part 3: Reimplement import export #825

Merged
merged 3 commits into from
Jul 19, 2016

Conversation

endophage
Copy link
Contributor

@endophage endophage commented Jul 11, 2016

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)

@endophage endophage force-pushed the reimplement_import_export branch 2 times, most recently from 2e34719 to 81e4114 Compare July 11, 2016 21:04
@endophage endophage changed the title WIP: #806 Part 3: Reimplement import export #806 Part 3: Reimplement import export Jul 11, 2016
@endophage endophage force-pushed the reimplement_import_export branch from 81e4114 to 1c967ee Compare July 11, 2016 23:01
if k.outFile == "" {
out = cmd.Out()
} else {
f, err := os.OpenFile(k.outFile, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, notary.PrivKeyPerms)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@endophage endophage force-pushed the reimplement_import_export branch from 1c967ee to 86b916a Compare July 11, 2016 23:34
@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@endophage endophage force-pushed the reimplement_import_export branch from 86b916a to b4bbd1c Compare July 12, 2016 00:54
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

riyazdf commented Jul 12, 2016

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

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

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

Choose a reason for hiding this comment

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

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

@endophage endophage force-pushed the reimplement_import_export branch 3 times, most recently from b21de94 to e710460 Compare July 13, 2016 20:31
@endophage endophage force-pushed the reimplement_import_export branch 8 times, most recently from 1919f33 to 1a1845c Compare July 14, 2016 00:34
@HuKeping
Copy link
Contributor

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

endophage and others added 2 commits July 18, 2016 10:09
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@endophage endophage force-pushed the reimplement_import_export branch from 6a650a6 to 7f60c41 Compare July 18, 2016 18:56
@endophage endophage force-pushed the reimplement_import_export branch 2 times, most recently from 269f314 to b861f8c Compare July 18, 2016 23:44
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
@endophage endophage force-pushed the reimplement_import_export branch from b861f8c to dff7044 Compare July 18, 2016 23:48
@riyazdf
Copy link
Contributor

riyazdf commented Jul 19, 2016

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

cyli commented Jul 19, 2016

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

@endophage
Copy link
Contributor Author

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

@endophage endophage merged commit 14bed4f into notaryproject:master Jul 19, 2016
@endophage endophage deleted the reimplement_import_export branch July 19, 2016 17:44
@HuKeping
Copy link
Contributor

HuKeping commented Jul 27, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants