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.

Sign up for GitHub

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

Jump to bottom

docker trust: view, revoke, sign subcommands (experimental) #472

Merged

vieux merged 20 commits into docker:master from riyazdf:docker-trust

Sep 26, 2017

riyazdf commented Aug 24, 2017 •

edited

Loading

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

riyazdf requested review from mdlinville, thaJeztah and vdemeester as code owners

August 24, 2017 23:24

GordonTheTurtle added the status/0-triage label

vdemeester added status/1-design-review and removed status/0-triage labels

vdemeester requested review from dnephin, cpuguy83 and vieux

August 25, 2017 06:21

mdlinville requested a review from gbarr01

August 25, 2017 17:36

mdlinville suggested changes

View reviewed changes

Contributor

mdlinville left a comment

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".

docs/reference/commandline/trust_inspect.md Outdated


		## Description

		Docker trust inspect provides detailed information on signed repositories.

Contributor

mdlinville Aug 25, 2017

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...."

docs/reference/commandline/trust_inspect.md Outdated

+              ## 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

Contributor

mdlinville Aug 25, 2017 •

edited

Loading

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

Contributor

eiais Aug 25, 2017

This will not display unsigned images.

docs/reference/commandline/trust_inspect.md Outdated

+              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.

Contributor

mdlinville Aug 25, 2017

s/will render/renders

docs/reference/commandline/trust_inspect.md Outdated

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

Contributor

mdlinville Aug 25, 2017

Please add a newline between input and output.

docs/reference/commandline/trust_inspect.md Outdated

+              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.

Contributor

mdlinville Aug 25, 2017

s/Note that the/The

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

docs/reference/commandline/trust_sign.md Outdated

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

Contributor

mdlinville Aug 25, 2017

s/should now list/lists

docs/reference/commandline/trust_sign.md Outdated


		## 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.

Contributor

mdlinville Aug 25, 2017

s/and then signs/before signing

vendor/github.com/docker/notary/README.md

@@ @@ -78,18 +78,21 @@ to use `notary` with Docker images. @@
               ## Building Notary
