diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index 4101987b2..29e8ba525 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/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) diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index 72adb20bc..4301f0630 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/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 diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index b0fe8ab2b..3fc9d8250 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/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 { diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 82aa602f6..b09e95779 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/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()) } diff --git a/internal/helm/repository/oci_chart_repository_test.go b/internal/helm/repository/oci_chart_repository_test.go index 7f281c62d..b63aef8da 100644 --- a/internal/helm/repository/oci_chart_repository_test.go +++ b/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") +}