Docker digest validation is too strict #32627

smarterclayton · 2016-09-14T00:32:40Z

Docker 1.10 does not guarantee that the pulled digest matches the digest
on disk when dealing with v1 schemas stored in a Docker registry. This
is the case for images like
centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf
which as a result of #30366 cannot be pulled by Kube from a Docker 1.10
system.

This partially reverts commit 875fd16.

This change is

smarterclayton · 2016-09-14T00:33:15Z

@eparis if you want to normal cherry-pick we can use this one - I wanted to make sure the patch compiled against 1.4 which is why I opened the other PR

ncdc · 2016-09-14T00:33:27Z

LGTM

k8s-cherrypick-bot · 2016-09-14T00:34:20Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

smarterclayton · 2016-09-14T01:12:43Z

Hold until I validate in the morning whether Docker 1.10 is correctly setting RepoDigest on all supported versions.

smarterclayton · 2016-09-14T01:58:23Z

Updated to use RepoDigest first, use stricter parsing (which could break if Docker updates, but prevents us from missing string format changes). I would prefer to use the Docker structures (since we already are for parsing the ref) so as to avoid potential prefix attacks due to comparison.

smarterclayton · 2016-09-14T01:59:53Z

All 3 major RHEL versions and 2 fedora versions I tested returned RepoDigest.

pwittrock · 2016-09-14T03:22:30Z

~~@yujuhong Why did you remove the lgtm?~~

Nevermind, I read Clayton's last comment

smarterclayton · 2016-09-14T03:24:39Z

Hold for re-review with the new code approach

smarterclayton · 2016-09-14T14:48:19Z

This is ready for re-review

ncdc · 2016-09-14T15:08:40Z

pkg/kubelet/dockertools/docker.go