-              Prerequisites:
+              Note that our [latest stable release](https://github.com/docker/notary/releases) is at the head of the

Contributor

mdlinville Aug 25, 2017

s/Note that our/Notary's

Author

riyazdf Aug 25, 2017

vendored code - we can address this in docker/notary

vendor/github.com/docker/notary/README.md

    
              Run `make binaries`, which creates the Notary Client CLI binary at `bin/notary`.

              Note that `make binaries` assumes a standard Go directory structure, in which

              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:

              ```

Contributor

mdlinville Aug 25, 2017

Newlines

Author

riyazdf Aug 25, 2017

vendored code - we can address this in docker/notary

vendor/github.com/docker/notary/README.md

@@ @@ -98,3 +101,5 @@ $GOPATH/ @@
                           docker/
                               notary/
               ```
+              To build the server and signer, please run `docker-compose build`.

Contributor

mdlinville Aug 25, 2017

s/please//

Author

riyazdf Aug 25, 2017

vendored code - we can address this in docker/notary

riyazdf force-pushed the docker-trust branch from 9dd2a09 to d094100 Compare

August 25, 2017 18:44

Author

riyazdf commented Aug 25, 2017 •

edited

Loading

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

dnephin reviewed

View reviewed changes

Contributor

dnephin left a comment

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

cli/command/image/trust.go

    
              // 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 {

Contributor

dnephin Aug 25, 2017

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?

Author

riyazdf Aug 25, 2017

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?

Contributor

dnephin Aug 26, 2017

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

cli/command/trust/cmd.go Outdated

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

Contributor

dnephin Aug 25, 2017

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

cli/command/trust/helpers.go Outdated

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

Contributor

dnephin Aug 25, 2017

minor: this if is unnecessary,

return cl.Clear("")

cli/command/trust/helpers.go Outdated


		const releasedRoleName = "Repo Admin"

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

Contributor

dnephin Aug 25, 2017

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

cli/command/trust/helpers.go Outdated

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

Contributor

dnephin Aug 25, 2017

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

cli/command/trust/inspect_test.go Outdated

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

Contributor

dnephin Aug 25, 2017

TestNotaryRoleToSigner

cli/command/trust/revoke.go Outdated

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

Contributor

dnephin Aug 25, 2017

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

cli/command/trust/revoke.go Outdated

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

Contributor

dnephin Aug 25, 2017

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

cli/command/trust/revoke.go Outdated

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

Contributor

dnephin Aug 25, 2017

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

from pkg/errors

There's a couple other instances of these as well

cli/command/trust/inspect_test.go Outdated

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

Contributor

dnephin Aug 25, 2017

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.

Author

riyazdf Aug 25, 2017

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

Contributor

dnephin Aug 25, 2017

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.

Author

riyazdf Aug 29, 2017

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?

Contributor

dnephin Aug 29, 2017

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.

Author

riyazdf Aug 29, 2017

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

Contributor

dnephin commented Aug 25, 2017

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 force-pushed the docker-trust branch 6 times, most recently from 1f18ec9 to ed1f469 Compare

August 26, 2017 00:53

Author

riyazdf commented Aug 26, 2017

@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.

Contributor

dnephin commented Aug 26, 2017

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 force-pushed the docker-trust branch 2 times, most recently from efa876c to 272b5a2 Compare

September 1, 2017 22:08

Author

riyazdf commented Sep 1, 2017

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 force-pushed the docker-trust branch from 272b5a2 to 58b0e75 Compare

September 12, 2017 16:54

Author

riyazdf commented Sep 12, 2017 •

edited

Loading

@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 reviewed

View reviewed changes

Contributor

dnephin left a comment

Thanks, looks good

cli/command/cli.go Outdated

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

Contributor

dnephin Sep 12, 2017

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

cli/command/cli.go Outdated

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

Contributor

dnephin Sep 12, 2017

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.

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 <[email protected]>


          trust sign: add docker trust sign command

fab6bb6

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


          trust sign: docs for docker trust sign

bc665ed

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


          trust revoke: add docker trust revoke command

609f8b4

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


          trust revoke: docs for docker trust revoke

43717a8

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


          docs: update docker trust docs with correct tense and formatting

007aff7

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


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

45c102a

…ommand semantics

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


          trust: add Repository client interface

7c5b836

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


          cli: introduce NotaryClient getter

e5c35ab

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


          trust: use mock CLI for testing offline

f667bd7

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


          tests: address review feedback

6fca400

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


          trust: update reference type and use golden output

4e89dc8

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


          tests: move trust test to proper package

67cf09c

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


          trust: update remove to error on empty references for consistency

4e95fcd

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


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

46a879e

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


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

c6db0cd

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


          mark command as experimental in docs and cli

e07f345

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

riyazdf force-pushed the docker-trust branch from 941f7cb to e07f345 Compare

September 25, 2017 16:52

thaJeztah approved these changes

View reviewed changes

Member

thaJeztah left a comment

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

Contributor

johnstep commented Sep 26, 2017

I verified that signing and viewing works from Windows.

vieux approved these changes

View reviewed changes

vieux merged commit af3cdcc into docker:master

GordonTheTurtle added this to the 17.10.0 milestone

thaJeztah added the impact/changelog label

riyazdf mentioned this pull request

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

Add bash completion for docker trust commands #609

Merged

thaJeztah mentioned this pull request

Add JSON output option for docker trust view #659

Closed

riyazdf mentioned this pull request

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

Merged

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

Reviewers

eiais eiais left review comments

mdlinville mdlinville approved these changes

thaJeztah thaJeztah approved these changes

vieux vieux approved these changes

dnephin dnephin approved these changes

vdemeester Awaiting requested review from vdemeester

cpuguy83 Awaiting requested review from cpuguy83

gbarr01 Awaiting requested review from gbarr01

Labels

impact/changelog status/2-code-review