From 4b1ace618aca147cafa5394d50825bd1e47b9cbd Mon Sep 17 00:00:00 2001 From: cappyzawa Date: Tue, 29 Jul 2025 01:25:55 +0900 Subject: [PATCH] Enforce TLS certificate verification in Helm/OCI Repository controllers Remove the insecure parameter from TLS configuration function calls to prevent InsecureSkipVerify from being set when using certificate-based authentication. This ensures TLS certificate verification is always performed when certificates are provided, aligning with our security policy. Updated pkg/runtime to v0.76.0 which no longer accepts the insecure parameter in TLS configuration functions. For OCIRepository, maintain backward compatibility by handling the specific case where no certificate is provided and insecure is explicitly set to true. This is the only allowed exception in Flux controllers. Signed-off-by: cappyzawa --- go.mod | 2 +- go.sum | 4 +- .../helmrepository_controller_test.go | 6 +-- .../controller/ocirepository_controller.go | 7 ++- internal/helm/getter/client_opts.go | 4 +- internal/helm/getter/client_opts_test.go | 46 +++++++++++++++++++ 6 files changed, 60 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index a713b6b8..3b62cc8e 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( github.com/fluxcd/pkg/lockedfile v0.6.0 github.com/fluxcd/pkg/masktoken v0.7.0 github.com/fluxcd/pkg/oci v0.51.0 - github.com/fluxcd/pkg/runtime v0.75.0 + github.com/fluxcd/pkg/runtime v0.76.0 github.com/fluxcd/pkg/sourceignore v0.13.0 github.com/fluxcd/pkg/ssh v0.20.0 github.com/fluxcd/pkg/tar v0.13.0 diff --git a/go.sum b/go.sum index e5204a36..ba948095 100644 --- a/go.sum +++ b/go.sum @@ -398,8 +398,8 @@ github.com/fluxcd/pkg/masktoken v0.7.0 h1:pitmyOg2pUVdW+nn2Lk/xqm2TaA08uxvOC0ns3 github.com/fluxcd/pkg/masktoken v0.7.0/go.mod h1:Lc1uoDjO1GY6+YdkK+ZqqBIBWquyV58nlSJ5S1N1IYU= github.com/fluxcd/pkg/oci v0.51.0 h1:9oYnm+T4SCVSBif9gn80ALJkMGSERabVMDJiaMIdr7Y= github.com/fluxcd/pkg/oci v0.51.0/go.mod h1:5J6IhHoDVYCVeBEC+4E3nPeKh7d0kjJ8IEL6NVCiTx4= -github.com/fluxcd/pkg/runtime v0.75.0 h1:wIaODmU5D54nyrehTqA9oQDFoi6BbBj/24adLStXc0I= -github.com/fluxcd/pkg/runtime v0.75.0/go.mod h1:iGhdaEq+lMJQTJNAFEPOU4gUJ7kt3yeDcJPZy7O9IUw= +github.com/fluxcd/pkg/runtime v0.76.0 h1:VoN508i65E/zK0iNXk1Ubvb2VcA8uADqckF+7nuof20= +github.com/fluxcd/pkg/runtime v0.76.0/go.mod h1:iGhdaEq+lMJQTJNAFEPOU4gUJ7kt3yeDcJPZy7O9IUw= github.com/fluxcd/pkg/sourceignore v0.13.0 h1:ZvkzX2WsmyZK9cjlqOFFW1onHVzhPZIqDbCh96rPqbU= github.com/fluxcd/pkg/sourceignore v0.13.0/go.mod h1:Z9H1GoBx0ljOhptnzoV0PL6Nd/UzwKcSphP27lqb4xI= github.com/fluxcd/pkg/ssh v0.20.0 h1:Ak0laIYIc/L8lEfqls/LDWRW8wYPESGaravQsCRGLb8= diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index c0a5b335..e5dbc3fc 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -482,7 +482,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ @@ -534,7 +534,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ @@ -588,7 +588,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { repoURL, err := repository.NormalizeURL(serverURL) t.Expect(err).ToNot(HaveOccurred()) - tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL, false) + tlsConfig, err := secrets.TLSConfigFromSecret(context.TODO(), secret, serverURL) t.Expect(err).ToNot(HaveOccurred()) getterOpts := []helmgetter.Option{ diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 9d9ca18d..11d66280 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -986,6 +986,11 @@ func (r *OCIRepositoryReconciler) transport(ctx context.Context, obj *sourcev1.O func (r *OCIRepositoryReconciler) getTLSConfig(ctx context.Context, obj *sourcev1.OCIRepository) (*cryptotls.Config, error) { if obj.Spec.CertSecretRef == nil || obj.Spec.CertSecretRef.Name == "" { if obj.Spec.Insecure { + // NOTE: This is the only place in Flux where InsecureSkipVerify is allowed. + // This exception is made for OCIRepository to maintain backward compatibility + // with tools like crane that require insecure connections without certificates. + // This only applies when no CertSecretRef is provided AND insecure is explicitly set. + // All other controllers must NOT allow InsecureSkipVerify per our security policy. return &cryptotls.Config{ InsecureSkipVerify: true, }, nil @@ -997,7 +1002,7 @@ func (r *OCIRepositoryReconciler) getTLSConfig(ctx context.Context, obj *sourcev Namespace: obj.Namespace, Name: obj.Spec.CertSecretRef.Name, } - return secrets.TLSConfigFromSecretRef(ctx, r.Client, secretName, obj.Spec.URL, obj.Spec.Insecure) + return secrets.TLSConfigFromSecretRef(ctx, r.Client, secretName, obj.Spec.URL) } // reconcileStorage ensures the current state of the storage matches the diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index cbcd09d9..50641c63 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -122,7 +122,7 @@ func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1 } certSecret = secret - tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, obj.Spec.URL, obj.Spec.Insecure) + tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, obj.Spec.URL) if err != nil { return false, nil, nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } @@ -138,7 +138,7 @@ func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1 } authSecret = secret - methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(obj.Spec.URL, obj.Spec.Insecure)) + methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTargetURL(obj.Spec.URL)) if err != nil { return false, nil, nil, fmt.Errorf("failed to detect authentication methods: %w", err) } diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index bf40e7f8..dbd56686 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -298,3 +298,49 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { }) } } + +func TestConfigureAuthentication_WithTargetURL(t *testing.T) { + g := NewWithT(t) + + tlsCA, err := os.ReadFile("../../controller/testdata/certs/ca.pem") + if err != nil { + t.Errorf("could not read CA file: %s", err) + return + } + + helmRepo := &helmv1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "default", + }, + Spec: helmv1.HelmRepositorySpec{ + URL: "https://example.com/charts", + }, + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "username": []byte("testuser"), + "password": []byte("testpass"), + "ca.crt": tlsCA, + }, + } + + client := fakeclient.NewClientBuilder().WithObjects(secret).Build() + helmRepo.Spec.SecretRef = &meta.LocalObjectReference{Name: secret.Name} + + opts := &ClientOpts{} + deprecatedTLS, certSecret, authSecret, err := configureAuthentication(context.TODO(), client, helmRepo, opts, helmRepo.Spec.URL) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(deprecatedTLS).To(BeTrue()) // TLS from SecretRef is deprecated + g.Expect(certSecret).To(BeNil()) + g.Expect(authSecret).To(Equal(secret)) + + // Regression test: verify ServerName is set from target URL when WithTargetURL is used + g.Expect(opts.TlsConfig).ToNot(BeNil()) + g.Expect(opts.TlsConfig.ServerName).To(Equal("example.com")) +}