Merge pull request fluxcd#1288 from fluxcd/helm-repo-insecure

Add `.spec.insecure` to `HelmRepository` for `type: oci`

Author: stefanprodan

api/v1beta2/helmrepository_types.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/fluxcd/pkg/apis/acl"
 	"github.com/fluxcd/pkg/apis/meta"
+
 	apiv1 "github.com/fluxcd/source-controller/api/v1"
 )
 
@@ -92,6 +93,11 @@ type HelmRepositorySpec struct {
 	// +optional
 	Interval metav1.Duration `json:"interval,omitempty"`
 
+	// Insecure allows connecting to a non-TLS HTTP container registry.
+	// This field is only taken into account if the .spec.type field is set to 'oci'.
+	// +optional
+	Insecure bool `json:"insecure,omitempty"`
+
 	// Timeout is used for the index fetch operation for an HTTPS helm repository,
 	// and for remote OCI Repository operations like pulling for an OCI helm
 	// chart by the associated HelmChart.

config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml

@@ -313,6 +313,11 @@ spec:
                 required:
                 - name
                 type: object
+              insecure:
+                description: Insecure allows connecting to a non-TLS HTTP container
+                  registry. This field is only taken into account if the .spec.type
+                  field is set to 'oci'.
+                type: boolean
               interval:
                 description: Interval at which the HelmRepository URL is checked for
                   updates. This interval is approximate and may be subject to jitter

docs/api/v1beta2/source.md

@@ -874,6 +874,19 @@ efficient use of resources.</p>
 </tr>
 <tr>
 <td>
+<code>insecure</code><br>
+<em>
+bool
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>Insecure allows connecting to a non-TLS HTTP container registry.
+This field is only taken into account if the .spec.type field is set to &lsquo;oci&rsquo;.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>timeout</code><br>
 <em>
 <a href="https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration">
@@ -2593,6 +2606,19 @@ efficient use of resources.</p>
 </tr>
 <tr>
 <td>
+<code>insecure</code><br>
+<em>
+bool
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>Insecure allows connecting to a non-TLS HTTP container registry.
+This field is only taken into account if the .spec.type field is set to &lsquo;oci&rsquo;.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>timeout</code><br>
 <em>
 <a href="https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration">

docs/spec/v1beta2/helmrepositories.md

@@ -147,14 +147,12 @@ valid [DNS subdomain name](https://kubernetes.io/docs/concepts/overview/working-
 A HelmRepository also needs a
 [`.spec` section](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status).
 
-
 ### Type
 
 `.spec.type` is an optional field that specifies the Helm repository type. 
 
 Possible values are `default` for a Helm HTTP/S repository, or `oci` for an OCI Helm repository.
 
-
 ### Provider
 
 `.spec.provider` is an optional field that allows specifying an OIDC provider used
@@ -347,6 +345,15 @@ the needed permission is instead `storage.objects.list` which can be bound as pa
 of the Container Registry Service Agent role. Take a look at [this guide](https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity)
 for more information about setting up GKE Workload Identity.
 
+### Insecure
+
+`.spec.insecure` is an optional field to allow connecting to an insecure (HTTP)
+container registry server, if set to `true`. The default value is `false`,
+denying insecure non-TLS connections when fetching Helm chart OCI artifacts.
+
+**Note**: The insecure field is supported only for Helm OCI repositories.
+The `spec.type` field must be set to `oci`.
+
 ### Interval
 
 **Note:** This field is ineffectual for [OCI Helm
@@ -422,8 +429,8 @@ metadata:
   name: example-user
   namespace: default
 stringData:
-  username: example
-  password: 123456
+  username: "user-123456"
+  password: "pass-123456"
 ```
 
 OCI Helm repository example:
@@ -448,8 +455,8 @@ metadata:
   name: oci-creds
   namespace: default
 stringData:
-  username: example
-  password: 123456
+  username: "user-123456"
+  password: "pass-123456"
 ```
 
 For OCI Helm repositories, Kubernetes secrets of type [kubernetes.io/dockerconfigjson](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types) are also supported.
@@ -465,7 +472,7 @@ flux create secret oci ghcr-auth \
 
 **Warning:** 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
+If the controller uses the secret specified by this field to configure TLS, then
 a deprecation warning will be logged.
 
 ### Cert secret reference

internal/controller/helmchart_controller.go

@@ -144,7 +144,7 @@ type HelmChartReconciler 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(tlsConfig *tls.Config, isLogin bool) (*helmreg.Client, string, error)
+type RegistryClientGeneratorFunc func(tlsConfig *tls.Config, isLogin, insecure bool) (*helmreg.Client, string, error)
 
 func (r *HelmChartReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
 	return r.SetupWithManagerAndOptions(ctx, mgr, HelmChartReconcilerOptions{})
@@ -555,7 +555,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.TlsConfig, clientOpts.MustLoginToRegistry())
+		registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), repo.Spec.Insecure)
 		if err != nil {
 			e := serror.NewGeneric(
 				fmt.Errorf("failed to construct Helm client: %w", err),
@@ -593,11 +593,17 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		// Tell the chart repository to use the OCI client with the configured getter
 		getterOpts = append(getterOpts, helmgetter.WithRegistryClient(registryClient))
-		ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL,
+		chartRepoOpts := []repository.OCIChartRepositoryOption{
 			repository.WithOCIGetter(r.Getters),
 			repository.WithOCIGetterOptions(getterOpts),
 			repository.WithOCIRegistryClient(registryClient),
-			repository.WithVerifiers(verifiers))
+			repository.WithVerifiers(verifiers),
+		}
+		if repo.Spec.Insecure {
+			chartRepoOpts = append(chartRepoOpts, repository.WithInsecureHTTP())
+		}
+
+		ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, chartRepoOpts...)
 		if err != nil {
 			return chartRepoConfigErrorReturn(err, obj)
 		}
@@ -1010,7 +1016,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 
 		var chartRepo repository.Downloader
 		if helmreg.IsOCI(normalizedURL) {
-			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry())
+			registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.MustLoginToRegistry(), obj.Spec.Insecure)
 			if err != nil {
 				return nil, fmt.Errorf("failed to create registry client: %w", err)
 			}

