Remote key escrow #1096

endophage · 2017-02-13T21:17:34Z

Remote key escrow via a GRPC implementation of the trustmanager.Storage interface.

Still finishing it up but intend for Notary Signer to be able to act as an escrow backend with configurable APIs (so it can host either or both of: current key storage for server and/or the new GRPC trustmanager.Storage server).

By implementing the low level trustmanager.Storage interface, passwords, and therefore key encryption and decryption, still happen at the client, so compromise of the remote key storage does not equate to compromise of the escrowed keys.

Signed-off-by: David Lawrence david.lawrence@docker.com (github: endophage)

endophage · 2017-02-16T16:47:35Z

This is about as tested as it's going to get. The compiled .pb.go is hurting coverage but I'm not going to write tests for generated code.

riyazdf

I really like how clean this is, awesome work!

Forward looking (can be a separate PR): it would be really cool to add this to our integration tests, spinning up the escrow service and making sure the same delegation and publish flow works

riyazdf · 2017-02-17T03:52:43Z

Makefile

@@ -176,6 +180,9 @@ client: ${PREFIX}/bin/notary
 binaries: ${PREFIX}/bin/notary-server ${PREFIX}/bin/notary ${PREFIX}/bin/notary-signer
 	@echo "+ $@"

+escrow: ${PREFIX}/bin/escrow


how do you feel about notary-escrow as a name?

No strong feelings. It didn't seem necessary as "escrow" isn't quite as generic as "server". I mostly just got tired of typing notary-* on the front of everything :-P

Should the escrow binary also be added to the binaries and static targets?

@cyli I think once we have configurable integration with the NotaryRepository that would make sense. At the moment it doesn't really plug into anything and until @n4ss's dependency injection PR is merged, there's no way to plug it in short of forked code.

riyazdf · 2017-02-17T03:53:20Z

cmd/escrow/config.go

+	"github.com/spf13/viper"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials"
+	"net"


non-blocking nit: did we want to do the imports formatting into separate blocks?

This is how gogland is arranging them for me :-/ I need to try breaking them up into our normal format and see what it does with new imports that get auto-added. If we're all having to constantly tweak the import list though, maybe we stop trying so hard to organize it. It looks nicer the way we've been doing it, but I think everyone also loves just having imports auto-managed.

riyazdf · 2017-02-17T03:57:25Z

cmd/escrow/config.toml

@@ -0,0 +1,9 @@
+[storage]


nit: as much as I prefer TOML, do you think it's worth keeping JSON for consistency? The parsing should still work across JSON and TOML, so maybe we can include a JSON example as well?

I'd actually prefer converting everything to toml, I just didn't want to do it in this PR. I'll file an issue for it. Viper will make it little more than just rewriting the json files and updating our Dockerfiles and docker-compose files to point at the toml versions.

sounds good to me

riyazdf · 2017-02-17T04:01:56Z

trustmanager/errors.go

+// ErrKeyNotFound is returned when the keystore fails to retrieve a specific key.
+func (err ErrKeyNotFound) Error() string {
+	return fmt.Sprintf("signing key not found: %s", err.KeyID)
+}


thanks for consolidating this 👍

riyazdf · 2017-02-17T04:02:15Z

trustmanager/interfaces.go

-// ErrKeyNotFound is returned when the keystore fails to retrieve a specific key.
-func (err ErrKeyNotFound) Error() string {
-	return fmt.Sprintf("signing key not found: %s", err.KeyID)
+// KeyInfo stores the role, path, and gun for a corresponding private key ID


nit since you're moving things around: remove path from description

riyazdf · 2017-02-17T04:04:48Z

trustmanager/remoteks/client.go

+	location string
+}
+
+var _ trustmanager.Storage = &RemoteStore{}


why is this necessary? Is this due to the nature of GRPC?

Hacky go compile time check to ensure a type implements the desired interface. You'll see it around distribution a lot too. It's relatively idiomatic if unofficial.

riyazdf · 2017-02-17T04:08:30Z

trustmanager/remoteks/client.go

+}
+
+// Get returns the file content found at fileName relative to the base directory
+// of the file store. The path is cleaned before being made absolute to ensure


