Skip to content

Commit

Permalink
fix(digests): do not mandate sha256 as the only algorithm used for ha…
Browse files Browse the repository at this point in the history 
…shing blobs

Signed-off-by: Andrei Aaron <[email protected]>
  • Loading branch information
@andaaron
andaaron committed Jul 17, 2024
1 parent 714d8c4 commit 5e49ab1
Show file tree
Hide file tree
Showing 15 changed files with 528 additions and 127 deletions.
205 changes: 205 additions & 0 deletions pkg/api/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11167,6 +11167,211 @@ func RunAuthorizationTests(t *testing.T, client *resty.Client, baseURL, user str
})
}

func TestSupportedDigestAlgorithms(t *testing.T) {
port := test.GetFreePort()
baseURL := test.GetBaseURL(port)

conf := config.New()
conf.HTTP.Port = port

dir := t.TempDir()

ctlr := api.NewController(conf)
ctlr.Config.Storage.RootDirectory = dir
ctlr.Config.Storage.Dedupe = false
ctlr.Config.Storage.GC = false

cm := test.NewControllerManager(ctlr)

cm.StartAndWait(port)
defer cm.StopServer()

Convey("Test SHA512 single-arch image", t, func() {
image := CreateImageWithDigestAlgorithm(godigest.SHA512).
RandomLayers(1, 10).DefaultConfig().Build()

name := "algo-sha512"
tag := "singlearch"

err := UploadImage(image, baseURL, name, tag)
So(err, ShouldBeNil)

client := resty.New()

// The server picks canonical digests when tags are pushed
// See https://github.com/opencontainers/distribution-spec/issues/494
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
// but there is no way to specify a client preference
// so all we can do is verify the correct algorithm is returned

expectedDigestStr := image.DigestForAlgorithm(godigest.Canonical).String()

verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
})

Convey("Test SHA512 single-arch image pushed by digest", t, func() {
image := CreateImageWithDigestAlgorithm(godigest.SHA512).
RandomLayers(1, 11).DefaultConfig().Build()

name := "algo-sha512-2"

err := UploadImage(image, baseURL, name, image.DigestStr())
So(err, ShouldBeNil)

client := resty.New()

expectedDigestStr := image.DigestForAlgorithm(godigest.SHA512).String()

verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
})

Convey("Test SHA384 single-arch image", t, func() {
image := CreateImageWithDigestAlgorithm(godigest.SHA384).
RandomLayers(1, 10).DefaultConfig().Build()

name := "algo-sha384"
tag := "singlearch"

err := UploadImage(image, baseURL, name, tag)
So(err, ShouldBeNil)

client := resty.New()

// The server picks canonical digests when tags are pushed
// See https://github.com/opencontainers/distribution-spec/issues/494
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
// but there is no way to specify a client preference
// so all we can do is verify the correct algorithm is returned

expectedDigestStr := image.DigestForAlgorithm(godigest.Canonical).String()

verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
})

Convey("Test SHA512 multi-arch image", t, func() {
subImage1 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
DefaultConfig().Build()
subImage2 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
DefaultConfig().Build()
multiarch := CreateMultiarchWithDigestAlgorithm(godigest.SHA512).
Images([]Image{subImage1, subImage2}).Build()

name := "algo-sha512"
tag := "multiarch"

err := UploadMultiarchImage(multiarch, baseURL, name, tag)
So(err, ShouldBeNil)

client := resty.New()

// The server picks canonical digests when tags are pushed
// See https://github.com/opencontainers/distribution-spec/issues/494
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
// but there is no way to specify a client preference
// so all we can do is verify the correct algorithm is returned
expectedDigestStr := multiarch.DigestForAlgorithm(godigest.Canonical).String()

verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)

// While the expected multiarch manifest digest is always using the canonical algorithm
// the sub-imgage manifest digest can use any algorith
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage1.ManifestDescriptor.Digest.String(), subImage1.ManifestDescriptor.Digest.String())
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage2.ManifestDescriptor.Digest.String(), subImage2.ManifestDescriptor.Digest.String())
})

Convey("Test SHA512 multi-arch image pushed by digest", t, func() {
subImage1 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
DefaultConfig().Build()
subImage2 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
DefaultConfig().Build()
multiarch := CreateMultiarchWithDigestAlgorithm(godigest.SHA512).
Images([]Image{subImage1, subImage2}).Build()

name := "algo-sha512-2"

t.Log(multiarch.DigestStr())

err := UploadMultiarchImage(multiarch, baseURL, name, multiarch.DigestStr())
So(err, ShouldBeNil)

client := resty.New()

expectedDigestStr := multiarch.DigestForAlgorithm(godigest.SHA512).String()
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)

// While the expected multiarch manifest digest is always using the canonical algorithm
// the sub-imgage manifest digest can use any algorith
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage1.ManifestDescriptor.Digest.String(), subImage1.ManifestDescriptor.Digest.String())
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage2.ManifestDescriptor.Digest.String(), subImage2.ManifestDescriptor.Digest.String())
})

