Reuse transport for helm chart download

Reuses the same transport across different helm chart downloads,
whilst resetting the tlsconfig to avoid cross-contamination.

Crypto material is now only processed in-memory and does not
touch the disk.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>

controllers/helmchart_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"crypto/tls"
 	"errors"
 	"fmt"
 	"net/url"
@@ -60,6 +61,7 @@ import (
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
+	"github.com/fluxcd/source-controller/internal/transport"
 	"github.com/fluxcd/source-controller/internal/util"
 )
 
@@ -368,6 +370,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
 
 func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *sourcev1.HelmChart,
 	repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) {
+	var tlsConfig *tls.Config
 
 	// Construct the Getter options from the HelmRepository data
 	clientOpts := []helmgetter.Option{
@@ -386,32 +389,35 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 			return sreconcile.ResultEmpty, e
 		}
 
-		// Create temporary working directory for credentials
-		authDir, err := util.TempDirForObj("", obj)
+		// Build client options from secret
+		opts, err := getter.ClientOptionsFromSecret(*secret)
 		if err != nil {
 			e := &serror.Event{
-				Err:    fmt.Errorf("failed to create temporary working directory: %w", err),
-				Reason: sourcev1.StorageOperationFailedReason,
+				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
+				Reason: sourcev1.AuthenticationFailedReason,
 			}
-			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedReason, e.Err.Error())
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
+			// Requeue as content of secret might change
 			return sreconcile.ResultEmpty, e
 		}
-		defer os.RemoveAll(authDir)
+		clientOpts = append(clientOpts, opts...)
 
-		// Build client options from secret
-		opts, err := getter.ClientOptionsFromSecret(authDir, *secret)
+		tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
 		if err != nil {
 			e := &serror.Event{
-				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
+				Err:    fmt.Errorf("failed to create tls client config with secret data: %w", err),
 				Reason: sourcev1.AuthenticationFailedReason,
 			}
 			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
 			// Requeue as content of secret might change
 			return sreconcile.ResultEmpty, e
 		}
-		clientOpts = append(clientOpts, opts...)
 	}
 
+	t := transport.NewOrIdle(tlsConfig)
+	clientOpts = append(clientOpts, helmgetter.WithTransport(t))
+	defer transport.Release(t)
+
 	// Initialize the chart repository
 	chartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, clientOpts)
 	if err != nil {
@@ -523,15 +529,8 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
 	}
 
 	// Setup dependency manager
-	authDir := filepath.Join(tmpDir, "creds")
-	if err = os.Mkdir(authDir, 0700); err != nil {
-		return sreconcile.ResultEmpty, &serror.Event{
-			Err:    fmt.Errorf("failed to create temporary directory for dependency credentials: %w", err),
-			Reason: meta.FailedReason,
-		}
-	}
 	dm := chart.NewDependencyManager(
-		chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, authDir, obj.GetNamespace())),
+		chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetNamespace())),
 	)
 	defer dm.Clear()
 
@@ -750,8 +749,9 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
 // Credentials for retrieved v1beta1.HelmRepository objects are stored in the given directory.
 // The returned callback returns a repository.ChartRepository configured with the retrieved v1beta1.HelmRepository,
 // or a shim with defaults if no object could be found.
