From 54d039bbea4a36b1e0e703bd26815e871bea6cbd Mon Sep 17 00:00:00 2001 From: detvdl Date: Wed, 14 Dec 2022 16:05:27 +0100 Subject: [PATCH 1/5] fix: support relative links in OCI tags query response Pagination for OCI tags retrieval is not supported when the Link header URI is relative. According to https://docs.docker.com/registry/spec/api/#pagination and the therein referenced RFC https://www.rfc-editor.org/rfc/rfc5988#section-5 relative links should be resolved to the initial request URL Signed-off-by: detvdl --- util/helm/client.go | 33 +++++++++++++++++++++++---------- util/helm/client_test.go | 30 ++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/util/helm/client.go b/util/helm/client.go index 7ef14e2cba36c..b0074e6986abf 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -33,6 +33,7 @@ import ( var ( globalLock = sync.NewKeyLock() indexLock = sync.NewKeyLock() + errNoLink = errors.New("no Link header in response") ) type Creds struct { @@ -408,7 +409,7 @@ func (c *nativeHelmChart) getTags(chart string) ([]byte, error) { for nextURL != "" { log.Debugf("fetching %s tags from %s", chart, text.Trunc(nextURL, 100)) data, nextURL, err = c.getTagsFromUrl(nextURL) - if err != nil { + if err != nil && err != errNoLink { return nil, fmt.Errorf("failed tags part: %v", err) } @@ -426,14 +427,26 @@ func (c *nativeHelmChart) getTags(chart string) ([]byte, error) { return data, nil } -func getNextUrl(linkHeader string) string { - nextUrl := "" - if linkHeader != "" { - // drop < >; ref= from the Link header, see: https://docs.docker.com/registry/spec/api/#pagination - nextUrl = strings.Split(linkHeader, ";")[0][1:] - nextUrl = nextUrl[:len(nextUrl)-1] +func getNextUrl(resp *http.Response) (string, error) { + link := resp.Header.Get("Link") + if link == "" { + return "", errNoLink + } + if link[0] != '<' { + return "", fmt.Errorf("invalid next link %q: missing '<'", link) + } + if i := strings.IndexByte(link, '>'); i == -1 { + return "", fmt.Errorf("invalid next link %q: missing '>'", link) + } else { + link = link[1:i] + } + fmt.Println(link) + linkURL, err := resp.Request.URL.Parse(link) + fmt.Println(linkURL) + if err != nil { + return "", err } - return nextUrl + return linkURL.String(), nil } func (c *nativeHelmChart) getTagsFromUrl(tagsURL string) ([]byte, string, error) { @@ -484,8 +497,8 @@ func (c *nativeHelmChart) getTagsFromUrl(tagsURL string) ([]byte, string, error) if err != nil { return nil, "", fmt.Errorf("failed to read body: %v", err) } - nextUrl := getNextUrl(resp.Header.Get("Link")) - return data, nextUrl, nil + nextUrl, err := getNextUrl(resp) + return data, nextUrl, err } func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error) { diff --git a/util/helm/client_test.go b/util/helm/client_test.go index 8f12eaf403c9e..44f52e8f4fdff 100644 --- a/util/helm/client_test.go +++ b/util/helm/client_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" + "net/url" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -177,11 +178,32 @@ func TestGetTagsFromUrl(t *testing.T) { } func Test_getNextUrl(t *testing.T) { - nextUrl := getNextUrl("") + baseUrl, err := url.Parse("https://my.repo.com/v2/chart/tags/list") + if err != nil { + t.Errorf("failed to parse url in test case: %v", err) + } + resp := &http.Response{ + Request: &http.Request{ + URL: baseUrl, + }, + } + nextUrl, err := getNextUrl(resp) assert.Equal(t, nextUrl, "") - - nextUrl = getNextUrl("; rel=next") - assert.Equal(t, nextUrl, "https://my.repo.com/v2/chart/tags/list?token=123") + assert.Equal(t, err, errNoLink) + + var nextUrlAbsolute = "https://my.repo.com/v2/chart/tags/list?n=123&orderby=" + resp.Header = http.Header{ + "Link": []string{fmt.Sprintf(`<%s>; rel="next"`, nextUrlAbsolute)}, + } + nextUrl, err = getNextUrl(resp) + assert.Equal(t, nextUrl, nextUrlAbsolute) + + var nextUrlRelative = "/v2/chart/tags/list?n=123&orderby=" + resp.Header = http.Header{ + "Link": []string{fmt.Sprintf(`<%s>; rel="next"`, nextUrlRelative)}, + } + nextUrl, err = getNextUrl(resp) + assert.Equal(t, nextUrl, "https://my.repo.com/v2/chart/tags/list?n=123&orderby=") } func Test_getTagsListURL(t *testing.T) { From 29d0bd7e64185b131795484c986ff36067b2d198 Mon Sep 17 00:00:00 2001 From: detvdl Date: Wed, 14 Dec 2022 16:45:01 +0100 Subject: [PATCH 2/5] chore: clean up unused prints & assert errors Signed-off-by: detvdl --- util/helm/client.go | 2 -- util/helm/client_test.go | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/util/helm/client.go b/util/helm/client.go index b0074e6986abf..32068bb9708c0 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -440,9 +440,7 @@ func getNextUrl(resp *http.Response) (string, error) { } else { link = link[1:i] } - fmt.Println(link) linkURL, err := resp.Request.URL.Parse(link) - fmt.Println(linkURL) if err != nil { return "", err } diff --git a/util/helm/client_test.go b/util/helm/client_test.go index 44f52e8f4fdff..5de873609d380 100644 --- a/util/helm/client_test.go +++ b/util/helm/client_test.go @@ -196,6 +196,7 @@ func Test_getNextUrl(t *testing.T) { "Link": []string{fmt.Sprintf(`<%s>; rel="next"`, nextUrlAbsolute)}, } nextUrl, err = getNextUrl(resp) + assert.NoError(t, err) assert.Equal(t, nextUrl, nextUrlAbsolute) var nextUrlRelative = "/v2/chart/tags/list?n=123&orderby=" @@ -203,6 +204,7 @@ func Test_getNextUrl(t *testing.T) { "Link": []string{fmt.Sprintf(`<%s>; rel="next"`, nextUrlRelative)}, } nextUrl, err = getNextUrl(resp) + assert.NoError(t, err) assert.Equal(t, nextUrl, "https://my.repo.com/v2/chart/tags/list?n=123&orderby=") } From 4398bf8a5aa88115bf8f7582599c4c6ff8cae2f1 Mon Sep 17 00:00:00 2001 From: detvdl Date: Wed, 14 Dec 2022 18:16:46 +0100 Subject: [PATCH 3/5] fix: stop double-escaping repoURL Signed-off-by: detvdl --- util/helm/client.go | 6 +++--- util/helm/client_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/util/helm/client.go b/util/helm/client.go index 32068bb9708c0..9b16a51fab166 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -391,10 +391,10 @@ func getTagsListURL(rawURL string, chart string) (string, error) { if err != nil { return "", fmt.Errorf("unable to parse repo url: %v", err) } + tagsPathFormat := "%s/v2/%s/tags/list" repoURL.Scheme = "https" - tagsList := strings.Join([]string{"v2", url.PathEscape(chart), "tags/list"}, "/") - repoURL.Path = strings.Join([]string{repoURL.Path, tagsList}, "/") - repoURL.RawPath = strings.Join([]string{repoURL.RawPath, tagsList}, "/") + repoURL.Path = fmt.Sprintf(tagsPathFormat, repoURL.Path, chart) + repoURL.RawPath = fmt.Sprintf(tagsPathFormat, repoURL.RawPath, url.PathEscape(chart)) return repoURL.String(), nil } diff --git a/util/helm/client_test.go b/util/helm/client_test.go index 5de873609d380..1f2e4da3eff20 100644 --- a/util/helm/client_test.go +++ b/util/helm/client_test.go @@ -221,4 +221,14 @@ func Test_getTagsListURL(t *testing.T) { tagsListURL, err = getTagsListURL("https://account.dkr.ecr.eu-central-1.amazonaws.com/", "dss") assert.Nil(t, err) assert.Equal(t, tagsListURL, "https://account.dkr.ecr.eu-central-1.amazonaws.com/v2/dss/tags/list") + + // with unescaped characters allowed by https://www.rfc-editor.org/rfc/rfc3986#page-50 + tagsListURL, err = getTagsListURL("https://account.dkr.ecr.eu-central-1.amazonaws.com/", "charts.-_~$&+=:@dss") + assert.Nil(t, err) + assert.Equal(t, tagsListURL, "https://account.dkr.ecr.eu-central-1.amazonaws.com/v2/charts.-_~$&+=:@dss/tags/list") + + // with escaped characters not allowed in path by https://www.rfc-editor.org/rfc/rfc3986#page-50 + tagsListURL, err = getTagsListURL("https://account.dkr.ecr.eu-central-1.amazonaws.com/", "charts%/dss") + assert.Nil(t, err) + assert.Equal(t, tagsListURL, "https://account.dkr.ecr.eu-central-1.amazonaws.com/v2/charts%25%2Fdss/tags/list") } From 526526d0b7a808e3c659fd8ec099b8dc66abca9a Mon Sep 17 00:00:00 2001 From: detvdl Date: Wed, 14 Dec 2022 18:17:20 +0100 Subject: [PATCH 4/5] chore: CodeQL CWE-117 log sanitizing Signed-off-by: detvdl --- util/helm/client.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/util/helm/client.go b/util/helm/client.go index 9b16a51fab166..71e6bb2e2a6bf 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -407,7 +407,7 @@ func (c *nativeHelmChart) getTags(chart string) ([]byte, error) { allTags := &TagsList{} var data []byte for nextURL != "" { - log.Debugf("fetching %s tags from %s", chart, text.Trunc(nextURL, 100)) + log.Debugf("fetching %s tags from %s", chart, sanitizeLog(text.Trunc(nextURL, 100))) data, nextURL, err = c.getTagsFromUrl(nextURL) if err != nil && err != errNoLink { return nil, fmt.Errorf("failed tags part: %v", err) @@ -447,6 +447,12 @@ func getNextUrl(resp *http.Response) (string, error) { return linkURL.String(), nil } +func sanitizeLog(input string) string { + sanitized := strings.ReplaceAll(input, "\r", "") + sanitized = strings.ReplaceAll(sanitized, "\n", "") + return sanitized +} + func (c *nativeHelmChart) getTagsFromUrl(tagsURL string) ([]byte, string, error) { req, err := http.NewRequest("GET", tagsURL, nil) if err != nil { From 9c0a47762ab749d8e839eed66bfd6b5ed10c54dc Mon Sep 17 00:00:00 2001 From: detvdl Date: Fri, 16 Dec 2022 15:55:37 +0100 Subject: [PATCH 5/5] chore: remove unnecessary error Signed-off-by: detvdl --- util/helm/client.go | 5 ++--- util/helm/client_test.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/util/helm/client.go b/util/helm/client.go index 71e6bb2e2a6bf..ec0c0bea4dbaa 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -33,7 +33,6 @@ import ( var ( globalLock = sync.NewKeyLock() indexLock = sync.NewKeyLock() - errNoLink = errors.New("no Link header in response") ) type Creds struct { @@ -409,7 +408,7 @@ func (c *nativeHelmChart) getTags(chart string) ([]byte, error) { for nextURL != "" { log.Debugf("fetching %s tags from %s", chart, sanitizeLog(text.Trunc(nextURL, 100))) data, nextURL, err = c.getTagsFromUrl(nextURL) - if err != nil && err != errNoLink { + if err != nil { return nil, fmt.Errorf("failed tags part: %v", err) } @@ -430,7 +429,7 @@ func (c *nativeHelmChart) getTags(chart string) ([]byte, error) { func getNextUrl(resp *http.Response) (string, error) { link := resp.Header.Get("Link") if link == "" { - return "", errNoLink + return "", nil } if link[0] != '<' { return "", fmt.Errorf("invalid next link %q: missing '<'", link) diff --git a/util/helm/client_test.go b/util/helm/client_test.go index 1f2e4da3eff20..8107a586e2772 100644 --- a/util/helm/client_test.go +++ b/util/helm/client_test.go @@ -189,7 +189,7 @@ func Test_getNextUrl(t *testing.T) { } nextUrl, err := getNextUrl(resp) assert.Equal(t, nextUrl, "") - assert.Equal(t, err, errNoLink) + assert.NoError(t, err) var nextUrlAbsolute = "https://my.repo.com/v2/chart/tags/list?n=123&orderby=" resp.Header = http.Header{