diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 19d320ecf..776766af7 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -523,7 +523,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return chartRepoConfigErrorReturn(err, obj) } - clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) + clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { e := serror.NewGeneric( err, @@ -532,9 +532,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) return sreconcile.ResultEmpty, e } - if certsTmpDir != "" { + if clientOpts.CertsTempDir != "" { defer func() { - if err := os.RemoveAll(certsTmpDir); err != nil { + if err := os.RemoveAll(clientOpts.CertsTempDir); err != nil { r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, "failed to delete temporary certificates directory: %s", err) } @@ -1018,7 +1018,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont ctxTimeout, cancel := context.WithTimeout(ctx, obj.GetTimeout()) defer cancel() - clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) + clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { return nil, err } @@ -1037,7 +1037,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.WithCertificatesStore(clientOpts.CertsTempDir), repository.WithCredentialsFile(credentialsFile)) if err != nil { errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err)) diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index cc1dac285..8bfa91657 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -1035,12 +1035,12 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret: secrets \"invalid\" not found")}, + wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid': secrets \"invalid\" not found")}, assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret: secrets \"invalid\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid': secrets \"invalid\" not found"), })) }, }, @@ -1304,12 +1304,12 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret: secrets \"invalid\" not found")}, + wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid': secrets \"invalid\" not found")}, assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) { g.Expect(build.Complete()).To(BeFalse()) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret: secrets \"invalid\" not found"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid': secrets \"invalid\" not found"), })) }, }, diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 2806f0c40..6bd701f1f 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -415,7 +415,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). diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index cbcd09d9d..23d5ff0bb 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -22,7 +22,7 @@ import ( "errors" "fmt" "os" - "path" + "path/filepath" "github.com/google/go-containerregistry/pkg/authn" helmgetter "helm.sh/helm/v3/pkg/getter" @@ -46,6 +46,9 @@ const ( var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") +// option applies configuration to ClientOpts. +type option func(*ClientOpts) error + // ClientOpts contains the various options to use while constructing // a Helm repository client. type ClientOpts struct { @@ -55,25 +58,26 @@ type ClientOpts struct { TlsConfig *tls.Config GetterOpts []helmgetter.Option Insecure bool + CertsTempDir string } // MustLoginToRegistry returns true if the client options contain at least -// one registry login option. +// one registry login option. This indicates that registry authentication +// has been configured, regardless of the specific login option type. func (o ClientOpts) MustLoginToRegistry() bool { - return len(o.RegLoginOpts) > 0 && o.RegLoginOpts[0] != nil + return len(o.RegLoginOpts) > 0 } // GetClientOpts uses the provided HelmRepository object and a normalized // URL to construct a HelmClientOpts object. If obj is an OCI HelmRepository, // then the returned options object will also contain the required registry // auth mechanisms. -// A temporary directory is created to store the certs files if needed and its path is returned along with the options object. It is the -// caller's responsibility to clean up the directory. -func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string) (*ClientOpts, string, error) { - // This function configures authentication for Helm repositories based on the provided secrets: - // - CertSecretRef: TLS client certificates (always takes priority) - // - SecretRef: Can contain Basic Auth or TLS certificates (deprecated) - // For OCI repositories, additional registry-specific authentication is configured (including Docker config) +// A temporary directory is created to store the certs files if needed and its path is available via ClientOpts.CertsTempDir. +// It is the caller's responsibility to clean up the directory. +func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string) (*ClientOpts, error) { + var certSecret, authSecret *corev1.Secret + var err error + opts := &ClientOpts{ GetterOpts: []helmgetter.Option{ helmgetter.WithURL(url), @@ -83,138 +87,135 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepos Insecure: obj.Spec.Insecure, } - // Process secrets and configure authentication - deprecatedTLS, certSecret, authSecret, err := configureAuthentication(ctx, c, obj, opts, url) - if err != nil { - return nil, "", err - } - - // Setup OCI registry specific configurations if needed - var tempCertDir string - if obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI { - tempCertDir, err = configureOCIRegistryWithSecrets(ctx, obj, opts, url, certSecret, authSecret) + if obj.Spec.CertSecretRef != nil { + certSecret, err = fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) if err != nil { - return nil, "", err + return nil, fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) + } + certOpt := withAuthFromCertSecret(ctx, certSecret, url, obj.Spec.Insecure) + if err := certOpt(opts); err != nil { + return nil, err } } var deprecatedErr error - if deprecatedTLS { - deprecatedErr = ErrDeprecatedTLSConfig - } - - return opts, tempCertDir, deprecatedErr -} - -// configureAuthentication processes all secret references and sets up authentication. -// Returns (deprecatedTLS, certSecret, authSecret, error) where: -// - deprecatedTLS: true if TLS config comes from SecretRef (deprecated pattern) -// - certSecret: the secret from CertSecretRef (nil if not specified) -// - authSecret: the secret from SecretRef (nil if not specified) -func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, opts *ClientOpts, url string) (bool, *corev1.Secret, *corev1.Secret, error) { - var deprecatedTLS bool - var certSecret, authSecret *corev1.Secret - - if obj.Spec.CertSecretRef != nil { - secret, err := fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) + if obj.Spec.SecretRef != nil { + authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) if err != nil { - return false, nil, nil, fmt.Errorf("failed to get TLS authentication secret: %w", err) + return nil, fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) } - certSecret = secret - tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, obj.Spec.URL, obj.Spec.Insecure) - if err != nil { - return false, nil, nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + secretRefOpt := withAuthFromSecret(ctx, authSecret, url, obj.Spec.Insecure) + if err := secretRefOpt(opts); err != nil { + if errors.Is(err, ErrDeprecatedTLSConfig) { + deprecatedErr = err + } else { + return nil, err + } } - opts.TlsConfig = tlsConfig } - // Extract all authentication methods from SecretRef. - // This secret may contain multiple auth types (Basic Auth, TLS). - if obj.Spec.SecretRef != nil { - secret, err := fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) + if obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI { + ociOpt := withOCIRegistry(ctx, obj, url, certSecret, authSecret) + if err := ociOpt(opts); err != nil { + return nil, err + } + } + + return opts, deprecatedErr +} + +// withAuthFromCertSecret applies TLS config from a pre-fetched secret. +func withAuthFromCertSecret(ctx context.Context, secret *corev1.Secret, url string, insecure bool) option { + return func(o *ClientOpts) error { + tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, url, insecure) if err != nil { - return false, nil, nil, fmt.Errorf("failed to get authentication secret: %w", err) + return fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } - authSecret = secret + o.TlsConfig = tlsConfig + return nil + } +} - methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(obj.Spec.URL, obj.Spec.Insecure)) +// withAuthFromSecret applies BasicAuth or TLS from a pre-fetched secret. +// Returns ErrDeprecatedTLSConfig if TLS config comes from SecretRef (deprecated pattern). +func withAuthFromSecret(ctx context.Context, secret *corev1.Secret, url string, insecure bool) option { + return func(o *ClientOpts) error { + methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(url, insecure)) if err != nil { - return false, nil, nil, fmt.Errorf("failed to detect authentication methods: %w", err) + return fmt.Errorf("failed to detect authentication methods: %w", err) } if methods.HasBasicAuth() { - opts.GetterOpts = append(opts.GetterOpts, + o.GetterOpts = append(o.GetterOpts, helmgetter.WithBasicAuth(methods.Basic.Username, methods.Basic.Password)) } - // Use TLS from SecretRef only if CertSecretRef is not specified (CertSecretRef takes priority) - if opts.TlsConfig == nil && methods.HasTLS() { - opts.TlsConfig = methods.TLS - deprecatedTLS = true + if o.TlsConfig == nil && methods.HasTLS() { + o.TlsConfig = methods.TLS + return ErrDeprecatedTLSConfig } + return nil } - - return deprecatedTLS, certSecret, authSecret, nil } -// configureOCIRegistryWithSecrets sets up OCI-specific configurations using pre-fetched secrets -func configureOCIRegistryWithSecrets(ctx context.Context, obj *sourcev1.HelmRepository, opts *ClientOpts, url string, certSecret, authSecret *corev1.Secret) (string, error) { - // Configure OCI authentication from authSecret if available - if authSecret != nil { - keychain, err := registry.LoginOptionFromSecret(url, *authSecret) - if err != nil { - return "", fmt.Errorf("failed to configure login options: %w", err) +// withOCIRegistry applies OCI-specific login and TLS file handling. +func withOCIRegistry(ctx context.Context, obj *sourcev1.HelmRepository, url string, certSecret, authSecret *corev1.Secret) option { + return func(o *ClientOpts) error { + if authSecret != nil { + keychain, err := registry.LoginOptionFromSecret(url, *authSecret) + if err != nil { + return fmt.Errorf("failed to configure login options: %w", err) + } + o.Keychain = keychain } - opts.Keychain = keychain - } - // Handle OCI provider authentication if no SecretRef - if obj.Spec.SecretRef == nil && obj.Spec.Provider != "" && obj.Spec.Provider != sourcev1.GenericOCIProvider { - authenticator, err := soci.OIDCAuth(ctx, url, obj.Spec.Provider) - if err != nil { - return "", fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, err) + if obj.Spec.SecretRef == nil && obj.Spec.Provider != "" && obj.Spec.Provider != sourcev1.GenericOCIProvider { + authenticator, err := soci.OIDCAuth(ctx, url, obj.Spec.Provider) + if err != nil { + return fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, err) + } + o.Authenticator = authenticator } - opts.Authenticator = authenticator - } - - // Setup registry login options - loginOpt, err := registry.NewLoginOption(opts.Authenticator, opts.Keychain, url) - if err != nil { - return "", err - } - - if loginOpt != nil { - opts.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)} - } - // Handle TLS certificate files for OCI - var tempCertDir string - if opts.TlsConfig != nil { - tempCertDir, err = os.MkdirTemp("", "helm-repo-oci-certs") + loginOpt, err := registry.NewLoginOption(o.Authenticator, o.Keychain, url) if err != nil { - return "", fmt.Errorf("cannot create temporary directory: %w", err) + return err } - var tlsSecret *corev1.Secret - if certSecret != nil { - tlsSecret = certSecret - } else if authSecret != nil { - tlsSecret = authSecret + if loginOpt != nil { + // NOTE: RegLoginOpts is rebuilt here intentionally (overwrites any existing entries). + o.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)} } - certFile, keyFile, caFile, err := storeTLSCertificateFilesForOCI(ctx, tlsSecret, nil, tempCertDir) - if err != nil { - return "", fmt.Errorf("cannot write certs files to path: %w", err) - } + if o.TlsConfig != nil { + tempCertDir, err := os.MkdirTemp("", "helm-repo-oci-certs") + if err != nil { + return fmt.Errorf("cannot create temporary directory: %w", err) + } + + var tlsSecret *corev1.Secret + if certSecret != nil { + tlsSecret = certSecret + } else if authSecret != nil { + tlsSecret = authSecret + } - tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) - if tlsLoginOpt != nil { - opts.RegLoginOpts = append(opts.RegLoginOpts, tlsLoginOpt) + certFile, keyFile, caFile, err := storeTLSCertificateFilesForOCI(tlsSecret, nil, tempCertDir) + if err != nil { + return fmt.Errorf("cannot write certs files to path: %w", err) + } + + tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) + if tlsLoginOpt != nil { + o.RegLoginOpts = append(o.RegLoginOpts, tlsLoginOpt) + } + + o.CertsTempDir = tempCertDir } - } - return tempCertDir, nil + return nil + } } func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) { @@ -233,7 +234,7 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) ( // Helm OCI registry client requires certificate file paths rather than in-memory data, // so we need to temporarily write the certificate data to disk. // Returns paths to the written cert, key, and CA files (any of which may be empty if not present). -func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret *corev1.Secret, path string) (string, string, string, error) { +func storeTLSCertificateFilesForOCI(certSecret, authSecret *corev1.Secret, dir string) (string, string, string, error) { var ( certFile string keyFile string @@ -241,7 +242,6 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret err error ) - // Try to get TLS data from certSecret first, then authSecret var tlsSecret *corev1.Secret if certSecret != nil { tlsSecret = certSecret @@ -252,11 +252,11 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret if tlsSecret != nil { if certData, exists := tlsSecret.Data[secrets.KeyTLSCert]; exists { if keyData, keyExists := tlsSecret.Data[secrets.KeyTLSPrivateKey]; keyExists { - certFile, err = writeToFile(certData, certFileName, path) + certFile, err = writeToFile(certData, certFileName, dir) if err != nil { return "", "", "", err } - keyFile, err = writeToFile(keyData, keyFileName, path) + keyFile, err = writeToFile(keyData, keyFileName, dir) if err != nil { return "", "", "", err } @@ -264,7 +264,7 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret } if caData, exists := tlsSecret.Data[secrets.KeyCACert]; exists { - caFile, err = writeToFile(caData, caFileName, path) + caFile, err = writeToFile(caData, caFileName, dir) if err != nil { return "", "", "", err } @@ -275,7 +275,7 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret } func writeToFile(data []byte, filename, tmpDir string) (string, error) { - file := path.Join(tmpDir, filename) + file := filepath.Join(tmpDir, filename) err := os.WriteFile(file, data, 0o600) if err != nil { return "", err diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index bf40e7f86..c89375847 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -164,7 +164,7 @@ func TestGetClientOpts(t *testing.T) { } c := clientBuilder.Build() - clientOpts, _, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") if tt.err != nil { g.Expect(err).To(Equal(tt.err)) } else { @@ -271,7 +271,7 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { } c := clientBuilder.Build() - clientOpts, tmpDir, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") if tt.wantErrMsg != "" { if err == nil { t.Errorf("GetClientOpts() expected error but got none") @@ -287,8 +287,8 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { t.Errorf("GetClientOpts() error = %v", err) return } - if tmpDir != "" { - defer os.RemoveAll(tmpDir) + if clientOpts.CertsTempDir != "" { + defer os.RemoveAll(clientOpts.CertsTempDir) } if tt.loginOptsN != len(clientOpts.RegLoginOpts) { // we should have a login option but no TLS option