+			if d, isDigested := named.(dockerref.Digested); isDigested {
+				// Allow prefix matches for shorter SHA values
+				if digest.Digest().Algorithm().String() == d.Digest().Algorithm().String() &&
+					strings.HasPrefix(digest.Digest().Hex(), digest.Digest().Hex()) {


Shouldn't one of these digests be d?

yujuhong · 2016-09-14T16:23:31Z

pkg/kubelet/dockertools/docker.go

+				continue
+			}
+			if d, isDigested := named.(dockerref.Digested); isDigested {
+				// Allow prefix matches for shorter SHA values


nit: remove the comment.

yujuhong · 2016-09-14T16:24:11Z

lgtm. just a nit to remove the outdated comment.

smarterclayton · 2016-09-14T16:25:42Z

Nit fixed

On Wed, Sep 14, 2016 at 12:24 PM, Yu-Ju Hong notifications@github.com
wrote:

lgtm. just a nit to remove the outdated comment.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#32627 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7uZvR7edB7-d1RYhJMFbf8ePnWsks5qqB-6gaJpZM4J8SYk
.

yujuhong · 2016-09-14T16:27:12Z

Thanks a lot for fixing this!

ncdc · 2016-09-14T17:06:21Z

Removing lgtm b/c of 1 issue which I'll comment on shortly

ncdc · 2016-09-14T17:06:33Z

pkg/kubelet/dockertools/docker.go

+			}
+			if d, isDigested := named.(dockerref.Digested); isDigested {
+				if digest.Digest().Algorithm().String() == d.Digest().Algorithm().String() &&
+					digest.Digest().Hex() == digest.Digest().Hex() {


double digest again

good catch. Add a unit test?

Just as an FYI, if we get into this for loop, it means:

pod.containers[i].image is digested

the image exists on the node

the kubelet was able to docker inspect the image using the digest

It should be impossible to get here and not have a match between pod.containers[i].image and the value from RepoDigests.

We should be defensive, so I'm not suggesting we remove or change anything here. Just an FYI

Docker 1.10 does not guarantee that the pulled digest matches the digest on disk when dealing with v1 schemas stored in a Docker registry. This is the case for images like centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf which as a result of kubernetes#30366 cannot be pulled by Kube from a Docker 1.10 system. Instead, use RepoDigests field as the primary match, validating the digest, and then fall back to ID (also validating the match). Adds more restrictive matching.

smarterclayton · 2016-09-14T18:14:43Z

Updated with all comments.

ncdc · 2016-09-14T18:20:17Z

LGTM

yujuhong · 2016-09-14T18:23:56Z

@smarterclayton I'll create a cherrypick PR using the script.

smarterclayton · 2016-09-14T18:49:12Z

Thanks

On Wed, Sep 14, 2016 at 2:24 PM, Yu-Ju Hong notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton I'll create a
cherrypick PR using the script.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32627 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8Xgi6TPaWHry578bJMOtOSf-kuRks5qqDvJgaJpZM4J8SYk
.

k8s-bot · 2016-09-14T18:52:53Z

GCE e2e build/test passed for commit 4a48bf8.

k8s-github-robot · 2016-09-14T19:29:04Z

Automatic merge from submit-queue

Automatic merge from submit-queue Docker digest validation is too strict Docker 1.10 does not guarantee that the pulled digest matches the digest on disk when dealing with v1 schemas stored in a Docker registry. This is the case for images like centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf which as a result of kubernetes#30366 cannot be pulled by Kube from a Docker 1.10 system. This partially reverts commit 875fd16. (cherry picked from commit b841a8b)

k8s-cherrypick-bot · 2016-09-15T00:04:25Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Automatic merge from submit-queue Docker digest validation is too strict Docker 1.10 does not guarantee that the pulled digest matches the digest on disk when dealing with v1 schemas stored in a Docker registry. This is the case for images like centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf which as a result of kubernetes#30366 cannot be pulled by Kube from a Docker 1.10 system. This partially reverts commit 875fd16. (cherry picked from commit b841a8b)

googlebot added the cla: yes label Sep 14, 2016

ncdc added the cherrypick-candidate label Sep 14, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 14, 2016

yujuhong added this to the v1.4 milestone Sep 14, 2016

yujuhong added cherrypick-candidate release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 14, 2016

k8s-github-robot assigned vishh Sep 14, 2016

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 14, 2016

yujuhong assigned yujuhong and unassigned vishh Sep 14, 2016

yujuhong added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. retest-not-required and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 14, 2016

yujuhong mentioned this pull request Sep 14, 2016

Validate SHA/Tag when checking docker images #30366

Merged

k8s-github-robot mentioned this pull request Sep 14, 2016

[k8s.io] Kubectl client [k8s.io] Kubectl taint should remove all the taints with the same key off a node {Kubernetes e2e suite} #32535

Closed

smarterclayton force-pushed the relax_content_id branch from 3657594 to 28e8dc3 Compare September 14, 2016 01:55

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 14, 2016

ncdc reviewed Sep 14, 2016
View reviewed changes

smarterclayton force-pushed the relax_content_id branch from 28e8dc3 to 0c9e994 Compare September 14, 2016 15:13

smarterclayton force-pushed the relax_content_id branch from 0c9e994 to 331a841 Compare September 14, 2016 16:16

yujuhong reviewed Sep 14, 2016
View reviewed changes

smarterclayton force-pushed the relax_content_id branch from 331a841 to 95cee6e Compare September 14, 2016 16:25

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2016

ncdc removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2016

ncdc reviewed Sep 14, 2016
View reviewed changes

smarterclayton force-pushed the relax_content_id branch from 95cee6e to 4a48bf8 Compare September 14, 2016 18:13

smarterclayton mentioned this pull request Sep 14, 2016

Docker digest validation is too strict #32624

Closed

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2016

yujuhong mentioned this pull request Sep 14, 2016

Automated cherry pick of #32627 #32689

Closed

k8s-github-robot merged commit b841a8b into kubernetes:master Sep 14, 2016

eparis added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 14, 2016

eparis mentioned this pull request Sep 14, 2016

Release 1.4 Cherrypick Batch #32717

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker digest validation is too strict #32627

Docker digest validation is too strict #32627

smarterclayton commented Sep 14, 2016 •

edited by k8s-oncall

Loading

smarterclayton commented Sep 14, 2016

ncdc commented Sep 14, 2016

k8s-cherrypick-bot commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

pwittrock commented Sep 14, 2016 •

edited

Loading

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

ncdc Sep 14, 2016

smarterclayton Sep 14, 2016

yujuhong Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

ncdc commented Sep 14, 2016

ncdc Sep 14, 2016

yujuhong Sep 14, 2016

ncdc Sep 14, 2016

smarterclayton commented Sep 14, 2016

ncdc commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

k8s-bot commented Sep 14, 2016

k8s-github-robot commented Sep 14, 2016

k8s-cherrypick-bot commented Sep 15, 2016

Docker digest validation is too strict #32627

Docker digest validation is too strict #32627

Conversation

smarterclayton commented Sep 14, 2016 • edited by k8s-oncall Loading

smarterclayton commented Sep 14, 2016

ncdc commented Sep 14, 2016

k8s-cherrypick-bot commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

pwittrock commented Sep 14, 2016 • edited Loading

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

ncdc Sep 14, 2016

Choose a reason for hiding this comment

smarterclayton Sep 14, 2016

Choose a reason for hiding this comment

yujuhong Sep 14, 2016

Choose a reason for hiding this comment

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

ncdc commented Sep 14, 2016

ncdc Sep 14, 2016

Choose a reason for hiding this comment

yujuhong Sep 14, 2016

Choose a reason for hiding this comment

ncdc Sep 14, 2016

Choose a reason for hiding this comment

smarterclayton commented Sep 14, 2016

ncdc commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

k8s-bot commented Sep 14, 2016

k8s-github-robot commented Sep 14, 2016

k8s-cherrypick-bot commented Sep 15, 2016

smarterclayton commented Sep 14, 2016 •

edited by k8s-oncall

Loading

pwittrock commented Sep 14, 2016 •

edited

Loading