refactor TLSconfig funcs

Signed-off-by: Soule BA <soule@weave.works>

docs/spec/v1beta2/helmrepositories.md

@@ -459,16 +459,8 @@ a deprecation warning will be logged.
 
 ### Cert secret reference
 
-<<<<<<< HEAD
-**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:
-=======
-To provide TLS credentials to use while connecting with the Helm repository,
-the referenced Secret is expected to contain `.data.certFile` and
-`.data.keyFile`, and/or `.data.caFile` values.
->>>>>>> 3df4c49 (refactoring and fix tests)
 
 * `certFile` and `keyFile`, to specify the client certificate and private key used for
 TLS client authentication. These must be used in conjunction, i.e. specifying one without
@@ -515,28 +507,6 @@ data:
   caFile: <BASE64>
 ```
 
-#### Provide TLS credentials in a secret of type kubernetes.io/dockerconfigjson
-
-For OCI Helm repositories, Kubernetes secrets of type [kubernetes.io/dockerconfigjson](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types)
-are also supported. It is possible to append TLS credentials to the secret data.
-
-For example:
-
-```yaml
-apiVersion: v1
-kind: Secret
-metadata:
-  name: example-tls
-  namespace: default
-type: kubernetes.io/dockerconfigjson
-data:
-  .dockerconfigjson: <BASE64>
-  certFile: <BASE64>
-  keyFile: <BASE64>
-  # NOTE: Can be supplied without the above values
-  caFile: <BASE64>
-```
-
 ### Pass credentials
 
 `.spec.passCredentials` is an optional field to allow the credentials from the

go.mod

@@ -40,12 +40,8 @@ require (
 	github.com/fluxcd/pkg/tar v0.2.0
 	github.com/fluxcd/pkg/testserver v0.4.0
 	github.com/fluxcd/pkg/version v0.2.2
-<<<<<<< HEAD
 	github.com/fluxcd/source-controller/api v1.0.0
-=======
-	github.com/fluxcd/source-controller/api v1.0.0-rc.5
 	github.com/foxcpp/go-mockdns v1.0.0
->>>>>>> 4e0d792 (Adapting setupRegistryServer to be able to use https with the docker)
 	github.com/go-git/go-billy/v5 v5.4.1
 	github.com/go-git/go-git/v5 v5.8.1
 	github.com/go-logr/logr v1.2.4

internal/controller/helmchart_controller.go

@@ -510,7 +510,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,
@@ -519,6 +520,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
+	if certsTmpDir != "" {
+		// this happens when the tls certs are stored in a temporary directory
+		defer os.RemoveAll(certsTmpDir)
+	}
 	getterOpts := clientOpts.GetterOpts
 
 	// Initialize the chart repository
@@ -534,7 +539,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.RegLoginOpts[0] != nil)
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to construct Helm client: %w", err),
@@ -583,8 +588,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.RegLoginOpts != nil {
+			err = ociChartRepo.Login(clientOpts.RegLoginOpts...)
 			if err != nil {
 				e := &serror.Event{
 					Err:    fmt.Errorf("failed to login to OCI registry: %w", err),
@@ -981,15 +986,19 @@ 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
 		}
+		if certsTmpDir != "" {
+			// this happens when the tls certs are stored in a temporary directory
+			defer os.RemoveAll(certsTmpDir)
+		}
 		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.RegLoginOpts[0] != nil)
 			if err != nil {
 				return nil, fmt.Errorf("failed to create registry client: %w", err)
 			}
@@ -1014,8 +1023,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.RegLoginOpts != nil {
+				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

@@ -2205,7 +2205,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 		registryOpts     registryOptions
 		secretOpts       secretOptions
 		secret           *corev1.Secret
-		withTLS          bool
+		insecure         bool
 		provider         string
 		providerImg      string
 		want             sreconcile.Result
@@ -2302,7 +2302,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			registryOpts: registryOptions{
 				withTLS: true,
 			},
-			withTLS: true,
+			insecure: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
@@ -2326,7 +2326,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 			registryOpts: registryOptions{
 				withTLS: true,
 			},
-			withTLS: true,
+			insecure: true,
 			secretOpts: secretOptions{
 				username: testRegistryUsername,
 				password: testRegistryPassword,
@@ -2361,9 +2361,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 
 			server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
 			g.Expect(err).NotTo(HaveOccurred())
-			if tt.withTLS {
-				defer server.stopSrv()
-			}
+			t.Cleanup(func() {
+				server.Close()
+			})
 
 			// Load a test chart
 			chartData, err := os.ReadFile(chartPath)

internal/controller/helmrepository_controller.go

@@ -398,7 +398,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).

internal/controller/helmrepository_controller_oci.go

@@ -307,50 +307,55 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S
 	conditions.Delete(obj, meta.StalledCondition)
 
 	var (
-		authenticator authn.Authenticator
-		keychain      authn.Keychain
-		tlsConfig     *tls.Config
-		tmpCertsDir   string
-		tlsLoginOpt   helmreg.LoginOption
-		err           error
+		authenticator             authn.Authenticator
+		keychain                  authn.Keychain
+		tlsConfig                 *tls.Config
+		tlsBytes                  *getter.TLSBytes
+		certFile, keyFile, caFile string
+		err                       error
 	)
 	// Configure any authentication related options.
 	if obj.Spec.SecretRef != nil {
-		// Attempt to retrieve secret.
-		name := types.NamespacedName{
-			Namespace: obj.GetNamespace(),
-			Name:      obj.Spec.SecretRef.Name,
-		}
-		var secret corev1.Secret
-		if err := r.Client.Get(ctx, name, &secret); err != nil {
-			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
-			result, retErr = ctrl.Result{}, err
-			return
-		}
-		keychain, err = authFromSecret(ctx, r.Client, obj.Spec.URL, secret)
+		secret, err := r.getSecret(ctx, obj)
 		if err != nil {
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
 			result, retErr = ctrl.Result{}, err
 			return
 		}
-		tlsConfig, err = getter.TLSClientConfigFromSecret(secret, obj.Spec.URL)
+		keychain, err = authFromSecret(ctx, r.Client, obj.Spec.URL, *secret)
 		if err != nil {
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
 			result, retErr = ctrl.Result{}, err
 			return
 		}
-		tlsLoginOpt, tmpCertsDir, err = makeTLSLoginOption(&secret)
+		tlsConfig, tlsBytes, err = getter.TLSClientConfigFromSecret(*secret, obj.Spec.URL)
 		if err != nil {
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
 			result, retErr = ctrl.Result{}, err
 			return
 		}
-		defer func() {
-			if err := os.RemoveAll(tmpCertsDir); err != nil {
-				r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
-					"failed to delete temporary certificates directory: %s", err)
+		// Now that we have checked for TLS config, persist the certs files to the path if needed.
+		if tlsBytes != nil {
+			certsTmpDir, err := os.MkdirTemp("", "helm-repo-oci-certs")
+			if err != nil {
+				conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
+				result, retErr = ctrl.Result{}, err
+				return
 			}
-		}()
+			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)
+				}
+			}()
+
+			certFile, keyFile, caFile, err = getter.StoreCertsFiles(tlsBytes, certsTmpDir)
+			if err != nil {
+				conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error())
+				result, retErr = ctrl.Result{}, err
+				return
+			}
+		}
 	} else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI {
 		auth, authErr := soci.OIDCAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
 		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
@@ -401,8 +406,11 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S
 	// Attempt to login to the registry if credentials are provided.
 	if loginOpt != nil {
 		opts := []helmreg.LoginOption{loginOpt}
-		if tlsLoginOpt != nil {
-			opts = append(opts, tlsLoginOpt)
+		if tlsBytes != nil {
+			tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile)
+			if tlsLoginOpt != nil {
+				opts = append(opts, tlsLoginOpt)
+			}
 		}
 		err = chartRepo.Login(opts...)
 		if err != nil {
@@ -430,6 +438,22 @@ func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj *
 	return ctrl.Result{}, nil
 }
 
+func (r *HelmRepositoryOCIReconciler) getSecret(ctx context.Context, obj *helmv1.HelmRepository) (*corev1.Secret, error) {
+	if obj.Spec.SecretRef == nil {
+		return nil, nil
+	}
+	name := types.NamespacedName{
+		Namespace: obj.GetNamespace(),
+		Name:      obj.Spec.SecretRef.Name,
+	}
+	var secret corev1.Secret
+	err := r.Client.Get(ctx, name, &secret)
+	if err != nil {
+		return nil, err
+	}
+	return &secret, nil
+}
+
 // eventLogf records events, and logs at the same time.
 //
 // This log is different from the debug log in the EventRecorder, in the sense
@@ -471,80 +495,6 @@ func makeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registry
 	return nil, nil
 }
 
-func makeTLSLoginOption(secret *corev1.Secret) (helmreg.LoginOption, string, error) {
-	var errs []error
-	certFile, keyFile, caFile, tmpDir, err := certsFilesFromSecret(secret)
-	if err != nil {
-		errs = append(errs, err)
-		if tmpDir != "" {
-			if err := os.RemoveAll(tmpDir); err != nil {
-				errs = append(errs, err)
-			}
-		}
-		return nil, "", kerrors.NewAggregate(errs)
-	}
-
-	if (certFile != "" && keyFile != "") || caFile != "" {
-		return helmreg.LoginOptTLSClientConfig(certFile, keyFile, caFile), tmpDir, nil
-	}
-
-	return nil, "", nil
-}
-
-func certsFilesFromSecret(secret *corev1.Secret) (string, string, string, string, error) {
-	certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"]
-	switch {
-	case len(certBytes)+len(keyBytes)+len(caBytes) == 0:
-		return "", "", "", "", nil
-	case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0):
-		return "", "", "", "", fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence",
-			secret.Name)
-	}
-
-	var (
-		certFile string
-		keyFile  string
-		caFile   string
-		err      error
-	)
-
-	// create temporary folder to store the certs
-	tmpDir, err := os.MkdirTemp("", "helm-repo-oci-certs")
-	if err != nil {
-		return "", "", "", "", err
-	}
-
-	if len(certBytes) > 0 && len(keyBytes) > 0 {
-		certFile, err = writeTofile(certBytes, "cert.pem", tmpDir)
-		if err != nil {
-			return "", "", "", "", err
-		}
-		keyFile, err = writeTofile(keyBytes, "key.pem", tmpDir)
-		if err != nil {
-			return "", "", "", "", err
-		}
-	}
-	if len(caBytes) > 0 {
-		caFile, err = writeTofile(caBytes, "ca.pem", tmpDir)
-		if err != nil {
-			return "", "", "", "", err
-		}
-	}
-	return certFile, keyFile, caFile, tmpDir, nil
-}
-
-func writeTofile(data []byte, filename, tmpDir string) (string, error) {
-	file, err := os.CreateTemp(tmpDir, filename)
-	if err != nil {
-		return "", err
-	}
-	defer file.Close()
-	if _, err := file.Write(data); err != nil {
-		return "", err
-	}
-	return file.Name(), nil
-}
-
 func conditionsDiff(a, b []string) []string {
 	bMap := make(map[string]struct{}, len(b))
 	for _, j := range b {