From d28bc3178caee9397dbdd4bc793ce3808611ec04 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 16 May 2023 18:12:32 +0200 Subject: [PATCH] Add support for custom certificate and skip-tls-verify in helm OCI If implemented user will be able to provide their own custom start and bypass tls verification when interacting with OCI registries over https to pull helmCharts. Signed-off-by: Soule BA --- docs/spec/v1beta2/helmrepositories.md | 2 - internal/controller/helmchart_controller.go | 28 ++- .../controller/helmchart_controller_test.go | 161 ++++++++++++++---- .../controller/helmrepository_controller.go | 2 +- .../helmrepository_controller_oci.go | 98 +++-------- .../helmrepository_controller_oci_test.go | 133 ++++++++++++--- .../helmrepository_controller_test.go | 2 +- internal/controller/suite_test.go | 48 +++++- internal/helm/getter/client_opts.go | 142 ++++++++++++--- internal/helm/getter/client_opts_test.go | 116 ++++++++++++- internal/helm/registry/auth.go | 9 + internal/helm/registry/client.go | 26 ++- .../helm/repository/oci_chart_repository.go | 27 ++- main.go | 1 - 14 files changed, 613 insertions(+), 182 deletions(-) diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index b48f4ff4a..e121b01e3 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -459,8 +459,6 @@ a deprecation warning will be logged. ### Cert secret reference -**Note:** TLS authentication is not yet supported by OCI Helm repositories. - `.spec.certSecretRef.name` is an optional field to specify a secret containing TLS certificate data. The secret can contain the following keys: diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index d393fcb32..a99fee36f 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -512,7 +512,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if err != nil { return chartRepoConfigErrorReturn(err, obj) } - clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) + + clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { e := &serror.Event{ Err: err, @@ -521,6 +522,15 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } + if certsTmpDir != "" { + defer func() { + if err := os.RemoveAll(certsTmpDir); err != nil { + r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, + "failed to delete temporary certificates directory: %s", err) + } + }() + } + getterOpts := clientOpts.GetterOpts // Initialize the chart repository @@ -536,7 +546,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(clientOpts.RegLoginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -585,8 +595,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // 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 clientOpts.RegLoginOpt != nil { - err = ociChartRepo.Login(clientOpts.RegLoginOpt) + if clientOpts.MustLoginToRegistry() { + err = ociChartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to login to OCI registry: %w", err), @@ -983,15 +993,16 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) + clientOpts, certsTmpDir, 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(clientOpts.RegLoginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) if err != nil { return nil, fmt.Errorf("failed to create registry client: %w", err) } @@ -1002,6 +1013,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(getterOpts), repository.WithOCIRegistryClient(registryClient), + repository.WithCertificatesStore(certsTmpDir), repository.WithCredentialsFile(credentialsFile)) if err != nil { errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err)) @@ -1016,8 +1028,8 @@ 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 clientOpts.RegLoginOpt != nil { - err = ociChartRepo.Login(clientOpts.RegLoginOpt) + if clientOpts.MustLoginToRegistry() { + err = ociChartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { errs = append(errs, fmt.Errorf("failed to login to OCI chart repository: %w", err)) // clean up the credentialsFile diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index c6f030170..ec067fd50 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -1109,7 +1109,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) // Upload the test chart - metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer) + metadata, err := loadTestChartToOCI(chartData, testRegistryServer, "", "", "") g.Expect(err).NotTo(HaveOccurred()) storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords) @@ -2244,6 +2244,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { url string registryOpts registryOptions secretOpts secretOptions + secret *corev1.Secret + certsecret *corev1.Secret + insecure bool provider string providerImg string want sreconcile.Result @@ -2251,16 +2254,18 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "HTTP without basic auth", - want: sreconcile.ResultSuccess, + name: "HTTP without basic auth", + want: sreconcile.ResultSuccess, + insecure: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { - name: "HTTP with basic auth secret", - want: sreconcile.ResultSuccess, + name: "HTTP with basic auth secret", + want: sreconcile.ResultSuccess, + insecure: true, registryOpts: registryOptions{ withBasicAuth: true, }, @@ -2268,15 +2273,23 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { username: testRegistryUsername, password: testRegistryPassword, }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, { - name: "HTTP registry - basic auth with invalid secret", - want: sreconcile.ResultEmpty, - wantErr: true, + name: "HTTP registry - basic auth with invalid secret", + want: sreconcile.ResultEmpty, + wantErr: true, + insecure: true, registryOpts: registryOptions{ withBasicAuth: true, }, @@ -2284,6 +2297,13 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { username: "wrong-pass", password: "wrong-pass", }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to login to OCI registry"), }, @@ -2291,6 +2311,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { { name: "with contextual login provider", wantErr: true, + insecure: true, provider: "aws", providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test", assertConditions: []metav1.Condition{ @@ -2303,16 +2324,87 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { registryOpts: registryOptions{ withBasicAuth: true, }, + insecure: true, secretOpts: secretOptions{ username: testRegistryUsername, password: testRegistryPassword, }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, provider: "azure", assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), }, }, + { + name: "HTTPS With invalid CA cert", + wantErr: true, + registryOpts: registryOptions{ + withTLS: true, + withClientCertAuth: true, + }, + secretOpts: secretOptions{ + username: testRegistryUsername, + password: testRegistryPassword, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Data: map[string][]byte{ + "caFile": []byte("invalid caFile"), + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid caFile"), + }, + }, + { + name: "HTTPS With CA cert", + want: sreconcile.ResultSuccess, + registryOpts: registryOptions{ + withTLS: true, + withClientCertAuth: true, + }, + secretOpts: secretOptions{ + username: testRegistryUsername, + password: testRegistryPassword, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + "certFile": clientPublicKey, + "keyFile": clientPrivateKey, + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"), + }, + }, } for _, tt := range tests { @@ -2325,7 +2417,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { workspaceDir := t.TempDir() - tt.registryOpts.disableDNSMocking = true + if tt.insecure { + tt.registryOpts.disableDNSMocking = true + } server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) t.Cleanup(func() { @@ -2337,7 +2431,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) // Upload the test chart - metadata, err := loadTestChartToOCI(chartData, chartPath, server) + metadata, err := loadTestChartToOCI(chartData, server, "testdata/certs/client.pem", "testdata/certs/client-key.pem", "testdata/certs/ca.pem") g.Expect(err).ToNot(HaveOccurred()) repo := &helmv1.HelmRepository{ @@ -2364,25 +2458,26 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { } if tt.secretOpts.username != "" && tt.secretOpts.password != "" { - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{ - ".dockerconfigjson": []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`, - server.registryHost, tt.secretOpts.username, tt.secretOpts.password)), - }, - } + tt.secret.Data[".dockerconfigjson"] = []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`, + server.registryHost, tt.secretOpts.username, tt.secretOpts.password)) + } + if tt.secret != nil { repo.Spec.SecretRef = &meta.LocalObjectReference{ - Name: secret.Name, + Name: tt.secret.Name, } - clientBuilder.WithObjects(secret, repo) - } else { - clientBuilder.WithObjects(repo) + clientBuilder.WithObjects(tt.secret) + } + + if tt.certsecret != nil { + repo.Spec.CertSecretRef = &meta.LocalObjectReference{ + Name: tt.certsecret.Name, + } + clientBuilder.WithObjects(tt.certsecret) } + clientBuilder.WithObjects(repo) + obj := &helmv1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", @@ -2456,7 +2551,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T g.Expect(err).ToNot(HaveOccurred()) // Upload the test chart - metadata, err := loadTestChartToOCI(chartData, chartPath, server) + metadata, err := loadTestChartToOCI(chartData, server, "", "", "") g.Expect(err).NotTo(HaveOccurred()) storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords) @@ -2687,30 +2782,24 @@ func extractChartMeta(chartData []byte) (*hchart.Metadata, error) { return ch.Metadata, nil } -func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClientTestServer) (*hchart.Metadata, error) { +func loadTestChartToOCI(chartData []byte, server *registryClientTestServer, certFile, keyFile, cafile string) (*hchart.Metadata, error) { // Login to the registry err := server.registryClient.Login(server.registryHost, helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword), - helmreg.LoginOptInsecure(true)) - if err != nil { - return nil, err - } - - // Load a test chart - chartData, err = os.ReadFile(chartPath) + helmreg.LoginOptTLSClientConfig(certFile, keyFile, cafile)) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to login to OCI registry: %w", err) } metadata, err := extractChartMeta(chartData) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to extract chart metadata: %w", err) } // Upload the test chart ref := fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version) _, err = server.registryClient.Push(chartData, ref) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to push chart: %w", err) } return metadata, nil diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 99ace6ec4..dd75ff915 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -399,7 +399,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc return sreconcile.ResultEmpty, e } - clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL) + clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL) if err != nil { if errors.Is(err, getter.ErrDeprecatedTLSConfig) { ctrl.LoggerFrom(ctx). diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index 2752a612c..87f504bef 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -18,20 +18,18 @@ package controller import ( "context" + "crypto/tls" "errors" "fmt" "net/url" "os" "time" - "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" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" @@ -42,7 +40,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/runtime/conditions" helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" @@ -51,10 +48,9 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1" helmv1 "github.com/fluxcd/source-controller/api/v1beta2" - "github.com/fluxcd/source-controller/internal/helm/registry" + "github.com/fluxcd/source-controller/internal/helm/getter" "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" ) @@ -79,7 +75,7 @@ type HelmRepositoryOCIReconciler struct { client.Client kuberecorder.EventRecorder helper.Metrics - Getters helmgetter.Providers + ControllerName string RegistryClientGenerator RegistryClientGeneratorFunc @@ -95,7 +91,7 @@ type HelmRepositoryOCIReconciler struct { // and an optional file name. // The file is used to store the registry client credentials. // The caller is responsible for deleting the file. -type RegistryClientGeneratorFunc func(isLogin bool) (*helmreg.Client, string, error) +type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error) func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error { return r.SetupWithManagerAndOptions(mgr, HelmRepositoryReconcilerOptions{}) @@ -226,7 +222,7 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S } // Check if it's a successful reconciliation. - if result.RequeueAfter == obj.GetRequeueAfter() && result.Requeue == false && + if result.RequeueAfter == obj.GetRequeueAfter() && !result.Requeue && retErr == nil { // Remove reconciling condition if the reconciliation was successful. conditions.Delete(obj, meta.ReconcilingCondition) @@ -305,43 +301,34 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S result, retErr = ctrl.Result{}, nil return } - conditions.Delete(obj, meta.StalledCondition) - var ( - authenticator authn.Authenticator - keychain authn.Keychain - err error - ) - // Configure any authentication related options. - if obj.Spec.SecretRef != nil { - keychain, err = authFromSecret(ctx, r.Client, obj) - if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) - result, retErr = ctrl.Result{}, err - return - } - } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI { - 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()) - result, retErr = ctrl.Result{}, e - return - } - if auth != nil { - authenticator = auth - } + normalizedURL, err := repository.NormalizeURL(obj.Spec.URL) + if err != nil { + conditions.MarkStalled(obj, sourcev1.URLInvalidReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.URLInvalidReason, err.Error()) + result, retErr = ctrl.Result{}, nil + return } - loginOpt, err := makeLoginOption(authenticator, keychain, obj.Spec.URL) + conditions.Delete(obj, meta.StalledCondition) + + clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) if err != nil { conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) result, retErr = ctrl.Result{}, err return } + if certsTmpDir != "" { + defer func() { + if err := os.RemoveAll(certsTmpDir); err != nil { + r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, + "failed to delete temporary certs directory: %s", err) + } + }() + } // Create registry client and login if needed. - registryClient, file, err := r.RegistryClientGenerator(loginOpt != nil) + registryClient, file, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry()) if err != nil { e := fmt.Errorf("failed to create registry client: %w", err) conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, e.Error()) @@ -368,8 +355,8 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S conditions.Delete(obj, meta.StalledCondition) // Attempt to login to the registry if credentials are provided. - if loginOpt != nil { - err = chartRepo.Login(loginOpt) + if clientOpts.MustLoginToRegistry() { + err = chartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err) conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) @@ -411,41 +398,6 @@ func (r *HelmRepositoryOCIReconciler) eventLogf(ctx context.Context, obj runtime r.Eventf(obj, eventType, reason, msg) } -// authFromSecret returns an authn.Keychain for the given HelmRepository. -// If the HelmRepository does not specify a secretRef, an anonymous keychain is returned. -func authFromSecret(ctx context.Context, client client.Client, obj *helmv1.HelmRepository) (authn.Keychain, error) { - // Attempt to retrieve secret. - name := types.NamespacedName{ - Namespace: obj.GetNamespace(), - Name: obj.Spec.SecretRef.Name, - } - var secret corev1.Secret - if err := client.Get(ctx, name, &secret); err != nil { - return nil, fmt.Errorf("failed to get secret '%s': %w", name.String(), err) - } - - // Construct login options. - keychain, err := registry.LoginOptionFromSecret(obj.Spec.URL, secret) - if err != nil { - return nil, fmt.Errorf("failed to configure Helm client with secret data: %w", err) - } - return keychain, nil -} - -// 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 registry.AuthAdaptHelper(auth) - } - - if keychain != nil { - return registry.KeychainAdaptHelper(keychain)(registryURL) - } - - return nil, nil -} - func conditionsDiff(a, b []string) []string { bMap := make(map[string]struct{}, len(b)) for _, j := range b { diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go index 88f1c0aaf..d1252e709 100644 --- a/internal/controller/helmrepository_controller_oci_test.go +++ b/internal/controller/helmrepository_controller_oci_test.go @@ -205,7 +205,10 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { name string url string registryOpts registryOptions + insecure bool secretOpts secretOptions + secret *corev1.Secret + certsSecret *corev1.Secret provider string providerImg string want ctrl.Result @@ -220,8 +223,9 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { }, }, { - name: "HTTP with basic auth secret", - want: ctrl.Result{RequeueAfter: interval}, + name: "HTTP with basic auth secret", + want: ctrl.Result{RequeueAfter: interval}, + insecure: true, registryOpts: registryOptions{ withBasicAuth: true, }, @@ -229,14 +233,22 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { username: testRegistryUsername, password: testRegistryPassword, }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), }, }, { - name: "HTTP registry - basic auth with invalid secret", - want: ctrl.Result{}, - wantErr: true, + name: "HTTP registry - basic auth with invalid secret", + want: ctrl.Result{}, + wantErr: true, + insecure: true, registryOpts: registryOptions{ withBasicAuth: true, }, @@ -244,6 +256,13 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { username: "wrong-pass", password: "wrong-pass", }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to login to registry"), @@ -252,6 +271,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { { name: "with contextual login provider", wantErr: true, + insecure: true, provider: "aws", providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test", assertConditions: []metav1.Condition{ @@ -265,15 +285,86 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { registryOpts: registryOptions{ withBasicAuth: true, }, + insecure: true, secretOpts: secretOptions{ username: testRegistryUsername, password: testRegistryPassword, }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, provider: "azure", assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), }, }, + { + name: "HTTPS With invalid CA cert", + wantErr: true, + registryOpts: registryOptions{ + withTLS: true, + withClientCertAuth: true, + }, + secretOpts: secretOptions{ + username: testRegistryUsername, + password: testRegistryPassword, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Data: map[string][]byte{ + "caFile": []byte("invalid caFile"), + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation 0 -> 1"), + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"), + }, + }, + { + name: "HTTPS With CA cert", + want: ctrl.Result{RequeueAfter: interval}, + registryOpts: registryOptions{ + withTLS: true, + withClientCertAuth: true, + }, + secretOpts: secretOptions{ + username: testRegistryUsername, + password: testRegistryPassword, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secretref", + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + "certFile": clientPublicKey, + "keyFile": clientPrivateKey, + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "Helm repository is ready"), + }, + }, } for _, tt := range tests { @@ -285,7 +376,9 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { WithStatusSubresource(&helmv1.HelmRepository{}) workspaceDir := t.TempDir() - tt.registryOpts.disableDNSMocking = true + if tt.insecure { + tt.registryOpts.disableDNSMocking = true + } server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) t.Cleanup(func() { @@ -317,28 +410,27 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { } if tt.secretOpts.username != "" && tt.secretOpts.password != "" { - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secretref", - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{ - ".dockerconfigjson": []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`, - server.registryHost, tt.secretOpts.username, tt.secretOpts.password)), - }, - } - - clientBuilder.WithObjects(secret) + tt.secret.Data[".dockerconfigjson"] = []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`, + server.registryHost, tt.secretOpts.username, tt.secretOpts.password)) + } + if tt.secret != nil { + clientBuilder.WithObjects(tt.secret) obj.Spec.SecretRef = &meta.LocalObjectReference{ - Name: secret.Name, + Name: tt.secret.Name, + } + } + + if tt.certsSecret != nil { + clientBuilder.WithObjects(tt.certsSecret) + obj.Spec.CertSecretRef = &meta.LocalObjectReference{ + Name: tt.certsSecret.Name, } } r := &HelmRepositoryOCIReconciler{ Client: clientBuilder.Build(), EventRecorder: record.NewFakeRecorder(32), - Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, patchOptions: getPatchOptions(helmRepositoryOCIOwnedConditions, "sc"), } @@ -349,7 +441,6 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { }() sp := patch.NewSerialPatcher(obj, r.Client) - got, err := r.reconcile(ctx, sp, obj) g.Expect(err != nil).To(Equal(tt.wantErr)) g.Expect(got).To(Equal(tt.want)) diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 9e8fc5d47..3d3e914c2 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -796,7 +796,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if tt.url != "" { repoURL = tt.url } - tlsConf, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) + tlsConf, _, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) if serr != nil { validSecret = false } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 2200fe123..6b8e4b996 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -19,12 +19,15 @@ package controller import ( "bytes" "context" + "crypto/tls" + "crypto/x509" "fmt" "io" "io/ioutil" "log" "math/rand" "net" + "net/http" "os" "path/filepath" "testing" @@ -148,15 +151,11 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry var out bytes.Buffer server.out = &out - // init test client - client, err := helmreg.NewClient( + // init test client options + clientOpts := []helmreg.ClientOption{ helmreg.ClientOptDebug(true), helmreg.ClientOptWriter(server.out), - ) - if err != nil { - return nil, fmt.Errorf("failed to create registry client: %s", err) } - server.registryClient = client config := &configuration.Configuration{} port, err := freeport.GetFreePort() @@ -218,6 +217,13 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry if opts.withClientCertAuth { config.HTTP.TLS.ClientCAs = []string{"testdata/certs/ca.pem"} } + + // add TLS configured HTTP client option to clientOpts + httpClient, err := tlsConfiguredHTTPCLient() + if err != nil { + return nil, fmt.Errorf("failed to create TLS configured HTTP client: %s", err) + } + clientOpts = append(clientOpts, helmreg.ClientOptHTTPClient(httpClient)) } // setup logger options @@ -232,12 +238,41 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry return nil, fmt.Errorf("failed to create docker registry: %w", err) } + // init test client + client, err := helmreg.NewClient(clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create registry client: %s", err) + } + server.registryClient = client + // Start Docker registry go dockerRegistry.ListenAndServe() return server, nil } +func tlsConfiguredHTTPCLient() (*http.Client, error) { + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(tlsCA) { + return nil, fmt.Errorf("failed to append CA certificate to pool") + } + cert, err := tls.LoadX509KeyPair("testdata/certs/server.pem", "testdata/certs/server-key.pem") + if err != nil { + return nil, fmt.Errorf("failed to load server certificate: %s", err) + } + httpClient := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: pool, + Certificates: []tls.Certificate{ + cert, + }, + }, + }, + } + return httpClient, nil +} + func (r *registryClientTestServer) Close() { if r.dnsServer != nil { mockdns.UnpatchNet(net.DefaultResolver) @@ -345,7 +380,6 @@ func TestMain(m *testing.M) { Client: testEnv, EventRecorder: record.NewFakeRecorder(32), Metrics: testMetricsH, - Getters: testGetters, RegistryClientGenerator: registry.ClientGenerator, }).SetupWithManagerAndOptions(testEnv, HelmRepositoryReconcilerOptions{ RateLimiter: controller.GetDefaultRateLimiter(), diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 2af928c8e..3dc8535f0 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -23,6 +23,8 @@ import ( "errors" "fmt" "net/url" + "os" + "path" "github.com/fluxcd/pkg/oci" "github.com/google/go-containerregistry/pkg/authn" @@ -37,23 +39,47 @@ import ( soci "github.com/fluxcd/source-controller/internal/oci" ) +const ( + certFileName = "cert.pem" + keyFileName = "key.pem" + caFileName = "ca.pem" +) + var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") +// TLSBytes contains the bytes of the TLS files. +type TLSBytes struct { + // CertBytes is the bytes of the certificate file. + CertBytes []byte + // KeyBytes is the bytes of the key file. + KeyBytes []byte + // CABytes is the bytes of the CA file. + CABytes []byte +} + // 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 + RegLoginOpts []helmreg.LoginOption TlsConfig *tls.Config GetterOpts []helmgetter.Option } +// MustLoginToRegistry returns true if the client options contain at least +// one registry login option. +func (o ClientOpts) MustLoginToRegistry() bool { + return len(o.RegLoginOpts) > 0 && o.RegLoginOpts[0] != nil +} + // 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) { +// A temporary directory is created to store the certs files if needed and its path is returned along with the options object. It is the +// caller's responsibility to clean up the directory. +func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmRepository, url string) (*ClientOpts, string, error) { hrOpts := &ClientOpts{ GetterOpts: []helmgetter.Option{ helmgetter.WithURL(url), @@ -63,18 +89,25 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI - var certSecret *corev1.Secret - var err error + var ( + certSecret *corev1.Secret + tlsBytes *TLSBytes + certFile string + keyFile string + caFile string + dir string + 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) + 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) + hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*certSecret, url) if err != nil { - return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } } @@ -83,22 +116,22 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit 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) + 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) + 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`. + // then try to use `.spec.secretRef`. if hrOpts.TlsConfig == nil { - hrOpts.TlsConfig, err = TLSClientConfigFromSecret(*authSecret, url) + hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*authSecret, url) if err != nil { - return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + 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 { @@ -109,13 +142,13 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit if ociRepo { hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret) if err != nil { - return nil, fmt.Errorf("failed to configure login options: %w", err) + 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) + return nil, "", fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr) } if authenticator != nil { hrOpts.Authenticator = authenticator @@ -123,16 +156,34 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } if ociRepo { - hrOpts.RegLoginOpt, err = registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) + // Persist the certs files to the path if needed. + if tlsBytes != nil { + dir, err = os.MkdirTemp("", "helm-repo-oci-certs") + if err != nil { + return nil, "", fmt.Errorf("cannot create temporary directory: %w", err) + } + certFile, keyFile, caFile, err = StoreTLSCertificateFiles(tlsBytes, dir) + if err != nil { + return nil, "", fmt.Errorf("cannot write certs files to path: %w", err) + } + } + loginOpt, err := registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) if err != nil { - return nil, err + return nil, "", err + } + if loginOpt != nil { + hrOpts.RegLoginOpts = []helmreg.LoginOption{loginOpt} + } + tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) + if tlsLoginOpt != nil { + hrOpts.RegLoginOpts = append(hrOpts.RegLoginOpts, tlsLoginOpt) } } if deprecatedTLSConfig { err = ErrDeprecatedTLSConfig } - return hrOpts, err + return hrOpts, dir, err } func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) { @@ -152,13 +203,14 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) ( // // 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) { +// If shouldStore is true, it will write the certs files to the path. +func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls.Config, *TLSBytes, 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 + return nil, 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", + return nil, nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", secret.Name) } @@ -166,7 +218,7 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls if len(certBytes) > 0 && len(keyBytes) > 0 { cert, err := tls.X509KeyPair(certBytes, keyBytes) if err != nil { - return nil, err + return nil, nil, err } tlsConf.Certificates = append(tlsConf.Certificates, cert) } @@ -174,10 +226,10 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls if len(caBytes) > 0 { cp, err := x509.SystemCertPool() if err != nil { - return nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) + return nil, 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") + return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") } tlsConf.RootCAs = cp @@ -187,10 +239,50 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls u, err := url.Parse(repositoryUrl) if err != nil { - return nil, fmt.Errorf("cannot parse repository URL: %w", err) + return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err) } tlsConf.ServerName = u.Hostname() - return tlsConf, nil + return tlsConf, &TLSBytes{ + CertBytes: certBytes, + KeyBytes: keyBytes, + CABytes: caBytes, + }, nil +} + +// StoreTLSCertificateFiles writes the certs files to the path and returns the path of the files. +func StoreTLSCertificateFiles(tlsBytes *TLSBytes, path string) (string, string, string, error) { + var ( + certFile string + keyFile string + caFile string + err error + ) + if len(tlsBytes.CertBytes) > 0 && len(tlsBytes.KeyBytes) > 0 { + certFile, err = writeToFile(tlsBytes.CertBytes, certFileName, path) + if err != nil { + return "", "", "", err + } + keyFile, err = writeToFile(tlsBytes.KeyBytes, keyFileName, path) + if err != nil { + return "", "", "", err + } + } + if len(tlsBytes.CABytes) > 0 { + caFile, err = writeToFile(tlsBytes.CABytes, caFileName, path) + if err != nil { + return "", "", "", err + } + } + return certFile, keyFile, caFile, nil +} + +func writeToFile(data []byte, filename, tmpDir string) (string, error) { + file := path.Join(tmpDir, filename) + err := os.WriteFile(file, data, 0o644) + if err != nil { + return "", err + } + return file, nil } diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index 2231e2a52..6b031851d 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -149,7 +149,7 @@ func TestGetClientOpts(t *testing.T) { } c := clientBuilder.Build() - clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + clientOpts, _, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") if tt.err != nil { g.Expect(err).To(Equal(tt.err)) } else { @@ -183,7 +183,7 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { tt.modify(secret) } - got, err := TLSClientConfigFromSecret(*secret, "") + got, _, err := TLSClientConfigFromSecret(*secret, "") if (err != nil) != tt.wantErr { t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) return @@ -196,6 +196,118 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { } } +func TestGetClientOpts_registryTLSLoginOption(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 + loginOptsN int + }{ + { + name: "with valid caFile", + certSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + }, + }, + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + loginOptsN: 2, + }, + { + name: "without caFile", + certSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{}, + }, + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + loginOptsN: 1, + }, + { + name: "without cert secret", + certSecret: nil, + authSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + loginOptsN: 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + helmRepo := &helmv1.HelmRepository{ + Spec: helmv1.HelmRepositorySpec{ + Timeout: &metav1.Duration{ + Duration: time.Second, + }, + 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, tmpDir, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + if err != nil { + t.Errorf("GetClientOpts() error = %v", err) + return + } + if tmpDir != "" { + defer os.RemoveAll(tmpDir) + } + if tt.loginOptsN != len(clientOpts.RegLoginOpts) { + // we should have a login option but no TLS option + t.Error("registryTLSLoginOption() != 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 { diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index c48ec0b2b..d2638015a 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -154,3 +154,12 @@ func NewLoginOption(auth authn.Authenticator, keychain authn.Keychain, registryU return nil, nil } + +// TLSLoginOption returns a LoginOption that can be used to configure the TLS client. It requires either the caFile to be not blank or both certFile and keyFile to be not blank. +func TLSLoginOption(certFile, keyFile, caFile string) registry.LoginOption { + if (certFile != "" && keyFile != "") || caFile != "" { + return registry.LoginOptTLSClientConfig(certFile, keyFile, caFile) + } + + return nil +} diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 1247347ab..7ac0d3d0b 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -17,7 +17,9 @@ limitations under the License. package registry import ( + "crypto/tls" "io" + "net/http" "os" "helm.sh/helm/v3/pkg/registry" @@ -27,7 +29,7 @@ import ( // ClientGenerator generates a registry client and a temporary credential file. // The client is meant to be used for a single reconciliation. // The file is meant to be used for a single reconciliation and deleted after. -func ClientGenerator(isLogin bool) (*registry.Client, string, error) { +func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, string, error) { if isLogin { // create a temporary file to store the credentials // this is needed because otherwise the credentials are stored in ~/.docker/config.json. @@ -37,7 +39,7 @@ func ClientGenerator(isLogin bool) (*registry.Client, string, error) { } var errs []error - rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard), registry.ClientOptCredentialsFile(credentialsFile.Name())) + rClient, err := newClient(credentialsFile.Name(), tlsConfig) if err != nil { errs = append(errs, err) // attempt to delete the temporary file @@ -52,9 +54,27 @@ func ClientGenerator(isLogin bool) (*registry.Client, string, error) { return rClient, credentialsFile.Name(), nil } - rClient, err := registry.NewClient(registry.ClientOptWriter(io.Discard)) + rClient, err := newClient("", tlsConfig) if err != nil { return nil, "", err } return rClient, "", nil } + +func newClient(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) { + opts := []registry.ClientOption{ + registry.ClientOptWriter(io.Discard), + } + if tlsConfig != nil { + opts = append(opts, registry.ClientOptHTTPClient(&http.Client{ + Transport: &http.Transport{ + TLSClientConfig: tlsConfig, + }, + })) + } + if credentialsFile != "" { + opts = append(opts, registry.ClientOptCredentialsFile(credentialsFile)) + } + + return registry.NewClient(opts...) +} diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 0e76ee0c4..6a119183b 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "crypto/tls" + "errors" "fmt" "net/url" "os" @@ -65,9 +66,13 @@ type OCIChartRepository struct { // RegistryClient is a client to use while downloading tags or charts from a registry. RegistryClient RegistryClient + // credentialsFile is a temporary credentials file to use while downloading tags or charts from a registry. credentialsFile string + // certificatesStore is a temporary store to use while downloading tags or charts from a registry. + certificatesStore string + // verifiers is a list of verifiers to use when verifying a chart. verifiers []oci.Verifier } @@ -120,6 +125,14 @@ func WithCredentialsFile(credentialsFile string) OCIChartRepositoryOption { } } +// WithCertificatesStore returns a ChartRepositoryOption that will set the certificates store +func WithCertificatesStore(store string) OCIChartRepositoryOption { + return func(r *OCIChartRepository) error { + r.certificatesStore = store + return nil + } +} + // NewOCIChartRepository constructs and returns a new ChartRepository with // the ChartRepository.Client configured to the getter.Getter for the // repository URL scheme. It returns an error on URL parsing failures. @@ -265,14 +278,24 @@ func (r *OCIChartRepository) HasCredentials() bool { // Clear deletes the OCI registry credentials file. func (r *OCIChartRepository) Clear() error { + var errs error // clean the credentials file if it exists if r.credentialsFile != "" { if err := os.Remove(r.credentialsFile); err != nil { - return err + errs = errors.Join(errs, err) } } r.credentialsFile = "" - return nil + + // clean the certificates store if it exists + if r.certificatesStore != "" { + if err := os.RemoveAll(r.certificatesStore); err != nil { + errs = errors.Join(errs, err) + } + } + r.certificatesStore = "" + + return errs } // getLastMatchingVersionOrConstraint returns the last version that matches the given version string. diff --git a/main.go b/main.go index ea840ace2..5071f8111 100644 --- a/main.go +++ b/main.go @@ -198,7 +198,6 @@ func main() { Client: mgr.GetClient(), EventRecorder: eventRecorder, Metrics: metrics, - Getters: getters, ControllerName: controllerName, RegistryClientGenerator: registry.ClientGenerator, }).SetupWithManagerAndOptions(mgr, controller.HelmRepositoryReconcilerOptions{