Merge pull request #1097 from souleb/custom-cert-oci

Helm OCI: Add support for TLS registries with self-signed certs

Author: stefanprodan

docs/spec/v1beta2/helmrepositories.md

@@ -459,8 +459,6 @@ a deprecation warning will be logged.
 
 ### Cert secret reference
 
-**Note:** TLS authentication is not yet supported by OCI Helm repositories.
-
 `.spec.certSecretRef.name` is an optional field to specify a secret containing TLS
 certificate data. The secret can contain the following keys:
 

internal/controller/helmchart_controller.go

@@ -512,7 +512,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 	if err != nil {
 		return chartRepoConfigErrorReturn(err, obj)
 	}
-	clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
+
+	clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
 	if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 		e := &serror.Event{
 			Err:    err,
@@ -521,6 +522,15 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
+	if certsTmpDir != "" {
+		defer func() {
+			if err := os.RemoveAll(certsTmpDir); err != nil {
+				r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
+					"failed to delete temporary certificates directory: %s", err)
+			}
+		}()
+	}
+
 	getterOpts := clientOpts.GetterOpts
 
 	// Initialize the chart repository
@@ -536,7 +546,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
 		// TODO@souleb: remove this once the registry move to Oras v2
 		// or rework to enable reusing credentials to avoid the unneccessary handshake operations
-		registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
+		registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
@@ -585,8 +595,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		// If login options are configured, use them to login to the registry
 		// The OCIGetter will later retrieve the stored credentials to pull the chart
-		if clientOpts.RegLoginOpt != nil {
-			err = ociChartRepo.Login(clientOpts.RegLoginOpt)
+		if clientOpts.MustLoginToRegistry() {
+			err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
 			if err != nil {
 				e := &serror.Event{
 					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
@@ -983,15 +993,15 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 		ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 		defer cancel()
 
-		clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
+		clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
 		if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			return nil, err
 		}
 		getterOpts := clientOpts.GetterOpts
 
 		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
-			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil)
+			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
 			if err != nil {
 				return nil, fmt.Errorf("failed to create registry client: %w", err)
 			}
@@ -1002,6 +1012,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
 				repository.WithOCIGetterOptions(getterOpts),
 				repository.WithOCIRegistryClient(registryClient),
+				repository.WithCertificatesStore(certsTmpDir),
 				repository.WithCredentialsFile(credentialsFile))
 			if err != nil {
 				errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err))
@@ -1016,8 +1027,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 
 			// If login options are configured, use them to login to the registry
 			// The OCIGetter will later retrieve the stored credentials to pull the chart
-			if clientOpts.RegLoginOpt != nil {
-				err = ociChartRepo.Login(clientOpts.RegLoginOpt)
+			if clientOpts.MustLoginToRegistry() {
+				err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
 				if err != nil {
 					errs = append(errs, fmt.Errorf("failed to login to OCI chart repository: %w", err))
 					// clean up the credentialsFile

internal/controller/helmchart_controller_test.go

@@ -1109,7 +1109,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 
 	// Upload the test chart
-	metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer)
+	metadata, err := loadTestChartToOCI(chartData, testRegistryServer, "", "", "")
 	g.Expect(err).NotTo(HaveOccurred())
 
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -2244,53 +2244,74 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 		url              string
 		registryOpts     registryOptions
 		secretOpts       secretOptions
+		secret           *corev1.Secret
+		certsecret       *corev1.Secret
+		insecure         bool
 		provider         string
 		providerImg      string
 		want             sreconcile.Result
 		wantErr          bool
 		assertConditions []metav1.Condition
 	}{
 		{
-			name: "HTTP without basic auth",
-			want: sreconcile.ResultSuccess,
+			name:     "HTTP without basic auth",
+			want:     sreconcile.ResultSuccess,
+			insecure: true,
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 			},
 		},
 		{
-			name: "HTTP with basic auth secret",
-			want: sreconcile.ResultSuccess,
+			name:     "HTTP with basic auth secret",
+			want:     sreconcile.ResultSuccess,
+			insecure: true,
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
 			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth-secretref",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 			},
 		},
 		{
-			name:    "HTTP registry - basic auth with invalid secret",
-			want:    sreconcile.ResultEmpty,
-			wantErr: true,
+			name:     "HTTP registry - basic auth with invalid secret",
+			want:     sreconcile.ResultEmpty,
+			wantErr:  true,
+			insecure: true,
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
 			secretOpts: secretOptions{
 				username: "wrong-pass",
 				password: "wrong-pass",
 			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth-secretref",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to login to OCI registry"),
 			},
 		},
 		{
 			name:        "with contextual login provider",
 			wantErr:     true,
+			insecure:    true,
 			provider:    "aws",
 			providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test",
 			assertConditions: []metav1.Condition{
@@ -2303,16 +2324,87 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			registryOpts: registryOptions{
 				withBasicAuth: true,
 			},
+			insecure: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
 			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth-secretref",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
 			provider: "azure",
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
 			},
 		},
