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.

Sign up for GitHub

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

Jump to bottom

Docker digest validation is too strict #32627

Merged
merged 1 commit into from
Sep 14, 2016
Merged

Docker digest validation is too strict #32627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 22 additions & 6 deletions pkg/kubelet/dockertools/docker.go
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ import (
"strings"
"time"

dockerdigest "github.com/docker/distribution/digest"
dockerref "github.com/docker/distribution/reference"
"github.com/docker/docker/pkg/jsonmessage"
dockerapi "github.com/docker/engine-api/client"
Expand Down Expand Up @@ -152,7 +153,6 @@ func filterHTTPError(err error, image string) error {

// Check if the inspected image matches what we are looking for
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
named, err := dockerref.ParseNamed(image)
Expand Down Expand Up @@ -180,11 +180,27 @@ func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool {
}

if isDigested {
algo := digest.Digest().Algorithm().String()
sha := digest.Digest().Hex()
// Look specifically for short and long sha(s)
if strings.Contains(inspected.ID, algo+":"+sha) {
// We found the short or long SHA requested
for _, repoDigest := range inspected.RepoDigests {
named, err := dockerref.ParseNamed(repoDigest)
if err != nil {
glog.V(4).Infof("couldn't parse image RepoDigest reference %q: %v", repoDigest, err)
continue
}
if d, isDigested := named.(dockerref.Digested); isDigested {
if digest.Digest().Algorithm().String() == d.Digest().Algorithm().String() &&
digest.Digest().Hex() == d.Digest().Hex() {
return true
}
}
}

// process the ID as a digest
Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up to my FYI above, I doubt this code will ever execute in real life

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree but it's good to keep it here just in case, since we are so close to release

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
}
}
Expand Down
104 changes: 102 additions & 2 deletions pkg/kubelet/dockertools/docker_test.go
View file
Open in desktop
Expand Up @@ -158,7 +158,7 @@ func TestContainerNaming(t *testing.T) {
}

func TestMatchImageTagOrSHA(t *testing.T) {
for _, testCase := range []struct {
for i, testCase := range []struct {
Inspected dockertypes.ImageInspect
Image string
Output bool
Expand Down Expand Up @@ -209,9 +209,109 @@ func TestMatchImageTagOrSHA(t *testing.T) {
Image: "myimage@sha256:2208",
Output: false,
},
{
// mismatched ID is ignored
Inspected: dockertypes.ImageInspect{
ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
},
Image: "myimage@sha256:0000f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
Output: false,
},
{
// invalid digest is ignored
Inspected: dockertypes.ImageInspect{
ID: "sha256:unparseable",
},
Image: "myimage@sha256:unparseable",
Output: false,
},
{
// v1 schema images can be pulled in one format and returned in another
Copy link
Member

Choose a reason for hiding this comment

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

Make a copy of this test case, change the value in RepoDigests so it's not a match, and the test will pass with the "double digest" line, and fail once you change one digest to d.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That test did fail.

On Wed, Sep 14, 2016 at 1:32 PM, Andy Goldstein notifications@github.com
wrote:

In pkg/kubelet/dockertools/docker_test.go
#32627 (comment)
:

  •       Inspected: dockertypes.ImageInspect{

  •           ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",

  •       },

  •       Image:  "myimage@sha256:0000f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",

  •       Output: false,

  •   },

  •   {

  •       // invalid digest is ignored

  •       Inspected: dockertypes.ImageInspect{

  •           ID: "sha256:unparseable",

  •       },

  •       Image:  "myimage@sha256:unparseable",

  •       Output: false,

  •   },

  •   {

  •       // v1 schema images can be pulled in one format and returned in another

Make a copy of this test case, change the value in RepoDigests so it's not
a match, and the test will pass with the "double digest" line, and fail
once you change one digest to d.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/32627/files/95cee6e53b91be6c7b5e37939c6374a37aa7e505#r78795950,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pxOA5fM_3Y3-oXQFj54-C86qFeMBks5qqC-ygaJpZM4J8SYk
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually did not. Fixing

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

That test did fail.

On Wed, Sep 14, 2016 at 1:32 PM, Andy Goldstein notifications@github.com
wrote:

In pkg/kubelet/dockertools/docker_test.go
#32627 (comment)
:

  •      Inspected: dockertypes.ImageInspect{

  •          ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",

  •      },

  •      Image:  "myimage@sha256:0000f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",

  •      Output: false,

  •  },

  •  {

  •      // invalid digest is ignored

  •      Inspected: dockertypes.ImageInspect{

  •          ID: "sha256:unparseable",

  •      },

  •      Image:  "myimage@sha256:unparseable",

  •      Output: false,

  •  },

  •  {

  •      // v1 schema images can be pulled in one format and returned in another

Make a copy of this test case, change the value in RepoDigests so it's
not a match, and the test will pass with the "double digest" line, and fail
once you change one digest to d.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/32627/files/95cee6e53b91be6c7b5e37939c6374a37aa7e505#r78795950,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pxOA5fM_3Y3-oXQFj54-C86qFeMBks5qqC-ygaJpZM4J8SYk
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a hard fail

Inspected: dockertypes.ImageInspect{
ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{"centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: true,
},
{
// RepoDigest match is is required
Inspected: dockertypes.ImageInspect{
ID: "",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:000084acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: false,
},
{
// RepoDigest match is allowed
Inspected: dockertypes.ImageInspect{
ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: true,
},
{
// RepoDigest and ID are checked
Inspected: dockertypes.ImageInspect{
ID: "sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227"},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: true,
},
{
// unparseable RepoDigests are skipped
Inspected: dockertypes.ImageInspect{
ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{
"centos/ruby-23-centos7@sha256:unparseable",
"docker.io/centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: true,
},
{
// unparseable RepoDigest is ignored
Inspected: dockertypes.ImageInspect{
ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:unparseable"},
},
Image: "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
Output: false,
},
{
// unparseable image digest is ignored
Inspected: dockertypes.ImageInspect{
ID: "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:unparseable"},
},
Image: "centos/ruby-23-centos7@sha256:unparseable",
Output: false,
},
{
// prefix match is rejected for ID and RepoDigest
Inspected: dockertypes.ImageInspect{
ID: "sha256:unparseable",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:unparseable"},
},
Image: "sha256:unparseable",
Output: false,
},
{
// possible SHA prefix match is rejected for ID and RepoDigest because it is not in the named format
Inspected: dockertypes.ImageInspect{
ID: "sha256:0000f247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
RepoDigests: []string{"docker.io/centos/ruby-23-centos7@sha256:0000f247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227"},
},
Image: "sha256:0000",
Output: false,
},
} {
match := matchImageTagOrSHA(testCase.Inspected, testCase.Image)
assert.Equal(t, testCase.Output, match, testCase.Image+" is not a match")
assert.Equal(t, testCase.Output, match, testCase.Image+fmt.Sprintf(" is not a match (%d)", i))
}
}

Expand Down