Improve credentials handling when fetching repo resources (#3057)

* Add pass credentials param. Check if they are under the same domain

* Minor test after handlng error

* Add PR review comments

* Minor test fix

cmd/apprepository-controller/controller.go

@@ -608,6 +608,10 @@ func apprepoSyncJobArgs(apprepo *apprepov1alpha1.AppRepository, config Config) [
 		args = append(args, "--tls-insecure-skip-verify")
 	}
 
+	if apprepo.Spec.PassCredentials {
+		args = append(args, "--pass-credentials")
+	}
+
 	if apprepo.Spec.FilterRule.JQ != "" {
 		rulesJSON, err := json.Marshal(apprepo.Spec.FilterRule)
 		if err != nil {

cmd/apprepository-controller/controller_test.go

@@ -1108,6 +1108,89 @@ func Test_newSyncJob(t *testing.T) {
 				},
 			},
 		},
+		{
+			"Paas credentials",
+			"",
+			&apprepov1alpha1.AppRepository{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AppRepository",
+					APIVersion: "kubeapps.com/v1alpha1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "my-charts",
+					Namespace: "kubeapps",
+					Labels: map[string]string{
+						"name":       "my-charts",
+						"created-by": "kubeapps",
+					},
+				},
+				Spec: apprepov1alpha1.AppRepositorySpec{
+					Type:            "oci",
+					URL:             "https://charts.acme.com/my-charts",
+					OCIRepositories: []string{"apache", "jenkins"},
+					PassCredentials: true,
+				},
+			},
+			batchv1.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "apprepo-kubeapps-sync-my-charts-",
+					OwnerReferences: []metav1.OwnerReference{
+						*metav1.NewControllerRef(
+							&apprepov1alpha1.AppRepository{ObjectMeta: metav1.ObjectMeta{Name: "my-charts"}},
+							schema.GroupVersionKind{
+								Group:   apprepov1alpha1.SchemeGroupVersion.Group,
+								Version: apprepov1alpha1.SchemeGroupVersion.Version,
+								Kind:    "AppRepository",
+							},
+						),
+					},
+				},
+				Spec: batchv1.JobSpec{
+					TTLSecondsAfterFinished: &defaultTTL,
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								LabelRepoName:      "my-charts",
+								LabelRepoNamespace: "kubeapps",
+							},
+						},
+						Spec: corev1.PodSpec{
+							RestartPolicy: "OnFailure",
+							Containers: []corev1.Container{
+								{
+									Name:            "sync",
+									Image:           repoSyncImage,
+									ImagePullPolicy: "IfNotPresent",
+									Command:         []string{"/chart-repo"},
+									Args: []string{
+										"sync",
+										"--database-url=postgresql.kubeapps",
+										"--database-user=admin",
+										"--database-name=assets",
+										"--namespace=kubeapps",
+										"my-charts",
+										"https://charts.acme.com/my-charts",
+										"oci",
+										"--oci-repositories",
+										"apache,jenkins",
+										"--pass-credentials",
+									},
+									Env: []corev1.EnvVar{
+										{
+											Name: "DB_PASSWORD",
+											ValueFrom: &corev1.EnvVarSource{
+												SecretKeyRef: &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "postgresql"}, Key: "postgresql-root-password"}},
+										},
+									},
+									VolumeMounts: nil,
+								},
+							},
+							Volumes: nil,
+						},
+					},
+				},
+			},
+		},
 		{
 			"Repository with filters",
 			"",

cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1/types.go

@@ -54,6 +54,8 @@ type AppRepositorySpec struct {
 	FilterRule FilterRuleSpec `json:"filterRule,omitempty"`
 	// (optional) description
 	Description string `json:"description,omitempty"`
+	// PassCredentials allows passing credentials with requests to other domains linked from the repository
+	PassCredentials bool `json:"passCredentials,omitempty"`
 }
 
 // AppRepositoryAuth is the auth for an AppRepository resource

cmd/asset-syncer/main.go

@@ -32,6 +32,7 @@ var (
 	ociRepositories       []string
 	tlsInsecureSkipVerify bool
 	filterRules           string
+	passCredentials       bool
 )
 
 var rootCmd = &cobra.Command{
@@ -59,6 +60,7 @@ func init() {
 	rootCmd.PersistentFlags().BoolVar(&debug, "debug", false, "verbose logging")
 	rootCmd.PersistentFlags().BoolVar(&tlsInsecureSkipVerify, "tls-insecure-skip-verify", false, "Skip TLS verification")
 	rootCmd.PersistentFlags().StringVar(&filterRules, "filter-rules", "", "JSON blob with the rules to filter assets")
+	rootCmd.PersistentFlags().BoolVar(&passCredentials, "pass-credentials", false, "pass credentials to all domains")
 
 	databasePassword = os.Getenv("DB_PASSWORD")
 

cmd/asset-syncer/utils.go

@@ -236,11 +236,18 @@ func (r *HelmRepo) Charts(fetchLatestOnly bool) ([]models.Chart, error) {
 
 // FetchFiles retrieves the important files of a chart and version from the repo
 func (r *HelmRepo) FetchFiles(name string, cv models.ChartVersion) (map[string]string, error) {
+	authorizationHeader := ""
+	chartTarballURL := chartTarballURL(r.RepoInternal, cv)
+
+	if passCredentials || len(r.AuthorizationHeader) > 0 && isURLDomainEqual(chartTarballURL, r.URL) {
+		authorizationHeader = r.AuthorizationHeader
+	}
+
 	return tarutil.FetchChartDetailFromTarball(
 		name,
-		chartTarballURL(r.RepoInternal, cv),
+		chartTarballURL,
 		userAgent(),
-		r.AuthorizationHeader,
+		authorizationHeader,
 		r.netClient)
 }
 
@@ -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 || len(r.AuthorizationHeader) > 0 && isURLDomainEqual(c.Icon, r.URL) {
 		req.Header.Set("Authorization", r.AuthorizationHeader)
 	}
 
@@ -826,7 +833,11 @@ func (f *fileImporter) fetchAndImportIcon(c models.Chart, r *models.RepoInternal
 	// TODO: make this configurable?
 	resizedImg := imaging.Fit(img, 160, 160, imaging.Lanczos)
 	var buf bytes.Buffer
-	imaging.Encode(&buf, resizedImg, imaging.PNG)
+	err = imaging.Encode(&buf, resizedImg, imaging.PNG)
+	if err != nil {
+		log.WithFields(log.Fields{"name": c.Name}).WithError(err).Error("failed to encode icon")
+		return err
+	}
 	b = buf.Bytes()
 	contentType = "image/png"
 
@@ -871,3 +882,18 @@ func (f *fileImporter) fetchAndImportFiles(name string, repo Repo, cv models.Cha
 	// entry if digest has changed
 	return f.manager.insertFiles(chartID, chartFiles)
 }
+
+// Check if two URL strings are in the same domain.
+// Return true if so, and false otherwise or when an error occurs
+func isURLDomainEqual(url1Str, url2Str string) bool {
+	url1, err := url.ParseRequestURI(url1Str)
+	if err != nil {
+		return false
+	}
+	url2, err := url.ParseRequestURI(url2Str)
+	if err != nil {
+		return false
+	}
+
+	return url1.Scheme == url2.Scheme && url1.Host == url2.Host
+}

cmd/asset-syncer/utils_test.go

@@ -81,6 +81,24 @@ func (h *goodHTTPClient) Do(req *http.Request) (*http.Response, error) {
 	return w.Result(), nil
 }
 
+type goodAuthenticatedHTTPClient struct{}
+
+func (h *goodAuthenticatedHTTPClient) Do(req *http.Request) (*http.Response, error) {
+	w := httptest.NewRecorder()
+
+	// Ensure we're sending an Authorization header
+	if req.Header.Get("Authorization") == "" {
+		w.WriteHeader(401)
+	}
+	// Ensure we're sending the right Authorization header
+	if !strings.Contains(req.Header.Get("Authorization"), "Bearer ThisSecretAccessTokenAuthenticatesTheClient") {
+		w.WriteHeader(403)
+	}
+
+	w.Write(iconBytes())
+	return w.Result(), nil
+}
+
 type authenticatedHTTPClient struct{}
 
 func (h *authenticatedHTTPClient) Do(req *http.Request) (*http.Response, error) {
@@ -125,7 +143,7 @@ type svgIconClient struct{}
 
 func (h *svgIconClient) Do(req *http.Request) (*http.Response, error) {
 	w := httptest.NewRecorder()
-	w.Write([]byte("foo"))
+	w.Write([]byte("<svg width='100' height='100'></svg>"))
 	res := w.Result()
 	res.Header.Set("Content-Type", "image/svg")
 	return res, nil
@@ -373,6 +391,8 @@ func Test_newManager(t *testing.T) {
 
 func Test_fetchAndImportIcon(t *testing.T) {
 	repo := &models.RepoInternal{Name: "test", Namespace: "repo-namespace"}
+	repoWithAuthorization := &models.RepoInternal{Name: "test", Namespace: "repo-namespace", AuthorizationHeader: "Bearer ThisSecretAccessTokenAuthenticatesTheClient", URL: "https://github.com/"}
+
 	t.Run("no icon", func(t *testing.T) {
 		pgManager, _, cleanup := getMockManager(t)
 		defer cleanup()
@@ -430,6 +450,52 @@ func Test_fetchAndImportIcon(t *testing.T) {
 		fImporter := fileImporter{pgManager, netClient}
 		assert.NoErr(t, fImporter.fetchAndImportIcon(c, repo))
 	})
+
+	t.Run("valid icon (not passing through the auth header by default)", func(t *testing.T) {
+		pgManager, _, cleanup := getMockManager(t)
+		defer cleanup()
+		netClient := &goodAuthenticatedHTTPClient{}
+
+		fImporter := fileImporter{pgManager, netClient}
+		assert.Err(t, fmt.Errorf("401 %s", charts[0].Icon), fImporter.fetchAndImportIcon(charts[0], repo))
+	})
+
+	t.Run("valid icon (not passing through the auth header)", func(t *testing.T) {
+		pgManager, _, cleanup := getMockManager(t)
+		defer cleanup()
+		netClient := &goodAuthenticatedHTTPClient{}
+
+		fImporter := fileImporter{pgManager, netClient}
+		passCredentials = false
+		assert.Err(t, fmt.Errorf("401 %s", charts[0].Icon), fImporter.fetchAndImportIcon(charts[0], repo))
+	})
+
+	t.Run("valid icon (passing through the auth header if same domain)", func(t *testing.T) {
+		pgManager, mock, cleanup := getMockManager(t)
+		defer cleanup()
+		netClient := &goodAuthenticatedHTTPClient{}
+
+		mock.ExpectQuery("UPDATE charts SET info *").
+			WithArgs("test/acs-engine-autoscaler", "repo-namespace", "test").
+			WillReturnRows(sqlmock.NewRows([]string{"ID"}).AddRow(1))
+
+		fImporter := fileImporter{pgManager, netClient}
+		assert.NoErr(t, fImporter.fetchAndImportIcon(charts[0], repoWithAuthorization))
+	})
+
+	t.Run("valid icon (passing through the auth header)", func(t *testing.T) {
+		pgManager, mock, cleanup := getMockManager(t)
+		defer cleanup()
+		netClient := &goodAuthenticatedHTTPClient{}
+
+		mock.ExpectQuery("UPDATE charts SET info *").
+			WithArgs("test/acs-engine-autoscaler", "repo-namespace", "test").
+			WillReturnRows(sqlmock.NewRows([]string{"ID"}).AddRow(1))
+
+		fImporter := fileImporter{pgManager, netClient}
+		passCredentials = true
+		assert.NoErr(t, fImporter.fetchAndImportIcon(charts[0], repoWithAuthorization))
+	})
 }
 
 type fakeRepo struct {
@@ -1235,3 +1301,95 @@ func Test_filterCharts(t *testing.T) {
 		})
 	}
 }
+
+func Test_isURLDomainEqual(t *testing.T) {
+	tests := []struct {
+		name     string
+		url1     string
+		url2     string
+		expected bool
+	}{
+		{
+			"it returns false if a url is malformed",
+			"abc",
+			"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,
+		},
+		{
+			"it returns false if they are under the same domain but using different schema",
+			"http://charts.bitnami.com/bitnami/airflow-10.2.0.tgz",
+			"https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png",
+			false,
+		},
+		{
+			"it returns false if attempting a CRLF injection",
+			"https://charts.bitnami.com",
+			"https://charts.bitnami.com%0A%0Ddevil.com",
+			false,
+		},
+		{
+			"it returns false if attempting a SSRF",
+			"https://ⓖⓞⓞⓖⓛⓔ.com",
+			"https://google.com",
+			false,
+		},
+		{
+			"it returns false if attempting a SSRF",
+			"https://wordpress.com",
+			"https://wordpreß.com",
+			false,
+		},
+		{
+			"it returns false if attempting a SSRF",
+			"http://foo@127.0.0.1 @bitnami.com:11211/",
+			"https://127.0.0.1:11211",
+			false,
+		},
+		{
+			"it returns false if attempting a SSRF",
+			"https://foo@evil.com@charts.bitnami.com",
+			"https://evil.com",
+			false,
+			// should be careful, curl would send a request to evil.com
+			// but the go net/url parser detects charts.bitnami.com
+		},
+		{
+			"it returns false if attempting a SSRF",
+			"https://charts.bitnami.com#@evil.com",
+			"https://charts.bitnami.com",
+			false,
+		},
+		{
+			"it returns true if they are under the same domain",
+			"https://charts.bitnami.com/bitnami/airflow-10.2.0.tgz",
+			"https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png",
+			true,
+		},
+		{
+			"it returns false if they are under the same domain but different ports",
+			"https://charts.bitnami.com:8080/bitnami/airflow-10.2.0.tgz",
+			"https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png",
+			false,
+		},
+		{
+			"it returns true if they are under the same domain",
+			"https://charts.bitnami.com/bitnami/airflow-10.2.0.tgz",
+			"https://charts.bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png",
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res := isURLDomainEqual(tt.url1, tt.url2)
+			if !cmp.Equal(res, tt.expected) {
+				t.Errorf("Unexpected result: %v", cmp.Diff(res, tt.expected))
+			}
+		})
+	}
+}