Skip to content

Notary integration#14546

Merged
icecrime merged 13 commits intomoby:masterfrom
dmcgowan:trusted-notary-integration
Jul 25, 2015
Merged

Notary integration#14546
icecrime merged 13 commits intomoby:masterfrom
dmcgowan:trusted-notary-integration

Conversation

@dmcgowan
Copy link
Member

Summary

Add support for trusted image distribution through the use of notary. Notary logic is done client side to allow cli users full configuration of trust and limiting access to private keys to user's machine. Trusted cli actions which involve pulling will always do a pull by digest to the daemon. Since the daemon already supports pull by digest, no changes to the daemon are needed to support the use of notary. All signatures will be done directly from the client on push. For more details on the design, see the design document.

UX Changes

Flags

  • --untrusted - Explicitly run a pull or push action as untrusted. Defaults to true in 1.7. This can be used to override the DOCKER_NOTARY environment variable.

Trusted Mode

Pull Changes

Pull by digest

The initial pull line will now always include the digest of the image being pulled.

Sequential pull all

When pulling multiple tags, the client will now sequentially call a pull by digest on every tag.

Full sample pull output

Expanded name to docker.io/dmcgowan/myapp
Pull (1 of 1): dmcgowan/myapp:latest@sha256:8d62273b15d0dd7726e5dd7e27b5a1c8bff2106557caccfa5e2f1ba6956556c8
sha256:8d62273b15d0dd7726e5dd7e27b5a1c8bff2106557caccfa5e2f1ba6956556c8: Pulling from dmcgowan/myapp
a8219747be10: Already exists 
91c95931e552: Already exists 
Digest: sha256:8d62273b15d0dd7726e5dd7e27b5a1c8bff2106557caccfa5e2f1ba6956556c8
Status: Downloaded newer image for dmcgowan/myapp@sha256:8d62273b15d0dd7726e5dd7e27b5a1c8bff2106557caccfa5e2f1ba6956556c8
Tagging dmcgowan/myapp@sha256:8d62273b15d0dd7726e5dd7e27b5a1c8bff2106557caccfa5e2f1ba6956556c8 as dmcgowan/myapp:latest

Push Changes

Status Line

Push status line which displays digest now shows tag, digest, and size. This was used for client to be able to parse from the output what the pushed manifests were. These output lines represent the changes which will be made to the notary repository.

old

Digest: sha256:f2ee4bbffcce22f295f8c9a987dcc55f0987b339bf8fc0277e52560563e5c118

new

latest: digest: sha256:f2ee4bbffcce22f295f8c9a987dcc55f0987b339bf8fc0277e52560563e5c118 size: 10115

Passphrase prompt on first push of repository

When a repository is pushed for the first time the cli will be prompted for a passphrase. The passphrase will not be echoed back to the user. If metadata is near expiration time, the passphrase may be prompted again to re-sign the metadata. Subsequent pushes should not require entering a passphrase. If entering in a passphrase is not desirable, then the notary tool may be used directly to initialize a repository.

Full sample push output

The push refers to a repository [dmcgowan/myapp] (len: 1)
91c95931e552: Image already exists 
a8219747be10: Image already exists 
latest: digest: sha256:f2ee4bbffcce22f295f8c9a987dcc55f0987b339bf8fc0277e52560563e5c118 size: 10115
Signing and pushing TUF metadata
Expanded name to docker.io/dmcgowan/myapp
Loading trusted collection.
Creating a new private key
Passphrase:
Saving changes to Trusted Collection.
Finished initializing "docker.io/dmcgowan/myapp"
Loading trusted collection.

Environment Variables

  • DOCKER_NOTARY - boolean whether to use Notary in Docker
  • NOTARY_SERVER - Notary server to use for all repositories

Remaining items for code review

  • Authentication integration with distribution client (need to solve problem with FQDN support)
  • Load Certificates from ~/.docker/tls/

@jessfraz
Copy link
Contributor

Can we combine this PR with #14453 so the conversation is not fragmented

@jessfraz
Copy link
Contributor

Also how will these changes be effected by the refactor of the registry code, which I hope will get in before this PR if I am not mistaken right?

@dmcgowan
Copy link
Member Author

@jfrazelle I decoupled the registry refactor for the initial PR so this would not be dependent on the other. The authentication code will require using code from the registry client though. Either way we can get this in without the other.

@calavera
Copy link
Contributor