+		{
+			name:    "HTTPS With invalid CA cert",
+			wantErr: true,
+			registryOpts: registryOptions{
+				withTLS:            true,
+				withClientCertAuth: true,
+			},
+			secretOpts: secretOptions{
+				username: testRegistryUsername,
+				password: testRegistryPassword,
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth-secretref",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
+			certsecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "certs-secretref",
+				},
+				Data: map[string][]byte{
+					"caFile": []byte("invalid caFile"),
+				},
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid caFile"),
+			},
+		},
+		{
+			name: "HTTPS With CA cert",
+			want: sreconcile.ResultSuccess,
+			registryOpts: registryOptions{
+				withTLS:            true,
+				withClientCertAuth: true,
+			},
+			secretOpts: secretOptions{
+				username: testRegistryUsername,
+				password: testRegistryPassword,
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "auth-secretref",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{},
+			},
+			certsecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "certs-secretref",
+				},
+				Data: map[string][]byte{
+					"caFile":   tlsCA,
+					"certFile": clientPublicKey,
+					"keyFile":  clientPrivateKey,
+				},
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
+				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -2325,7 +2417,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 
 			workspaceDir := t.TempDir()
 
-			tt.registryOpts.disableDNSMocking = true
+			if tt.insecure {
+				tt.registryOpts.disableDNSMocking = true
+			}
 			server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
 			g.Expect(err).NotTo(HaveOccurred())
 			t.Cleanup(func() {
@@ -2337,7 +2431,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			g.Expect(err).ToNot(HaveOccurred())
 
 			// Upload the test chart
-			metadata, err := loadTestChartToOCI(chartData, chartPath, server)
+			metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/client.pem", "testdata/certs/client-key.pem", "testdata/certs/ca.pem")
 			g.Expect(err).ToNot(HaveOccurred())
 
 			repo := &helmv1.HelmRepository{
@@ -2364,25 +2458,26 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			}
 
 			if tt.secretOpts.username != "" && tt.secretOpts.password != "" {
-				secret := &corev1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "auth-secretref",
-					},
-					Type: corev1.SecretTypeDockerConfigJson,
-					Data: map[string][]byte{
-						".dockerconfigjson": []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`,
-							server.registryHost, tt.secretOpts.username, tt.secretOpts.password)),
-					},
-				}
+				tt.secret.Data[".dockerconfigjson"] = []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`,
+					server.registryHost, tt.secretOpts.username, tt.secretOpts.password))
+			}
 
+			if tt.secret != nil {
 				repo.Spec.SecretRef = &meta.LocalObjectReference{
-					Name: secret.Name,
+					Name: tt.secret.Name,
 				}
-				clientBuilder.WithObjects(secret, repo)
-			} else {
-				clientBuilder.WithObjects(repo)
+				clientBuilder.WithObjects(tt.secret)
+			}
+
+			if tt.certsecret != nil {
+				repo.Spec.CertSecretRef = &meta.LocalObjectReference{
+					Name: tt.certsecret.Name,
+				}
+				clientBuilder.WithObjects(tt.certsecret)
 			}
 
+			clientBuilder.WithObjects(repo)
+
 			obj := &helmv1.HelmChart{
 				ObjectMeta: metav1.ObjectMeta{
 					GenerateName: "auth-strategy-",
@@ -2456,7 +2551,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
 	g.Expect(err).ToNot(HaveOccurred())
 
 	// Upload the test chart
-	metadata, err := loadTestChartToOCI(chartData, chartPath, server)
+	metadata, err := loadTestChartToOCI(chartData, server, "", "", "")
 	g.Expect(err).NotTo(HaveOccurred())
 
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -2687,30 +2782,24 @@ func extractChartMeta(chartData []byte) (*hchart.Metadata, error) {
 	return ch.Metadata, nil
 }
 
-func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClientTestServer) (*hchart.Metadata, error) {
+func loadTestChartToOCI(chartData []byte, server *registryClientTestServer, certFile, keyFile, cafile string) (*hchart.Metadata, error) {
 	// Login to the registry
 	err := server.registryClient.Login(server.registryHost,
 		helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
-		helmreg.LoginOptInsecure(true))
-	if err != nil {
-		return nil, err
-	}
-
-	// Load a test chart
-	chartData, err = os.ReadFile(chartPath)
+		helmreg.LoginOptTLSClientConfig(certFile, keyFile, cafile))
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to login to OCI registry: %w", err)
 	}
 	metadata, err := extractChartMeta(chartData)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to extract chart metadata: %w", err)
 	}
 
 	// Upload the test chart
 	ref := fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
 	_, err = server.registryClient.Push(chartData, ref)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to push chart: %w", err)
 	}
 
 	return metadata, nil

internal/controller/helmrepository_controller.go

@@ -399,7 +399,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 		return sreconcile.ResultEmpty, e
 	}
 
-	clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
+	clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
 	if err != nil {
 		if errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			ctrl.LoggerFrom(ctx).