Convey("Test SHA384 multi-arch image", t, func() {
subImage1 := CreateImageWithDigestAlgorithm(godigest.SHA384).RandomLayers(1, 10).
DefaultConfig().Build()
subImage2 := CreateImageWithDigestAlgorithm(godigest.SHA384).RandomLayers(1, 10).
DefaultConfig().Build()
multiarch := CreateMultiarchWithDigestAlgorithm(godigest.SHA384).
Images([]Image{subImage1, subImage2}).Build()

name := "algo-sha384"
tag := "multiarch"

err := UploadMultiarchImage(multiarch, baseURL, name, tag)
So(err, ShouldBeNil)

client := resty.New()

// The server picks canonical digests when tags are pushed
// See https://github.com/opencontainers/distribution-spec/issues/494
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
// but there is no way to specify a client preference
// so all we can do is verify the correct algorithm is returned
expectedDigestStr := multiarch.DigestForAlgorithm(godigest.Canonical).String()

verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)

// While the expected multiarch manifest digest is always using the canonical algorithm
// the sub-imgage manifest digest can use any algorith
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage1.ManifestDescriptor.Digest.String(), subImage1.ManifestDescriptor.Digest.String())
verifyReturnedManifestDigest(t, client, baseURL, name,
subImage2.ManifestDescriptor.Digest.String(), subImage2.ManifestDescriptor.Digest.String())
})
}

func verifyReturnedManifestDigest(t *testing.T, client *resty.Client, baseURL, repoName,
reference, expectedDigestStr string,
) {
t.Helper()

t.Logf("Verify Docker-Content-Digest returned for repo %s reference %s is %s",
repoName, reference, expectedDigestStr)

getResponse, err := client.R().Get(fmt.Sprintf("%s/v2/%s/manifests/%s", baseURL, repoName, reference))
So(err, ShouldBeNil)
So(getResponse, ShouldNotBeNil)
So(getResponse.StatusCode(), ShouldEqual, http.StatusOK)

contentDigestStr := getResponse.Header().Get("Docker-Content-Digest")
So(contentDigestStr, ShouldEqual, expectedDigestStr)

getResponse, err = client.R().Head(fmt.Sprintf("%s/v2/%s/manifests/%s", baseURL, repoName, reference))
So(err, ShouldBeNil)
So(getResponse, ShouldNotBeNil)
So(getResponse.StatusCode(), ShouldEqual, http.StatusOK)

contentDigestStr = getResponse.Header().Get("Docker-Content-Digest")
So(contentDigestStr, ShouldEqual, expectedDigestStr)
}

func getEmptyImageConfig() ([]byte, godigest.Digest) {
config := ispec.Image{}

Expand Down
51 changes: 30 additions & 21 deletions pkg/storage/common/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,19 @@ func GetManifestDescByReference(index ispec.Index, reference string) (ispec.Desc

func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaType string, body []byte,
log zlog.Logger,
) (godigest.Digest, error) {
) error {
// validate the manifest
if !IsSupportedMediaType(mediaType) {
log.Debug().Interface("actual", mediaType).
Msg("bad manifest media type")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest
}

if len(body) == 0 {
log.Debug().Int("len", len(body)).Msg("invalid body length")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest
}

switch mediaType {
Expand All @@ -86,13 +86,13 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
if err := ValidateManifestSchema(body); err != nil {
log.Error().Err(err).Msg("failed to validate OCIv1 image manifest schema")

return "", zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
return zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
}

if err := json.Unmarshal(body, &manifest); err != nil {
log.Error().Err(err).Msg("failed to unmarshal JSON")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest

Check warning on line 95 in pkg/storage/common/common.go

View check run for this annotation

Codecov / codecov/patch

pkg/storage/common/common.go#L95 

Added line #L95 was not covered by tests
}

// validate blobs only for known media types
Expand All @@ -104,7 +104,7 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
log.Error().Err(err).Str("digest", manifest.Config.Digest.String()).
Msg("failed to stat blob due to missing config blob")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest
}

// validate layers - a lightweight check if the blob is present
Expand All @@ -121,7 +121,7 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
log.Error().Err(err).Str("digest", layer.Digest.String()).
Msg("failed to validate manifest due to missing layer blob")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest
}
}
}
Expand All @@ -130,43 +130,52 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
if err := ValidateImageIndexSchema(body); err != nil {
log.Error().Err(err).Msg("failed to validate OCIv1 image index manifest schema")

return "", zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
return zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
}

