helmrepo: allow OCI helmrepos to connect to insecure registries

Signed-off-by: Sanskar Jaiswal <[email protected]>

Author: aryan9600

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/getter/client_opts.go

@@ -74,7 +74,6 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
 			helmgetter.WithURL(url),
 			helmgetter.WithTimeout(obj.GetTimeout()),
 			helmgetter.WithPassCredentialsAll(obj.Spec.PassCredentials),
-			helmgetter.WithPlainHTTP(obj.Spec.Insecure),
 		},
 	}
 	ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI

internal/helm/getter/client_opts_test.go

@@ -68,7 +68,7 @@ func TestGetClientOpts(t *testing.T) {
 			},
 			afterFunc: func(t *WithT, hcOpts *ClientOpts) {
 				t.Expect(hcOpts.TlsConfig).ToNot(BeNil())
-				t.Expect(len(hcOpts.GetterOpts)).To(Equal(5))
+				t.Expect(len(hcOpts.GetterOpts)).To(Equal(4))
 			},
 		},
 		{
@@ -85,7 +85,7 @@ func TestGetClientOpts(t *testing.T) {
 			},
 			afterFunc: func(t *WithT, hcOpts *ClientOpts) {
 				t.Expect(hcOpts.TlsConfig).ToNot(BeNil())
-				t.Expect(len(hcOpts.GetterOpts)).To(Equal(5))
+				t.Expect(len(hcOpts.GetterOpts)).To(Equal(4))
 			},
 			err: ErrDeprecatedTLSConfig,
 		},

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{

internal/helm/repository/oci_chart_repository.go

@@ -75,6 +75,9 @@ type OCIChartRepository struct {
 
 	// verifiers is a list of verifiers to use when verifying a chart.
 	verifiers []oci.Verifier
+
+	// insecureHTTP indicates that the chart is hosted on an insecure registry.
+	insecure bool
 }
 
 // OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository
@@ -89,6 +92,13 @@ func WithVerifiers(verifiers []oci.Verifier) OCIChartRepositoryOption {
 	}
 }
 
+func WithInsecureHTTP() OCIChartRepositoryOption {
+	return func(r *OCIChartRepository) error {
+		r.insecure = true
+		return nil
+	}
+}
+
 // WithOCIRegistryClient returns a ChartRepositoryOption that will set the registry client
 func WithOCIRegistryClient(client RegistryClient) OCIChartRepositoryOption {
 	return func(r *OCIChartRepository) error {
@@ -358,7 +368,12 @@ func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartV
 		return fmt.Errorf("chart '%s' has no downloadable URLs", chart.Name)
 	}
 
-	ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme)))
+	var nameOpts []name.Option
+	if r.insecure {
+		nameOpts = append(nameOpts, name.Insecure)
+	}
+
+	ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme)), nameOpts...)
 	if err != nil {
 		return fmt.Errorf("invalid chart reference: %s", err)
 	}