Skip to content
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.

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

Make V2 code more defensive against malformed content #8393

Merged
merged 8 commits into from Oct 11, 2014

Conversation

@dmcgowan
Copy link
Member

commented Oct 3, 2014

Added more checks in the V2 code against possible malformed or changed content. This code will only be hit when pulling from the V2 registry, which is still only for official images.

Ping @crosbymichael

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2014

I would really love to be able to use your local registry v2 :)

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2014

@jfrazelle our fear is that since the V2 registry we are testing with right now does not also serve up V1, its behavior can be misleading. The goal is to provide a fully compatible registry early next week. We are also going to plan integrating some of the tests we are using into the integration tests.

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2014

@dmcgowan I mean if I know that ahead of time, I'm not going to think it's broken if it breaks because it can't reach v1.
I kinda just want the ability to test the same way you are, I can wait til you guys are ready next week

@dmp42 dmp42 added the Distribution label Oct 6, 2014

@crosbymichael crosbymichael added this to the 1.3.0 milestone Oct 8, 2014

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

@vieux assigned you

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2014

thank you @crosbymichael

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

Added two additional PRs for using the registry directly in V2 and moved the UX message to make it clear which image has the signature

ping @dmp42

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2014

screen shot 2014-10-08 at 14 48 46-1

@dmcgowan @dmp42 the Status line is missing with V2

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

What does this mean? For this layer it is using v1?

[info] pulling blob "tarsum+sha256:2a7812e636235448785062100bb9103096aa6655a8f6bb9ac9b13fe8290f66df" to V1 img 511136ea3c5a64f264b78b5433614aec563103b4d4702f3ba7d4d2698e22c158

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2014

regarding ^ I think it should be in debug, not info

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2014

Docker, Inc is hard coded. Shouldn't this come from the image itself ?

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

I do think The image you are pulling has been digitally signed by Docker, Inc. should come last, just from a user stand point feels more secure, but since its signing the manifest now we could keep it where it is for now

@tianon

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

Shouldn't it only show that message if the manifest is signed, and then
even more importantly if the layers the daemon actually receives match the
manifest correctly? Also, shouldn't it throw up a big scary warning if
they don't? (ie, MITM attack)

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

@tianon the big scary warning will come in the next version when we don't fallback to V1. It does only show the message is the manifest is signed and the signature key is verified.

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

we should try to make it prettier than this:

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
Someone could be eavesdropping on you right now (man-in-the-middle attack)!
It is also possible that a host key has just been changed.
The fingerprint for the RSA key sent by the remote host is 
Please contact your system administrator.

but that's a whole different convo lol

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2014

"prettier"

@@ -19,6 +19,7 @@ const CONFIGFILE = ".dockercfg"

// Only used for user auth + account creation
const INDEXSERVER = "https://index.docker.io/v1/"
const REGISTRYSERVER = "https://registry-1.docker.io/v1/"

This comment has been minimized.

Copy link
@jessfraz

jessfraz Oct 8, 2014

Contributor

also can we make this a const () block, that will be my only nit i promise :)

This comment has been minimized.

Copy link
@dmcgowan

dmcgowan Oct 8, 2014

Author Member

done, that's an easy one :)

@dmcgowan dmcgowan force-pushed the dmcgowan:provenance_pull_enhance branch from 0ce3204 to 4985442 Oct 8, 2014

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

@vieux added status and update pull message. The tty output for pull -a is still overriding the wrong line, however I will need to investigate it more to see if it is just a problem with the tty writer. The output looks fine when stdout is redirected.

@dmcgowan dmcgowan force-pushed the dmcgowan:provenance_pull_enhance branch from 4985442 to 1efd5bd Oct 8, 2014

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

I have a way to test the MITM when you all have that part built out :)

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 9, 2014

This is still waiting on some format changes to the manifest. The images will also have to be updated again after this change is made.

@dmp42

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

Confirmed: @shykes requested some changes yesterday that we need to get in. @dmcgowan is working on it right now and will PR soon, then we will have to resign the test image.

The next step will be for us to fix the "push" part so that @tianon can start signing all official images.

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 9, 2014

Thanks, please ping us when you have finished.

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

@dmcgowan @dmp42 is this good to review again? Please ping us after making changes because github does not send any type of notification if a PR is updated.

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@crosbymichael not quite yet, still waiting to get the updated manifests pushed. Will ping when it is ready.

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@crosbymichael this PR should be ready to test again. The test official images are "ubuntu-1" right now, when it is confirmed no one is testing on the prior code we will clean up the name.

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

ping @jfrazelle @vieux can you please help me test this again?

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

Ref: #8391

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

@dmcgowan can you rebase this on master and ensure that the check is reenabled for v2?

dmcgowan added 8 commits Oct 3, 2014
Make V2 code more defensive against malformed content
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Use direct registry url
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Update pull message and log
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Add status message for V2 pull
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Support tarsum dev version to fix issue with mtime
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Update manifest format to rename blobsums and use arrays of dictionaries
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Update verification message
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Enable V2 pull flow
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

@dmcgowan dmcgowan force-pushed the dmcgowan:provenance_pull_enhance branch from b401d7d to 3be4551 Oct 10, 2014

@dmcgowan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@crosbymichael @vieux @jfrazelle rebased and reenabled

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

LGTM

1 similar comment
@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2014

LGTM

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

@jfrazelle feel free to merge when you are happy

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2014

LGTM

jessfraz pushed a commit that referenced this pull request Oct 11, 2014
Jessie Frazelle
Merge pull request #8393 from dmcgowan/provenance_pull_enhance
Make V2 code more defensive against malformed content

@jessfraz jessfraz merged commit 9f482a6 into moby:master Oct 11, 2014

1 check passed

default The build succeeded on drone.io
Details

@dmcgowan dmcgowan deleted the dmcgowan:provenance_pull_enhance branch Jun 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.