From 4c1561e9fb43af39df7dcf32f9878aa614e1967b Mon Sep 17 00:00:00 2001 From: "Jose D. Gomez R" Date: Fri, 31 Mar 2023 13:16:29 +0200 Subject: [PATCH] Fix runaway allocation on /v2/_catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduced a Catalog entry in the configuration struct. With it, it's possible to control the maximum amount of entries returned by /v2/catalog (`GetCatalog` in registry/handlers/catalog.go). It's set to a default value of 1000. `GetCatalog` returns 100 entries by default if no `n` is provided. When provided it will be validated to be between `0` and `MaxEntries` defined in Configuration. When `n` is outside the aforementioned boundary, an error response is returned. `GetCatalog` now handles `n=0` gracefully with an empty response as well. Signed-off-by: José D. Gómez R. <1josegomezr@gmail.com> --- configuration/configuration.go | 18 +- configuration/configuration_test.go | 4 + registry/api/v2/descriptors.go | 29 +-- registry/api/v2/errors.go | 3 +- registry/handlers/api_test.go | 314 +++++++++++++++++++++++++--- registry/handlers/catalog.go | 56 +++-- 6 files changed, 369 insertions(+), 55 deletions(-) diff --git a/configuration/configuration.go b/configuration/configuration.go index 1f773976..f8a501f7 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -197,7 +197,8 @@ type Configuration struct { } `yaml:"pool,omitempty"` } `yaml:"redis,omitempty"` - Health Health `yaml:"health,omitempty"` + Health Health `yaml:"health,omitempty"` + Catalog Catalog `yaml:"catalog,omitempty"` Proxy Proxy `yaml:"proxy,omitempty"` @@ -260,6 +261,16 @@ type Configuration struct { } `yaml:"policy,omitempty"` } +// Catalog is composed of MaxEntries. +// Catalog endpoint (/v2/_catalog) configuration, it provides the configuration +// options to control the maximum number of entries returned by the catalog endpoint. +type Catalog struct { + // Max number of entries returned by the catalog endpoint. Requesting n entries + // to the catalog endpoint will return at most MaxEntries entries. + // An empty or a negative value will set a default of 1000 maximum entries by default. + MaxEntries int `yaml:"maxentries,omitempty"` +} + // LogHook is composed of hook Level and Type. // After hooks configuration, it can execute the next handling automatically, // when defined levels of log message emitted. @@ -686,6 +697,11 @@ func Parse(rd io.Reader) (*Configuration, error) { if v0_1.Loglevel != Loglevel("") { v0_1.Loglevel = Loglevel("") } + + if v0_1.Catalog.MaxEntries <= 0 { + v0_1.Catalog.MaxEntries = 1000 + } + if v0_1.Storage.Type() == "" { return nil, errors.New("no storage configuration provided") } diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go index 38319f3d..88210d7f 100644 --- a/configuration/configuration_test.go +++ b/configuration/configuration_test.go @@ -70,6 +70,9 @@ var configStruct = Configuration{ }, }, }, + Catalog: Catalog{ + MaxEntries: 1000, + }, HTTP: struct { Addr string `yaml:"addr,omitempty"` Net string `yaml:"net,omitempty"` @@ -521,6 +524,7 @@ func copyConfig(config Configuration) *Configuration { configCopy.Version = MajorMinorVersion(config.Version.Major(), config.Version.Minor()) configCopy.Loglevel = config.Loglevel configCopy.Log = config.Log + configCopy.Catalog = config.Catalog configCopy.Log.Fields = make(map[string]interface{}, len(config.Log.Fields)) for k, v := range config.Log.Fields { configCopy.Log.Fields[k] = v diff --git a/registry/api/v2/descriptors.go b/registry/api/v2/descriptors.go index 25d91924..8622d0d2 100644 --- a/registry/api/v2/descriptors.go +++ b/registry/api/v2/descriptors.go @@ -134,6 +134,19 @@ var ( }, } + invalidPaginationResponseDescriptor = ResponseDescriptor{ + Name: "Invalid pagination number", + Description: "The received parameter n was invalid in some way, as described by the error code. The client should resolve the issue and retry the request.", + StatusCode: http.StatusBadRequest, + Body: BodyDescriptor{ + ContentType: "application/json", + Format: errorsBody, + }, + ErrorCodes: []errcode.ErrorCode{ + ErrorCodePaginationNumberInvalid, + }, + } + repositoryNotFoundResponseDescriptor = ResponseDescriptor{ Name: "No Such Repository Error", StatusCode: http.StatusNotFound, @@ -489,18 +502,7 @@ var routeDescriptors = []RouteDescriptor{ }, }, Failures: []ResponseDescriptor{ - { - Name: "Invalid pagination number", - Description: "The received parameter n was invalid in some way, as described by the error code. The client should resolve the issue and retry the request.", - StatusCode: http.StatusBadRequest, - Body: BodyDescriptor{ - ContentType: "application/json", - Format: errorsBody, - }, - ErrorCodes: []errcode.ErrorCode{ - ErrorCodePaginationNumberInvalid, - }, - }, + invalidPaginationResponseDescriptor, unauthorizedResponseDescriptor, repositoryNotFoundResponseDescriptor, deniedResponseDescriptor, @@ -1589,6 +1591,9 @@ var routeDescriptors = []RouteDescriptor{ }, }, }, + Failures: []ResponseDescriptor{ + invalidPaginationResponseDescriptor, + }, }, }, }, diff --git a/registry/api/v2/errors.go b/registry/api/v2/errors.go index 23a87f8c..dcbabfca 100644 --- a/registry/api/v2/errors.go +++ b/registry/api/v2/errors.go @@ -151,7 +151,8 @@ var ( Value: "PAGINATION_NUMBER_INVALID", Message: "invalid number of results requested", Description: `Returned when the "n" parameter (number of results - to return) is not an integer, or "n" is negative.`, + to return) is not an integer, "n" is negative or "n" is bigger than + the maximum allowed.`, HTTPStatusCode: http.StatusBadRequest, }) ) diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index dcbc1de6..0dc096ae 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -80,22 +80,23 @@ func TestCheckAPI(t *testing.T) { // TestCatalogAPI tests the /v2/_catalog endpoint func TestCatalogAPI(t *testing.T) { - chunkLen := 2 env := newTestEnv(t, false) defer env.Shutdown() - values := url.Values{ - "last": []string{""}, - "n": []string{strconv.Itoa(chunkLen)}, + maxEntries := env.config.Catalog.MaxEntries + allCatalog := []string{ + "foo/aaaa", "foo/bbbb", "foo/cccc", "foo/dddd", "foo/eeee", "foo/ffff", } - catalogURL, err := env.builder.BuildCatalogURL(values) + chunkLen := maxEntries - 1 + + catalogURL, err := env.builder.BuildCatalogURL() if err != nil { t.Fatalf("unexpected error building catalog url: %v", err) } // ----------------------------------- - // try to get an empty catalog + // Case No. 1: Empty catalog resp, err := http.Get(catalogURL) if err != nil { t.Fatalf("unexpected error issuing request: %v", err) @@ -113,23 +114,23 @@ func TestCatalogAPI(t *testing.T) { t.Fatalf("error decoding fetched manifest: %v", err) } - // we haven't pushed anything to the registry yet + // No images pushed = no image returned if len(ctlg.Repositories) != 0 { - t.Fatalf("repositories has unexpected values") + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", 0, len(ctlg.Repositories)) } + // No pagination should be returned if resp.Header.Get("Link") != "" { t.Fatalf("repositories has more data when none expected") } - // ----------------------------------- - // push something to the registry and try again - images := []string{"foo/aaaa", "foo/bbbb", "foo/cccc"} - - for _, image := range images { + for _, image := range allCatalog { createRepository(env, t, image, "sometag") } + // ----------------------------------- + // Case No. 2: Catalog populated & n is not provided nil (n internally will be min(100, maxEntries)) + resp, err = http.Get(catalogURL) if err != nil { t.Fatalf("unexpected error issuing request: %v", err) @@ -143,27 +144,95 @@ func TestCatalogAPI(t *testing.T) { t.Fatalf("error decoding fetched manifest: %v", err) } - if len(ctlg.Repositories) != chunkLen { - t.Fatalf("repositories has unexpected values") + // it must match max entries + if len(ctlg.Repositories) != maxEntries { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", maxEntries, len(ctlg.Repositories)) } - for _, image := range images[:chunkLen] { + // it must return the first maxEntries entries from the catalog + for _, image := range allCatalog[:maxEntries] { if !contains(ctlg.Repositories, image) { t.Fatalf("didn't find our repository '%s' in the catalog", image) } } + // fail if there's no pagination link := resp.Header.Get("Link") if link == "" { t.Fatalf("repositories has less data than expected") } + // ----------------------------------- + // Case No. 2.1: Second page (n internally will be min(100, maxEntries)) + + // build pagination link + values := checkLink(t, link, maxEntries, ctlg.Repositories[len(ctlg.Repositories)-1]) + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) + } + + expectedRemainder := len(allCatalog) - maxEntries + + if len(ctlg.Repositories) != expectedRemainder { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", expectedRemainder, len(ctlg.Repositories)) + } + + // ----------------------------------- + // Case No. 3: request n = maxentries + values = url.Values{ + "last": []string{""}, + "n": []string{strconv.Itoa(maxEntries)}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) + } - newValues := checkLink(t, link, chunkLen, ctlg.Repositories[len(ctlg.Repositories)-1]) + if len(ctlg.Repositories) != maxEntries { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", maxEntries, len(ctlg.Repositories)) + } + + // fail if there's no pagination + link = resp.Header.Get("Link") + if link == "" { + t.Fatalf("repositories has less data than expected") + } // ----------------------------------- - // get the last chunk of data + // Case No. 3.1: Second (last) page + + // build pagination link + values = checkLink(t, link, maxEntries, ctlg.Repositories[len(ctlg.Repositories)-1]) - catalogURL, err = env.builder.BuildCatalogURL(newValues) + catalogURL, err = env.builder.BuildCatalogURL(values) if err != nil { t.Fatalf("unexpected error building catalog url: %v", err) } @@ -181,18 +250,202 @@ func TestCatalogAPI(t *testing.T) { t.Fatalf("error decoding fetched manifest: %v", err) } - if len(ctlg.Repositories) != 1 { - t.Fatalf("repositories has unexpected values") + expectedRemainder = len(allCatalog) - maxEntries + + if len(ctlg.Repositories) != expectedRemainder { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", expectedRemainder, len(ctlg.Repositories)) + } + + // ----------------------------------- + // Case No. 4: request n < maxentries + + values = url.Values{ + "n": []string{strconv.Itoa(chunkLen)}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) } - lastImage := images[len(images)-1] - if !contains(ctlg.Repositories, lastImage) { - t.Fatalf("didn't find our repository '%s' in the catalog", lastImage) + // returns the requested amount + if len(ctlg.Repositories) != chunkLen { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", expectedRemainder, len(ctlg.Repositories)) } + // fail if there's no pagination link = resp.Header.Get("Link") - if link != "" { - t.Fatalf("catalog has unexpected data") + if link == "" { + t.Fatalf("repositories has less data than expected") + } + + // ----------------------------------- + // Case No. 4.1: request n < maxentries (second page) + + // build pagination link + values = checkLink(t, link, chunkLen, ctlg.Repositories[len(ctlg.Repositories)-1]) + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) + } + + expectedRemainder = len(allCatalog) - chunkLen + + if len(ctlg.Repositories) != expectedRemainder { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", expectedRemainder, len(ctlg.Repositories)) + } + + // ----------------------------------- + // Case No. 5: request n > maxentries + + values = url.Values{ + "n": []string{strconv.Itoa(maxEntries + 10)}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusBadRequest) + checkBodyHasErrorCodes(t, "invalid number of results requested", resp, v2.ErrorCodePaginationNumberInvalid) + + // ----------------------------------- + // Case No. 6: request n > maxentries but <= total catalog + + values = url.Values{ + "n": []string{strconv.Itoa(len(allCatalog))}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusBadRequest) + checkBodyHasErrorCodes(t, "invalid number of results requested", resp, v2.ErrorCodePaginationNumberInvalid) + + // ----------------------------------- + // Case No. 7: n = 0 + values = url.Values{ + "n": []string{"0"}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) + } + + // it must match max entries + if len(ctlg.Repositories) != 0 { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", 0, len(ctlg.Repositories)) + } + + // ----------------------------------- + // Case No. 8: n = -1 + values = url.Values{ + "n": []string{"-1"}, + } + + catalogURL, err = env.builder.BuildCatalogURL(values) + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusBadRequest) + checkBodyHasErrorCodes(t, "invalid number of results requested", resp, v2.ErrorCodePaginationNumberInvalid) + + // ----------------------------------- + // Case No. 9: n = 5, max = 5, total catalog = 4 + values = url.Values{ + "n": []string{strconv.Itoa(maxEntries)}, + } + + envWithLessImages := newTestEnv(t, false) + for _, image := range allCatalog[0:(maxEntries - 1)] { + createRepository(envWithLessImages, t, image, "sometag") + } + + catalogURL, err = envWithLessImages.builder.BuildCatalogURL(values) + + if err != nil { + t.Fatalf("unexpected error building catalog url: %v", err) + } + + resp, err = http.Get(catalogURL) + if err != nil { + t.Fatalf("unexpected error issuing request: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, "issuing catalog api check", resp, http.StatusOK) + + dec = json.NewDecoder(resp.Body) + if err = dec.Decode(&ctlg); err != nil { + t.Fatalf("error decoding fetched manifest: %v", err) + } + + // it must match max entries + if len(ctlg.Repositories) != maxEntries-1 { + t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", maxEntries-1, len(ctlg.Repositories)) } } @@ -363,7 +616,7 @@ func checkLink(t *testing.T, urlStr string, numEntries int, last string) url.Val urlValues := linkURL.Query() if urlValues.Get("n") != strconv.Itoa(numEntries) { - t.Fatalf("Catalog link entry size is incorrect") + t.Fatalf("Catalog link entry size is incorrect (expected: %v, returned: %v)", urlValues.Get("n"), strconv.Itoa(numEntries)) } if urlValues.Get("last") != last { @@ -2299,6 +2552,9 @@ func newTestEnvMirror(t *testing.T, deleteEnabled bool) *testEnv { Proxy: configuration.Proxy{ RemoteURL: upstreamEnv.server.URL, }, + Catalog: configuration.Catalog{ + MaxEntries: 5, + }, } config.Compatibility.Schema1.Enabled = true @@ -2314,6 +2570,9 @@ func newTestEnv(t *testing.T, deleteEnabled bool) *testEnv { "enabled": false, }}, }, + Catalog: configuration.Catalog{ + MaxEntries: 5, + }, } config.Compatibility.Schema1.Enabled = true @@ -2594,7 +2853,6 @@ func checkResponse(t *testing.T, msg string, resp *http.Response, expectedStatus if resp.StatusCode != expectedStatus { t.Logf("unexpected status %s: %v != %v", msg, resp.StatusCode, expectedStatus) maybeDumpResponse(t, resp) - t.FailNow() } diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index b8ed3f45..0e633878 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -9,11 +9,12 @@ import ( "strconv" "github.com/distribution/distribution/v3/registry/api/errcode" + v2 "github.com/distribution/distribution/v3/registry/api/v2" "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/gorilla/handlers" ) -const maximumReturnedEntries = 100 +const defaultReturnedEntries = 100 func catalogDispatcher(ctx *Context, r *http.Request) http.Handler { catalogHandler := &catalogHandler{ @@ -38,29 +39,58 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() lastEntry := q.Get("last") - maxEntries, err := strconv.Atoi(q.Get("n")) - if err != nil || maxEntries < 0 { - maxEntries = maximumReturnedEntries + + entries := defaultReturnedEntries + maximumConfiguredEntries := ch.App.Config.Catalog.MaxEntries + + // parse n, if n is negative abort with an error + if n := q.Get("n"); n != "" { + parsedMax, err := strconv.Atoi(n) + if err != nil || parsedMax < 0 { + ch.Errors = append(ch.Errors, v2.ErrorCodePaginationNumberInvalid.WithDetail(map[string]string{"n": n})) + return + } + + // if a client requests more than it's allowed to receive + if parsedMax > maximumConfiguredEntries { + ch.Errors = append(ch.Errors, v2.ErrorCodePaginationNumberInvalid.WithDetail(map[string]int{"n": parsedMax})) + return + } + entries = parsedMax } - repos := make([]string, maxEntries) + // then enforce entries to be between 0 & maximumConfiguredEntries + // max(0, min(entries, maximumConfiguredEntries)) + if entries < 0 || entries > maximumConfiguredEntries { + entries = maximumConfiguredEntries + } - filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) - _, pathNotFound := err.(driver.PathNotFoundError) + repos := make([]string, entries) + filled := 0 - if err == io.EOF || pathNotFound { + // entries is guaranteed to be >= 0 and < maximumConfiguredEntries + if entries == 0 { moreEntries = false - } else if err != nil { - ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) - return + } else { + returnedRepositories, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) + if err != nil { + _, pathNotFound := err.(driver.PathNotFoundError) + if err != io.EOF && !pathNotFound { + ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + return + } + // err is either io.EOF or not PathNotFoundError + moreEntries = false + } + filled = returnedRepositories } w.Header().Set("Content-Type", "application/json") // Add a link header if there are more entries to retrieve if moreEntries { - lastEntry = repos[len(repos)-1] - urlStr, err := createLinkEntry(r.URL.String(), maxEntries, lastEntry) + lastEntry = repos[filled-1] + urlStr, err := createLinkEntry(r.URL.String(), entries, lastEntry) if err != nil { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) return -- 2.40.1