Validate SHA/Tag when checking docker images #30366

dims · 2016-08-10T14:28:31Z

Docker API does not validate the tag/sha, for example, all the following
calls work say for a alpine image with short SHA "4e38e38c8ce0"
echo -e "GET /images/alpine:4e38e38c8ce0/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock
echo -e "GET /images/alpine:4e38e38c/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock
echo -e "GET /images/alpine:4/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock

So we should check the response from the Docker API and look for the tags or SHA explicitly.

Fixes #30355

This change is

yujuhong · 2016-08-10T17:10:11Z

pkg/kubelet/dockertools/kube_docker_client.go

@@ -184,7 +185,27 @@ func (d *kubeDockerClient) InspectImage(image string) (*dockertypes.ImageInspect
 		}
 		return nil, err
 	}
-	return &resp, nil
+	parts := strings.SplitN(image, ":", 2)


Could probably reuse/share the logic in applyDefaultImageTag

kubernetes/pkg/kubelet/dockertools/docker.go

Line 161 in 9f1c3a4

_, isDigested := named.(dockerref.Digested)

yujuhong · 2016-08-10T17:11:23Z

Could you add a unit test for this? Thanks!

/cc @Random-Liu

dims · 2016-08-10T17:17:25Z

@yujuhong I don't see ANY tests for anything in this file !!

yujuhong · 2016-08-10T18:08:04Z

@yujuhong I don't see ANY tests for anything in this file !!

That's not a very good reason to not add a test, is it? :-)
I think this file was supposed to be a simple wrapper, but as we add more logic in it. It's worth adding tests for the additional logic. Especially now we have an ongoing integration with the new runtime interface (CRI), it'd be best to add tests for new changes to ensure that any regression would be caught in the new implementation.

The applyDefaultImageTag function actually has a test. In the same file, there are other image pulling tests as well. You can just add higher-level image pulling test there.

dims · 2016-08-10T18:37:31Z

@yujuhong LOL, of course :)

yujuhong · 2016-08-10T21:19:59Z

pkg/kubelet/dockertools/kube_docker_client.go

@@ -184,6 +184,10 @@ func (d *kubeDockerClient) InspectImage(image string) (*dockertypes.ImageInspect
 		}
 		return nil, err
 	}
