Skip to content

Commit

Permalink
Reuse transport for helm chart download
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
Paulo Gomes committed Feb 28, 2022
1 parent df86d44 commit 260637b
Show file tree
Hide file tree
Showing 11 changed files with 287 additions and 115 deletions.
46 changes: 23 additions & 23 deletions controllers/helmchart_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"crypto/tls"
"errors"
"fmt"
"net/url"
Expand Down Expand Up @@ -368,6 +369,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{
Expand All @@ -386,34 +388,33 @@ 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...)
}

// Initialize the chart repository
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, clientOpts)
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts)
if err != nil {
// Any error requires a change in generation,
// which we should be informed about by the watcher
Expand Down Expand Up @@ -523,15 +524,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()

Expand Down Expand Up @@ -747,11 +741,11 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
}

// namespacedChartRepositoryCallback returns a chart.GetChartRepositoryCallback scoped to the given namespace.
// 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
Expand All @@ -774,13 +768,19 @@ 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)
}
}
chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, clientOpts)

chartRepo, err := repository.NewChartRepository(repo.Spec.URL, "", r.Getters, tlsConfig, clientOpts)
if err != nil {
return nil, err
}
Expand Down
28 changes: 16 additions & 12 deletions controllers/helmrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"crypto/tls"
"errors"
"fmt"
"net/url"
Expand Down Expand Up @@ -260,6 +261,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),
Expand All @@ -284,18 +287,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),
Expand All @@ -306,10 +299,21 @@ 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
}
}

// Construct Helm chart repository with options and download index
newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, clientOpts)
newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, tlsConfig, clientOpts)
if err != nil {
switch err.(type) {
case *url.Error:
Expand Down
4 changes: 2 additions & 2 deletions controllers/helmrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
},
},
{
Expand Down Expand Up @@ -600,7 +600,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(cacheFile.Close()).ToNot(HaveOccurred())

chartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil)
chartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil, nil)
g.Expect(err).ToNot(HaveOccurred())
chartRepo.CachePath = cachePath

Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1759,6 +1759,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=
Expand Down
71 changes: 23 additions & 48 deletions internal/helm/getter/getter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand All @@ -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:
Expand All @@ -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
}
Loading

0 comments on commit 260637b

Please sign in to comment.