From 87aa433a5ddcb8cc98fa94f70d6ef97c3ab31a8a Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 18 Jul 2023 18:38:20 +0530 Subject: [PATCH] helm: add support for specifying TLS auth via `.spec.certSecretRef` Add support for specifying TLS auth data via `.spec.certSecretRef` in HelmRepository and emit a deprecation event if TLS is configured via `.spec.secretRef`. Introduce (and refactor) Helm client builder and auth helpers to reduce duplicated code and increase uniformity and testability. Signed-off-by: Sanskar Jaiswal --- internal/controller/helmchart_controller.go | 140 +++--------------- .../controller/helmchart_controller_test.go | 8 +- .../controller/helmrepository_controller.go | 64 ++------ .../helmrepository_controller_oci.go | 2 +- .../helmrepository_controller_test.go | 71 +++++---- .../controller/ocirepository_controller.go | 25 +--- internal/helm/registry/auth.go | 41 +++++ internal/helm/repository/client_opts.go | 126 ++++++++++++++++ internal/helm/repository/client_opts_test.go | 133 +++++++++++++++++ 9 files changed, 383 insertions(+), 227 deletions(-) create mode 100644 internal/helm/repository/client_opts.go create mode 100644 internal/helm/repository/client_opts_test.go diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 6095c60ed..92b957892 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -54,7 +54,6 @@ import ( eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/git" - "github.com/fluxcd/pkg/oci" "github.com/fluxcd/pkg/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" @@ -68,7 +67,6 @@ import ( serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" "github.com/fluxcd/source-controller/internal/helm/getter" - "github.com/fluxcd/source-controller/internal/helm/registry" "github.com/fluxcd/source-controller/internal/helm/repository" soci "github.com/fluxcd/source-controller/internal/oci" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" @@ -506,11 +504,6 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, sp *patch.Ser // object, and returns early. func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *helmv1.HelmChart, repo *helmv1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { - var ( - tlsConfig *tls.Config - authenticator authn.Authenticator - keychain authn.Keychain - ) // Used to login with the repository declared provider ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration) defer cancel() @@ -519,64 +512,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if err != nil { return chartRepoConfigErrorReturn(err, obj) } - // Construct the Getter options from the HelmRepository data - clientOpts := []helmgetter.Option{ - helmgetter.WithURL(normalizedURL), - helmgetter.WithTimeout(repo.Spec.Timeout.Duration), - helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials), - } - 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), - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Return error as the world as observed may change - return sreconcile.ResultEmpty, e - } - - // Build client options from secret - opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL) - 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 - } - clientOpts = append(clientOpts, opts...) - tlsConfig = tlsCfg - - // Build registryClient options from secret - keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) - if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to configure Helm client with secret data: %w", 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 - } - } else if repo.Spec.Provider != helmv1.GenericOCIProvider && repo.Spec.Type == helmv1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuth(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) - if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { - e := &serror.Event{ - Err: fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr), - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e - } - if auth != nil { - authenticator = auth - } - } - - loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL) + hcOpts, err := repository.GetHelmClientOpts(ctxTimeout, r.Client, repo, normalizedURL, true) if err != nil { e := &serror.Event{ Err: err, @@ -585,6 +521,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } + clientOpts := hcOpts.GetterOpts // Initialize the chart repository var chartRepo repository.Downloader @@ -599,7 +536,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(loginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(hcOpts.LoginOpt != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -621,7 +558,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * var verifiers []soci.Verifier if obj.Spec.Verify != nil { provider := obj.Spec.Verify.Provider - verifiers, err = r.makeVerifiers(ctx, obj, authenticator, keychain) + verifiers, err = r.makeVerifiers(ctx, obj, hcOpts.Authenticator, hcOpts.Keychain) if err != nil { if obj.Spec.Verify.SecretRef == nil { provider = fmt.Sprintf("%s keyless", provider) @@ -645,12 +582,11 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if err != nil { return chartRepoConfigErrorReturn(err, obj) } - chartRepo = ociChartRepo // 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 loginOpt != nil { - err = ociChartRepo.Login(loginOpt) + if hcOpts.LoginOpt != nil { + err = ociChartRepo.Login(hcOpts.LoginOpt) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to login to OCI registry: %w", err), @@ -660,8 +596,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } } + chartRepo = ociChartRepo default: - httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts...) + httpChartRepo, err := repository.NewChartRepository(normalizedURL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, hcOpts.TlsConfig, clientOpts...) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -1024,12 +961,6 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *helmv1.He // The callback returns an object with a state, so the caller has to do the necessary cleanup. func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback { return func(url string) (repository.Downloader, error) { - var ( - tlsConfig *tls.Config - authenticator authn.Authenticator - keychain authn.Keychain - ) - normalizedURL, err := repository.NormalizeURL(url) if err != nil { return nil, err @@ -1047,55 +978,26 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont }, } } + objKey := types.NamespacedName{ + Name: obj.Name, + Namespace: obj.Namespace, + } // Used to login with the repository declared provider ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - clientOpts := []helmgetter.Option{ - helmgetter.WithURL(normalizedURL), - helmgetter.WithTimeout(obj.Spec.Timeout.Duration), - helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), - } - if secret, err := r.getHelmRepositorySecret(ctx, obj); secret != nil || err != nil { - if err != nil { - return nil, err - } - - // Build client options from secret - opts, tlsCfg, err := r.clientOptionsFromSecret(secret, normalizedURL) - if err != nil { - return nil, err - } - clientOpts = append(clientOpts, opts...) - tlsConfig = tlsCfg - - // Build registryClient options from secret - keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) - if err != nil { - return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", obj.Name, 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) { - return nil, fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr) - } - if auth != nil { - authenticator = auth - } - } - - loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL) + hcOpts, err := repository.GetHelmClientOpts(ctxTimeout, r.Client, obj, normalizedURL, true) if err != nil { return nil, err } + clientOpts := hcOpts.GetterOpts var chartRepo repository.Downloader if helmreg.IsOCI(normalizedURL) { - registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(hcOpts.LoginOpt != nil) if err != nil { - return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", obj.Name, err) + return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", objKey.String(), err) } var errs []error @@ -1106,7 +1008,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont repository.WithOCIRegistryClient(registryClient), repository.WithCredentialsFile(credentialsFile)) if err != nil { - errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", obj.Name, err)) + errs = append(errs, fmt.Errorf("failed to create OCI chart repository for HelmRepository '%s': %w", objKey.String(), err)) // clean up the credentialsFile if credentialsFile != "" { if err := os.Remove(credentialsFile); err != nil { @@ -1118,10 +1020,10 @@ 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 loginOpt != nil { - err = ociChartRepo.Login(loginOpt) + if hcOpts.LoginOpt != nil { + err = ociChartRepo.Login(hcOpts.LoginOpt) if err != nil { - errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", obj.Name, err)) + errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", objKey.String(), err)) // clean up the credentialsFile errs = append(errs, ociChartRepo.Clear()) return nil, kerrors.NewAggregate(errs) @@ -1130,7 +1032,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont chartRepo = ociChartRepo } else { - httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, tlsConfig, clientOpts...) + httpChartRepo, err := repository.NewChartRepository(normalizedURL, "", r.Getters, hcOpts.TlsConfig, clientOpts...) if err != nil { return nil, err } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 7e94ac775..92c6e69e1 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -922,12 +922,12 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("failed to get secret 'invalid'")}, + wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")}, assertFunc: func(g *WithT, obj *helmv1.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 secret 'invalid'"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid'"), })) }, }, @@ -1190,12 +1190,12 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { } }, want: sreconcile.ResultEmpty, - wantErr: &serror.Event{Err: errors.New("failed to get secret 'invalid'")}, + wantErr: &serror.Event{Err: errors.New("failed to get authentication secret '/invalid'")}, assertFunc: func(g *WithT, obj *helmv1.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 secret 'invalid'"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid'"), })) }, }, diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index d5175fdf1..c8706ca27 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "crypto/tls" "errors" "fmt" "net/url" @@ -29,7 +28,6 @@ import ( helmgetter "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -51,7 +49,6 @@ import ( "github.com/fluxcd/source-controller/internal/cache" intdigest "github.com/fluxcd/source-controller/internal/digest" serror "github.com/fluxcd/source-controller/internal/error" - "github.com/fluxcd/source-controller/internal/helm/getter" "github.com/fluxcd/source-controller/internal/helm/repository" intpredicates "github.com/fluxcd/source-controller/internal/predicates" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" @@ -390,59 +387,22 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, sp *pat // pointer is set to the newly fetched index. func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch.SerialPatcher, obj *helmv1.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), - helmgetter.WithURL(obj.Spec.URL), - helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), + hcOpts, err := repository.GetHelmClientOpts(ctx, r.Client, obj, obj.Spec.URL, false) + if err != nil { + e := serror.NewGeneric( + err, + sourcev1.AuthenticationFailedReason, + ) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e } - - // 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 { - e := &serror.Event{ - Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err), - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e - } - - // Construct actual options - opts, err := getter.ClientOptionsFromSecret(secret) - if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Return err as the content of the secret may change. - 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, e.Reason, e.Err.Error()) - // Requeue as content of secret might change - return sreconcile.ResultEmpty, e - } + if hcOpts.EmitDeprecationWarning { + r.Event(obj, "Warning", "DeprecationWarning", + "specifying TLS authentication data via `.spec.secretRef` is deprecated, please use `.spec.certSecretRef` instead") } // Construct Helm chart repository with options and download index - newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, tlsConfig, clientOpts...) + newChartRepo, err := repository.NewChartRepository(obj.Spec.URL, "", r.Getters, hcOpts.TlsConfig, hcOpts.GetterOpts...) if err != nil { switch err.(type) { case *url.Error: diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index 2af060f30..a90f8b35e 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -318,7 +318,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S return } } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) + auth, authErr := registry.OIDCAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { e := fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr) conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index d6f56920c..e45f2400c 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -388,7 +388,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "HTTPS with secretRef pointing to CA cert but public repo URL succeeds", + name: "HTTPS with certSecretRef pointing to CA cert but public repo URL succeeds", protocol: "http", url: "https://stefanprodan.github.io/podinfo", want: sreconcile.ResultSuccess, @@ -400,6 +400,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { "caFile": tlsCA, }, }, + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { + obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "ca-file"} + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), @@ -450,7 +453,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, { - name: "HTTPS with CAFile secret makes ArtifactOutdated=True", + name: "HTTPS with invalid CAFile in certSecretRef makes FetchFailed=True and returns error", protocol: "https", server: options{ publicKey: tlsPublicKey, @@ -459,28 +462,32 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "ca-file", + Name: "invalid-ca", }, Data: map[string][]byte{ - "caFile": tlsCA, + "caFile": []byte("invalid"), }, }, beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { - obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} + obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} + conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") + conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, - want: sreconcile.ResultSuccess, + wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), - *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), + *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { - t.Expect(chartRepo.Path).ToNot(BeEmpty()) - t.Expect(chartRepo.Index).ToNot(BeNil()) - t.Expect(artifact.Revision).ToNot(BeEmpty()) + // No repo index due to fetch fail. + t.Expect(chartRepo.Path).To(BeEmpty()) + t.Expect(chartRepo.Index).To(BeNil()) + t.Expect(artifact.Revision).To(BeEmpty()) }, }, { - name: "HTTPS with invalid CAFile secret makes FetchFailed=True and returns error", + name: "HTTPS with CAFile in secretRef makes ArtifactOutdated=True and emits deprecation event", protocol: "https", server: options{ publicKey: tlsPublicKey, @@ -489,28 +496,24 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "invalid-ca", + Name: "ca-file", }, Data: map[string][]byte{ - "caFile": []byte("invalid"), + "caFile": tlsCA, }, }, beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { - obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} - conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") - conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} }, - wantErr: true, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to create TLS client config with secret data: cannot append certificate into certificate pool: invalid caFile"), - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), - *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), }, afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { - // No repo index due to fetch fail. - t.Expect(chartRepo.Path).To(BeEmpty()) - t.Expect(chartRepo.Index).To(BeNil()) - t.Expect(artifact.Revision).To(BeEmpty()) + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) }, }, { @@ -807,9 +810,6 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { g.Expect(newChartRepo.LoadFromPath()).To(Succeed()) rev = newChartRepo.Digest(intdigest.Canonical) } - if tt.beforeFunc != nil { - tt.beforeFunc(g, obj, rev, dig) - } r := &HelmRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), @@ -818,6 +818,9 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Getters: testGetters, patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), } + if tt.beforeFunc != nil { + tt.beforeFunc(g, obj, rev, dig) + } g.Expect(r.Client.Create(context.TODO(), obj)).ToNot(HaveOccurred()) defer func() { @@ -838,6 +841,18 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if tt.afterFunc != nil { tt.afterFunc(g, obj, artifact, &chartRepo) } + // If TLS auth data was specified using secretRef and not certSecretRef then assert + // that we emit a deprecation event. + if tt.secret != nil { + if _, ok := tt.secret.Data["caFile"]; ok && obj.Spec.CertSecretRef == nil && obj.Spec.SecretRef != nil { + events := r.EventRecorder.(*record.FakeRecorder).Events + event := <-events + g.Expect(event).To(ContainSubstring("DeprecationWarning")) + g.Expect(event).To(ContainSubstring( + "specifying TLS authentication data via `.spec.secretRef` is deprecated, please use `.spec.certSecretRef` instead", + )) + } + } // In-progress status condition validity. checker := conditionscheck.NewInProgressChecker(r.Client) diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 9ab36c748..aef39c759 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -55,7 +55,6 @@ import ( eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/oci" - "github.com/fluxcd/pkg/oci/auth/login" "github.com/fluxcd/pkg/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" @@ -68,6 +67,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1" ociv1 "github.com/fluxcd/source-controller/api/v1beta2" serror "github.com/fluxcd/source-controller/internal/error" + "github.com/fluxcd/source-controller/internal/helm/registry" soci "github.com/fluxcd/source-controller/internal/oci" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" @@ -345,7 +345,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch if _, ok := keychain.(soci.Anonymous); obj.Spec.Provider != ociv1.GenericOCIProvider && ok { var authErr error - auth, authErr = oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) + auth, authErr = registry.OIDCAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { e := serror.NewGeneric( fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr), @@ -870,27 +870,6 @@ func (r *OCIRepositoryReconciler) transport(ctx context.Context, obj *ociv1.OCIR return transport, nil } -// oidcAuth generates the OIDC credential authenticator based on the specified cloud provider. -func oidcAuth(ctx context.Context, url, provider string) (authn.Authenticator, error) { - u := strings.TrimPrefix(url, ociv1.OCIRepositoryPrefix) - ref, err := name.ParseReference(u) - if err != nil { - return nil, fmt.Errorf("failed to parse URL '%s': %w", u, err) - } - - opts := login.ProviderOptions{} - switch provider { - case ociv1.AmazonOCIProvider: - opts.AwsAutoLogin = true - case ociv1.AzureOCIProvider: - opts.AzureAutoLogin = true - case ociv1.GoogleOCIProvider: - opts.GcpAutoLogin = true - } - - return login.NewManager().Login(ctx, u, ref, opts) -} - // reconcileStorage ensures the current state of the storage matches the // desired and previously observed state. // diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index d843d7d3b..00d207473 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -18,14 +18,20 @@ package registry import ( "bytes" + "context" "fmt" "net/url" + "strings" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/credentials" + "github.com/fluxcd/pkg/oci/auth/login" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" ) @@ -139,3 +145,38 @@ func (r stringResource) String() string { func (r stringResource) RegistryStr() string { return r.registry } + +// OIDCAuth generates the OIDC credential authenticator based on the specified cloud provider. +func OIDCAuth(ctx context.Context, url, provider string) (authn.Authenticator, error) { + u := strings.TrimPrefix(url, sourcev1.OCIRepositoryPrefix) + ref, err := name.ParseReference(u) + if err != nil { + return nil, fmt.Errorf("failed to parse URL '%s': %w", u, err) + } + + opts := login.ProviderOptions{} + switch provider { + case sourcev1.AmazonOCIProvider: + opts.AwsAutoLogin = true + case sourcev1.AzureOCIProvider: + opts.AzureAutoLogin = true + case sourcev1.GoogleOCIProvider: + opts.GcpAutoLogin = true + } + + return login.NewManager().Login(ctx, u, ref, opts) +} + +// MakeLoginOption returns a registry login option for the given HelmRepository. +// If the HelmRepository does not specify a secretRef, a nil login option is returned. +func MakeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registryURL string) (helmreg.LoginOption, error) { + if auth != nil { + return AuthAdaptHelper(auth) + } + + if keychain != nil { + return KeychainAdaptHelper(keychain)(registryURL) + } + + return nil, nil +} diff --git a/internal/helm/repository/client_opts.go b/internal/helm/repository/client_opts.go new file mode 100644 index 000000000..e188d3e0d --- /dev/null +++ b/internal/helm/repository/client_opts.go @@ -0,0 +1,126 @@ +package repository + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + + "github.com/fluxcd/pkg/oci" + "github.com/google/go-containerregistry/pkg/authn" + helmgetter "helm.sh/helm/v3/pkg/getter" + helmreg "helm.sh/helm/v3/pkg/registry" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + helmv1 "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/fluxcd/source-controller/internal/helm/getter" + "github.com/fluxcd/source-controller/internal/helm/registry" +) + +// HelmClientOpts contains the various options to use while constructing +// a Helm client. +type HelmClientOpts struct { + Authenticator authn.Authenticator + Keychain authn.Keychain + LoginOpt helmreg.LoginOption + TlsConfig *tls.Config + GetterOpts []helmgetter.Option + EmitDeprecationWarning bool +} + +// GetHelmClientOpts uses the provided HelmRepository object and a normalized URL +// to construct a HelmClientOpts object. ociAuth, if set to true will result in +// the configuration of the necessary OCI auth mechanisms required to +// authenticate against the registry. +func GetHelmClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmRepository, url string, ociAuth bool) (*HelmClientOpts, error) { + objKey := types.NamespacedName{ + Name: obj.Name, + Namespace: obj.Namespace, + } + + hrOpts := &HelmClientOpts{ + GetterOpts: []helmgetter.Option{ + helmgetter.WithURL(url), + helmgetter.WithTimeout(obj.Spec.Timeout.Duration), + helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), + }, + } + + var certSecret *corev1.Secret + var err error + // Check `.spec.certSecretRef` first for any TLS auth data. + if obj.Spec.CertSecretRef != nil { + certSecret, err = fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) + } + + hrOpts.TlsConfig, err = getter.TLSClientConfigFromSecret(*certSecret, url) + if err != nil { + return nil, fmt.Errorf("failed to construct Helm client's TLS config for HelmRepository '%s': %w", objKey.String(), err) + } + } + + var authSecret *corev1.Secret + if obj.Spec.SecretRef != nil { + authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) + } + + // Construct actual Helm client options. + opts, err := getter.ClientOptionsFromSecret(*authSecret) + if err != nil { + return nil, fmt.Errorf("failed to configure Helm client for HelmRepository '%s': %w", objKey.String(), err) + } + hrOpts.GetterOpts = append(hrOpts.GetterOpts, opts...) + + // If the TLS config is nil, i.e. one couldn't be constructed using `.spec.certSecretRef` + // then try to use `.spec.certSecretRef`. + if hrOpts.TlsConfig == nil { + hrOpts.TlsConfig, err = getter.TLSClientConfigFromSecret(*authSecret, url) + if err != nil { + return nil, fmt.Errorf("failed to construct Helm client's TLS config for HelmRepository '%s': %w", objKey.String(), err) + } + hrOpts.EmitDeprecationWarning = true + } + + if ociAuth { + hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret) + if err != nil { + return nil, fmt.Errorf("failed to configure login options for HelmRepository '%s': %w", objKey.String(), err) + } + } + } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI && ociAuth { + authenticator, authErr := registry.OIDCAuth(ctx, obj.Spec.URL, obj.Spec.Provider) + if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { + return nil, fmt.Errorf("failed to get credential from '%s' for HelmRepository '%s': %w", obj.Spec.Provider, objKey.String(), authErr) + } + if authenticator != nil { + hrOpts.Authenticator = authenticator + } + } + + if ociAuth { + hrOpts.LoginOpt, err = registry.MakeLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) + if err != nil { + return nil, err + } + } + + return hrOpts, nil +} + +func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) { + key := types.NamespacedName{ + Namespace: namespace, + Name: name, + } + var secret corev1.Secret + if err := c.Get(ctx, key, &secret); err != nil { + return nil, err + } + return &secret, nil +} diff --git a/internal/helm/repository/client_opts_test.go b/internal/helm/repository/client_opts_test.go new file mode 100644 index 000000000..e069b1c68 --- /dev/null +++ b/internal/helm/repository/client_opts_test.go @@ -0,0 +1,133 @@ +package repository + +import ( + "context" + "os" + "testing" + "time" + + "github.com/fluxcd/pkg/apis/meta" + "github.com/google/go-containerregistry/pkg/name" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + + helmv1 "github.com/fluxcd/source-controller/api/v1beta2" +) + +func TestGetHelmClientOpts(t *testing.T) { + tlsCA, err := os.ReadFile("../../controller/testdata/certs/ca.pem") + if err != nil { + t.Errorf("could not read CA file: %s", err) + } + + tests := []struct { + name string + certSecret *corev1.Secret + authSecret *corev1.Secret + afterFunc func(t *WithT, hrOpts *HelmClientOpts) + ociAuth bool + }{ + { + name: "HelmRepository with certSecretRef discards TLS config in secretRef", + certSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + }, + }, + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + "caFile": []byte("invalid"), + }, + }, + afterFunc: func(t *WithT, hrOpts *HelmClientOpts) { + t.Expect(hrOpts.TlsConfig).ToNot(BeNil()) + t.Expect(hrOpts.EmitDeprecationWarning).To(BeFalse()) + t.Expect(len(hrOpts.GetterOpts)).To(Equal(4)) + }, + }, + { + name: "HelmRepository with TLS config only in secretRef is marked as deprecated", + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-tls", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + "caFile": tlsCA, + }, + }, + afterFunc: func(t *WithT, hrOpts *HelmClientOpts) { + t.Expect(hrOpts.TlsConfig).ToNot(BeNil()) + t.Expect(hrOpts.EmitDeprecationWarning).To(BeTrue()) + t.Expect(len(hrOpts.GetterOpts)).To(Equal(4)) + }, + }, + { + name: "OCI HelmRepository with secretRef has auth configured", + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + afterFunc: func(t *WithT, hrOpts *HelmClientOpts) { + repo, err := name.NewRepository("ghcr.io/dummy") + t.Expect(err).ToNot(HaveOccurred()) + authenticator, err := hrOpts.Keychain.Resolve(repo) + t.Expect(err).ToNot(HaveOccurred()) + config, err := authenticator.Authorization() + t.Expect(err).ToNot(HaveOccurred()) + t.Expect(config.Username).To(Equal("user")) + t.Expect(config.Password).To(Equal("pass")) + }, + ociAuth: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + helmRepo := &helmv1.HelmRepository{ + Spec: helmv1.HelmRepositorySpec{ + Timeout: &metav1.Duration{ + Duration: time.Second, + }, + }, + } + + clientBuilder := fakeclient.NewClientBuilder() + if tt.authSecret != nil { + clientBuilder.WithObjects(tt.authSecret.DeepCopy()) + helmRepo.Spec.SecretRef = &meta.LocalObjectReference{ + Name: tt.authSecret.Name, + } + } + if tt.certSecret != nil { + clientBuilder.WithObjects(tt.certSecret.DeepCopy()) + helmRepo.Spec.CertSecretRef = &meta.LocalObjectReference{ + Name: tt.certSecret.Name, + } + } + c := clientBuilder.Build() + + hcOpts, err := GetHelmClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy", tt.ociAuth) + g.Expect(err).ToNot(HaveOccurred()) + tt.afterFunc(g, hcOpts) + }) + } +}