Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,15 @@ linters:
- legacy
- std-error-handling
rules:
- linters:
- godot
path: pkg/api/routes.go
- linters:
- godot
path: pkg/extensions/extension_image_trust.go
- linters:
- godot
path: pkg/extensions/extension_mgmt.go
Comment thread
andaaron marked this conversation as resolved.
- linters:
- lll
- varnamelen
Expand Down
2 changes: 2 additions & 0 deletions errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@

details := make(map[string]string)

if errors.As(err, &internalErr) {

Check failure on line 42 in errors/errors.go

View workflow job for this annotation

GitHub Actions / coverage

condition "errors.As(err, &internalErr)" was never evaluated
details = internalErr.GetDetails()
}

Expand Down Expand Up @@ -117,6 +117,8 @@
ErrEmptyRepoName = errors.New("repo name can't be empty string")
ErrEmptyTag = errors.New("tag can't be empty string")
ErrEmptyDigest = errors.New("digest can't be empty string")
ErrEmptyManifestTagQuery = errors.New("empty tag query parameter")
ErrInvalidManifestTagQuery = errors.New("invalid tag query parameter: not a valid OCI/Docker tag")
ErrInvalidRepoRefFormat = errors.New("invalid image reference format, use [repo:tag] or [repo@digest]")
ErrLimitIsNegative = errors.New("pagination limit has negative value")
ErrLimitIsExcessive = errors.New("pagination limit has excessive value")
Expand Down
23 changes: 17 additions & 6 deletions pkg/api/constants/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,23 @@ package constants
import "time"

const (
RoutePrefix = "/v2"
Blobs = "blobs"
Uploads = "uploads"
DistAPIVersion = "Docker-Distribution-API-Version"
DistContentDigestKey = "Docker-Content-Digest"
SubjectDigestKey = "OCI-Subject"
RoutePrefix = "/v2"
Blobs = "blobs"
Uploads = "uploads"
DistAPIVersion = "Docker-Distribution-API-Version"
DistContentDigestKey = "Docker-Content-Digest"
// OCITagResponseKey is returned on digest manifest pushes that include tag query
// parameters (distribution-spec PR #600).
OCITagResponseKey = "OCI-Tag"
SubjectDigestKey = "OCI-Subject"
// MaxManifestDigestQueryTags is the maximum number of raw `tag=` query parameters accepted on
// PUT .../manifests/<digest>?tag=... (draft OCI distribution-spec: registries MUST support at
// least 10 and MAY respond with 414 beyond this limit). It uses the OCI tag max length (128;
// must match pkg/regexp.TagMaxLen) and an ~8KiB request-target budget, reserving 2048 bytes
// for path and digest:
//
// (8192 - 2048) / (len("tag=") + 128 + 1) == 46
Comment thread
andaaron marked this conversation as resolved.
MaxManifestDigestQueryTags = (8192 - 2048) / (len("tag=") + 128 + 1)
Comment thread
andaaron marked this conversation as resolved.
Comment thread
vrajashkr marked this conversation as resolved.
BlobUploadUUID = "Blob-Upload-UUID"
DefaultMediaType = "application/json"
BinaryMediaType = "application/octet-stream"
Expand Down
18 changes: 18 additions & 0 deletions pkg/api/constants/consts_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package constants_test

import (
"testing"

"zotregistry.dev/zot/v2/pkg/api/constants"
zreg "zotregistry.dev/zot/v2/pkg/regexp"
)

func TestMaxManifestDigestQueryTagsDerived(t *testing.T) {
t.Parallel()

want := (8192 - 2048) / (len("tag=") + zreg.TagMaxLen + 1)

if constants.MaxManifestDigestQueryTags != want {
t.Fatalf("MaxManifestDigestQueryTags = %d, want %d", constants.MaxManifestDigestQueryTags, want)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: since we've used github.com/stretchr/testify/assert, it would be ideal to use that here too for consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we use it in a single file in an older test, I haven't noticed it until now. We mosty use goconvey which has its own assertions. Here we could have used github.com/stretchr/testify/assert since goconvey doesn't work well with t.Parallel() as far as I understand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool. Sounds good. I'm fine with either the standard testing library way of doing it or testify assert. It would be good to standardize on one though to help with consistency across tests.

}
}
137 changes: 137 additions & 0 deletions pkg/api/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import (
extconf "zotregistry.dev/zot/v2/pkg/extensions/config"
"zotregistry.dev/zot/v2/pkg/log"
"zotregistry.dev/zot/v2/pkg/meta"
zreg "zotregistry.dev/zot/v2/pkg/regexp"
"zotregistry.dev/zot/v2/pkg/storage"
storageConstants "zotregistry.dev/zot/v2/pkg/storage/constants"
"zotregistry.dev/zot/v2/pkg/storage/gc"
Expand Down Expand Up @@ -7808,6 +7809,142 @@ func TestManifestValidation(t *testing.T) {
})
}

