Skip to content

Commit 7e0f24e

Browse files
andaaronandaaron
committed
fix(digests): do not mandate sha256 as the only algorithm used for hashing blobs
Signed-off-by: Andrei Aaron <[email protected]>
1 parent 2e733b3 commit 7e0f24e

File tree

16 files changed

+486
-130
lines changed

16 files changed

+486
-130
lines changed

‎pkg/api/controller_test.go‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10533,6 +10533,167 @@ func RunAuthorizationTests(t *testing.T, client *resty.Client, baseURL, user str
1053310533
})
1053410534
}
1053510535

10536+
func TestSupportedDigestAlgorithms(t *testing.T) {
10537+
port := test.GetFreePort()
10538+
baseURL := test.GetBaseURL(port)
10539+
10540+
conf := config.New()
10541+
conf.HTTP.Port = port
10542+
10543+
dir := t.TempDir()
10544+
10545+
ctlr := api.NewController(conf)
10546+
ctlr.Config.Storage.RootDirectory = dir
10547+
ctlr.Config.Storage.Dedupe = false
10548+
ctlr.Config.Storage.GC = false
10549+
10550+
cm := test.NewControllerManager(ctlr)
10551+
10552+
cm.StartAndWait(port)
10553+
defer cm.StopServer()
10554+
10555+
Convey("Test SHA512 single-arch image", t, func() {
10556+
image := CreateImageWithDigestAlgorithm(godigest.SHA512).
10557+
RandomLayers(1, 10).DefaultConfig().Build()
10558+
10559+
name := "algo-sha256"
10560+
tag := "singlearch"
10561+
10562+
err := UploadImage(image, baseURL, name, tag)
10563+
So(err, ShouldBeNil)
10564+
10565+
client := resty.New()
10566+
10567+
// The server picks canonical digests when tags are pushed
10568+
// See https://github.com/opencontainers/distribution-spec/issues/494
10569+
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
10570+
// but there is no way to specify a client preference
10571+
// so all we can do is verify the correct algorithm is returned
10572+
10573+
expectedDigestStr := image.DigestForAlgorithm(godigest.Canonical).String()
10574+
10575+
verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
10576+
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
10577+
})
10578+
10579+
Convey("Test SHA384 single-arch image", t, func() {
10580+
image := CreateImageWithDigestAlgorithm(godigest.SHA384).
10581+
RandomLayers(1, 10).DefaultConfig().Build()
10582+
10583+
name := "algo-sha384"
10584+
tag := "singlearch"
10585+
10586+
err := UploadImage(image, baseURL, name, tag)
10587+
So(err, ShouldBeNil)
10588+
10589+
client := resty.New()
10590+
10591+
// The server picks canonical digests when tags are pushed
10592+
// See https://github.com/opencontainers/distribution-spec/issues/494
10593+
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
10594+
// but there is no way to specify a client preference
10595+
// so all we can do is verify the correct algorithm is returned
10596+
10597+
expectedDigestStr := image.DigestForAlgorithm(godigest.Canonical).String()
10598+
10599+
verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
10600+
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
10601+
})
10602+
10603+
Convey("Test SHA512 multi-arch image", t, func() {
10604+
subImage1 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
10605+
DefaultConfig().Build()
10606+
subImage2 := CreateImageWithDigestAlgorithm(godigest.SHA512).RandomLayers(1, 10).
10607+
DefaultConfig().Build()
10608+
multiarch := CreateMultiarchWithDigestAlgorithm(godigest.SHA512).
10609+
Images([]Image{subImage1, subImage2}).Build()
10610+
10611+
name := "algo-sha256"
10612+
tag := "multiarch"
10613+
10614+
err := UploadMultiarchImage(multiarch, baseURL, name, tag)
10615+
So(err, ShouldBeNil)
10616+
10617+
client := resty.New()
10618+
10619+
// The server picks canonical digests when tags are pushed
10620+
// See https://github.com/opencontainers/distribution-spec/issues/494
10621+
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
10622+
// but there is no way to specify a client preference
10623+
// so all we can do is verify the correct algorithm is returned
10624+
expectedDigestStr := multiarch.DigestForAlgorithm(godigest.Canonical).String()
10625+
10626+
verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
10627+
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
10628+
10629+
// While the expected multiarch manifest digest is always using the canonical algorithm
10630+
// the sub-imgage manifest digest can use any algorith
10631+
verifyReturnedManifestDigest(t, client, baseURL, name,
10632+
subImage1.ManifestDescriptor.Digest.String(), subImage1.ManifestDescriptor.Digest.String())
10633+
verifyReturnedManifestDigest(t, client, baseURL, name,
10634+
subImage2.ManifestDescriptor.Digest.String(), subImage2.ManifestDescriptor.Digest.String())
10635+
})
10636+
10637+
Convey("Test SHA384 multi-arch image", t, func() {
10638+
subImage1 := CreateImageWithDigestAlgorithm(godigest.SHA384).RandomLayers(1, 10).
10639+
DefaultConfig().Build()
10640+
subImage2 := CreateImageWithDigestAlgorithm(godigest.SHA384).RandomLayers(1, 10).
10641+
DefaultConfig().Build()
10642+
multiarch := CreateMultiarchWithDigestAlgorithm(godigest.SHA384).
10643+
Images([]Image{subImage1, subImage2}).Build()
10644+
10645+
name := "algo-sha384"
10646+
tag := "multiarch"
10647+
10648+
err := UploadMultiarchImage(multiarch, baseURL, name, tag)
10649+
So(err, ShouldBeNil)
10650+
10651+
client := resty.New()
10652+
10653+
// The server picks canonical digests when tags are pushed
10654+
// See https://github.com/opencontainers/distribution-spec/issues/494
10655+
// It would be nice to be able to push tags with other digest algorithms and verify those are returned
10656+
// but there is no way to specify a client preference
10657+
// so all we can do is verify the correct algorithm is returned
10658+
expectedDigestStr := multiarch.DigestForAlgorithm(godigest.Canonical).String()
10659+
10660+
verifyReturnedManifestDigest(t, client, baseURL, name, tag, expectedDigestStr)
10661+
verifyReturnedManifestDigest(t, client, baseURL, name, expectedDigestStr, expectedDigestStr)
10662+
10663+
// While the expected multiarch manifest digest is always using the canonical algorithm
10664+
// the sub-imgage manifest digest can use any algorith
10665+
verifyReturnedManifestDigest(t, client, baseURL, name,
10666+
subImage1.ManifestDescriptor.Digest.String(), subImage1.ManifestDescriptor.Digest.String())
10667+
verifyReturnedManifestDigest(t, client, baseURL, name,
10668+
subImage2.ManifestDescriptor.Digest.String(), subImage2.ManifestDescriptor.Digest.String())
10669+
})
10670+
}
10671+
10672+
func verifyReturnedManifestDigest(t *testing.T, client *resty.Client, baseURL, repoName,
10673+
reference, expectedDigestStr string,
10674+
) {
10675+
t.Helper()
10676+
10677+
t.Logf("Verify Docker-Content-Digest returned for repo %s reference %s is %s",
10678+
repoName, reference, expectedDigestStr)
10679+
10680+
getResponse, err := client.R().Get(fmt.Sprintf("%s/v2/%s/manifests/%s", baseURL, repoName, reference))
10681+
So(err, ShouldBeNil)
10682+
So(getResponse, ShouldNotBeNil)
10683+
So(getResponse.StatusCode(), ShouldEqual, http.StatusOK)
10684+
10685+
contentDigestStr := getResponse.Header().Get("Docker-Content-Digest")
10686+
So(contentDigestStr, ShouldEqual, expectedDigestStr)
10687+
10688+
getResponse, err = client.R().Head(fmt.Sprintf("%s/v2/%s/manifests/%s", baseURL, repoName, reference))
10689+
So(err, ShouldBeNil)
10690+
So(getResponse, ShouldNotBeNil)
10691+
So(getResponse.StatusCode(), ShouldEqual, http.StatusOK)
10692+
10693+
contentDigestStr = getResponse.Header().Get("Docker-Content-Digest")
10694+
So(contentDigestStr, ShouldEqual, expectedDigestStr)
10695+
}
10696+
1053610697
func getEmptyImageConfig() ([]byte, godigest.Digest) {
1053710698
config := ispec.Image{}
1053810699

‎pkg/storage/common/common.go‎

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -64,19 +64,19 @@ func GetManifestDescByReference(index ispec.Index, reference string) (ispec.Desc
6464

6565
func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaType string, body []byte,
6666
log zlog.Logger,
67-
) (godigest.Digest, error) {
67+
) error {
6868
// validate the manifest
6969
if !IsSupportedMediaType(mediaType) {
7070
log.Debug().Interface("actual", mediaType).
7171
Msg("bad manifest media type")
7272

73-
return "", zerr.ErrBadManifest
73+
return zerr.ErrBadManifest
7474
}
7575

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

79-
return "", zerr.ErrBadManifest
79+
return zerr.ErrBadManifest
8080
}
8181

8282
switch mediaType {
@@ -87,13 +87,13 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
8787
if err := ValidateManifestSchema(body); err != nil {
8888
log.Error().Err(err).Msg("OCIv1 image manifest schema validation failed")
8989

90-
return "", zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
90+
return zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
9191
}
9292

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

96-
return "", zerr.ErrBadManifest
96+
return zerr.ErrBadManifest
9797
}
9898

9999
// validate blobs only for known media types
@@ -104,7 +104,7 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
104104
if !ok || err != nil {
105105
log.Error().Err(err).Str("digest", manifest.Config.Digest.String()).Msg("missing config blob")
106106

107-
return "", zerr.ErrBadManifest
107+
return zerr.ErrBadManifest
108108
}
109109

110110
// validate layers - a lightweight check if the blob is present
@@ -120,7 +120,7 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
120120
if !ok || err != nil {
121121
log.Error().Err(err).Str("digest", layer.Digest.String()).Msg("missing layer blob")
122122

123-
return "", zerr.ErrBadManifest
123+
return zerr.ErrBadManifest
124124
}
125125
}
126126
}
@@ -129,49 +129,58 @@ func ValidateManifest(imgStore storageTypes.ImageStore, repo, reference, mediaTy
129129
if err := json.Unmarshal(body, &m); err != nil {
130130
log.Error().Err(err).Msg("unable to unmarshal JSON")
131131

132-
return "", zerr.ErrBadManifest
132+
return zerr.ErrBadManifest
133133
}
134134
case ispec.MediaTypeImageIndex:
135135
// validate manifest
136136
if err := ValidateImageIndexSchema(body); err != nil {
137137
log.Error().Err(err).Msg("OCIv1 image index manifest schema validation failed")
138138

139-
return "", zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
139+
return zerr.NewError(zerr.ErrBadManifest).AddDetail("jsonSchemaValidation", err.Error())
140140
}
141141

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

146-
return "", zerr.ErrBadManifest
146+
return zerr.ErrBadManifest
147147
}
148148

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

