diff --git a/api/v1beta2/helmrepository_types.go b/api/v1beta2/helmrepository_types.go index 1c25a9eb..4e53fdfd 100644 --- a/api/v1beta2/helmrepository_types.go +++ b/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. diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml index a17ab56d..7eb709b9 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml +++ b/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 diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index 46649e23..04c3e328 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -874,6 +874,19 @@ efficient use of resources.

+insecure
+ +bool + + + +(Optional) +

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’.

+ + + + timeout
@@ -2593,6 +2606,19 @@ efficient use of resources.

+insecure
+ +bool + + + +(Optional) +

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’.

+ + + + timeout
diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index a05155eb..0fd33ed0 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/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 diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 41b90194..b8d23be5 100644 --- a/internal/controller/helmchart_controller.go +++ b/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) } diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index e8d4f64a..c7c753b9 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/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()) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index c52c8dc3..02fafa12 100644 --- a/internal/controller/suite_test.go +++ b/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)) diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index 7ac0d3d0..8f2b315c 100644 --- a/internal/helm/registry/client.go +++ b/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{ diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 6a119183..d1244e7c 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/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) }