var indexManifest ispec.Index
if err := json.Unmarshal(body, &indexManifest); err != nil {
log.Error().Err(err).Msg("failed to unmarshal JSON")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest

Check warning on line 140 in pkg/storage/common/common.go

View check run for this annotation

Codecov / codecov/patch

pkg/storage/common/common.go#L140 

Added line #L140 was not covered by tests
}

for _, manifest := range indexManifest.Manifests {
if ok, _, _, err := imgStore.StatBlob(repo, manifest.Digest); !ok || err != nil {
log.Error().Err(err).Str("digest", manifest.Digest.String()).
Msg("failed to stat manifest due to missing manifest blob")

return "", zerr.ErrBadManifest
return zerr.ErrBadManifest
}
}
}

return "", nil
return nil
}

func GetAndValidateRequestDigest(body []byte, digestStr string, log zlog.Logger) (godigest.Digest, error) {
bodyDigest := godigest.FromBytes(body)
// Returns the canonical digest or the digest provided by the reference if any
// Per spec, the canonical digest would always be returned to the client in
// request headers, but that does not make sense if the client requested a different digest algorithm
// See https://github.com/opencontainers/distribution-spec/issues/494
func GetAndValidateRequestDigest(body []byte, reference string, log zlog.Logger) (
godigest.Digest, error,
) {
expectedDigest, err := godigest.Parse(reference)
if err != nil {
// This is a non-digest reference
return godigest.Canonical.FromBytes(body), err
}

actualDigest := expectedDigest.Algorithm().FromBytes(body)

d, err := godigest.Parse(digestStr)
if err == nil {
if d.String() != bodyDigest.String() {
log.Error().Str("actual", bodyDigest.String()).Str("expected", d.String()).
Msg("failed to validate manifest digest")
if expectedDigest.String() != actualDigest.String() {
log.Error().Str("actual", actualDigest.String()).Str("expected", expectedDigest.String()).
Msg("failed to validate manifest digest")

return "", zerr.ErrBadManifest
}
return actualDigest, zerr.ErrBadManifest
}

return bodyDigest, err
return actualDigest, nil
}

/*
Expand Down
23 changes: 23 additions & 0 deletions pkg/storage/common/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,29 @@ func TestValidateManifest(t *testing.T) {
So(err, ShouldBeNil)
So(clen, ShouldEqual, len(cblob))

Convey("bad manifest mediatype", func() {
manifest := ispec.Manifest{}

body, err := json.Marshal(manifest)
So(err, ShouldBeNil)

_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageConfig, body)
So(err, ShouldNotBeNil)
So(err, ShouldEqual, zerr.ErrBadManifest)
})

Convey("empty manifest with bad media type", func() {
_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageConfig, []byte(""))
So(err, ShouldNotBeNil)
So(err, ShouldEqual, zerr.ErrBadManifest)
})

Convey("empty manifest with correct media type", func() {
_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageManifest, []byte(""))
So(err, ShouldNotBeNil)
So(err, ShouldEqual, zerr.ErrBadManifest)
})

Convey("bad manifest schema version", func() {
manifest := ispec.Manifest{
Config: ispec.Descriptor{
Expand Down
11 changes: 5 additions & 6 deletions pkg/storage/gc/gc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,20 +581,19 @@ func (gc GarbageCollect) removeUnreferencedBlobs(repo string, delay time.Duratio

gcBlobs := make([]godigest.Digest, 0)

for _, blob := range allBlobs {
digest := godigest.NewDigestFromEncoded(godigest.SHA256, blob)
for _, digest := range allBlobs {
if err = digest.Validate(); err != nil {
log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", blob).
Msg("failed to parse digest")
log.Error().Err(err).Str("module", "gc").Str("repository", repo).
Str("digest", digest.String()).Msg("failed to parse digest")

Check warning on line 587 in pkg/storage/gc/gc.go

View check run for this annotation

Codecov / codecov/patch

pkg/storage/gc/gc.go#L586-L587 

Added lines #L586 - L587 were not covered by tests

return err
}

if _, ok := refBlobs[digest.String()]; !ok {
canGC, err := isBlobOlderThan(gc.imgStore, repo, digest, delay, log)
if err != nil {
log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", blob).
Msg("failed to determine GC delay")
log.Error().Err(err).Str("module", "gc").Str("repository", repo).
Str("digest", digest.String()).Msg("failed to determine GC delay")

Check warning on line 596 in pkg/storage/gc/gc.go

View check run for this annotation

Codecov / codecov/patch

pkg/storage/gc/gc.go#L595-L596 

Added lines #L595 - L596 were not covered by tests

return err
}
Expand Down
Loading

0 comments on commit 5e49ab1

Please sign in to comment.