153-
return "", zerr.ErrBadManifest
153+
return zerr.ErrBadManifest
154154
}
155155
}
156156
}
157157

158-
return "", nil
158+
return nil
159159
}
160160

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

164-
d, err := godigest.Parse(digestStr)
165-
if err == nil {
166-
if d.String() != bodyDigest.String() {
167-
log.Error().Str("actual", bodyDigest.String()).Str("expected", d.String()).
168-
Msg("manifest digest is not valid")
176+
if expectedDigest.String() != actualDigest.String() {
177+
log.Error().Str("actual", actualDigest.String()).Str("expected", expectedDigest.String()).
178+
Msg("manifest digest is not valid")
169179

170-
return "", zerr.ErrBadManifest
171-
}
180+
return actualDigest, zerr.ErrBadManifest
172181
}
173182

174-
return bodyDigest, err
183+
return actualDigest, nil
175184
}
176185

177186
/*

‎pkg/storage/common/common_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,29 @@ func TestValidateManifest(t *testing.T) {
5151
So(err, ShouldBeNil)
5252
So(clen, ShouldEqual, len(cblob))
5353

54+
Convey("bad manifest mediatype", func() {
55+
manifest := ispec.Manifest{}
56+
57+
body, err := json.Marshal(manifest)
58+
So(err, ShouldBeNil)
59+
60+
_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageConfig, body)
61+
So(err, ShouldNotBeNil)
62+
So(err, ShouldEqual, zerr.ErrBadManifest)
63+
})
64+
65+
Convey("empty manifest with bad media type", func() {
66+
_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageConfig, []byte(""))
67+
So(err, ShouldNotBeNil)
68+
So(err, ShouldEqual, zerr.ErrBadManifest)
69+
})
70+
71+
Convey("empty manifest with correct media type", func() {
72+
_, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageManifest, []byte(""))
73+
So(err, ShouldNotBeNil)
74+
So(err, ShouldEqual, zerr.ErrBadManifest)
75+
})
76+
5477
Convey("bad manifest schema version", func() {
5578
manifest := ispec.Manifest{
5679
Config: ispec.Descriptor{

‎pkg/storage/gc/gc.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -579,20 +579,19 @@ func (gc GarbageCollect) removeUnreferencedBlobs(repo string, delay time.Duratio
579579

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

582-
for _, blob := range allBlobs {
583-
digest := godigest.NewDigestFromEncoded(godigest.SHA256, blob)
582+
for _, digest := range allBlobs {
584583
if err = digest.Validate(); err != nil {
585-
log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", blob).
586-
Msg("unable to parse digest")
584+
log.Error().Err(err).Str("module", "gc").Str("repository", repo).
585+
Str("digest", digest.String()).Msg("unable to parse digest")
587586

588587
return err
589588
}
590589

591590
if _, ok := refBlobs[digest.String()]; !ok {
592591
canGC, err := isBlobOlderThan(gc.imgStore, repo, digest, delay, log)
593592
if err != nil {
594-
log.Error().Err(err).Str("module", "gc").Str("repository", repo).Str("digest", blob).
595-
Msg("unable to determine GC delay")
593+
log.Error().Err(err).Str("module", "gc").Str("repository", repo).
594+
Str("digest", digest.String()).Msg("unable to determine GC delay")
596595

597596
return err
598597
}

‎pkg/storage/gc/gc_internal_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -440,8 +440,8 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) {
440440
GetIndexContentFn: func(repo string) ([]byte, error) {
441441
return returnedIndexJSONBuf, nil
442442
},
443-
GetAllBlobsFn: func(repo string) ([]string, error) {
444-
return []string{}, errGC
443+
GetAllBlobsFn: func(repo string) ([]godigest.Digest, error) {
444+
return []godigest.Digest{}, errGC
445445
},
446446
}
447447

0 commit comments

Comments
 (0)