From 2991bfcef1a94ea9a448836a44d7edfe1f4fc61d Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Fri, 30 Sep 2016 14:30:48 -0400 Subject: [PATCH] Add method to inspect Docker images by ID Previously, the `InspectImage` method of the Docker interface expected a "pullable" image ref (name, tag, or manifest digest). If you tried to inspect an image by its ID (config digest), the inspect would fail to validate the image against the input identifier. This commit changes the original method to be named `InspectImageByRef`, and introduces a new method called `InspectImageByID` which validates that the input identifier was an image ID. --- pkg/kubelet/dockertools/docker.go | 48 +++++++++- pkg/kubelet/dockertools/docker_manager.go | 4 +- pkg/kubelet/dockertools/docker_test.go | 89 ++++++++++++++++++- pkg/kubelet/dockertools/fake_docker_client.go | 14 ++- .../dockertools/instrumented_docker.go | 13 ++- pkg/kubelet/dockertools/kube_docker_client.go | 34 +++++-- 6 files changed, 185 insertions(+), 17 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index ebe50bc10fd..425f433ce65 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -66,7 +66,8 @@ type DockerInterface interface { StartContainer(id string) error StopContainer(id string, timeout int) error RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error - InspectImage(image string) (*dockertypes.ImageInspect, error) + InspectImageByRef(imageRef string) (*dockertypes.ImageInspect, error) + InspectImageByID(imageID string) (*dockertypes.ImageInspect, error) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error) PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error) @@ -136,7 +137,11 @@ func filterHTTPError(err error, image string) error { } } -// Check if the inspected image matches what we are looking for +// matchImageTagOrSHA checks if the given image specifier is a valid image ref, +// and that it matches the given image. It should fail on things like image IDs +// (config digests) and other digest-only references, but succeed on image names +// (`foo`), tag references (`foo:bar`), and manifest digest references +// (`foo@sha256:xyz`). func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool { // The image string follows the grammar specified here // https://github.com/docker/distribution/blob/master/reference/reference.go#L4 @@ -193,6 +198,43 @@ func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool { return false } +// matchImageIDOnly checks that the given image specifier is a digest-only +// reference, and that it matches the given image. +func matchImageIDOnly(inspected dockertypes.ImageInspect, image string) bool { + // If the image ref is literally equal to the inspected image's ID, + // just return true here (this might be the case for Docker 1.9, + // where we won't have a digest for the ID) + if inspected.ID == image { + return true + } + + // Otherwise, we should try actual parsing to be more correct + ref, err := dockerref.Parse(image) + if err != nil { + glog.V(4).Infof("couldn't parse image reference %q: %v", image, err) + return false + } + + digest, isDigested := ref.(dockerref.Digested) + if !isDigested { + glog.V(4).Infof("the image reference %q was not a digest reference") + return false + } + + id, err := dockerdigest.ParseDigest(inspected.ID) + if err != nil { + glog.V(4).Infof("couldn't parse image ID reference %q: %v", id, err) + return false + } + + if digest.Digest().Algorithm().String() == id.Algorithm().String() && digest.Digest().Hex() == id.Hex() { + return true + } + + glog.V(4).Infof("The reference %s does not directly refer to the given image's ID (%q)", image, inspected.ID) + return false +} + func (p dockerPuller) Pull(image string, secrets []api.Secret) error { keyring, err := credentialprovider.MakeDockerKeyring(secrets, p.keyring) if err != nil { @@ -246,7 +288,7 @@ func (p dockerPuller) Pull(image string, secrets []api.Secret) error { } func (p dockerPuller) IsImagePresent(image string) (bool, error) { - _, err := p.client.InspectImage(image) + _, err := p.client.InspectImageByRef(image) if err == nil { return true, nil } diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go index c8346a382bf..2ed8705e505 100644 --- a/pkg/kubelet/dockertools/docker_manager.go +++ b/pkg/kubelet/dockertools/docker_manager.go @@ -913,7 +913,7 @@ func (dm *DockerManager) IsImagePresent(image kubecontainer.ImageSpec) (bool, er // Removes the specified image. func (dm *DockerManager) RemoveImage(image kubecontainer.ImageSpec) error { // If the image has multiple tags, we need to remove all the tags - if inspectImage, err := dm.client.InspectImage(image.Image); err == nil && len(inspectImage.RepoTags) > 1 { + if inspectImage, err := dm.client.InspectImageByID(image.Image); err == nil && len(inspectImage.RepoTags) > 1 { for _, tag := range inspectImage.RepoTags { if _, err := dm.client.RemoveImage(tag, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil { return err @@ -2414,7 +2414,7 @@ func (dm *DockerManager) verifyNonRoot(container *api.Container) error { // or the user is set to root. If there is an error inspecting the image this method will return // false and return the error. func (dm *DockerManager) isImageRoot(image string) (bool, error) { - img, err := dm.client.InspectImage(image) + img, err := dm.client.InspectImageByRef(image) if err != nil { return false, err } diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 745c5cf6dce..4516cef7164 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -315,6 +315,85 @@ func TestMatchImageTagOrSHA(t *testing.T) { } } +func TestMatchImageIDOnly(t *testing.T) { + for i, testCase := range []struct { + Inspected dockertypes.ImageInspect + Image string + Output bool + }{ + // shouldn't match names or tagged names + { + Inspected: dockertypes.ImageInspect{RepoTags: []string{"ubuntu:latest"}}, + Image: "ubuntu", + Output: false, + }, + { + Inspected: dockertypes.ImageInspect{RepoTags: []string{"colemickens/hyperkube-amd64:217.9beff63"}}, + Image: "colemickens/hyperkube-amd64:217.9beff63", + Output: false, + }, + // should match name@digest refs if they refer to the image ID (but only the full ID) + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + }, + Image: "myimage@sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + Output: true, + }, + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + }, + Image: "myimage@sha256:2208f7a29005", + Output: false, + }, + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + }, + Image: "myimage@sha256:2208", + Output: false, + }, + // should match when the IDs are literally the same + { + Inspected: dockertypes.ImageInspect{ + ID: "foobar", + }, + Image: "foobar", + Output: true, + }, + // shouldn't match mismatched IDs + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + }, + Image: "myimage@sha256:0000f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d", + Output: false, + }, + // shouldn't match invalid IDs or refs + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:unparseable", + }, + Image: "myimage@sha256:unparseable", + Output: false, + }, + // shouldn't match against repo digests + { + Inspected: dockertypes.ImageInspect{ + ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227", + RepoDigests: []string{"centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"}, + }, + Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf", + Output: false, + }, + } { + match := matchImageIDOnly(testCase.Inspected, testCase.Image) + assert.Equal(t, testCase.Output, match, fmt.Sprintf("%s is not a match (%d)", testCase.Image, i)) + } + +} + func TestPullWithNoSecrets(t *testing.T) { tests := []struct { imageName string @@ -614,8 +693,14 @@ type imageTrackingDockerClient struct { imageName string } -func (f *imageTrackingDockerClient) InspectImage(name string) (image *dockertypes.ImageInspect, err error) { - image, err = f.FakeDockerClient.InspectImage(name) +func (f *imageTrackingDockerClient) InspectImageByID(name string) (image *dockertypes.ImageInspect, err error) { + image, err = f.FakeDockerClient.InspectImageByID(name) + f.imageName = name + return +} + +func (f *imageTrackingDockerClient) InspectImageByRef(name string) (image *dockertypes.ImageInspect, err error) { + image, err = f.FakeDockerClient.InspectImageByRef(name) f.imageName = name return } diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index 85bea450b0c..5a6c64164d1 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -310,9 +310,19 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS return nil, fmt.Errorf("container %q not found", id) } -// InspectImage is a test-spy implementation of DockerInterface.InspectImage. +// InspectImageByRef is a test-spy implementation of DockerInterface.InspectImageByRef. // It adds an entry "inspect" to the internal method call record. -func (f *FakeDockerClient) InspectImage(name string) (*dockertypes.ImageInspect, error) { +func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageInspect, error) { + f.Lock() + defer f.Unlock() + f.called = append(f.called, calledDetail{name: "inspect_image"}) + err := f.popError("inspect_image") + return f.Image, err +} + +// InspectImageByID is a test-spy implementation of DockerInterface.InspectImageByID. +// It adds an entry "inspect" to the internal method call record. +func (f *FakeDockerClient) InspectImageByID(name string) (*dockertypes.ImageInspect, error) { f.Lock() defer f.Unlock() f.called = append(f.called, calledDetail{name: "inspect_image"}) diff --git a/pkg/kubelet/dockertools/instrumented_docker.go b/pkg/kubelet/dockertools/instrumented_docker.go index eb15226498a..66de3aecfea 100644 --- a/pkg/kubelet/dockertools/instrumented_docker.go +++ b/pkg/kubelet/dockertools/instrumented_docker.go @@ -107,11 +107,20 @@ func (in instrumentedDockerInterface) RemoveContainer(id string, opts dockertype return err } -func (in instrumentedDockerInterface) InspectImage(image string) (*dockertypes.ImageInspect, error) { +func (in instrumentedDockerInterface) InspectImageByRef(image string) (*dockertypes.ImageInspect, error) { const operation = "inspect_image" defer recordOperation(operation, time.Now()) - out, err := in.client.InspectImage(image) + out, err := in.client.InspectImageByRef(image) + recordError(operation, err) + return out, err +} + +func (in instrumentedDockerInterface) InspectImageByID(image string) (*dockertypes.ImageInspect, error) { + const operation = "inspect_image" + defer recordOperation(operation, time.Now()) + + out, err := in.client.InspectImageByID(image) recordError(operation, err) return out, err } diff --git a/pkg/kubelet/dockertools/kube_docker_client.go b/pkg/kubelet/dockertools/kube_docker_client.go index e6355748c93..32297fa28c2 100644 --- a/pkg/kubelet/dockertools/kube_docker_client.go +++ b/pkg/kubelet/dockertools/kube_docker_client.go @@ -182,25 +182,47 @@ func (d *kubeDockerClient) RemoveContainer(id string, opts dockertypes.Container return err } -func (d *kubeDockerClient) InspectImage(image string) (*dockertypes.ImageInspect, error) { +func (d *kubeDockerClient) inspectImageRaw(ref string) (*dockertypes.ImageInspect, error) { ctx, cancel := d.getTimeoutContext() defer cancel() - resp, _, err := d.client.ImageInspectWithRaw(ctx, image, true) + resp, _, err := d.client.ImageInspectWithRaw(ctx, ref, true) if ctxErr := contextError(ctx); ctxErr != nil { return nil, ctxErr } if err != nil { if dockerapi.IsErrImageNotFound(err) { - err = imageNotFoundError{ID: image} + err = imageNotFoundError{ID: ref} } return nil, err } - if !matchImageTagOrSHA(resp, image) { - return nil, imageNotFoundError{ID: image} - } + return &resp, nil } +func (d *kubeDockerClient) InspectImageByID(imageID string) (*dockertypes.ImageInspect, error) { + resp, err := d.inspectImageRaw(imageID) + if err != nil { + return nil, err + } + + if !matchImageIDOnly(*resp, imageID) { + return nil, imageNotFoundError{ID: imageID} + } + return resp, nil +} + +func (d *kubeDockerClient) InspectImageByRef(imageRef string) (*dockertypes.ImageInspect, error) { + resp, err := d.inspectImageRaw(imageRef) + if err != nil { + return nil, err + } + + if !matchImageTagOrSHA(*resp, imageRef) { + return nil, imageNotFoundError{ID: imageRef} + } + return resp, nil +} + func (d *kubeDockerClient) ImageHistory(id string) ([]dockertypes.ImageHistory, error) { ctx, cancel := d.getTimeoutContext() defer cancel()