internal/controller/helmchart_controller_test.go

@@ -23,6 +23,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"os"
 	"path"
@@ -32,6 +33,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/foxcpp/go-mockdns"
 	. "github.com/onsi/gomega"
 	coptions "github.com/sigstore/cosign/v2/cmd/cosign/cli/options"
 	"github.com/sigstore/cosign/v2/cmd/cosign/cli/sign"
@@ -1295,6 +1297,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 					Timeout:  &metav1.Duration{Duration: timeout},
 					Provider: helmv1.GenericOCIProvider,
 					Type:     helmv1.HelmRepositoryTypeOCI,
+					Insecure: true,
 				},
 			}
 			obj := &helmv1.HelmChart{
@@ -1314,12 +1317,14 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 			}
 			got, err := r.buildFromHelmRepository(context.TODO(), obj, repository, &b)
 
-			g.Expect(err != nil).To(Equal(tt.wantErr != nil))
 			if tt.wantErr != nil {
+				g.Expect(err).To(HaveOccurred())
 				g.Expect(reflect.TypeOf(err).String()).To(Equal(reflect.TypeOf(tt.wantErr).String()))
 				g.Expect(err.Error()).To(ContainSubstring(tt.wantErr.Error()))
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(got).To(Equal(tt.want))
 			}
-			g.Expect(got).To(Equal(tt.want))
 
 			if tt.assertFunc != nil {
 				tt.assertFunc(g, obj, b)
@@ -1333,6 +1338,14 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
 
 	tmpDir := t.TempDir()
 
+	// Unpatch the changes we make to the default DNS resolver in `setupRegistryServer()`.
+	// This is required because the changes somehow also cause remote lookups to fail and
+	// this test tests functionality related to remote dependencies.
+	mockdns.UnpatchNet(net.DefaultResolver)
+	defer func() {
+		testRegistryServer.dnsServer.PatchNet(net.DefaultResolver)
+	}()
+
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
 	g.Expect(err).ToNot(HaveOccurred())
 
@@ -2430,9 +2443,6 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 
 			workspaceDir := t.TempDir()
 
-			if tt.insecure {
-				tt.registryOpts.disableDNSMocking = true
-			}
 			server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
 			g.Expect(err).NotTo(HaveOccurred())
 			t.Cleanup(func() {
@@ -2457,6 +2467,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
 					Type:     helmv1.HelmRepositoryTypeOCI,
 					Provider: helmv1.GenericOCIProvider,
 					URL:      fmt.Sprintf("oci://%s/testrepo", server.registryHost),
+					Insecure: tt.insecure,
 				},
 			}
 
@@ -2726,9 +2737,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
 	g := NewWithT(t)
 
 	tmpDir := t.TempDir()
-	server, err := setupRegistryServer(ctx, tmpDir, registryOptions{
-		disableDNSMocking: true,
-	})
+	server, err := setupRegistryServer(ctx, tmpDir, registryOptions{})
 	g.Expect(err).ToNot(HaveOccurred())
 	t.Cleanup(func() {
 		server.Close()
@@ -2871,6 +2880,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
 					Timeout:  &metav1.Duration{Duration: timeout},
 					Provider: helmv1.GenericOCIProvider,
 					Type:     helmv1.HelmRepositoryTypeOCI,
+					Insecure: true,
 				},
 			}
 
@@ -2925,7 +2935,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T
 					Upload:           true,
 					SkipConfirmation: true,
 					TlogUpload:       false,
-					Registry:         coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowInsecure: true},
+					Registry:         coptions.RegistryOptions{Keychain: oci.Anonymous{}, AllowHTTPRegistry: true},
 				},
 					[]string{fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)})
 				g.Expect(err).ToNot(HaveOccurred())