I wonder if we should put this in experimental. That way we'd have more time to polish it.

@jessfraz
Copy link
Contributor

is this the same vendored uuid library we were having issues with

@dmcgowan
Copy link
Member Author

I think it is the same library, will submit a PR to notary to use the distribution version

@dmcgowan
Copy link
Member Author

Updated uuid library

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we really going to depend on a fork on the standard encoding/json for a diff which I personally can't find because the file content was reordered?

@jessfraz
Copy link
Contributor

some tests would be nice, even just unit tests, but integration maybe at least one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt there be a TODO for when isTrusted(repo) is false we will eventually want to explode and die with 80 bajillion warnings

@dmcgowan
Copy link
Member Author

I would ask what the value is, I could easily squash the 3rd to last, the last one was left unsquash to easily backout (no emotional attachment), and the second to last I left untouched to make updates easier and the squash should be ugly to put each change in their proper place 😄.

dmcgowan and others added 4 commits July 24, 2015 16:31
Clean up tests to remove duplicate code

Add tests which run pull and create in an isolated configuration directory.
Add build test for untrusted tag

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Nathan McCauley <nathan.mccauley@docker.com>
Signed-off-by: Diogo Monica <diogo@docker.com>
Update help line to allow 90 characters instead of 80

The trust flag pushes out the help description column wider, requiring more room to display help messages.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@dmcgowan dmcgowan force-pushed the trusted-notary-integration branch from 380139f to 259cadb Compare July 24, 2015 23:31
@dmcgowan
Copy link
Member Author

Bah ci failed anyway, squashed away 2 more and pushed

@jessfraz
Copy link
Contributor

started lxc build, bc why nottttttt 😇

@icecrime
Copy link
Contributor

Please ignore ping failures against www.docker.io in CI.

@icecrime
Copy link
Contributor

Ignoring failures on www.docker.io pings: we're good to go.

icecrime pushed a commit that referenced this pull request Jul 25, 2015
@icecrime icecrime merged commit 4f5b677 into moby:master Jul 25, 2015
@jlhawn
Copy link
Contributor

jlhawn commented Jul 25, 2015

🎉

@jlhawn
Copy link
Contributor

jlhawn commented Jul 25, 2015

so, should I submit https://github.com/dmcgowan/docker/pull/25/files against upstream then?

duglin pushed a commit to duglin/docker that referenced this pull request Jul 27, 2015
moby#14546 actually fixed issue moby#14837
but I don't see a new test to ensure we don't regress. So this PR adds
a test and then we can close moby#14837.

Closes moby#14837

Signed-off-by: Doug Davis <dug@us.ibm.com>
sallyom pushed a commit to sallyom/docker that referenced this pull request Aug 13, 2015
moby#14546 actually fixed issue moby#14837
but I don't see a new test to ensure we don't regress. So this PR adds
a test and then we can close moby#14837.

Closes moby#14837

Signed-off-by: Doug Davis <dug@us.ibm.com>
duglin pushed a commit to duglin/docker that referenced this pull request Oct 7, 2015
I disagree with moby#14546 that pushed the help text past 80 chars.
Aside from it now making the help text look ugly on 80 char displays,
which I use, one thing I like about the previous limitation is that it
forced us to keep our options down to more reasonable phrases/words.
For example, I think
`    --disable-content-trust=true`
could have been:
`    --disable-trust=true`
or even:
`    --disable-ctrust=true`

But regardless, let's at least make the comments match what the code does.

Signed-off-by: Doug Davis <dug@us.ibm.com>
@phemmer
Copy link
Contributor

phemmer commented Oct 16, 2015

Just FYI, this PR is the cause of #15785
Specifically 3021b7a

rwincewicz pushed a commit to rwincewicz/docker that referenced this pull request Oct 21, 2015
I disagree with moby#14546 that pushed the help text past 80 chars.
Aside from it now making the help text look ugly on 80 char displays,
which I use, one thing I like about the previous limitation is that it
forced us to keep our options down to more reasonable phrases/words.
For example, I think
`    --disable-content-trust=true`
could have been:
`    --disable-trust=true`
or even:
`    --disable-ctrust=true`

But regardless, let's at least make the comments match what the code does.

Signed-off-by: Doug Davis <dug@us.ibm.com>
@dmcgowan dmcgowan deleted the trusted-notary-integration branch June 15, 2016 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.