+	if !matchImageTagOrSHA(resp, image) {
+		err = imageNotFoundError{ID: image}


nit: just return nil, imageNotFoundError{ID: image}

yujuhong · 2016-08-10T22:12:00Z

pkg/kubelet/dockertools/docker.go

+// Check if the inspected image matches what we are looking for
+func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool {
+	parts := strings.SplitN(image, ":", 2)
+	if len(parts) == 1 {


Can we reuse the same dockerref code for parsing the given string?

kubernetes/pkg/kubelet/dockertools/docker.go

Line 161 in 9f1c3a4

_, isDigested := named.(dockerref.Digested)

yujuhong · 2016-08-11T01:54:42Z

!!! 'gofmt -s -w' needs to be run on the following files: 
./pkg/kubelet/dockertools/docker_test.go
FAILED   hack/make-rules/../../hack/verify-gofmt.sh 11s

Docker API does not validate the tag/sha, for example, all the following calls work say for a alpine image with short SHA "4e38e38c8ce0" echo -e "GET /images/alpine:4e38e38c8ce0/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock echo -e "GET /images/alpine:4e38e38c/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock echo -e "GET /images/alpine:4/json HTTP/1.0\r\n" | nc -U /var/run/docker.sock So we should check the response from the Docker API and look for the tags or SHA explicitly. Fixes kubernetes#30355

k8s-bot · 2016-08-11T13:28:07Z

GCE e2e build/test passed for commit 875fd16.

k8s-github-robot · 2016-08-13T06:03:27Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-13T06:42:15Z

GCE e2e build/test passed for commit 875fd16.

k8s-github-robot · 2016-08-13T06:42:40Z

Automatic merge from submit-queue

smarterclayton · 2016-09-14T01:00:16Z

It looks correct on the Red Hat 1.10 I have but I don't think it was supported in Docker 1.9. Did we make a formal statement that we dropped 1.9?

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

I'm much less confident in RepoDigests being correct on every Docker variant version - probably couldn't say without trying a fair number of them.

yujuhong · 2016-09-14T01:01:18Z

It looks correct on the Red Hat 1.10 I have but I don't think it was supported in Docker 1.9. Did we make a formal statement that we dropped 1.9?

I just checked 1.9 on my desktop and it reported correct RepoDigest nonetheless:

    "Id": "c38cc6bc48807ea450f17762c784114de8bf74ec1f33eab62b68d931f82ad167",
    "RepoTags": [],
    "RepoDigests": [
        "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"
    ],

smarterclayton · 2016-09-14T01:07:24Z

Tomorrow I'll have someone look at the various 1.10 versions and make sure they're correct against schema 1.10. If i can prove that RepoDigest is correct I'll change the patch to prefer RepoDigests for a prefix match and then fall back to ID. The prefix match seemed slightly questionable, but I assume we validated there was no security risk in that?

yujuhong · 2016-09-14T01:11:40Z

@smarterclayton thanks! Should we hold on to the PR #32627 for now until this is verified?

yujuhong · 2016-09-14T01:32:57Z

but I assume we validated there was no security risk in that?

The motivation of adding this check is because docker's image inspection syntax is too loose and may mix up tags and digests. This is merely a sanity check to make sure docker has the right image. What security risk do you have in mind?

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-14T02:01:32Z

The prefix matching wasn't bounded, so you could get partial matches on tags by specifying part of a name that didn't end on a boundary:

pull "foo/bar:latest" would match "server/foo/bar:latest" as well as "superfoo/bar:latest"

yujuhong · 2016-09-14T02:08:39Z

Yes, I had that in the comment and also in the comment from the PR #30789 (comment). There will be false positives. The alternative is to hard code the docker default registry (docker.io).

smarterclayton · 2016-09-14T02:20:07Z

That's not enough because the default registry can be remapped. Basically, no tag prefix checking is really safe or accurate.

smarterclayton · 2016-09-14T02:20:25Z

Only a SHA and a force verification can help

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.

yujuhong · 2016-09-14T16:33:14Z

That's not enough because the default registry can be remapped. Basically, no tag prefix checking is really safe or accurate.

Yes, it's a best effort check, which is better than nothing IMO.

smarterclayton · 2016-09-14T16:57:37Z

But if it fails, you've broken a user who is doing something the container
engine allows.

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

That's not enough because the default registry can be remapped. Basically,
no tag prefix checking is really safe or accurate.

Yes, it's a best effort check, which is better than nothing IMO.

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

smarterclayton · 2016-09-14T16:58:47Z

The tag check in this case fails closed (safer) but breaks allowed
scenarios. Remapping is really common in OpenShift production deployments

I'd say at least 50% of the enterprises I know of deploying docker at
scale use registry remapping.

On Wed, Sep 14, 2016 at 12:57 PM, Clayton Coleman ccoleman@redhat.com
wrote:

But if it fails, you've broken a user who is doing something the container
engine allows.

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

That's not enough because the default registry can be remapped.
Basically, no tag prefix checking is really safe or accurate.

Yes, it's a best effort check, which is better than nothing IMO.

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

yujuhong · 2016-09-14T17:01:39Z

@smarterclayton I meant the current check (which checks only the suffix matches) allow false positive matches, so that it's not as strict, but would still catch some cases.

I'd say at least 50% of the enterprises I know of deploying docker at
scale use registry remapping.

That's why I didn't hard code the registry name to enforce a more strict check.

ncdc · 2016-09-14T17:04:49Z

Given that a prefix match against the inspected image's ID will match the config.digest of a schema version 2 image manifest, which is not a valid pull-by-id spec, the only way a prefix match can work is if you know:

Node N has an image with ID A already on it (and remember, that's not the pull spec)
Your pod specifies a container to run image ID A
Your pod is scheduled to node N

yujuhong · 2016-09-14T17:06:46Z

@ncdc I was talking about the "tag" check, not the digest check :-)

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.

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 #30366 cannot be pulled by Kube from a Docker 1.10 system. This partially reverts commit 875fd16.

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)

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.

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 Aug 10, 2016

dims force-pushed the fix-issue-30355 branch from c5b2314 to fba98e1 Compare August 10, 2016 14:28

k8s-github-robot assigned yujuhong Aug 10, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 10, 2016

yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 10, 2016

yujuhong reviewed Aug 10, 2016
View reviewed changes

dims force-pushed the fix-issue-30355 branch from fba98e1 to 2e5518a Compare August 10, 2016 20:50

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 10, 2016

yujuhong reviewed Aug 10, 2016
View reviewed changes

dims force-pushed the fix-issue-30355 branch from 2e5518a to 2c7a512 Compare August 10, 2016 21:40

yujuhong reviewed Aug 10, 2016
View reviewed changes

dims force-pushed the fix-issue-30355 branch from 2c7a512 to 875fd16 Compare August 11, 2016 12:53

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

k8s-github-robot merged commit 8a35d4c into kubernetes:master Aug 13, 2016

dims deleted the fix-issue-30355 branch November 16, 2017 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate SHA/Tag when checking docker images #30366

Validate SHA/Tag when checking docker images #30366

dims commented Aug 10, 2016 •

edited by k8s-oncall

Loading

yujuhong Aug 10, 2016

yujuhong commented Aug 10, 2016

dims commented Aug 10, 2016

yujuhong commented Aug 10, 2016 •

edited

Loading

dims commented Aug 10, 2016

yujuhong Aug 10, 2016

dims Aug 10, 2016

yujuhong Aug 10, 2016

yujuhong commented Aug 11, 2016

k8s-bot commented Aug 11, 2016

k8s-github-robot commented Aug 13, 2016

k8s-bot commented Aug 13, 2016

k8s-github-robot commented Aug 13, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016 •

edited

Loading

ncdc commented Sep 14, 2016

yujuhong commented Sep 14, 2016

Validate SHA/Tag when checking docker images #30366

Validate SHA/Tag when checking docker images #30366

Conversation

dims commented Aug 10, 2016 • edited by k8s-oncall Loading

yujuhong Aug 10, 2016

Choose a reason for hiding this comment

yujuhong commented Aug 10, 2016

dims commented Aug 10, 2016

yujuhong commented Aug 10, 2016 • edited Loading

dims commented Aug 10, 2016

yujuhong Aug 10, 2016

Choose a reason for hiding this comment

dims Aug 10, 2016

Choose a reason for hiding this comment

yujuhong Aug 10, 2016

Choose a reason for hiding this comment

yujuhong commented Aug 11, 2016

k8s-bot commented Aug 11, 2016

k8s-github-robot commented Aug 13, 2016

k8s-bot commented Aug 13, 2016

k8s-github-robot commented Aug 13, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

smarterclayton commented Sep 14, 2016

yujuhong commented Sep 14, 2016 • edited Loading

ncdc commented Sep 14, 2016

yujuhong commented Sep 14, 2016

dims commented Aug 10, 2016 •

edited by k8s-oncall

Loading

yujuhong commented Aug 10, 2016 •

edited

Loading

yujuhong commented Sep 14, 2016 •

edited

Loading