-func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, dir, namespace string) chart.GetChartRepositoryCallback {
+func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, namespace string) chart.GetChartRepositoryCallback {
 	return func(url string) (*repository.ChartRepository, error) {
+		var tlsConfig *tls.Config
 		repo, err := r.resolveDependencyRepository(ctx, url, namespace)
 		if err != nil {
 			// Return Kubernetes client errors, but ignore others
@@ -774,12 +774,22 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			if err != nil {
 				return nil, err
 			}
-			opts, err := getter.ClientOptionsFromSecret(dir, *secret)
+			opts, err := getter.ClientOptionsFromSecret(*secret)
 			if err != nil {
 				return nil, err
 			}
 			clientOpts = append(clientOpts, opts...)
+
+			tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, repo.Spec.URL)
+			if err != nil {
+				return nil, fmt.Errorf("failed to create tls client config for HelmRepository '%s': %w", repo.Name, err)
+			}
 		}
+
+		t := transport.NewOrIdle(tlsConfig)
+		clientOpts = append(clientOpts, helmgetter.WithTransport(t))
+		defer transport.Release(t)
+
 		chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, clientOpts)
 		if err != nil {
 			return nil, err

controllers/helmrepository_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"crypto/tls"
 	"errors"
 	"fmt"
 	"net/url"
@@ -49,6 +50,7 @@ import (
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
+	"github.com/fluxcd/source-controller/internal/transport"
 )
 
 // helmRepoReadyConditions contains all the conditions information needed
@@ -260,6 +262,8 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so
 // If the download is successful, the given artifact pointer is set to a new artifact with the available metadata, and
 // the index pointer is set to the newly downloaded index.
 func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
+	var tlsConfig *tls.Config
+
 	// Configure Helm client to access repository
 	clientOpts := []helmgetter.Option{
 		helmgetter.WithTimeout(obj.Spec.Timeout.Duration),
@@ -284,18 +288,8 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 			return sreconcile.ResultEmpty, e
 		}
 
-		// Get client options from secret
-		tmpDir, err := os.MkdirTemp("", fmt.Sprintf("%s-%s-auth-", obj.Name, obj.Namespace))
-		if err != nil {
-			return sreconcile.ResultEmpty, &serror.Event{
-				Err:    fmt.Errorf("failed to create temporary directory for credentials: %w", err),
-				Reason: sourcev1.StorageOperationFailedReason,
-			}
-		}
-		defer os.RemoveAll(tmpDir)
-
 		// Construct actual options
-		opts, err := getter.ClientOptionsFromSecret(tmpDir, secret)
+		opts, err := getter.ClientOptionsFromSecret(secret)
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
@@ -306,8 +300,23 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou
 			return sreconcile.ResultEmpty, e
 		}
 		clientOpts = append(clientOpts, opts...)
+
+		tlsConfig, err = getter.TLSClientConfigFromSecret(secret, obj.Spec.URL)
+		if err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("failed to create tls client config with secret data: %w", err),
+				Reason: sourcev1.AuthenticationFailedReason,
+			}
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, e.Err.Error())
+			// Requeue as content of secret might change
+			return sreconcile.ResultEmpty, e
+		}
 	}
 