internal/controller/suite_test.go

@@ -127,11 +127,6 @@ type registryOptions struct {
 	withBasicAuth      bool
 	withTLS            bool
 	withClientCertAuth bool
-	// Allow disbaling DNS mocking since Helm OCI doesn't yet suppot
-	// insecure OCI registries, which means we need Docker's automatic
-	// connection downgrading if the registry is hosted on localhost.
-	// Once Helm OCI supports insecure registries, we can get rid of this.
-	disableDNSMocking bool
 }
 
 func setupRegistryServer(ctx context.Context, workspaceDir string, opts registryOptions) (*registryClientTestServer, error) {
@@ -158,27 +153,23 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry
 		return nil, fmt.Errorf("failed to get free port: %s", err)
 	}
 
-	server.registryHost = fmt.Sprintf("localhost:%d", port)
-
 	// Change the registry host to a host which is not localhost and
 	// mock DNS to map example.com to 127.0.0.1.
 	// This is required because Docker enforces HTTP if the registry
 	// is hosted on localhost/127.0.0.1.
-	if !opts.disableDNSMocking {
-		server.registryHost = fmt.Sprintf("example.com:%d", port)
-		// Disable DNS server logging as it is extremely chatty.
-		dnsLog := log.Default()
-		dnsLog.SetOutput(io.Discard)
-		server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{
-			"example.com.": {
-				A: []string{"127.0.0.1"},
-			},
-		}, dnsLog, false)
-		if err != nil {
-			return nil, err
-		}
-		server.dnsServer.PatchNet(net.DefaultResolver)
+	server.registryHost = fmt.Sprintf("example.com:%d", port)
+	// Disable DNS server logging as it is extremely chatty.
+	dnsLog := log.Default()
+	dnsLog.SetOutput(io.Discard)
+	server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{
+		"example.com.": {
+			A: []string{"127.0.0.1"},
+		},
+	}, dnsLog, false)
+	if err != nil {
+		return nil, err
 	}
+	server.dnsServer.PatchNet(net.DefaultResolver)
 
 	config.HTTP.Addr = fmt.Sprintf(":%d", port)
 	config.HTTP.DrainTimeout = time.Duration(10) * time.Second
@@ -219,6 +210,8 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry
 			return nil, fmt.Errorf("failed to create TLS configured HTTP client: %s", err)
 		}
 		clientOpts = append(clientOpts, helmreg.ClientOptHTTPClient(httpClient))
+	} else {
+		clientOpts = append(clientOpts, helmreg.ClientOptPlainHTTP())
 	}
 
 	// setup logger options
@@ -312,8 +305,7 @@ func TestMain(m *testing.M) {
 		panic(fmt.Sprintf("failed to create workspace directory: %v", err))
 	}
 	testRegistryServer, err = setupRegistryServer(ctx, testWorkspaceDir, registryOptions{
-		withBasicAuth:     true,
-		disableDNSMocking: true,
+		withBasicAuth: true,
 	})
 	if err != nil {
 		panic(fmt.Sprintf("Failed to create a test registry server: %v", err))

internal/helm/registry/client.go

@@ -29,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(tlsConfig *tls.Config, isLogin bool) (*registry.Client, string, error) {
+func ClientGenerator(tlsConfig *tls.Config, isLogin, insecureHTTP 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.
@@ -39,7 +39,7 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str
 		}
 
 		var errs []error
-		rClient, err := newClient(credentialsFile.Name(), tlsConfig)
+		rClient, err := newClient(credentialsFile.Name(), tlsConfig, insecureHTTP)
 		if err != nil {
 			errs = append(errs, err)
 			// attempt to delete the temporary file
@@ -54,17 +54,20 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str
 		return rClient, credentialsFile.Name(), nil
 	}
 
-	rClient, err := newClient("", tlsConfig)
+	rClient, err := newClient("", tlsConfig, insecureHTTP)
 	if err != nil {
 		return nil, "", err
 	}
 	return rClient, "", nil
 }
 
-func newClient(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) {
+func newClient(credentialsFile string, tlsConfig *tls.Config, insecureHTTP bool) (*registry.Client, error) {
 	opts := []registry.ClientOption{
 		registry.ClientOptWriter(io.Discard),
 	}
+	if insecureHTTP {
+		opts = append(opts, registry.ClientOptPlainHTTP())
+	}
 	if tlsConfig != nil {
 		opts = append(opts, registry.ClientOptHTTPClient(&http.Client{
 			Transport: &http.Transport{