adding --role flag to key import #882

avaid96 · 2016-07-28T07:07:53Z

closes #881

Signed-off-by: Avi Vaid avaid1996@gmail.com

docker-jenkins · 2016-07-28T07:07:54Z

Can one of the admins verify this patch?

avaid96 · 2016-07-28T07:10:13Z

This is still WIP
Discussion:

should we have a --gun flag for compatibility with the flat key store? as of now you can specify the path if you want a key associated with a gun but this won't be possible in a flat keystore. followup: should we even have a path if we have a flat keystore
if there is a role flag should we limit to importing only one key or add that to all specified keys?

HuKeping · 2016-07-28T07:53:19Z

Thanks for the quick fixing!
btw, isn't it 1:00 AM at CA?

HuKeping · 2016-07-28T08:58:23Z

Do you get this error when import keys:

failed to import key to store: PEM headers did not contain import path

cyli · 2016-07-28T17:07:19Z

@avaid96 I think taking a GUN makes sense, and a GUN header makes sense more than a path header in the flattened keystore. I think #825 included both headers - can import guess the path based on the GUN for now until flatten keystore is merged?

riyazdf · 2016-07-28T20:46:13Z

utils/keys.go

 		toWrite []byte
 	)
 	for block, rest := pem.Decode(data); block != nil; block, rest = pem.Decode(rest) {
+		if role != "" {


I think this conditional logic should be slightly different to match the usage comment - we should check if block.Headers["role"] exists, and set it to role if it's empty.

Also, I think this logic should combine with the pathing logic below since to-be-imported delegation keys may not have a path PEM header. We can check if we have a provided role and if it's a delegation we can write it to private/tuf_keys/<KEY_ID>.key. If we follow @cyli's suggestion for GUN headers we can include that in the path we construct for non-delegation keys as well.

cyli · 2016-07-29T17:21:42Z

@HuKeping Yep, that's what we are trying to fix. :)

cyli · 2016-08-03T05:43:37Z

utils/keys.go

 // header. If the file already exists, the file is truncated. Multiple
 // adjacent PEMs with the same "path" header are appended together.
-func ImportKeys(from io.Reader, to []Importer) error {
+func ImportKeys(from io.Reader, to []Importer, role string, gun string, passRet notary.PassRetriever) error {


nit: can we call role and gun -> fallbackRole and fallbackGun or some such, to make it clear they're only used if there is no provided role or gun/path?

riyazdf · 2016-08-04T21:49:13Z

utils/keys.go

 func ExportKeysByGUN(to io.Writer, s Exporter, gun string) error {
 	keys := s.ListFiles()
-	sort.Strings(keys) // ensure consistenct. ListFiles has no order guarantee
+	sort.Strings(keys) // ensure consistent. ListFiles has no order guarantee


nit: I think this was originally a typo for consistency

riyazdf · 2016-08-05T21:11:33Z

utils/test.key

@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----


nit: can we put these keys in the fixtures directory? Also, you may not need this key without the role header because we other keys (ex: secure.example.com.key) without headers that we can test with

riyazdf · 2016-08-08T17:27:48Z

jenkins, ok to test.

cyli · 2016-08-08T18:38:43Z

utils/keys.go

+			keyID := decodedKey.ID()
+			if block.Headers["role"] == tufdata.CanonicalRootRole {
+				// this is a root key so import it to trustDir/root_keys/
+				delete(block.Headers, "gun")


Non-blocking: Should we move this logic out of the if block? E.g. after setting the path, we can delete the "gun" header if the role is root or a delegation? Since currently it's only deleted if the path is not specified.

cyli · 2016-08-08T20:37:57Z

utils/keys_test.go

-	require.Len(t, bFinal.Headers, 0) // path header is stripped during import
+	_, ok := bFinal.Headers["path"]
+	require.False(t, ok, "expected no path header, should have been removed at import")
+	require.Equal(t, notary.DefaultImportRole, bFinal.Headers["role"]) // if no role is specified we assume it is a delegation key


Should we add an assertion that no gun is included in the header as well?

added it, thanks 😄

cyli · 2016-08-09T21:37:37Z

utils/keys_test.go

-	from, _ := os.OpenFile("../fixtures/precedence.example.com.key", os.O_RDONLY, notary.PrivKeyPerms)
+}
+
+func TestBlockHeaderPrecedencePathGun(t *testing.T) {


Non-blocking nitpick: should this be "TestBlockHeaderPrecedencePathRole", since the header has both the path and the role, but not the gun?

cyli · 2016-08-09T23:40:53Z

utils/keys_test.go

+	err := ImportKeys(in, []Importer{s}, "", "", passphraseRetriever)
+	require.NoError(t, err)
+
+	bFinal, bRest := pem.Decode(s.data[filepath.Join(notary.NonRootKeysSubdir, "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497")])


Non-blocking question, just because I don't know what we decided for this: it seems like we are ok with one of the base non-root roles not having a gun also (we don't delete it, but we don't error if we can't infer one) - we infer a path being just the non-root subdirectory + the key ID, as you've included in your test.

I'm ok with that, since the user messed this up, but this would mean that earlier versions of docker probably wouldn't be able to use this key? cc @riyazdf @endophage

I'm okay with either method, but I would be tempted to lean towards not importing it since we don't import encrypted keys without a path etc (cases where the user messed up as well). Would make sense to have some consistency there. This would mean changing the code and this test etc since as @cyli mentioned correctly we now just import the key. Happy to do it once we decide @riyazdf @endophage

…e PEM data. need to write tests + address additional concerns Signed-off-by: Avi Vaid <avaid1996@gmail.com>

… defaults etc, need to write tests for this new functionality Signed-off-by: Avi Vaid <avaid1996@gmail.com>

…rors when we don't have enough info to import a key Signed-off-by: Avi Vaid <avaid1996@gmail.com>

Signed-off-by: Avi Vaid <avaid1996@gmail.com>

… of tests covering more cases, added a delegation add- import and publish flow test Signed-off-by: Avi Vaid <avaid1996@gmail.com>

…being encryption tests and export import testflow test Signed-off-by: Avi Vaid <avaid1996@gmail.com>

endophage · 2016-08-10T18:13:39Z

LGTM when CI goes green!

riyazdf · 2016-08-10T20:47:57Z

cmd/notary/integration_test.go

+
+	nBytes, err = tempFile3.Write(pemBytes)
+	require.NoError(t, err)
+	tempFile2.Close()


nit: tempFile3.Close()

riyazdf · 2016-08-10T20:59:08Z

cmd/notary/keys.go

 		defer from.Close()
-
-		if err = utils.ImportKeys(from, importers); err != nil {
+		passRetriever := k.getRetriever()


super non-blocking nit: we can remove this line and just directly use k.getRetriever in the next line since we don't use the passRetriever variable elsewhere

…ithout a gun Signed-off-by: Avi Vaid <avaid1996@gmail.com>

riyazdf · 2016-08-10T22:36:33Z

LGTM pending CI. Thank you for all of your hard work on this feature!

avaid96 · 2016-08-10T22:43:43Z

Of course, thanks for the detailed review on this PR! 😄

cyli · 2016-08-10T22:53:25Z

Thank you for all your work on this @avaid96! :D

GordonTheTurtle added the status/0-triage label Jul 28, 2016

riyazdf reviewed Jul 28, 2016
View reviewed changes

riyazdf added this to the Notary 0.4 milestone Jul 29, 2016

avaid96 changed the title ~~adding --role flag to key import~~ WIP: adding --role flag to key import Aug 1, 2016

cyli reviewed Aug 3, 2016
View reviewed changes

avaid96 changed the title ~~WIP: adding --role flag to key import~~ adding --role flag to key import Aug 4, 2016

riyazdf reviewed Aug 4, 2016
View reviewed changes

avaid96 self-assigned this Aug 5, 2016

riyazdf reviewed Aug 5, 2016
View reviewed changes

avaid96 force-pushed the rolefl branch 2 times, most recently from ce5cf73 to 722a65d Compare August 5, 2016 23:40

avaid96 force-pushed the rolefl branch from 971842e to ea5343c Compare August 8, 2016 18:28

cyli reviewed Aug 8, 2016
View reviewed changes

avaid96 force-pushed the rolefl branch from ea5343c to 8f7bee4 Compare August 8, 2016 18:47

cyli reviewed Aug 8, 2016
View reviewed changes

cyli reviewed Aug 9, 2016
View reviewed changes

avaid96 force-pushed the rolefl branch 2 times, most recently from 6dbafff to 130decd Compare August 9, 2016 23:37

cyli reviewed Aug 9, 2016
View reviewed changes

avaid96 added 8 commits August 10, 2016 09:54

added -r, --role flag to CLI and the addition of specified role to th…

c97ceff

…e PEM data. need to write tests + address additional concerns Signed-off-by: Avi Vaid <avaid1996@gmail.com>

added functionality to import roles with gun and role along with role…

ad9ecf3

… defaults etc, need to write tests for this new functionality Signed-off-by: Avi Vaid <avaid1996@gmail.com>

tests for path inference, block over flag precedence in import and er…

7f709c9

…rors when we don't have enough info to import a key Signed-off-by: Avi Vaid <avaid1996@gmail.com>

fixed lint issues and added passphrase retriever for encryption of keys

66be5d9

Signed-off-by: Avi Vaid <avaid1996@gmail.com>

fixed make lint issues/errors

1a5b332

Signed-off-by: Avi Vaid <avaid1996@gmail.com>

made changes as per comments on PR

13d986a

Signed-off-by: Avi Vaid <avaid1996@gmail.com>

cleaned up a lot of the control flow in key imports and added a bunch…

e19af6b

… of tests covering more cases, added a delegation add- import and publish flow test Signed-off-by: Avi Vaid <avaid1996@gmail.com>

addressed comments- made a bunch of fixes and added tests- main ones …

ba2508f

…being encryption tests and export import testflow test Signed-off-by: Avi Vaid <avaid1996@gmail.com>

avaid96 force-pushed the rolefl branch 2 times, most recently from 15f51cc to ef393b6 Compare August 10, 2016 17:51

riyazdf reviewed Aug 10, 2016
View reviewed changes

avaid96 force-pushed the rolefl branch from ef393b6 to 36638ad Compare August 10, 2016 20:54

riyazdf reviewed Aug 10, 2016
View reviewed changes

added skip import if a key is imported to a non root canonical role w…

d8a9ca9

…ithout a gun Signed-off-by: Avi Vaid <avaid1996@gmail.com>

avaid96 force-pushed the rolefl branch from 36638ad to d8a9ca9 Compare August 10, 2016 21:27

avaid96 merged commit ba795f0 into notaryproject:master Aug 10, 2016

adding --role flag to key import #882

adding --role flag to key import #882

Uh oh!

Conversation

avaid96 commented Jul 28, 2016

Uh oh!

docker-jenkins commented Jul 28, 2016

Uh oh!

avaid96 commented Jul 28, 2016

Uh oh!

HuKeping commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuKeping commented Jul 28, 2016

Uh oh!

cyli commented Jul 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli commented Jul 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riyazdf commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

endophage commented Aug 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riyazdf commented Aug 10, 2016

Uh oh!

avaid96 commented Aug 10, 2016

Uh oh!

cyli commented Aug 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HuKeping commented Jul 28, 2016 •

edited

Loading