+	t := transport.NewOrIdle(tlsConfig)
+	clientOpts = append(clientOpts, helmgetter.WithTransport(t))
+	defer transport.Release(t)
+
 	// Construct Helm chart repository with options and download index
 	newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, clientOpts)
 	if err != nil {

controllers/helmrepository_controller_test.go

@@ -364,7 +364,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 			},
 			wantErr: true,
 			assertConditions: []metav1.Condition{
-				*conditions.TrueCondition(sourcev1.FetchFailedCondition, meta.FailedReason, "can't create TLS config for client: failed to append certificates from file"),
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create tls client config with secret data: cannot append certificate into certificate pool: invalid caFile"),
 			},
 		},
 		{

go.mod

@@ -39,6 +39,7 @@ require (
 	k8s.io/api v0.23.3
 	k8s.io/apimachinery v0.23.3
 	k8s.io/client-go v0.23.3
+	k8s.io/helm v2.17.0+incompatible
 	k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
 	sigs.k8s.io/cli-utils v0.28.0
 	sigs.k8s.io/controller-runtime v0.11.1

go.sum

@@ -1751,6 +1751,8 @@ k8s.io/gengo v0.0.0-20200428234225-8167cfdcfc14/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8
 k8s.io/gengo v0.0.0-20201113003025-83324d819ded/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/gengo v0.0.0-20201214224949-b6c5ce23f027/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
+k8s.io/helm v2.17.0+incompatible h1:Bpn6o1wKLYqKM3+Osh8e+1/K2g/GsQJ4F4yNF2+deao=
+k8s.io/helm v2.17.0+incompatible/go.mod h1:LZzlS4LQBHfciFOurYBFkCMTaZ0D1l+p0teMg7TSULI=
 k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
 k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
 k8s.io/klog/v2 v2.4.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=

internal/helm/getter/getter.go

@@ -17,16 +17,18 @@ limitations under the License.
 package getter
 
 import (
+	"crypto/tls"
+	"crypto/x509"
 	"fmt"
-	"os"
 
 	"helm.sh/helm/v3/pkg/getter"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/helm/pkg/urlutil"
 )
 
 // ClientOptionsFromSecret constructs a getter.Option slice for the given secret.
 // It returns the slice, or an error.
-func ClientOptionsFromSecret(dir string, secret corev1.Secret) ([]getter.Option, error) {
+func ClientOptionsFromSecret(secret corev1.Secret) ([]getter.Option, error) {
 	var opts []getter.Option
 	basicAuth, err := BasicAuthFromSecret(secret)
 	if err != nil {
@@ -35,13 +37,6 @@ func ClientOptionsFromSecret(dir string, secret corev1.Secret) ([]getter.Option,
 	if basicAuth != nil {
 		opts = append(opts, basicAuth)
 	}
-	tlsClientConfig, err := TLSClientConfigFromSecret(dir, secret)
-	if err != nil {
-		return opts, err
-	}
-	if tlsClientConfig != nil {
-		opts = append(opts, tlsClientConfig)
-	}
 	return opts, nil
 }
 
@@ -62,13 +57,11 @@ func BasicAuthFromSecret(secret corev1.Secret) (getter.Option, error) {
 }
 
 // TLSClientConfigFromSecret attempts to construct a TLS client config
-// getter.Option for the given v1.Secret, placing the required TLS config
-// related files in the given directory. It returns the getter.Option, or
-// an error.
+// for the given v1.Secret. It returns the TLS client config or an error.
 //
 // Secrets with no certFile, keyFile, AND caFile are ignored, if only a
 // certBytes OR keyBytes is defined it returns an error.
-func TLSClientConfigFromSecret(dir string, secret corev1.Secret) (getter.Option, error) {
+func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, error) {
 	certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"]
 	switch {
 	case len(certBytes)+len(keyBytes)+len(caBytes) == 0:
@@ -78,49 +71,31 @@ func TLSClientConfigFromSecret(dir string, secret corev1.Secret) (getter.Option,
 			secret.Name)
 	}
 
-	var certPath, keyPath, caPath string
+	tlsConf := &tls.Config{}
 	if len(certBytes) > 0 && len(keyBytes) > 0 {
-		certFile, err := os.CreateTemp(dir, "cert-*.crt")
-		if err != nil {
-			return nil, err
-		}
-		if _, err = certFile.Write(certBytes); err != nil {
-			_ = certFile.Close()
-			return nil, err
-		}
-		if err = certFile.Close(); err != nil {
-			return nil, err
-		}
-		certPath = certFile.Name()
-
-		keyFile, err := os.CreateTemp(dir, "key-*.crt")
+		cert, err := tls.X509KeyPair(certBytes, keyBytes)
 		if err != nil {
 			return nil, err
 		}
-		if _, err = keyFile.Write(keyBytes); err != nil {
-			_ = keyFile.Close()
-			return nil, err
-		}
-		if err = keyFile.Close(); err != nil {
-			return nil, err
-		}
-		keyPath = keyFile.Name()
+		tlsConf.Certificates = append(tlsConf.Certificates, cert)
 	}
 
 	if len(caBytes) > 0 {
-		caFile, err := os.CreateTemp(dir, "ca-*.pem")
-		if err != nil {
-			return nil, err
+		cp := x509.NewCertPool()
+		if !cp.AppendCertsFromPEM(caBytes) {
+			return nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile")
 		}
-		if _, err = caFile.Write(caBytes); err != nil {
-			_ = caFile.Close()
-			return nil, err
-		}
-		if err = caFile.Close(); err != nil {
-			return nil, err
-		}
-		caPath = caFile.Name()
+
+		tlsConf.RootCAs = cp
+	}
+
+	tlsConf.BuildNameToCertificate()
+
+	sni, err := urlutil.ExtractHostname(url)
+	if err != nil {
+		return nil, err
 	}
+	tlsConf.ServerName = sni
 
-	return getter.WithTLSClientConfig(certPath, keyPath, caPath), nil
+	return tlsConf, nil
 }