[release/v1.9.x] fix: pin OCI chart verification by digest

internal/controller/helmchart_controller_test.go

@@ -2908,7 +2908,8 @@ func TestHelmChartRepository_reconcileSource_verifyOCISourceSignature_keyless(t
 					},
 				},
 			}
-			chartUrl := fmt.Sprintf("%s/%s:%s", repository.Spec.URL, obj.Spec.Chart, obj.Spec.Version)
+			chartDigest := tt.revision[strings.Index(tt.revision, "@")+1:]
+			chartUrl := fmt.Sprintf("%s/%s@%s", repository.Spec.URL, obj.Spec.Chart, chartDigest)
 
 			assertConditions := tt.assertConditions
 			for k := range assertConditions {
@@ -3210,7 +3211,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignatureNotation(t *t
 				},
 			}
 
-			chartUrl := fmt.Sprintf("oci://%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
+			chartDesc, err := server.registryClient.Resolve(fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version))
+			g.Expect(err).ToNot(HaveOccurred())
+			chartUrl := fmt.Sprintf("oci://%s/testrepo/%s@%s", server.registryHost, metadata.Name, chartDesc.Digest)
 
 			if tt.beforeFunc != nil {
 				tt.beforeFunc(obj)
@@ -3462,7 +3465,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignatureCosign(t *tes
 				},
 			}
 
