docker trust: view, revoke, sign subcommands (experimental) by riyazdf · Pull Request #472 · docker/cli

riyazdf · 2017-08-24T23:24:09Z

This PR introduces the docker trust subcommand. The command has been fleshed out in detail in this design document. docker trust provides a direct notary signing experience beyond what is provided with the DOCKER_CONTENT_TRUST environment variable.

In this PR, we introduce three subcommands:

docker trust view: displays signatures on image tags, and who signed them
docker trust revoke: revokes signatures from signed image tag(s)
docker trust sign: adds a signature to an image tag, pushing the image if it doesn't already exist

cc @ashfall and @eiais who helped implement this feature

mdlinville

Several copyedit-style comments. In addition, scrub all occurrences of "like so", "please", and future-facing prose like "will sign" or "will create". In docs, we live in the eternal present and we don't say "please". :) Likewise, the phrase "note that".

mdlinville · 2017-08-25T18:03:57Z

+
+## Description
+
+Docker trust inspect provides detailed information on signed repositories.


Mark this up: docker trust inspect. If you don't feel comfortable starting the sentence with lowercase mono text, then The docker trust inspect` command...."

mdlinville · 2017-08-25T18:04:27Z

+## Description
+
+Docker trust inspect provides detailed information on signed repositories.
+This includes all image tags that are signed, who signed them, and who can sign


Does it show unsigned images? This kind of implies not.

This will not display unsigned images.

mdlinville · 2017-08-25T18:04:41Z

+This includes all image tags that are signed, who signed them, and who can sign
+new tags.
+
+By default, `docker trust inspect` will render results in a table.


s/will render/renders

mdlinville · 2017-08-25T18:04:57Z

+
+```bash
+$ docker trust inspect alpine:latest
+SIGNED TAG          DIGEST                                                             SIGNERS


Please add a newline between input and output.

mdlinville · 2017-08-25T18:05:37Z

+Root Key:	a2489bcac7a79aa67b19b96c4a3bf0c675ffdf00c6d2fabe1a5df1115e80adce
+```
+
+Note that the `SIGNED TAG` maps to the image tag itself, and associates to given image `DIGEST`. `SIGNERS` lists all entities who have signed.


s/Note that the/The

"and associates to given image" this phrase isn't parsing right for me. Maybe a rewrite?

mdlinville · 2017-08-25T18:12:42Z

+Successfully signed "docker.io/example/trust-demo":v1
+```
+
+`docker trust inspect` should now list the new signature:


s/should now list/lists

mdlinville · 2017-08-25T18:13:01Z

+
+## Initialize a new repo and sign a tag
+
+When signing an image on a repo for the first time, `docker trust sign` sets up new keys and then signs the image.


s/and then signs/before signing

mdlinville · 2017-08-25T18:13:46Z

 ## Building Notary

-Prerequisites:
+Note that our [latest stable release](https://github.com/docker/notary/releases) is at the head of the


s/Note that our/Notary's

vendored code - we can address this in docker/notary

mdlinville · 2017-08-25T18:14:04Z

+Run `make client`, which creates the Notary Client CLI binary at `bin/notary`.
+Note that `make client` assumes a standard Go directory structure, in which
 Notary is checked out to the `src` directory in your `GOPATH`. For example:
 ```


vendored code - we can address this in docker/notary

