From 9986d9918eb4eeaed81024f0e1cf1379879a1ea6 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 18 Jul 2023 18:36:13 +0530 Subject: [PATCH 1/3] helmrepo: add `.spec.certSecretRef` for specifying TLS auth data Add `.spec.certSecretRef` to HelmRepository for specifying TLS auth data in a secret using the `certFile`, `caFile` and `keyFile` keys. Mark support for these keys in the secret specified in `.spec.secretRef` as deprecated. Signed-off-by: Sanskar Jaiswal --- api/v1beta2/helmrepository_types.go | 11 ++++- api/v1beta2/zz_generated.deepcopy.go | 5 +++ ...ce.toolkit.fluxcd.io_helmrepositories.yaml | 17 +++++++- docs/api/v1beta2/source.md | 42 +++++++++++++++++-- 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/api/v1beta2/helmrepository_types.go b/api/v1beta2/helmrepository_types.go index 44b036a2b..4dcf0a454 100644 --- a/api/v1beta2/helmrepository_types.go +++ b/api/v1beta2/helmrepository_types.go @@ -51,11 +51,18 @@ type HelmRepositorySpec struct { // for the HelmRepository. // For HTTP/S basic auth the secret must contain 'username' and 'password' // fields. - // For TLS the secret must contain a 'certFile' and 'keyFile', and/or - // 'caFile' fields. + // Support for TLS auth using the 'certFile' and 'keyFile', and/or 'caFile' + // keys is deprecated. Please use `.spec.certSecretRef` instead. // +optional SecretRef *meta.LocalObjectReference `json:"secretRef,omitempty"` + // CertSecretRef specifies the Secret containing the TLS authentication + // data. The secret must contain a 'certFile' and 'keyFile', and/or 'caFile' + // fields. It takes precedence over the values specified in the Secret + // referred to by `.spec.secretRef`. + // +optional + CertSecretRef *meta.LocalObjectReference `json:"certSecretRef,omitempty"` + // PassCredentials allows the credentials from the SecretRef to be passed // on to a host that does not match the host as defined in URL. // This may be required if the host of the advertised chart URLs in the diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 3a0850fd9..5c2169a33 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -577,6 +577,11 @@ func (in *HelmRepositorySpec) DeepCopyInto(out *HelmRepositorySpec) { *out = new(meta.LocalObjectReference) **out = **in } + if in.CertSecretRef != nil { + in, out := &in.CertSecretRef, &out.CertSecretRef + *out = new(meta.LocalObjectReference) + **out = **in + } out.Interval = in.Interval if in.Timeout != nil { in, out := &in.Timeout, &out.Timeout diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml index 8cf269ecb..8af5734be 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml @@ -296,6 +296,18 @@ spec: required: - namespaceSelectors type: object + certSecretRef: + description: CertSecretRef specifies the Secret containing the TLS + authentication data. The secret must contain a 'certFile' and 'keyFile', + and/or 'caFile' fields. It takes precedence over the values specified + in the Secret referred to by `.spec.secretRef`. + properties: + name: + description: Name of the referent. + type: string + required: + - name + type: object interval: description: Interval at which to check the URL for updates. pattern: ^([0-9]+(\.[0-9]+)?(ms|s|m|h))+$ @@ -323,8 +335,9 @@ spec: secretRef: description: SecretRef specifies the Secret containing authentication credentials for the HelmRepository. For HTTP/S basic auth the secret - must contain 'username' and 'password' fields. For TLS the secret - must contain a 'certFile' and 'keyFile', and/or 'caFile' fields. + must contain 'username' and 'password' fields. Support for TLS auth + using the 'certFile' and 'keyFile', and/or 'caFile' keys is deprecated. + Please use `.spec.certSecretRef` instead. properties: name: description: Name of the referent. diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index 35701f254..373e34e60 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -792,8 +792,25 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference for the HelmRepository. For HTTP/S basic auth the secret must contain ‘username’ and ‘password’ fields. -For TLS the secret must contain a ‘certFile’ and ‘keyFile’, and/or -‘caFile’ fields.

+Support for TLS auth using the ‘certFile’ and ‘keyFile’, and/or ‘caFile’ +keys is deprecated. Please use .spec.certSecretRef instead.

+ + + + +certSecretRef
+ + +github.com/fluxcd/pkg/apis/meta.LocalObjectReference + + + + +(Optional) +

CertSecretRef specifies the Secret containing the TLS authentication +data. The secret must contain a ‘certFile’ and ‘keyFile’, and/or ‘caFile’ +fields. It takes precedence over the values specified in the Secret +referred to by .spec.secretRef.

@@ -2459,8 +2476,25 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference for the HelmRepository. For HTTP/S basic auth the secret must contain ‘username’ and ‘password’ fields. -For TLS the secret must contain a ‘certFile’ and ‘keyFile’, and/or -‘caFile’ fields.

+Support for TLS auth using the ‘certFile’ and ‘keyFile’, and/or ‘caFile’ +keys is deprecated. Please use .spec.certSecretRef instead.

+ + + + +certSecretRef
+ + +github.com/fluxcd/pkg/apis/meta.LocalObjectReference + + + + +(Optional) +

CertSecretRef specifies the Secret containing the TLS authentication +data. The secret must contain a ‘certFile’ and ‘keyFile’, and/or ‘caFile’ +fields. It takes precedence over the values specified in the Secret +referred to by .spec.secretRef.

From 79adec586b8150168e8d968fe6b1d3c6e4a56d74 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 18 Jul 2023 18:38:20 +0530 Subject: [PATCH 2/3] helm: add support for specifying TLS auth via `.spec.certSecretRef` Add support for specifying TLS auth data via `.spec.certSecretRef` in HelmRepository and log a deprecation warning 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 | 189 ++----------- .../controller/helmchart_controller_test.go | 85 +----- .../controller/helmrepository_controller.go | 60 ++--- .../helmrepository_controller_oci.go | 3 +- .../helmrepository_controller_test.go | 61 ++--- .../controller/ocirepository_controller.go | 24 +- internal/helm/getter/client_opts.go | 196 ++++++++++++++ internal/helm/getter/client_opts_test.go | 254 ++++++++++++++++++ internal/helm/getter/getter.go | 61 +---- internal/helm/getter/getter_test.go | 106 +------- internal/helm/registry/auth.go | 15 ++ internal/oci/auth.go | 33 ++- 12 files changed, 574 insertions(+), 513 deletions(-) create mode 100644 internal/helm/getter/client_opts.go create mode 100644 internal/helm/getter/client_opts_test.go diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 6095c60ed..548b4bc53 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "crypto/tls" "errors" "fmt" "net/url" @@ -28,7 +27,6 @@ import ( "strings" "time" - "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/opencontainers/go-digest" helmgetter "helm.sh/helm/v3/pkg/getter" @@ -54,7 +52,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 +65,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 +502,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,65 +510,8 @@ 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) - if err != nil { + clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) + if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { e := &serror.Event{ Err: err, Reason: sourcev1.AuthenticationFailedReason, @@ -585,6 +519,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } + getterOpts := clientOpts.GetterOpts // Initialize the chart repository var chartRepo repository.Downloader @@ -599,7 +534,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(clientOpts.RegLoginOpt != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -621,7 +556,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, *clientOpts) if err != nil { if obj.Spec.Verify.SecretRef == nil { provider = fmt.Sprintf("%s keyless", provider) @@ -636,21 +571,20 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Tell the chart repository to use the OCI client with the configured getter - clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient)) + getterOpts = append(getterOpts, helmgetter.WithRegistryClient(registryClient)) ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), - repository.WithOCIGetterOptions(clientOpts), + repository.WithOCIGetterOptions(getterOpts), repository.WithOCIRegistryClient(registryClient), repository.WithVerifiers(verifiers)) 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 clientOpts.RegLoginOpt != nil { + err = ociChartRepo.Login(clientOpts.RegLoginOpt) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to login to OCI registry: %w", err), @@ -660,8 +594,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, clientOpts.TlsConfig, getterOpts...) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -1024,12 +959,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 @@ -1052,61 +981,28 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont 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) - if err != nil { + clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) + if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { return nil, err } + getterOpts := clientOpts.GetterOpts var chartRepo repository.Downloader if helmreg.IsOCI(normalizedURL) { - registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != 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: %w", err) } var errs []error // Tell the chart repository to use the OCI client with the configured getter - clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient)) + getterOpts = append(getterOpts, helmgetter.WithRegistryClient(registryClient)) ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), - repository.WithOCIGetterOptions(clientOpts), + repository.WithOCIGetterOptions(getterOpts), 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: %w", err)) // clean up the credentialsFile if credentialsFile != "" { if err := os.Remove(credentialsFile); err != nil { @@ -1118,10 +1014,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 clientOpts.RegLoginOpt != nil { + err = ociChartRepo.Login(clientOpts.RegLoginOpt) 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: %w", err)) // clean up the credentialsFile errs = append(errs, ociChartRepo.Clear()) return nil, kerrors.NewAggregate(errs) @@ -1130,7 +1026,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, clientOpts.TlsConfig, getterOpts...) if err != nil { return nil, err } @@ -1178,36 +1074,6 @@ func (r *HelmChartReconciler) resolveDependencyRepository(ctx context.Context, u return nil, fmt.Errorf("no HelmRepository found for '%s' in '%s' namespace", url, namespace) } -func (r *HelmChartReconciler) clientOptionsFromSecret(secret *corev1.Secret, normalizedURL string) ([]helmgetter.Option, *tls.Config, error) { - opts, err := getter.ClientOptionsFromSecret(*secret) - if err != nil { - return nil, nil, fmt.Errorf("failed to configure Helm client with secret data: %w", err) - } - - tlsConfig, err := getter.TLSClientConfigFromSecret(*secret, normalizedURL) - if err != nil { - return nil, nil, fmt.Errorf("failed to create TLS client config with secret data: %w", err) - } - - return opts, tlsConfig, nil -} - -func (r *HelmChartReconciler) getHelmRepositorySecret(ctx context.Context, repository *helmv1.HelmRepository) (*corev1.Secret, error) { - if repository.Spec.SecretRef == nil { - return nil, nil - } - name := types.NamespacedName{ - Namespace: repository.GetNamespace(), - Name: repository.Spec.SecretRef.Name, - } - var secret corev1.Secret - err := r.Client.Get(ctx, name, &secret) - if err != nil { - return nil, err - } - return &secret, nil -} - func (r *HelmChartReconciler) indexHelmRepositoryByURL(o client.Object) []string { repo, ok := o.(*helmv1.HelmRepository) if !ok { @@ -1412,13 +1278,14 @@ func chartRepoConfigErrorReturn(err error, obj *helmv1.HelmChart) (sreconcile.Re } // makeVerifiers returns a list of verifiers for the given chart. -func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *helmv1.HelmChart, auth authn.Authenticator, keychain authn.Keychain) ([]soci.Verifier, error) { +func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *helmv1.HelmChart, clientOpts getter.ClientOpts) ([]soci.Verifier, error) { var verifiers []soci.Verifier verifyOpts := []remote.Option{} - if auth != nil { - verifyOpts = append(verifyOpts, remote.WithAuth(auth)) + + if clientOpts.Authenticator != nil { + verifyOpts = append(verifyOpts, remote.WithAuth(clientOpts.Authenticator)) } else { - verifyOpts = append(verifyOpts, remote.WithAuthFromKeychain(keychain)) + verifyOpts = append(verifyOpts, remote.WithAuthFromKeychain(clientOpts.Keychain)) } switch obj.Spec.Verify.Provider { diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 7e94ac775..b7002245a 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'"), })) }, }, @@ -1649,83 +1649,6 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { } } -func TestHelmChartReconciler_getHelmRepositorySecret(t *testing.T) { - mock := &corev1.Secret{ - TypeMeta: metav1.TypeMeta{ - Kind: "Secret", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "secret", - Namespace: "foo", - }, - Data: map[string][]byte{ - "key": []byte("bar"), - }, - } - - r := &HelmChartReconciler{ - Client: fakeclient.NewClientBuilder(). - WithObjects(mock). - Build(), - patchOptions: getPatchOptions(helmChartReadyCondition.Owned, "sc"), - } - - tests := []struct { - name string - repository *helmv1.HelmRepository - want *corev1.Secret - wantErr bool - }{ - { - name: "Existing secret reference", - repository: &helmv1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: mock.Namespace, - }, - Spec: helmv1.HelmRepositorySpec{ - SecretRef: &meta.LocalObjectReference{ - Name: mock.Name, - }, - }, - }, - want: mock, - }, - { - name: "Empty secret reference", - repository: &helmv1.HelmRepository{ - Spec: helmv1.HelmRepositorySpec{ - SecretRef: nil, - }, - }, - want: nil, - }, - { - name: "Error on client error", - repository: &helmv1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "different", - }, - Spec: helmv1.HelmRepositorySpec{ - SecretRef: &meta.LocalObjectReference{ - Name: mock.Name, - }, - }, - }, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - got, err := r.getHelmRepositorySecret(context.TODO(), tt.repository) - g.Expect(err != nil).To(Equal(tt.wantErr)) - g.Expect(got).To(Equal(tt.want)) - }) - } -} - func TestHelmChartReconciler_getSource(t *testing.T) { mocks := []client.Object{ &helmv1.HelmRepository{ diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index d5175fdf1..1b6161ee0 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" @@ -390,59 +388,33 @@ 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), - } - - // 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 + normalizedURL, err := repository.NormalizeURL(obj.Spec.URL) + if err != nil { + e := &serror.Stalling{ + Err: fmt.Errorf("invalid Helm repository URL: %w", err), + Reason: sourcev1.URLInvalidReason, } - clientOpts = append(clientOpts, opts...) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } - tlsConfig, err = getter.TLSClientConfigFromSecret(secret, obj.Spec.URL) - if err != nil { + clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL) + if err != nil { + if errors.Is(err, getter.ErrDeprecatedTLSConfig) { + ctrl.LoggerFrom(ctx). + Info("warning: specifying TLS authentication data via `.spec.secretRef` is deprecated, please use `.spec.certSecretRef` instead") + } else { e := &serror.Event{ - Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), + 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 } } // 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, clientOpts.TlsConfig, clientOpts.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..048227978 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -54,6 +54,7 @@ import ( "github.com/fluxcd/source-controller/internal/helm/registry" "github.com/fluxcd/source-controller/internal/helm/repository" "github.com/fluxcd/source-controller/internal/object" + soci "github.com/fluxcd/source-controller/internal/oci" intpredicates "github.com/fluxcd/source-controller/internal/predicates" ) @@ -318,7 +319,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 := soci.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..bd3e45f6a 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,37 +453,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, { - name: "HTTPS with CAFile secret makes ArtifactOutdated=True", - protocol: "https", - server: options{ - publicKey: tlsPublicKey, - privateKey: tlsPrivateKey, - ca: tlsCA, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ca-file", - }, - Data: map[string][]byte{ - "caFile": tlsCA, - }, - }, - beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { - obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} - }, - want: sreconcile.ResultSuccess, - 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"), - }, - 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()) - }, - }, - { - name: "HTTPS with invalid CAFile secret makes FetchFailed=True and returns error", + name: "HTTPS with invalid CAFile in certSecretRef makes FetchFailed=True and returns error", protocol: "https", server: options{ publicKey: tlsPublicKey, @@ -496,13 +469,13 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, }, beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev, dig digest.Digest) { - obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} + obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "invalid-ca"} conditions.MarkReconciling(obj, meta.ProgressingReason, "foo") conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar") }, wantErr: true, 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(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"), }, @@ -766,32 +739,32 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { } // Calculate the artifact digest for valid repos configurations. - clientOpts := []helmgetter.Option{ + getterOpts := []helmgetter.Option{ helmgetter.WithURL(server.URL()), } var newChartRepo *repository.ChartRepository - var tOpts *tls.Config + var tlsConf *tls.Config validSecret := true if secret != nil { // Extract the client options from secret, ignoring any invalid // value. validSecret is used to determine if the index digest // should be calculated below. - var cOpts []helmgetter.Option + var gOpts []helmgetter.Option var serr error - cOpts, serr = getter.ClientOptionsFromSecret(*secret) + gOpts, serr = getter.GetterOptionsFromSecret(*secret) if serr != nil { validSecret = false } - clientOpts = append(clientOpts, cOpts...) + getterOpts = append(getterOpts, gOpts...) repoURL := server.URL() if tt.url != "" { repoURL = tt.url } - tOpts, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) + tlsConf, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) if serr != nil { validSecret = false } - newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tOpts, clientOpts...) + newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tlsConf, getterOpts...) } else { newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil) } @@ -807,9 +780,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 +788,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() { diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 9ab36c748..7b282979e 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" @@ -345,7 +344,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 = soci.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 +869,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/getter/client_opts.go b/internal/helm/getter/client_opts.go new file mode 100644 index 000000000..2af928c8e --- /dev/null +++ b/internal/helm/getter/client_opts.go @@ -0,0 +1,196 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package getter + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net/url" + + "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/registry" + soci "github.com/fluxcd/source-controller/internal/oci" +) + +var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") + +// ClientOpts contains the various options to use while constructing +// a Helm repository client. +type ClientOpts struct { + Authenticator authn.Authenticator + Keychain authn.Keychain + RegLoginOpt helmreg.LoginOption + TlsConfig *tls.Config + GetterOpts []helmgetter.Option +} + +// 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. +func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmRepository, url string) (*ClientOpts, error) { + hrOpts := &ClientOpts{ + GetterOpts: []helmgetter.Option{ + helmgetter.WithURL(url), + helmgetter.WithTimeout(obj.Spec.Timeout.Duration), + helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials), + }, + } + ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI + + 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 = TLSClientConfigFromSecret(*certSecret, url) + if err != nil { + return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + } + } + + var authSecret *corev1.Secret + var deprecatedTLSConfig bool + 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 := GetterOptionsFromSecret(*authSecret) + if err != nil { + return nil, fmt.Errorf("failed to configure Helm client: %w", 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 = TLSClientConfigFromSecret(*authSecret, url) + if err != nil { + return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + } + // Constructing a TLS config using the auth secret is deprecated behavior. + if hrOpts.TlsConfig != nil { + deprecatedTLSConfig = true + } + } + + if ociRepo { + hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret) + if err != nil { + return nil, fmt.Errorf("failed to configure login options: %w", err) + } + } + } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI && ociRepo { + authenticator, authErr := soci.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': %w", obj.Spec.Provider, authErr) + } + if authenticator != nil { + hrOpts.Authenticator = authenticator + } + } + + if ociRepo { + hrOpts.RegLoginOpt, err = registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) + if err != nil { + return nil, err + } + } + if deprecatedTLSConfig { + err = ErrDeprecatedTLSConfig + } + + return hrOpts, err +} + +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 +} + +// TLSClientConfigFromSecret attempts to construct a TLS client config +// 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(secret corev1.Secret, repositoryUrl 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: + return nil, nil + case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): + return nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", + secret.Name) + } + + tlsConf := &tls.Config{} + if len(certBytes) > 0 && len(keyBytes) > 0 { + cert, err := tls.X509KeyPair(certBytes, keyBytes) + if err != nil { + return nil, err + } + tlsConf.Certificates = append(tlsConf.Certificates, cert) + } + + if len(caBytes) > 0 { + cp, err := x509.SystemCertPool() + if err != nil { + return nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) + } + if !cp.AppendCertsFromPEM(caBytes) { + return nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") + } + + tlsConf.RootCAs = cp + } + + tlsConf.BuildNameToCertificate() + + u, err := url.Parse(repositoryUrl) + if err != nil { + return nil, fmt.Errorf("cannot parse repository URL: %w", err) + } + + tlsConf.ServerName = u.Hostname() + + return tlsConf, nil +} diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go new file mode 100644 index 000000000..2231e2a52 --- /dev/null +++ b/internal/helm/getter/client_opts_test.go @@ -0,0 +1,254 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package getter + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "math/big" + "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 TestGetClientOpts(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, hcOpts *ClientOpts) + oci bool + err error + }{ + { + 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, hcOpts *ClientOpts) { + t.Expect(hcOpts.TlsConfig).ToNot(BeNil()) + t.Expect(len(hcOpts.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, hcOpts *ClientOpts) { + t.Expect(hcOpts.TlsConfig).ToNot(BeNil()) + t.Expect(len(hcOpts.GetterOpts)).To(Equal(4)) + }, + err: ErrDeprecatedTLSConfig, + }, + { + 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, hcOpts *ClientOpts) { + repo, err := name.NewRepository("ghcr.io/dummy") + t.Expect(err).ToNot(HaveOccurred()) + authenticator, err := hcOpts.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")) + }, + oci: 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, + }, + }, + } + if tt.oci { + helmRepo.Spec.Type = helmv1.HelmRepositoryTypeOCI + } + + 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() + + clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + if tt.err != nil { + g.Expect(err).To(Equal(tt.err)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + tt.afterFunc(g, clientOpts) + }) + } +} + +func Test_tlsClientConfigFromSecret(t *testing.T) { + tlsSecretFixture := validTlsSecret(t) + + tests := []struct { + name string + secret corev1.Secret + modify func(secret *corev1.Secret) + wantErr bool + wantNil bool + }{ + {"certFile, keyFile and caFile", tlsSecretFixture, nil, false, false}, + {"without certFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "certFile") }, true, true}, + {"without keyFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "keyFile") }, true, true}, + {"without caFile", tlsSecretFixture, 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.modify != nil { + tt.modify(secret) + } + + got, err := TLSClientConfigFromSecret(*secret, "") + if (err != nil) != tt.wantErr { + t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantNil && got != nil { + t.Error("TLSClientConfigFromSecret() != nil") + return + } + }) + } +} + +// validTlsSecret creates a secret containing key pair and CA certificate that are +// valid from a syntax (minimum requirements) perspective. +func validTlsSecret(t *testing.T) corev1.Secret { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal("Private key cannot be created.", err.Error()) + } + + certTemplate := x509.Certificate{ + SerialNumber: big.NewInt(1337), + } + cert, err := x509.CreateCertificate(rand.Reader, &certTemplate, &certTemplate, &key.PublicKey, key) + if err != nil { + t.Fatal("Certificate cannot be created.", err.Error()) + } + + ca := &x509.Certificate{ + SerialNumber: big.NewInt(7331), + IsCA: true, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + } + + caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) + if err != nil { + t.Fatal("CA private key cannot be created.", err.Error()) + } + + caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) + if err != nil { + t.Fatal("CA certificate cannot be created.", err.Error()) + } + + keyPem := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + + certPem := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert, + }) + + caPem := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: caBytes, + }) + + return corev1.Secret{ + Data: map[string][]byte{ + "certFile": []byte(certPem), + "keyFile": []byte(keyPem), + "caFile": []byte(caPem), + }, + } +} diff --git a/internal/helm/getter/getter.go b/internal/helm/getter/getter.go index 25214372f..18661da16 100644 --- a/internal/helm/getter/getter.go +++ b/internal/helm/getter/getter.go @@ -17,20 +17,17 @@ limitations under the License. package getter import ( - "crypto/tls" - "crypto/x509" "fmt" - "net/url" "helm.sh/helm/v3/pkg/getter" corev1 "k8s.io/api/core/v1" ) -// ClientOptionsFromSecret constructs a getter.Option slice for the given secret. +// GetterOptionsFromSecret constructs a getter.Option slice for the given secret. // It returns the slice, or an error. -func ClientOptionsFromSecret(secret corev1.Secret) ([]getter.Option, error) { +func GetterOptionsFromSecret(secret corev1.Secret) ([]getter.Option, error) { var opts []getter.Option - basicAuth, err := BasicAuthFromSecret(secret) + basicAuth, err := basicAuthFromSecret(secret) if err != nil { return opts, err } @@ -40,12 +37,12 @@ func ClientOptionsFromSecret(secret corev1.Secret) ([]getter.Option, error) { return opts, nil } -// BasicAuthFromSecret attempts to construct a basic auth getter.Option for the +// basicAuthFromSecret attempts to construct a basic auth getter.Option for the // given v1.Secret and returns the result. // // Secrets with no username AND password are ignored, if only one is defined it // returns an error. -func BasicAuthFromSecret(secret corev1.Secret) (getter.Option, error) { +func basicAuthFromSecret(secret corev1.Secret) (getter.Option, error) { username, password := string(secret.Data["username"]), string(secret.Data["password"]) switch { case username == "" && password == "": @@ -55,51 +52,3 @@ func BasicAuthFromSecret(secret corev1.Secret) (getter.Option, error) { } return getter.WithBasicAuth(username, password), nil } - -// TLSClientConfigFromSecret attempts to construct a TLS client config -// 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(secret corev1.Secret, repositoryUrl 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: - return nil, nil - case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", - secret.Name) - } - - tlsConf := &tls.Config{} - if len(certBytes) > 0 && len(keyBytes) > 0 { - cert, err := tls.X509KeyPair(certBytes, keyBytes) - if err != nil { - return nil, err - } - tlsConf.Certificates = append(tlsConf.Certificates, cert) - } - - if len(caBytes) > 0 { - cp, err := x509.SystemCertPool() - if err != nil { - return nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) - } - if !cp.AppendCertsFromPEM(caBytes) { - return nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") - } - - tlsConf.RootCAs = cp - } - - tlsConf.BuildNameToCertificate() - - u, err := url.Parse(repositoryUrl) - if err != nil { - return nil, fmt.Errorf("cannot parse repository URL: %w", err) - } - - tlsConf.ServerName = u.Hostname() - - return tlsConf, nil -} diff --git a/internal/helm/getter/getter_test.go b/internal/helm/getter/getter_test.go index a13c029e3..cffe0064f 100644 --- a/internal/helm/getter/getter_test.go +++ b/internal/helm/getter/getter_test.go @@ -17,11 +17,6 @@ limitations under the License. package getter import ( - "crypto/rand" - "crypto/rsa" - "crypto/x509" - "encoding/pem" - "math/big" "testing" corev1 "k8s.io/api/core/v1" @@ -36,7 +31,7 @@ var ( } ) -func TestClientOptionsFromSecret(t *testing.T) { +func TestGetterOptionsFromSecret(t *testing.T) { tests := []struct { name string secrets []corev1.Secret @@ -53,7 +48,7 @@ func TestClientOptionsFromSecret(t *testing.T) { } } - got, err := ClientOptionsFromSecret(secret) + got, err := GetterOptionsFromSecret(secret) if err != nil { t.Errorf("ClientOptionsFromSecret() error = %v", err) return @@ -65,7 +60,7 @@ func TestClientOptionsFromSecret(t *testing.T) { } } -func TestBasicAuthFromSecret(t *testing.T) { +func Test_basicAuthFromSecret(t *testing.T) { tests := []struct { name string secret corev1.Secret @@ -84,7 +79,7 @@ func TestBasicAuthFromSecret(t *testing.T) { if tt.modify != nil { tt.modify(secret) } - got, err := BasicAuthFromSecret(*secret) + got, err := basicAuthFromSecret(*secret) if (err != nil) != tt.wantErr { t.Errorf("BasicAuthFromSecret() error = %v, wantErr %v", err, tt.wantErr) return @@ -96,96 +91,3 @@ func TestBasicAuthFromSecret(t *testing.T) { }) } } - -func TestTLSClientConfigFromSecret(t *testing.T) { - tlsSecretFixture := validTlsSecret(t) - - tests := []struct { - name string - secret corev1.Secret - modify func(secret *corev1.Secret) - wantErr bool - wantNil bool - }{ - {"certFile, keyFile and caFile", tlsSecretFixture, nil, false, false}, - {"without certFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "certFile") }, true, true}, - {"without keyFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "keyFile") }, true, true}, - {"without caFile", tlsSecretFixture, 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.modify != nil { - tt.modify(secret) - } - - got, err := TLSClientConfigFromSecret(*secret, "") - if (err != nil) != tt.wantErr { - t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.wantNil && got != nil { - t.Error("TLSClientConfigFromSecret() != nil") - return - } - }) - } -} - -// validTlsSecret creates a secret containing key pair and CA certificate that are -// valid from a syntax (minimum requirements) perspective. -func validTlsSecret(t *testing.T) corev1.Secret { - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal("Private key cannot be created.", err.Error()) - } - - certTemplate := x509.Certificate{ - SerialNumber: big.NewInt(1337), - } - cert, err := x509.CreateCertificate(rand.Reader, &certTemplate, &certTemplate, &key.PublicKey, key) - if err != nil { - t.Fatal("Certificate cannot be created.", err.Error()) - } - - ca := &x509.Certificate{ - SerialNumber: big.NewInt(7331), - IsCA: true, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - } - - caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - t.Fatal("CA private key cannot be created.", err.Error()) - } - - caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) - if err != nil { - t.Fatal("CA certificate cannot be created.", err.Error()) - } - - keyPem := pem.EncodeToMemory(&pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(key), - }) - - certPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert, - }) - - caPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: caBytes, - }) - - return corev1.Secret{ - Data: map[string][]byte{ - "certFile": []byte(certPem), - "keyFile": []byte(keyPem), - "caFile": []byte(caPem), - }, - } -} diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index d843d7d3b..c48ec0b2b 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -26,6 +26,7 @@ import ( "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "helm.sh/helm/v3/pkg/registry" + helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" ) @@ -139,3 +140,17 @@ func (r stringResource) String() string { func (r stringResource) RegistryStr() string { return r.registry } + +// NewLoginOption returns a registry login option for the given HelmRepository. +// If the HelmRepository does not specify a secretRef, a nil login option is returned. +func NewLoginOption(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/oci/auth.go b/internal/oci/auth.go index 88b0e9442..7b3eab896 100644 --- a/internal/oci/auth.go +++ b/internal/oci/auth.go @@ -16,7 +16,17 @@ limitations under the License. package oci -import "github.com/google/go-containerregistry/pkg/authn" +import ( + "context" + "fmt" + "strings" + + "github.com/fluxcd/pkg/oci/auth/login" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" + + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" +) // Anonymous is an authn.AuthConfig that always returns an anonymous // authenticator. It is useful for registries that do not require authentication @@ -28,3 +38,24 @@ type Anonymous authn.AuthConfig func (a Anonymous) Resolve(_ authn.Resource) (authn.Authenticator, error) { return authn.Anonymous, 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, 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) +} From 4a55ce2b08f7e333bc317725078c20acffa41241 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 18 Jul 2023 18:40:48 +0530 Subject: [PATCH 3/3] helmrepo: add docs for `.spec.certSecretRef` Signed-off-by: Sanskar Jaiswal --- docs/spec/v1beta2/helmrepositories.md | 34 ++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index 34ddfe468..b48f4ff4a 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -452,15 +452,37 @@ flux create secret oci ghcr-auth \ --password=${GITHUB_PAT} ``` -#### TLS authentication +**Note:** Support for specifying TLS authentication data using this API has been +deprecated. Please use [`.spec.certSecretRef`](#cert-secret-reference) instead. +If the controller uses the secret specfied by this field to configure TLS, then +a deprecation warning will be logged. + +### Cert secret reference **Note:** TLS authentication is not yet supported by OCI Helm repositories. -To provide TLS credentials to use while connecting with the Helm repository, -the referenced Secret is expected to contain `.data.certFile` and -`.data.keyFile`, and/or `.data.caFile` values. +`.spec.certSecretRef.name` is an optional field to specify a secret containing TLS +certificate data. The secret can contain the following keys: -For example: +* `certFile` and `keyFile`, to specify the client certificate and private key used for +TLS client authentication. These must be used in conjunction, i.e. specifying one without +the other will lead to an error. +* `caFile`, to specify the CA certificate used to verify the server, which is required +if the server is using a self-signed certificate. + +If the server is using a self-signed certificate and has TLS client authentication enabled, +all three values are required. + +All the files in the secret are expected to be [PEM-encoded][pem-encoding]. Assuming you have +three files; `client.key`, `client.crt` and `ca.crt` for the client private key, client +certificate and the CA certificate respectively, you can generate the required secret using +the `flux creat secret helm` command: + +```sh +flux create secret helm tls --key-file=client.key --cert-file=client.crt --ca-file=ca.crt +``` + +Example usage: ```yaml --- @@ -472,7 +494,7 @@ metadata: spec: interval: 5m0s url: https://example.com - secretRef: + certSecretRef: name: example-tls --- apiVersion: v1