-			chartUrl := fmt.Sprintf("oci://%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
+			chartDesc, err := server.registryClient.Resolve(fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version))
+			g.Expect(err).ToNot(HaveOccurred())
+			chartUrl := fmt.Sprintf("oci://%s/testrepo/%s@%s", server.registryHost, metadata.Name, chartDesc.Digest)
 
 			if tt.beforeFunc != nil {
 				tt.beforeFunc(obj)

internal/helm/chart/builder_remote_test.go

@@ -28,6 +28,8 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"helm.sh/helm/v4/pkg/chart/common"
 	helmchart "helm.sh/helm/v4/pkg/chart/v2"
 	chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
@@ -51,6 +53,11 @@ func (m *mockRegistryClient) Tags(url string) ([]string, error) {
 	return nil, fmt.Errorf("no tags found for %s", url)
 }
 
+func (m *mockRegistryClient) Resolve(ref string) (ocispec.Descriptor, error) {
+	m.requestedURL = ref
+	return ocispec.Descriptor{Digest: digest.FromString(ref)}, nil
+}
+
 func (m *mockRegistryClient) Login(url string, opts ...registry.LoginOption) error {
 	m.requestedURL = url
 	return nil

internal/helm/chart/dependency_manager_test.go

@@ -28,6 +28,8 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	helmchart "helm.sh/helm/v4/pkg/chart/v2"
 	helmgetter "helm.sh/helm/v4/pkg/getter"
 	"helm.sh/helm/v4/pkg/registry"
@@ -41,6 +43,10 @@ type mockTagsGetter struct {
 	tags map[string][]string
 }
 
+func (m *mockTagsGetter) Resolve(ref string) (ocispec.Descriptor, error) {
+	return ocispec.Descriptor{Digest: digest.FromString(ref)}, nil
+}
+
 func (m *mockTagsGetter) Tags(requestURL string) ([]string, error) {
 	u, err := url.Parse(requestURL)
 	if err != nil {

internal/helm/repository/oci_chart_repository.go

@@ -33,6 +33,7 @@ import (
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/google/go-containerregistry/pkg/name"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 
 	"github.com/fluxcd/pkg/http/transport"
 	"github.com/fluxcd/pkg/version"
@@ -45,6 +46,7 @@ import (
 // from OCI registries
 type RegistryClient interface {
 	Tags(url string) ([]string, error)
+	Resolve(ref string) (ocispec.Descriptor, error)
 }
 
 // OCIChartRepository represents a Helm chart repository, and the configuration
@@ -312,11 +314,23 @@ func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartV
 		return oci.VerificationResultFailed, fmt.Errorf("invalid chart reference: %s", err)
 	}
 
+	// Resolve the reference to a digest and pin the chart URL to it,
+	// so verification and download refer to the same content.
+	desc, err := r.RegistryClient.Resolve(ref.String())
+	if err != nil {
+		return oci.VerificationResultFailed, fmt.Errorf("failed to resolve digest for '%s': %w", chart.URLs[0], err)
+	}
+	if err := desc.Digest.Validate(); err != nil {
+		return oci.VerificationResultFailed, fmt.Errorf("invalid digest resolved for '%s': %w", chart.URLs[0], err)
+	}
+	digestRef := ref.Context().Digest(desc.Digest.String())
+	chart.URLs[0] = fmt.Sprintf("%s://%s", registry.OCIScheme, digestRef.String())
+
 	verificationResult := oci.VerificationResultFailed
 
 	// verify the chart
 	for _, verifier := range r.verifiers {
-		result, err := verifier.Verify(ctx, ref)
+		result, err := verifier.Verify(ctx, digestRef)
 		if err != nil {
 			return result, fmt.Errorf("failed to verify %s: %w", chart.URLs[0], err)
 		}
@@ -330,5 +344,5 @@ func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartV
 		return verificationResult, nil
 	}
 
-	return oci.VerificationResultFailed, fmt.Errorf("no matching signatures were found for '%s'", ref.Name())
+	return oci.VerificationResultFailed, fmt.Errorf("no matching signatures were found for '%s'", digestRef.Name())
 }

internal/helm/repository/oci_chart_repository_test.go

@@ -18,17 +18,23 @@ package repository
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"net/url"
 	"path"
 	"strings"
 	"testing"
 
+	"github.com/google/go-containerregistry/pkg/name"
 	. "github.com/onsi/gomega"
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	helmgetter "helm.sh/helm/v4/pkg/getter"
 	"helm.sh/helm/v4/pkg/registry"
 	repo "helm.sh/helm/v4/pkg/repo/v1"
+
+	"github.com/fluxcd/source-controller/internal/oci"
 )
 
 type OCIMockGetter struct {
@@ -52,6 +58,11 @@ func (m *mockRegistryClient) Tags(urlStr string) ([]string, error) {
 	return m.tags, nil
 }
 
+func (m *mockRegistryClient) Resolve(ref string) (ocispec.Descriptor, error) {
+	m.LastCalledURL = ref
+	return ocispec.Descriptor{Digest: digest.FromString(ref)}, nil
+}
+
 func (m *mockRegistryClient) Login(url string, opts ...registry.LoginOption) error {
 	m.LastCalledURL = url
 	return nil
@@ -272,3 +283,191 @@ func TestOCIChartRepository_DownloadChart(t *testing.T) {
 		})
 	}
 }
+
+type mockChartVerifier struct {
+	result      oci.VerificationResult
+	err         error
+	verifiedRef name.Reference
+}
+
+func (v *mockChartVerifier) Verify(_ context.Context, ref name.Reference) (oci.VerificationResult, error) {
+	v.verifiedRef = ref
+	return v.result, v.err
+}
+
+func TestOCIChartRepository_VerifyChart(t *testing.T) {
+	g := NewWithT(t)
+
+	u, err := url.Parse("oci://localhost:5000/my_repo")
+	g.Expect(err).ToNot(HaveOccurred())
+
+	chartRef := "oci://localhost:5000/my_repo/podinfo:1.0.0"
+	// The digest the mock registry client resolves the tag to.
+	resolvedDigest := digest.FromString("localhost:5000/my_repo/podinfo:1.0.0")
+	pinnedRef := fmt.Sprintf("oci://localhost:5000/my_repo/podinfo@%s", resolvedDigest)
+
+	t.Run("verifies the resolved digest and pins the chart URL to it", func(t *testing.T) {
+		g := NewWithT(t)
+
+		verifier := &mockChartVerifier{result: oci.VerificationResultSuccess}
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClient{},
+			verifiers:      []oci.Verifier{verifier},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(result).To(Equal(oci.VerificationResultSuccess))
+		g.Expect(verifier.verifiedRef.String()).To(Equal(fmt.Sprintf("localhost:5000/my_repo/podinfo@%s", resolvedDigest)))
+		g.Expect(cv.URLs[0]).To(Equal(pinnedRef))
+	})
+
+	t.Run("fails when the digest can not be resolved", func(t *testing.T) {
+		g := NewWithT(t)
+
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClientResolveErr{},
+			verifiers:      []oci.Verifier{&mockChartVerifier{result: oci.VerificationResultSuccess}},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("failed to resolve digest"))
+		g.Expect(result).To(Equal(oci.VerificationResultFailed))
+		g.Expect(cv.URLs[0]).To(Equal(chartRef))
+	})
+
+	t.Run("fails on an invalid resolved digest", func(t *testing.T) {
+		g := NewWithT(t)
+
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClientInvalidDigest{},
+			verifiers:      []oci.Verifier{&mockChartVerifier{result: oci.VerificationResultSuccess}},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("invalid digest"))
+		g.Expect(result).To(Equal(oci.VerificationResultFailed))
+		g.Expect(cv.URLs[0]).To(Equal(chartRef))
+	})
+
+	t.Run("keeps an already pinned digest reference", func(t *testing.T) {
+		g := NewWithT(t)
+
+		verifier := &mockChartVerifier{result: oci.VerificationResultSuccess}
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClientFixedDigest{dig: resolvedDigest},
+			verifiers:      []oci.Verifier{verifier},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{pinnedRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(result).To(Equal(oci.VerificationResultSuccess))
+		g.Expect(cv.URLs[0]).To(Equal(pinnedRef))
+	})
+
+	t.Run("pins the chart URL when the result is ignored", func(t *testing.T) {
+		g := NewWithT(t)
+
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClient{},
+			verifiers:      []oci.Verifier{&mockChartVerifier{result: oci.VerificationResultIgnored}},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(result).To(Equal(oci.VerificationResultIgnored))
+		g.Expect(cv.URLs[0]).To(Equal(pinnedRef))
+	})
+
+	t.Run("downloads the reference that was verified", func(t *testing.T) {
+		g := NewWithT(t)
+
+		mg := OCIMockGetter{}
+		r := OCIChartRepository{
+			URL:            *u,
+			Client:         &mg,
+			RegistryClient: &mockRegistryClient{},
+			verifiers:      []oci.Verifier{&mockChartVerifier{result: oci.VerificationResultSuccess}},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		_, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		_, err = r.DownloadChart(cv)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(mg.LastCalledURL).To(Equal(fmt.Sprintf("localhost:5000/my_repo/podinfo@%s", resolvedDigest)))
+	})
+
+	t.Run("fails without verifiers", func(t *testing.T) {
+		g := NewWithT(t)
+
+		r := OCIChartRepository{
+			URL:            *u,
+			RegistryClient: &mockRegistryClient{},
+		}
+
+		cv := &repo.ChartVersion{
+			Metadata: &chart.Metadata{Name: "podinfo", Version: "1.0.0"},
+			URLs:     []string{chartRef},
+		}
+		result, err := r.VerifyChart(t.Context(), cv)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(result).To(Equal(oci.VerificationResultFailed))
+	})
+}
+
+type mockRegistryClientInvalidDigest struct {
+	mockRegistryClient
+}
+
+func (m *mockRegistryClientInvalidDigest) Resolve(ref string) (ocispec.Descriptor, error) {
+	return ocispec.Descriptor{}, nil
+}
+
+type mockRegistryClientFixedDigest struct {
+	mockRegistryClient
+	dig digest.Digest
+}
+
+func (m *mockRegistryClientFixedDigest) Resolve(ref string) (ocispec.Descriptor, error) {
+	return ocispec.Descriptor{Digest: m.dig}, nil
+}
+
+type mockRegistryClientResolveErr struct {
+	mockRegistryClient
+}
+
+func (m *mockRegistryClientResolveErr) Resolve(ref string) (ocispec.Descriptor, error) {
+	return ocispec.Descriptor{}, fmt.Errorf("manifest unknown")
+}