Improve credentials handling when fetching repo resources #3057
Conversation
| @@ -54,6 +54,8 @@ type AppRepositorySpec struct { | |||
| FilterRule FilterRuleSpec `json:"filterRule,omitempty"` | |||
| // (optional) description | |||
| Description string `json:"description,omitempty"` | |||
| // PassCredentials allows passing credentials to all domains | |||
There was a problem hiding this comment.
| // PassCredentials allows passing credentials to all domains | |
| // PassCredentials allows passing credentials with requests to other domains linked from the repository |
cmd/asset-syncer/utils.go
Outdated
| authorizationHeader := "" | ||
| chartTarballURL := chartTarballURL(r.RepoInternal, cv) | ||
|
|
||
| if (passCredentials || compareURLDomains(chartTarballURL, r.URL)) && len(r.AuthorizationHeader) > 0 { |
There was a problem hiding this comment.
Tiny point, but I'd generally have the len(r.AuthorizationHeader) > 0 condition first so the rest is only evaluated if there's authorization?
cmd/asset-syncer/utils.go
Outdated
| @@ -783,7 +790,7 @@ func (f *fileImporter) fetchAndImportIcon(c models.Chart, r *models.RepoInternal | |||
| return err | |||
| } | |||
| req.Header.Set("User-Agent", userAgent()) | |||
| if len(r.AuthorizationHeader) > 0 { | |||
| if (passCredentials || compareURLDomains(c.Icon, r.URL)) && len(r.AuthorizationHeader) > 0 { | |||
| if err != nil { | ||
| log.WithFields(log.Fields{"name": c.Name}).WithError(err).Error("failed to encode icon") | ||
| return err | ||
| } |
There was a problem hiding this comment.
Nice catch for the missing error.
There was a problem hiding this comment.
I can't take the credit here :P the merit is on vscode and https://github.com/golangci/golangci-lint this time:
There was a problem hiding this comment.
Mmm I'm dropping a card in the next interaction discussion for considering addressing all linter issues at once and adding it as part of the build process of our go dockerfiles
cmd/asset-syncer/utils.go
Outdated
|
|
||
| // Check if two URL strings are in the same domain. | ||
| // Return true if so, and false otherwise or when an error occurs | ||
| func compareURLDomains(url1Str, url2Str string) bool { |
There was a problem hiding this comment.
Slight preference for a name that indicates the boolean nature of this, such as isURLDomainEqual or hostsEqual.
cmd/asset-syncer/utils.go
Outdated
|
|
||
| a := url1.Hostname() | ||
| b := url2.Hostname() | ||
| // return url1.Hostname() == url2.Hostname() |
There was a problem hiding this comment.
Guessing this is what you intended to return rather than using the a and b unnecessarily?
But related to this, I reckon we should also ensure that the proto is the same (ie. both https). If the index.yaml was listed as https://example.com/index.yaml but referred to an icon at http://example.com/icons/foo.png, I'd hope we wouldn't assume it's the same host (ie. same host name but different proto). Thoughts?
There was a problem hiding this comment.
Yep, just a leftover when running the debugger :P
Good catch! We have to verify the URL scheme as well, of course!
By the way, there is an interesting go testing feature that will get released in 1.17: Fuzzing tests: https://blog.golang.org/fuzz-beta
Testing this method would be an interesting use case indeed.
cmd/asset-syncer/utils_test.go
Outdated
| assert.Err(t, fmt.Errorf("401 %s", charts[0].Icon), fImporter.fetchAndImportIcon(charts[0], repo)) | ||
| }) | ||
|
|
||
| // t.Run("valid icon (passing through if same domain)", func(t *testing.T) { |
There was a problem hiding this comment.
Not sure why this is commented out?
There was a problem hiding this comment.
Sorry, I commented all of them out so that running the test suite was quicker. I forgot to enable this one again 😬
Thanks for pointing it out!
| "https://charts.bitnami.com/bitnami/airflow-10.2.0.tgz", | ||
| "https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png", | ||
| true, | ||
| }, |
There was a problem hiding this comment.
possibly tests for different proto?
cmd/asset-syncer/utils_test.go
Outdated
| { | ||
| "it returns false if they are different subdomains", | ||
| "https://charts.bitnami.com/bitnami/index.yaml", | ||
| "https://bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png", | ||
| false, | ||
| }, | ||
| { | ||
| "it returns false if they are under different subdomains", | ||
| "https://bitnami.com/bitnami/index.yaml", | ||
| "https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png", | ||
| false, | ||
| }, |
There was a problem hiding this comment.
These two tests appear to be testing the same thing? Also, this is interesting - the subdomain. Is this the same behaviour in the helm patch? Asking because it's much more likely that I'd have my assets on a subdomain of the repo... but fine either way.
There was a problem hiding this comment.
They are using the same approach (below), so I think we should stick close to it. If a user happens to really need to pass the credentials down to a different subdomain, we may consider adding a --allowed-pass-credentials-subdomains flag
(u1.Scheme == u2.Scheme && u1.Host == u2.Host)
Side note: we were using .Hostname(), which yields the Host value without ports. Having noticed they also compare the ports, I've changed the PR's code to match their appraoch.
Description of the change
This PR adds a similar change as the Helm folks did in helm/helm@179f901
Now, the credentials will be passed to an external URL if: 1) a user adds
pass-credentialsto the repo OR 2) the domain in the chart index.yaml file matches the icon/tarball domain.Benefits
Not passing credentials to external domains (such as icons and tarballs)
Possible drawbacks
If users don't explicitly decide to allow passing the credentials, in scenarios when the index.yaml URL doesn't match the icon/tarball URL domain, the credentials won't be passed through anymore.
Applicable issues
Additional information
More testing is required, I feel the mock clients we use here are not enough to make sure we are doing it right.