From 5c4bb797aaa0cfd68ee8e5863afd09a2cdab91fe Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 30 May 2023 11:17:23 +0200 Subject: [PATCH] refactor TLSconfig funcs Signed-off-by: Soule BA --- docs/spec/v1beta2/helmrepositories.md | 2 +- internal/controller/helmchart_controller.go | 80 ++++++----- .../helmrepository_controller_oci.go | 134 +++++------------- internal/helm/registry/auth.go | 80 +++++++++++ internal/helm/registry/auth_test.go | 70 +++++++++ 5 files changed, 228 insertions(+), 138 deletions(-) diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index 630680fc2..bcba1e5c3 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -485,7 +485,7 @@ data: caFile: ``` -#### Provide TLS credentials in a secret of type kubernetes.io/dockerconfigjson +##### 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. diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index dd7618539..60fa9dcc2 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -510,8 +510,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * tlsConfig *tls.Config authenticator authn.Authenticator keychain authn.Keychain - tlsLoginOpt helmreg.LoginOption - tmpCertsDir string + secret *corev1.Secret ) // Used to login with the repository declared provider ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration) @@ -527,7 +526,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * helmgetter.WithTimeout(repo.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), } - if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil { + if secret, err = r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil { if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err), @@ -551,22 +550,6 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } clientOpts = append(clientOpts, opts...) tlsConfig = tlsCfg - tlsLoginOpt, tmpCertsDir, err = makeTLSLoginOption(secret) - if err != nil { - e := &serror.Event{ - Err: err, - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Requeue as content of secret might change - return sreconcile.ResultEmpty, e - } - 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) - } - }() // Build registryClient options from secret keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) @@ -669,8 +652,26 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // The OCIGetter will later retrieve the stored credentials to pull the chart if loginOpt != nil { opts := []helmreg.LoginOption{loginOpt} - if tlsLoginOpt != nil { - opts = append(opts, tlsLoginOpt) + if tlsConfig != nil && secret != nil { + tlsLoginOpt, tmpCertsDir, err := registry.TLSLoginOptionFromSecret(secret) + if err != nil { + e := &serror.Event{ + Err: err, + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + // Requeue as content of secret might change + return sreconcile.ResultEmpty, e + } + 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) + } + }() + if tlsLoginOpt != nil { + opts = append(opts, tlsLoginOpt) + } } err = ociChartRepo.Login(opts...) if err != nil { @@ -1048,8 +1049,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont return func(url string) (repo repository.Downloader, err error) { var ( tlsConfig *tls.Config - tlsLoginOpt helmreg.LoginOption - tmpCertsDir string + secret *corev1.Secret authenticator authn.Authenticator keychain authn.Keychain ) @@ -1081,7 +1081,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont helmgetter.WithTimeout(obj.Spec.Timeout.Duration), helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), } - if secret, err := r.getHelmRepositorySecret(ctx, obj); secret != nil || err != nil { + if secret, err = r.getHelmRepositorySecret(ctx, obj); secret != nil || err != nil { if err != nil { return nil, err } @@ -1093,19 +1093,6 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont } clientOpts = append(clientOpts, opts...) tlsConfig = tlsCfg - tlsLoginOpt, tmpCertsDir, err = makeTLSLoginOption(secret) - if err != nil { - return nil, err - } - defer func() { - var errs []error - if errf := os.RemoveAll(tmpCertsDir); errf != nil { - errs = append(errs, errf) - } - errs = append(errs, err) - err = kerrors.NewAggregate(errs) - return - }() // Build registryClient options from secret keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) @@ -1157,8 +1144,23 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont // The OCIGetter will later retrieve the stored credentials to pull the chart if loginOpt != nil { opts := []helmreg.LoginOption{loginOpt} - if tlsLoginOpt != nil { - opts = append(opts, tlsLoginOpt) + if tlsConfig != nil && secret != nil { + tlsLoginOpt, tmpCertsDir, err := registry.TLSLoginOptionFromSecret(secret) + if err != nil { + return nil, err + } + defer func() { + var errs []error + if errf := os.RemoveAll(tmpCertsDir); errf != nil { + errs = append(errs, errf) + } + errs = append(errs, err) + err = kerrors.NewAggregate(errs) + return + }() + if tlsLoginOpt != nil { + opts = append(opts, tlsLoginOpt) + } } err = ociChartRepo.Login(opts...) if err != nil { diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index 1325a4116..eadea6c7e 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -309,47 +309,29 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S authenticator authn.Authenticator keychain authn.Keychain tlsConfig *tls.Config - tmpCertsDir string - tlsLoginOpt helmreg.LoginOption + secret *corev1.Secret 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, 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) - } - }() } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI { auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { @@ -400,8 +382,22 @@ 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 tlsConfig != nil && secret != nil { + tlsLoginOpt, tmpCertsDir, err := registry.TLSLoginOptionFromSecret(secret) + 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) + } + }() + if tlsLoginOpt != nil { + opts = append(opts, tlsLoginOpt) + } } err = chartRepo.Login(opts...) if err != nil { @@ -429,6 +425,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 @@ -470,80 +482,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 { diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index d843d7d3b..46186dff3 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -20,6 +20,7 @@ import ( "bytes" "fmt" "net/url" + "os" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/credentials" @@ -27,6 +28,7 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/util/errors" ) // helper is a subset of the Docker credential helper credentials.Helper interface used by NewKeychainFromHelper. @@ -139,3 +141,81 @@ func (r stringResource) String() string { func (r stringResource) RegistryStr() string { return r.registry } + +// TLSLoginOptionFromSecret derives TLS authentication data from a Secret to login to an OCI registry. +// It creates temporary files for the TLS certificates and returns a LoginOption that can be used to +// configure the TLS client. The returned string is the path to the temporary directory that holds the +// certificates. It is the caller's responsibility to remove this directory when it is no longer needed. +func TLSLoginOptionFromSecret(secret *corev1.Secret) (registry.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 registry.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 +} diff --git a/internal/helm/registry/auth_test.go b/internal/helm/registry/auth_test.go index 14942a5bb..00de28c73 100644 --- a/internal/helm/registry/auth_test.go +++ b/internal/helm/registry/auth_test.go @@ -18,6 +18,7 @@ package registry import ( "net/url" + "os" "testing" "github.com/google/go-containerregistry/pkg/authn" @@ -191,3 +192,72 @@ func TestKeychainAdaptHelper(t *testing.T) { }) } } + +func TestTLSLoginOptionFromSecret(t *testing.T) { + defaultTLSSecret := corev1.Secret{ + Data: map[string][]byte{ + "certFile": []byte("-----BEGIN CERTIFICATE-----"), + "keyFile": []byte("key"), + "caFile": []byte("---BEGIN CERTIFICATE---"), + }, + } + tests := []struct { + name string + secret corev1.Secret + mutate func(*corev1.Secret) + wantErr bool + wantNil bool + }{ + { + "certFile, keyFile and caFile", + defaultTLSSecret, + nil, + false, + false}, + { + "without certFile", + defaultTLSSecret, + func(s *corev1.Secret) { delete(s.Data, "certFile") }, + true, + true}, + { + "without keyFile", + defaultTLSSecret, + func(s *corev1.Secret) { delete(s.Data, "keyFile") }, + true, + true}, + { + "without caFile", + defaultTLSSecret, + func(s *corev1.Secret) { delete(s.Data, "caFile") }, + false, + false}, + { + "empty", + corev1.Secret{}, + nil, + false, + true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + secret := tt.secret.DeepCopy() + if tt.mutate != nil { + tt.mutate(secret) + } + + got, tmpDir, err := TLSLoginOptionFromSecret(secret) + if tmpDir != "" { + defer os.RemoveAll(tmpDir) + } + if (err != nil) != tt.wantErr { + t.Errorf("TLSLoginOptionFromSecret() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantNil && got != nil { + t.Error("TLSLoginOptionFromSecret() got != nil, want nil") + return + } + }) + } +}