Skip to content

fix: support relative links in OCI tags query response #11708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crenshaw-dev merged 6 commits into from
Dec 16, 2022
+66 −16
Merged

fix: support relative links in OCI tags query response #11708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
54d039b
fix: support relative links in OCI tags query response
detvdl Dec 14, 2022
29d0bd7
chore: clean up unused prints & assert errors
detvdl Dec 14, 2022
4398bf8
fix: stop double-escaping repoURL
detvdl Dec 14, 2022
526526d
chore: CodeQL CWE-117 log sanitizing
detvdl Dec 14, 2022
7504365
Merge branch 'master' into fix/OCI-relative-link-header
detvdl Dec 14, 2022
9c0a477
chore: remove unnecessary error
detvdl Dec 16, 2022
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
42 changes: 29 additions & 13 deletions util/helm/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,10 +390,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
Comment on lines +393 to 397
Copy link
Contributor Author

@detvdl detvdl Dec 14, 2022

Choose a reason for hiding this comment

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

As per the documentation on url.String() and url.EscapedPath():

EscapedPath returns the escaped form of u.Path. In general there are multiple possible escaped forms of any path. EscapedPath returns u.RawPath when it is a valid escaping of u.Path. Otherwise EscapedPath ignores u.RawPath and computes an escaped form on its own.

This means that repoURL.RawPath should contain a valid escaping.
In the case of a chart with a / in its name, the following occurred:

  • url.PathEscape("chart/name") generated chart%2Fname
  • repoURL.Path and repoURL.RawPath both contain chart%2Fname
  • repoURL.String() invokes repoURL.EscapedPath()
  • repoURL.EscapedPath() does not recognize chart%2Fname as a valid escaping of chart%2Fname
  • repoURL.EscapedPath() escapes again, resulting in chart%252Fname

and the resulting URL is not correct and results in 404 responses

}

Expand All @@ -406,7 +406,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 {
return nil, fmt.Errorf("failed tags part: %v", err)
Expand All @@ -426,14 +426,30 @@ 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 "", nil
}
return nextUrl
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]
}
linkURL, err := resp.Request.URL.Parse(link)
Copy link
Member

@alexef alexef Dec 15, 2022

Choose a reason for hiding this comment

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

Is this the place where you convert from relative link to absolute?

Copy link
Contributor Author

@detvdl detvdl Dec 15, 2022

Choose a reason for hiding this comment

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

yes, more precisely just the resp.Request.URL.Parse() call, which will accept and resolve relative links to the original request URL.

Granted, this code was inspired a great deal by https://github.com/oras-project/oras-go/blob/main/registry/remote/utils.go#L40 which also begs the question: shouldn't we use the oras-go project for this OCI functionality?

Helm itself is using it internally

Copy link
Member

@alexef alexef Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@detvdl as I'm new to the project, I was reluctant to introduce new dependencies in my pr. But I agree, it makes sense to use the library rather than copying the code.

As 2.6RC is just around the corner, it probably makes sense to merge this fix as is, and then replace the code with oras in a separate PR?

Will let @crenshaw-dev / @alexmt decide.

Copy link
Contributor Author

@detvdl detvdl Dec 15, 2022

Choose a reason for hiding this comment

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

I would agree with that, let's wait for their decision indeed

Copy link
Member

@crenshaw-dev crenshaw-dev Dec 15, 2022

Choose a reason for hiding this comment

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

In general, I'm cool w/ Oras if it saves, say, 30 lines of code that they're more likely to get correct.

But I think that can wait for a follow-up PR. :-)

if err != nil {
return "", err
}
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) {
Expand Down Expand Up @@ -484,8 +500,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) {
Expand Down
40 changes: 37 additions & 3 deletions util/helm/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"net/http"
"net/http/httptest"
"net/url"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -177,11 +178,34 @@ 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, "")
assert.NoError(t, err)

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.NoError(t, err)
assert.Equal(t, nextUrl, nextUrlAbsolute)

nextUrl = getNextUrl("<https://my.repo.com/v2/chart/tags/list?token=123>; rel=next")
assert.Equal(t, nextUrl, "https://my.repo.com/v2/chart/tags/list?token=123")
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.NoError(t, err)
assert.Equal(t, nextUrl, "https://my.repo.com/v2/chart/tags/list?n=123&orderby=")
}

func Test_getTagsListURL(t *testing.T) {
Expand All @@ -197,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")
}