func TestManifestDigestQueryTags(t *testing.T) {
Convey("Manifest PUT with digest ?tag= query parameters", t, func() {
port := test.GetFreePort()
baseURL := test.GetBaseURL(port)

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

dir := t.TempDir()
ctlr := makeController(conf, dir)
cm := test.NewControllerManager(ctlr)
cm.StartServer()
time.Sleep(1000 * time.Millisecond)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, we had a way to test for server readiness so we could potentially wait only as long as needed. (Not sure if I'm getting this mixed up with the BATS tests).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we really needed this here. Must be a leftover.


defer cm.StopServer()

repoName := "digest-query-tags"
img := CreateRandomImage()
manifestBytes := img.ManifestDescriptor.Data
manifestDigest := img.ManifestDescriptor.Digest

err := UploadImage(img, baseURL, repoName, "initial")
So(err, ShouldBeNil)

putManifestByDigest := func(rawQuery string) *resty.Response {
t.Helper()

manifestPutURL, perr := url.Parse(baseURL + fmt.Sprintf("/v2/%s/manifests/%s", repoName, manifestDigest.String()))
So(perr, ShouldBeNil)
manifestPutURL.RawQuery = rawQuery

resp, rerr := resty.R().
SetHeader("Content-Type", ispec.MediaTypeImageManifest).
SetBody(manifestBytes).
Put(manifestPutURL.String())
So(rerr, ShouldBeNil)

return resp
}

Convey("multiple tag query parameters add tags and return OCI-Tag headers", func() {
manifestPutURL, err := url.Parse(baseURL + fmt.Sprintf("/v2/%s/manifests/%s", repoName, manifestDigest.String()))
So(err, ShouldBeNil)

q := manifestPutURL.Query()
q.Add("tag", "v1.0.0")
q.Add("tag", "v1.0")
q.Add("tag", "edge")
manifestPutURL.RawQuery = q.Encode()

resp, err := resty.R().
SetHeader("Content-Type", ispec.MediaTypeImageManifest).
SetBody(manifestBytes).
Put(manifestPutURL.String())
So(err, ShouldBeNil)
So(resp.StatusCode(), ShouldEqual, http.StatusCreated)
So(resp.Header().Get(constants.DistContentDigestKey), ShouldEqual, manifestDigest.String())

ociTags := resp.Header().Values(constants.OCITagResponseKey)
sort.Strings(ociTags)
So(ociTags, ShouldResemble, []string{"edge", "v1.0", "v1.0.0"})

for _, tag := range []string{"v1.0.0", "v1.0", "edge"} {
gresp, gerr := resty.R().Get(baseURL + fmt.Sprintf("/v2/%s/manifests/%s", repoName, tag))
So(gerr, ShouldBeNil)
So(gresp.StatusCode(), ShouldEqual, http.StatusOK)
}
})

Convey("tag query with non-digest path reference returns 400", func() {
manifestPutURL, err := url.Parse(baseURL + fmt.Sprintf("/v2/%s/manifests/initial", repoName))
So(err, ShouldBeNil)
manifestPutURL.RawQuery = "tag=notallowed"

resp, err := resty.R().
SetHeader("Content-Type", ispec.MediaTypeImageManifest).
SetBody(manifestBytes).
Put(manifestPutURL.String())
So(err, ShouldBeNil)
So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest)
})

Convey("empty tag query parameter returns 400", func() {
resp := putManifestByDigest("tag=")
So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest)
})

Convey("more than max tag query parameters returns 414", func() {
q := url.Values{}
for i := range constants.MaxManifestDigestQueryTags + 1 {
q.Add("tag", fmt.Sprintf("t%d", i))
}

resp := putManifestByDigest(q.Encode())
So(resp.StatusCode(), ShouldEqual, http.StatusRequestURITooLong)
})

Convey("more than max raw tag parameters returns 414 even when values are duplicates", func() {
q := url.Values{}
for range constants.MaxManifestDigestQueryTags + 1 {
q.Add("tag", "same")
}

resp := putManifestByDigest(q.Encode())
So(resp.StatusCode(), ShouldEqual, http.StatusRequestURITooLong)
})

Convey("invalid tag query value returns 400", func() {
q := url.Values{}
q.Set("tag", "bad/ref")

resp := putManifestByDigest(q.Encode())
So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest)
})

Convey("tag query value longer than distribution-spec max length returns 400", func() {
longTag := strings.Repeat("a", zreg.TagMaxLen+1)
q := url.Values{}
q.Set("tag", longTag)

resp := putManifestByDigest(q.Encode())
So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest)
})

Convey("duplicate tag query values are deduplicated in response headers", func() {
q := url.Values{}
q.Add("tag", "dup")
q.Add("tag", "dup")

resp := putManifestByDigest(q.Encode())
So(resp.StatusCode(), ShouldEqual, http.StatusCreated)
So(resp.Header().Values(constants.OCITagResponseKey), ShouldResemble, []string{"dup"})
})
})
}

func TestArtifactReferences(t *testing.T) {
Convey("Validate Artifact References", t, func() {
// start a new server
Expand Down
Loading
Loading