Merge pull request from GHSA-hqxw-f8mx-cpmw

Fix runaway allocation on /v2/_catalog

configuration/configuration.go

@@ -198,6 +198,7 @@ type Configuration struct {
 	} `yaml:"redis,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")
 					}

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

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{
-							{
+							invalidPaginationResponseDescriptor,
-								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,
-								},
-							},
 							unauthorizedResponseDescriptor,
 							repositoryNotFoundResponseDescriptor,
 							deniedResponseDescriptor,
@@ -1589,6 +1591,9 @@ var routeDescriptors = []RouteDescriptor{
 								},
 							},
 						},
+						Failures: []ResponseDescriptor{
+							invalidPaginationResponseDescriptor,
+						},
 					},
 				},
 			},

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,
 	})
 )

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{
+	maxEntries := env.config.Catalog.MaxEntries
-		"last": []string{""},
+	allCatalog := []string{
-		"n":    []string{strconv.Itoa(chunkLen)},
+		"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")
 	}
 
-	// -----------------------------------
+	for _, image := range allCatalog {
-	// push something to the registry and try again
-	images := []string{"foo/aaaa", "foo/bbbb", "foo/cccc"}
-
-	for _, image := range images {
 		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,30 @@ func TestCatalogAPI(t *testing.T) {
 		t.Fatalf("error decoding fetched manifest: %v", err)
 	}
 
-	if len(ctlg.Repositories) != chunkLen {
+	// it must match max entries
-		t.Fatalf("repositories has unexpected values")
+	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")
 	}
-
-	newValues := checkLink(t, link, chunkLen, ctlg.Repositories[len(ctlg.Repositories)-1])
-
 	// -----------------------------------
-	// get the last chunk of data
+	// Case No. 2.1: Second page (n internally will be min(100, maxEntries))
 
-	catalogURL, err = env.builder.BuildCatalogURL(newValues)
+	// 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)
 	}
@@ -181,18 +185,267 @@ func TestCatalogAPI(t *testing.T) {
 		t.Fatalf("error decoding fetched manifest: %v", err)
 	}
 
-	if len(ctlg.Repositories) != 1 {
+	expectedRemainder := len(allCatalog) - maxEntries
-		t.Fatalf("repositories has unexpected values")
+
+	if len(ctlg.Repositories) != expectedRemainder {
+		t.Fatalf("repositories returned unexpected entries (expected: %d, returned: %d)", expectedRemainder, len(ctlg.Repositories))
 	}
 
-	lastImage := images[len(images)-1]
+	// -----------------------------------
-	if !contains(ctlg.Repositories, lastImage) {
+	// Case No. 3: request n = maxentries
-		t.Fatalf("didn't find our repository '%s' in the catalog", lastImage)
+	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)
+	}
+
+	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 != "" {
+	if link == "" {
-		t.Fatalf("catalog has unexpected data")
+		t.Fatalf("repositories has less data than expected")
+	}
+
+	// -----------------------------------
+	// 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(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. 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)
+	}
+
+	// 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("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 //nolint:staticcheck // Ignore SA1019: "github.com/distribution/distribution/v3/manifest/schema1" is deprecated, as it's used for backward compatibility.
 
@@ -2314,6 +2570,9 @@ func newTestEnv(t *testing.T, deleteEnabled bool) *testEnv {
 				"enabled": false,
 			}},
 		},
+		Catalog: configuration.Catalog{
+			MaxEntries: 5,
+		},
 	}
 
 	config.Compatibility.Schema1.Enabled = true //nolint:staticcheck // Ignore SA1019: "github.com/distribution/distribution/v3/manifest/schema1" is deprecated, as it's used for backward compatibility.
@@ -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()
 	}
 

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 {
+	entries := defaultReturnedEntries
-		maxEntries = maximumReturnedEntries
+	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
 		}
 
-	repos := make([]string, maxEntries)
+		// 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
+	}
 
-	filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry)
+	// then enforce entries to be between 0 & maximumConfiguredEntries
-	_, pathNotFound := err.(driver.PathNotFoundError)
+	// max(0, min(entries, maximumConfiguredEntries))
+	if entries < 0 || entries > maximumConfiguredEntries {
+		entries = maximumConfiguredEntries
+	}
 
-	if err == io.EOF || pathNotFound {
+	repos := make([]string, entries)
+	filled := 0
+
+	// entries is guaranteed to be >= 0 and < maximumConfiguredEntries
+	if entries == 0 {
 		moreEntries = false
-	} else if err != nil {
+	} 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]
+		lastEntry = repos[filled-1]
-		urlStr, err := createLinkEntry(r.URL.String(), maxEntries, lastEntry)
+		urlStr, err := createLinkEntry(r.URL.String(), entries, lastEntry)
 		if err != nil {
 			ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err))
 			return