GH is unfortunately collapsing many diffs and so I'm having trouble finding the cleaning logic - could you please point me to it? (or are we relying on the underlying storage?) Also would be nice to have dedicated tests for exercising this, if possible.

These comments are just copied from the interface definition. The cleaning actually happens in the server side after passing through to the real underlying storage.

riyazdf · 2017-02-17T04:11:11Z

trustmanager/remoteks/client.go

+func (s *RemoteStore) ListFiles() []string {
+	logrus.Infof("listing files from %s", s.location)
+	fl, err := s.client.ListFiles(context.Background(), &google_protobuf.Empty{})
+	if err != nil {


can we add a test case for empty ListFiles()?

Already do :-) https://github.com/docker/notary/pull/1096/files/ca60db8420cc08ec94536811fcdcc48e789b7f96#diff-fc36b6ceeef2f9d048a363a6eeab3b0cR69

The GRPC API for Go always adds an error, but ListFiles never actually errors. The only way this error case would be hit is an error at the GRPC level.

riyazdf · 2017-02-17T04:11:23Z

trustmanager/remoteks/client.go

+}
+
+// Location returns a human readable indication of where the storage is located.
+func (s *RemoteStore) Location() string {


nit: can we add a test case for Location in case of any regressions?

ecordell

This looks good to me for just the keystore - but what about for a full repository? I liked the interface defined in the first commit a lot.

ecordell · 2017-02-17T23:03:08Z

trustmanager/remoteks/client_test.go

+	// can't just use NewRemoteStore because it correctly sets up tls
+	// config and for testing purposes it's easier for the client to just
+	// be insecure
+	cc, err := grpc.Dial(


NewRemoteStore could swap out grpc.WithTransportCredentials for grpc.WithInsecure if the tlsconfig is nil?

You are correct but it seems like InsecureSkipVerify and WithInsecure achieve the same thing so I figured it was easier to just use the well understood tls config and let users set that up however they please, rather than having 2 ways to set clients to insecure mode.

would it be easier to just use the certs already included in the project for testing? it's not a big deal since anyone actually running this will certainly discover problems with tls config right away, but it would be nice to have it in the test path if possible.

ecordell · 2017-02-17T23:04:07Z

trustmanager/remoteks/server_test.go

+}
+
+// Set, Get, List, Remove, List, Get
+func TestGRPCStorage(t *testing.T) {


What about the error cases?

endophage · 2017-02-19T19:05:16Z

@ecordell the full repo will be coming in follow up PRs, with something identical or at least very, very close to that interface. This was a nice self contained piece of functionality (that is also somewhat necessary for the remote full repository).

The dependency injection PR from @n4ss is also foundational work for the full remote repo.

cyli · 2017-02-21T22:02:52Z

trustmanager/remoteks/client.go

+		FileName: fileName,
+		Data:     data,
+	}
+	_, err := s.client.Set(context.Background(), sm)


Non-blocking - should we be setting timeouts for these by default?

cyli · 2017-02-22T02:22:18Z

trustmanager/remoteks/keystore.proto

+
+message StringListMsg {
+    repeated string FileNames = 1;
+}


Non-blocking: missing newline

cyli · 2017-02-22T02:28:09Z

trustmanager/remoteks/server.go

+// ListFiles returns all known identifiers in the storage backend.
+func (s *GRPCStorage) ListFiles(ctx context.Context, _ *google_protobuf.Empty) (*StringListMsg, error) {
+	lst := s.backend.ListFiles()
+	logrus.Debugf("found keys: %s", strings.Join(lst, ","))


If the signer is eventually going to be a remote escrow, should we actually list every key in the signer in the debug logs? Perhaps we can debug log the number?

I really need an "ultra debug" mode. Listing keys has been useful but you're right that in this case it could be a lot.

cyli · 2017-02-22T02:28:55Z

trustmanager/remoteks/server.go

+
+// Set writes the provided data under the given identifier.
+func (s *GRPCStorage) Set(ctx context.Context, msg *SetMsg) (*google_protobuf.Empty, error) {
+	logrus.Debugf("storing: %s", msg.FileName)


Non-blocking: Should the other RPC calls also be logged? Or at least Remove, since both are writes?

cyli · 2017-02-22T02:32:27Z

Could we also add trustmanager/remoteks/*.pb.go to the codecov ignore section?

…age interface Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

…ory for reference and in case it doesn't exist yet elsewhere) Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

…codecov ignore, removing debug list of all keys in remote store Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage · 2017-02-22T22:00:03Z

I think I've addressed everything here. Just rebased, tests passed locally, hopefully circle agrees.

cyli · 2017-02-22T22:18:54Z

codecov.yml

@@ -20,5 +20,6 @@ coverage:
    - "tuf/testutils/*"
    - "vendor/*"
    - "proto/*.pb.go"
+    - "trustmanager/remoteks/keystore.proto"


I think you may need to exclude the *.pb.go files, since the coverage will be calculated for the generated go files, not the proto files (I don't think codecov knows about protos)

Oh sorry, I totally crossed my wires. I meant to exclude the pb.go!

Ok fixed now. Must have been one of those occasions where I was thinking of something else while typing and the wrong thing came out.

I do that at least half the time :)

cyli · 2017-02-22T22:20:58Z

I've re-kicked the yubikey tests, and they passed. Codecov is being weird. :( This otherwise LGTM, modulo a codecov ignore change (which does not account for the codecov weirdness atm)

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

riyazdf

LGTM, this is very slick!

endophage force-pushed the pr_interfaces branch 13 times, most recently from 99fe973 to 59bba92 Compare February 16, 2017 16:20

endophage changed the title ~~WIP: remote key escrow~~ Remote key escrow Feb 16, 2017

endophage requested review from cyli and riyazdf February 16, 2017 21:17

endophage force-pushed the pr_interfaces branch 2 times, most recently from 8123c42 to ca60db8 Compare February 17, 2017 01:17

riyazdf reviewed Feb 17, 2017

View reviewed changes

endophage force-pushed the pr_interfaces branch from ca60db8 to 65ebb3d Compare February 17, 2017 19:40

ecordell suggested changes Feb 17, 2017

View reviewed changes

cyli reviewed Feb 21, 2017

View reviewed changes

cyli reviewed Feb 22, 2017

View reviewed changes

endophage force-pushed the pr_interfaces branch from 0d0d206 to 26896bf Compare February 22, 2017 19:18

remote key storage via a GRPC implementation of the trustmanager.Stor…

b5af7d6

…age interface Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

David Lawrence added 7 commits February 22, 2017 13:58

removing stuff not relevant to this pr (leaving it in the commit hist…

460726f

…ory for reference and in case it doesn't exist yet elsewhere) Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

final cleanup and tests

06ba29b

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

adding tls config for escrow server

3d907db

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

regenerating certs and adding certs for escrow

55f2204

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

dockerfile for escrow service

ea26073

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

adding some error tests

9d6a549

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

adding default timeout to keystore client, adding generated pb.go to …

ab009c7

…codecov ignore, removing debug list of all keys in remote store Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage force-pushed the pr_interfaces branch from 26896bf to 725b7d2 Compare February 22, 2017 21:59

cyli reviewed Feb 22, 2017

View reviewed changes

add tls to tests

a62ba41

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage force-pushed the pr_interfaces branch from 725b7d2 to a62ba41 Compare February 22, 2017 22:23

riyazdf approved these changes Feb 22, 2017

View reviewed changes

ecordell approved these changes Feb 22, 2017

View reviewed changes

endophage merged commit 7b2ba1c into notaryproject:master Feb 23, 2017

endophage deleted the pr_interfaces branch February 23, 2017 00:50

Remote key escrow #1096

Remote key escrow #1096

Conversation

endophage commented Feb 13, 2017

endophage commented Feb 16, 2017

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Feb 22, 2017

endophage commented Feb 22, 2017

cyli Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Feb 22, 2017 • edited Loading

riyazdf left a comment

Choose a reason for hiding this comment

riyazdf Feb 17, 2017 •

edited

Loading

cyli Feb 22, 2017 •

edited

Loading

cyli commented Feb 22, 2017 •

edited

Loading