mdlinville · 2017-08-25T18:14:19Z

                notary/
 ```
+
+To build the server and signer, please run `docker-compose build`.


vendored code - we can address this in docker/notary

riyazdf · 2017-08-25T18:47:38Z

thank you @mstanleyjones for the review! Just updated with the fixes, and left a comment about one potential command output change.

dnephin

Cool!

This is a big change so I looked over the first two commands for now.

Re: testing, I think the tests you have look great, but we need to fake the notary client so we're not actually making HTTP requests in unit tests.

An end-to-end test for each of the commands (inspect, revoke, sign) would be great as well. We'll want to add a notary service to compose-env.yaml so we're testing against an isolated service. I think that can be done in a separate follow up PR.

We just merged https://github.com/docker/cli/blob/master/TESTING.md which has a bit more information as well

dnephin · 2017-08-25T19:04:42Z

-// trustedPush handles content trust pushing of an image
-func trustedPush(ctx context.Context, cli command.Cli, repoInfo *registry.RepositoryInfo, ref reference.Named, authConfig types.AuthConfig, requestPrivilege types.RequestPrivilegeFunc) error {
+// TrustedPush handles content trust pushing of an image
+func TrustedPush(ctx context.Context, cli command.Cli, repoInfo *registry.RepositoryInfo, ref reference.Named, authConfig types.AuthConfig, requestPrivilege types.RequestPrivilegeFunc) error {


We have a package at cli/trust. Do you think it would make sense to move these functions (this one and the few below that are either already exported, or were changed to exported) into the cli/trust package, now that they are being shared between multiple commands?

I think we certainly can for the GetSignableRoles function and so I've moved over.

This one is a little more tricky because it calls imagePushPrivileged - can we export that function?

Ya, I think it makes sense to export it. There's nothing in there that's really specific to the command routing. The only change I would make is to accept a client.ImageAPIClient instead of the full command.Cli

dnephin · 2017-08-25T19:10:24Z

+func NewTrustCommand(dockerCli command.Cli) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "trust",
+		Short: "Sign images to establish trust",


The current convention for these commands is "Manage x" (see docker --help). So maybe something like: "Manage trust on Docker images" ?

dnephin · 2017-08-25T19:11:57Z

+	}
+	if err = cl.Clear(""); err != nil {
+		return err
+	}


minor: this if is unnecessary,

return cl.Clear("")

dnephin · 2017-08-25T19:14:44Z

+
+const releasedRoleName = "Repo Admin"
+
+func checkLocalImageExistence(ctx context.Context, cli command.Cli, imageName string) error {


I believe this is only used in sign.go so would be more appropriate immediately following the function that calls it.

dnephin · 2017-08-25T19:22:14Z

+	return err
+}
+
+func getImageReferencesAndAuth(cli command.Cli, imgName string) (context.Context, reference.Named, *registry.RepositoryInfo, *types.AuthConfig, error) {


Returning a context.Context is a bit unconventional. I think this function should accept the context as the first arg.

Maybe as a follow up, it would be nice to have a type for reference.Named, *registry.RepositoryInfo, *types.AuthConfig. It seems they are all directly related, so I think having a type would reduce the number of args sent to a couple of functions.

I also notice that getTag() is always called after this. With a type we could parse out the tag immediately in this single step, and access the tag from .Tag().

dnephin · 2017-08-25T19:56:51Z

+	assert.NotContains(t, cli.OutBuffer().String(), "(Repo Admin)")
+}
+
+func TestTUFToSigner(t *testing.T) {


TestNotaryRoleToSigner

dnephin · 2017-08-25T19:59:45Z

+		},
+	}
+	flags := cmd.Flags()
+	flags.BoolVarP(&options.forceYes, "yes", "y", false, "Answer yes to the removal question (no confirmation)")


To be consistent with other commands, I believe we use the text Do not prompt for confirmation

dnephin · 2017-08-25T20:00:46Z

+	return nil
+}
+
+func revokeTestHelper(notaryRepo *client.NotaryRepository, tag string) error {


What does TestHelper refer to here? Could this be revokeSignature() ?

dnephin · 2017-08-25T20:02:43Z

+	}
+	defer clearChangeList(notaryRepo)
+	if err := revokeTestHelper(notaryRepo, tag); err != nil {
+		return fmt.Errorf("could not remove signature for %s: %s", remote, err)


errors.Wrapf(err, "could not remove...")

from pkg/errors

There's a couple other instances of these as well

dnephin · 2017-08-25T20:07:46Z

+	cli := test.NewFakeCli(&fakeClient{})
+	cmd := newInspectCommand(cli)
+	cmd.SetArgs([]string{"alpine"})
+	assert.NoError(t, cmd.Execute())


Does this contact the "real" notary server? Is that what you were referring to by fixture images in the description?

I think maybe trust.GetNotaryRepository() should be changed to Cli.GetNotaryClient() (or something like that) which would return an interface instead of a client.NotaryRepostiory struct . Once it's on the command.Cli interface it makes it really easy to fake it in tests.

it does contact a real notary server. Is the idea that Cli.GetNotaryClient would wrap a notary repo with a subset of the API? It seems that could certainly work

It doesn't necessary have to wrap it with new structs, it could just return an Interface type instead of a struct, so that we can use a fake during testing.

I've been trying to wrap my head around this but I'm not sure if it makes sense since a NotaryRepository is custom per-image. Is that OK in the context of command.Cli, or are assuming that a command.Cli may persist for longer than a single image command?

That does make it more difficult. We ran into this problem in #138 with the distribution client. In that case I wrapped the whole thing in an interface that was more appropriate for our use case in the cli (https://github.com/docker/cli/pull/138/files#diff-cb6e6f1ab385cb866626ee63bfa1a273R29).

It sounds like notary is using a similar pattern.

I think something like this in cli/command/cli.go should work:

func (cli *DockerCli) NotaryClient(ref trust.ImageRefAndAuth) trust.Repository { ... }

where trust.Repository is the local interface we define (maybe in cli/trust).

We can still have multiple instances per command.Cli. In tests we can return a fakeTrustRepository.

awesome, that would work. I'll implement this 👍

dnephin · 2017-08-25T20:16:32Z

I think the design looks good. It sounds like you've already done a lot of work on the design before opening this PR so I mostly focused on the code.

riyazdf · 2017-08-26T00:56:00Z

@dnephin: thank you for the review! I've addressed all comments except for the following:

for the notary tag you suggest: there isn't a "canonical" notary server or a way to currently pre-configure one per-image (you could in theory contact a different one for different images) - so I'm inclined to add this later if you think that makes sense
I've flattened the map for the admin keys. Haven't done so for signers and the signatures because I want to make sure I don't mess up the table printing 😅
totally agree for mocking out the notary client behind the CLI so we can mock it. Will get to this.

dnephin · 2017-08-26T01:05:32Z

so I'm inclined to add [the notary tag] later if you think that makes sense

Sounds good. I think we can forget about the tag, I can't think of a good reason to hide the commands.

riyazdf · 2017-09-01T22:38:15Z

Some small updates: I've made a couple of tweaks to address small discrepancies in CLI output and we also have an interface for a trust repository incoming from Notary notaryproject/notary#1220 that will help provide testable mocks for this PR. I'll update here once that is merged and vendored.

riyazdf · 2017-09-12T16:55:18Z

@dnephin: I've rebased and vendored in the notary client interface and exposed it as cli.NotaryClient. I'm rewriting the tests now to use mocks of that, but figured I'd push the update so you can see how the cli changes look

dnephin

Thanks, looks good

dnephin · 2017-09-12T17:06:07Z

+}
+
+// ImageRefAndAuth contains all reference information and the auth config for an image request
+type ImageRefAndAuth struct {


I think the cli/trust package might be a better place for this type

dnephin · 2017-09-12T17:11:16Z

 }

+// NotaryClient provides a Notary Repository to interact with signed metadata for an image
+func (cli *DockerCli) NotaryClient(imgRefAndAuth ImageRefAndAuth, forWrite bool) (notaryclient.Repository, error) {


I always try to avoid boolean arguments because from the caller it's not really clear what true or false means. Maybe this could takes a []string of actions, and there could be two predefined slices in cli/trust

var ( ActionsPullOnly = []string{"pull"} ActionsPushAndPull = []string{"pull", "push"} )

So from the caller it will be obvious what is being requested.

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

…ommand semantics Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

thaJeztah

changes and putting it as "experimental" LGTM - we can address tweaking the commands/UX in follow ups

johnstep · 2017-09-26T00:03:35Z

I verified that signing and viewing works from Windows.

riyazdf requested review from mdlinville, thaJeztah and vdemeester as code owners August 24, 2017 23:24

GordonTheTurtle added the status/0-triage label Aug 24, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Aug 25, 2017

vdemeester requested review from cpuguy83, dnephin and vieux August 25, 2017 06:21

mdlinville requested a review from gbarr01 August 25, 2017 17:36

mdlinville suggested changes Aug 25, 2017

View reviewed changes

riyazdf force-pushed the docker-trust branch from 9dd2a09 to d094100 Compare August 25, 2017 18:44

dnephin reviewed Aug 25, 2017

View reviewed changes

riyazdf force-pushed the docker-trust branch 6 times, most recently from 1f18ec9 to ed1f469 Compare August 26, 2017 00:53

riyazdf force-pushed the docker-trust branch 2 times, most recently from efa876c to 272b5a2 Compare September 1, 2017 22:08

riyazdf force-pushed the docker-trust branch from 272b5a2 to 58b0e75 Compare September 12, 2017 16:54

dnephin reviewed Sep 12, 2017

View reviewed changes

riyazdf force-pushed the docker-trust branch from 58b0e75 to 75bc720 Compare September 12, 2017 19:17

riyazdf added 17 commits September 25, 2017 09:34

trust inspect: docs for docker trust inspect

809ef0f

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust sign: add docker trust sign command

fab6bb6

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust sign: docs for docker trust sign

bc665ed

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust revoke: add docker trust revoke command

609f8b4

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust revoke: docs for docker trust revoke

43717a8

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

docs: update docker trust docs with correct tense and formatting

007aff7

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: address review feedback, refactor to align with existing cli/c…

45c102a

…ommand semantics Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: add Repository client interface

7c5b836

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

cli: introduce NotaryClient getter

e5c35ab

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: use mock CLI for testing offline

f667bd7

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

tests: address review feedback

6fca400

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: update reference type and use golden output

4e89dc8

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

tests: move trust test to proper package

67cf09c

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: update remove to error on empty references for consistency

4e95fcd

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

tests: use alice/bob/claire conventional names for signers

46a879e

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

trust: rename inspect to view, add repo name to signer table header

c6db0cd

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

mark command as experimental in docs and cli

e07f345

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

riyazdf force-pushed the docker-trust branch from 941f7cb to e07f345 Compare September 25, 2017 16:52

thaJeztah approved these changes Sep 25, 2017

View reviewed changes

vieux approved these changes Sep 26, 2017

View reviewed changes

vieux merged commit af3cdcc into docker:master Sep 26, 2017

GordonTheTurtle added this to the 17.10.0 milestone Sep 26, 2017

thaJeztah added the impact/changelog label Sep 26, 2017

riyazdf mentioned this pull request Sep 27, 2017

docker trust: interact with signers and keys #566

Merged

This was referenced Sep 29, 2017

revendor logrus and x/crypto #572

Merged

trust sign: add --local flag #575

Merged

albers mentioned this pull request Oct 7, 2017

Add bash completion for docker trust commands #609

Merged

thaJeztah mentioned this pull request Oct 31, 2017

Add JSON output option for docker trust view #659

Closed

riyazdf mentioned this pull request Nov 1, 2017

remove please and change our to Notary's in README notaryproject/notary#1256

Merged


		## Description

		Docker trust inspect provides detailed information on signed repositories.


		## Initialize a new repo and sign a tag

		When signing an image on a repo for the first time, `docker trust sign` sets up new keys and then signs the image.


		const releasedRoleName = "Repo Admin"

		func checkLocalImageExistence(ctx context.Context, cli command.Cli, imageName string) error {

Conversation

riyazdf commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdlinville left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdlinville Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

riyazdf commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

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!

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!

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!

dnephin commented Aug 25, 2017

Uh oh!

riyazdf commented Aug 26, 2017

riyazdf commented Aug 24, 2017 •

edited

Loading

mdlinville Aug 25, 2017 •

edited

Loading

riyazdf commented Aug 25, 2017 •

edited

Loading

riyazdf commented Sep 12, 2017 •

edited

Loading