Flattened the filekeystore: Post0.4 #872

avaid96 · 2016-07-25T22:25:50Z

Stores all keys under the private directory
Key data now contains the gun that was otherwise inferred by filepath
Root keys delegations keys etc don't get a GUN
Backwards compatibility to 0.1 (already there) and 0.3 (added tests and fixtures) has been tested for

closes #838

docker-jenkins · 2016-07-25T22:25:52Z

Can one of the admins verify this patch?

riyazdf · 2016-07-25T22:27:58Z

jenkins, test this please.

cyli · 2016-07-26T01:47:02Z

tuf/utils/x509.go

@@ -409,6 +409,17 @@ func KeyToPEM(privKey data.PrivateKey, role string) ([]byte, error) {
 			"role": role,
 		}
 	}
+	if gun != "" {
+		headers = map[string]string{


Totally non-blocking - can we just set the header value for the given key in two if blocks, rather than redeclaring it in three if blocks?

cyli · 2016-07-26T06:14:19Z

Apologies! I'm sure this would work fine with the new code - could we add a delegation to the fixtures so we have a delegation key imported into the key store? It should be fine, since it could have had any role anyway, but just wanted to make sure that something with that that particular key can still be used.

This looks great! So clean!

avaid96 · 2016-07-28T00:04:16Z

Thanks for this comment @cyli

"Apologies! I'm sure this would work fine with the new code - could we add a delegation to the fixtures so we have a delegation key imported into the key store? It should be fine, since it could have had any role anyway, but just wanted to make sure that something with that that particular key can still be used.

This looks great! So clean!"

it picked up a case that I hadn't looked for. Made the necessary changes with a new fixture that uses delegations and tests that with the new keystore

cyli · 2016-08-22T23:29:54Z

storage/filestore.go

+			f.moveKeyTo0Dot4Location(filepath.Join(notary.RootKeysSubdir, file))
+		}
+		// delete the old directory
+		if rootKeysSubDir == "" || rootKeysSubDir == "/" {


Non-blocking: Can this ever happen, since we control the constants notary.RootKeysSubdir and notary.NonRootKeysSubdir - they're not empty, and we do a filepath.Join, so won't it always contain at least some of the constant values?

Update: I'm not sure filepath.Abs(filepath.Clean(filepath.Join(...))) could result in rootKeysSubDir being either empty or "/".

https://play.golang.org/p/OCxAa4jtjL were the only cases I tried, so I might have missed some stuff.

@cyli is 100% correct. I asked @avaid96 to put these checks in as a sanity check in case anyone is making updates in the future and does something silly.

@cyli has also made the suggestion that we check f.Location and the (Non)RootKeysSubdir constants at the start instead of checking the joined values here and later. That would stop a possible walk over / which would take a long time.

@cyli you're 100% correct about the fact that as of now we won't enter these statements but as correctly pointed out by @endophage these are just to make the migrations futureproof

@cyli the prior checks make sense. I have implemented something similar in the latest push (coming soon)- checking the concatenated value but checking this before the walk over the directory. Let me know if you like this or would prefer separate checks. I prefer this because it will be simpler to understand what is going on

cyli · 2016-08-22T23:56:52Z

So sorry I didn't comment sooner. :( I will be happy to address the comments above in a few days.

avaid96 · 2016-08-23T13:15:08Z

No issues at all @cyli. Thanks for the awesome comments. I think they're all mostly addressed besides the migration test which I can write some time this week 😄 Let me know if there's anything else!

avaid96 · 2016-08-24T17:38:14Z

added the tests for the migrations @cyli 😃

cyli · 2016-09-08T05:35:02Z

@avaid96 So so sorry I hadn't finished going over this yet. Thank you for adding those tests! LGTM, and thank you so much for slogging through all of this and keeping it up to date!

avaid96 · 2016-09-09T14:15:33Z

bumping this for a final review and merge. Has one slightly out of date LGTM from @riyazdf (doesn't include last commit) and another up to date one from @cyli

cyli · 2016-09-09T17:06:32Z

@avaid96 Many apologies, this seems to require some conflict resolution. :| I think github allows collaboration on forks now (https://github.com/blog/2247-improving-collaboration-with-forks), so if it's enabled we'd be happy to rebase :)

riyazdf · 2016-09-09T17:07:46Z

Final commit LGTM!

And yeah, unfortunately we'll need to rebase -- let us know if you hit that checkbox and we can handle that :)

…newer functionality and tests to be compatible with the flat KS Signed-off-by: Avi Vaid <[email protected]>

have been added for backwards compatibility testing Signed-off-by: avaid96 <[email protected]>

@riyazdf

…ormatted repos which are irrelevant after migration. Address @riyazdf 's concern about having an empty role Signed-off-by: Avi Vaid <[email protected]>

…with it Signed-off-by: Avi Vaid <[email protected]>

Signed-off-by: Avi Vaid <[email protected]>

…sts. Work is required on the integration tests still Signed-off-by: Avi Vaid <[email protected]>

Signed-off-by: Avi Vaid <[email protected]>

…@riyazdf

…@riyazdf Signed-off-by: Avi Vaid <[email protected]>

Signed-off-by: Avi Vaid <[email protected]>

avaid96 · 2016-09-10T14:17:13Z

No worries, I rebased. Sorry I think Circle ran into something 😞 Could you rebuild?

I've also allowed edits from maintainers in case there's any minor tweak you'll want to make 😄

Signed-off-by: Riyaz Faizullabhoy <[email protected]>

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

GordonTheTurtle added dco/no labels Jul 26, 2016

avaid96 force-pushed the flattenKS branch from 79dbeb3 to a9668ac Compare July 26, 2016 00:34

GordonTheTurtle added dco/no and removed dco/no labels Jul 26, 2016

cyli reviewed Jul 26, 2016
View reviewed changes

avaid96 force-pushed the flattenKS branch 2 times, most recently from 698dc61 to 7505832 Compare July 28, 2016 00:53

avaid96 changed the title ~~Flattened the keystore on disk~~ POC: Flattened the keystore on disk, for post 0.4 Jul 28, 2016

avaid96 force-pushed the flattenKS branch 2 times, most recently from ef12247 to 1516bb7 Compare August 11, 2016 18:49

avaid96 changed the title ~~POC: Flattened the keystore on disk, for post 0.4~~ WIP: Flattened the keystore on disk, for post 0.4 Aug 11, 2016

avaid96 force-pushed the flattenKS branch 2 times, most recently from d8bb796 to 3d6c8f5 Compare August 11, 2016 22:49

avaid96 changed the title ~~WIP: Flattened the keystore on disk, for post 0.4~~ Flattened the filekeystore Aug 11, 2016

cyli reviewed Aug 22, 2016
View reviewed changes

avaid96 force-pushed the flattenKS branch 2 times, most recently from eb2f04c to 68bfaa6 Compare August 23, 2016 13:21

avaid96 added 10 commits September 10, 2016 06:31

xrebased to latest changes, this will fail as of now- need to change …

31118f2

…newer functionality and tests to be compatible with the flat KS Signed-off-by: Avi Vaid <[email protected]>

appended to the fixture README to describe the notary0.3 fixtures that

e989eaa

have been added for backwards compatibility testing Signed-off-by: avaid96 <[email protected]>

migration function on initialization. TODO: Remove checks for older f…

ebb9802

…ormatted repos which are irrelevant after migration. Address @riyazdf 's concern about having an empty role Signed-off-by: Avi Vaid <[email protected]>

added fixture with delegation as per @cyli's comment and tests to go …

f4f2d6f

…with it Signed-off-by: Avi Vaid <[email protected]>

fixed integration test- TestPurge for the flat key store

220813a

Signed-off-by: Avi Vaid <[email protected]>

fixed import to be compatible with a flat key store and the direct te…

13c9a0d

…sts. Work is required on the integration tests still Signed-off-by: Avi Vaid <[email protected]>

fixed exports, imports and tests for the flattened KS

f667b03

Signed-off-by: Avi Vaid <[email protected]>

pushed migrations down to a storage layer from a keystore layer

b984b2a

Signed-off-by: Avi Vaid <[email protected]>

refactored migrations to only look at root_keys/ and tuf_keys/, thanks …

a787583

…@riyazdf Signed-off-by: Avi Vaid <[email protected]>

added tests for migrations with the backwards_compatibility fixtures

25086b9

Signed-off-by: Avi Vaid <[email protected]>

avaid96 force-pushed the flattenKS branch from 4af0e49 to 25086b9 Compare September 10, 2016 13:47

Ineffassign fixes

5be0a47

Signed-off-by: Riyaz Faizullabhoy <[email protected]>

riyazdf force-pushed the flattenKS branch from bf9843a to 5be0a47 Compare September 10, 2016 16:40

riyazdf mentioned this pull request Sep 12, 2016

notary key import delegation.key --gun being ignored #950

Closed

cyli merged commit bc9b3d8 into notaryproject:master Sep 12, 2016

This was referenced Sep 28, 2016

Be able to read keys even if they are not in the root/non-root subdirs #981

Merged

root key location? #987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flattened the filekeystore: Post0.4 #872

Flattened the filekeystore: Post0.4 #872

avaid96 commented Jul 25, 2016 •

edited

Loading

docker-jenkins commented Jul 25, 2016

riyazdf commented Jul 25, 2016

cyli Jul 26, 2016

cyli commented Jul 26, 2016 •

edited

Loading

avaid96 commented Jul 28, 2016

cyli Aug 22, 2016 •

edited

Loading

cyli Aug 22, 2016

endophage Aug 23, 2016

avaid96 Aug 23, 2016

cyli commented Aug 22, 2016

avaid96 commented Aug 23, 2016

avaid96 commented Aug 24, 2016 •

edited

Loading

cyli commented Sep 8, 2016

avaid96 commented Sep 9, 2016

cyli commented Sep 9, 2016

riyazdf commented Sep 9, 2016

avaid96 commented Sep 10, 2016 •

edited

Loading

Flattened the filekeystore: Post0.4 #872

Flattened the filekeystore: Post0.4 #872

Conversation

avaid96 commented Jul 25, 2016 • edited Loading

docker-jenkins commented Jul 25, 2016

riyazdf commented Jul 25, 2016

cyli Jul 26, 2016

Choose a reason for hiding this comment

cyli commented Jul 26, 2016 • edited Loading

avaid96 commented Jul 28, 2016

cyli Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

cyli Aug 22, 2016

Choose a reason for hiding this comment

endophage Aug 23, 2016

Choose a reason for hiding this comment

avaid96 Aug 23, 2016

Choose a reason for hiding this comment

cyli commented Aug 22, 2016

avaid96 commented Aug 23, 2016

avaid96 commented Aug 24, 2016 • edited Loading

cyli commented Sep 8, 2016

avaid96 commented Sep 9, 2016

cyli commented Sep 9, 2016

riyazdf commented Sep 9, 2016

avaid96 commented Sep 10, 2016 • edited Loading

avaid96 commented Jul 25, 2016 •

edited

Loading

cyli commented Jul 26, 2016 •

edited

Loading

cyli Aug 22, 2016 •

edited

Loading

avaid96 commented Aug 24, 2016 •

edited

Loading

avaid96 commented Sep 10, 2016 •

edited

Loading