diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 33f3198b..fae94c72 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -363,14 +363,32 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) - // Determine if the advertised artifact is still in storage var artifactMissing bool - if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { - obj.Status.Artifact = nil - obj.Status.URL = "" - artifactMissing = true - // Remove the condition as the artifact doesn't exist. - conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if artifact := obj.GetArtifact(); artifact != nil { + // Determine if the advertised artifact is still in storage + if !r.Storage.ArtifactExist(*artifact) { + artifactMissing = true + } + + // If the artifact is in storage, verify if the advertised digest still + // matches the actual artifact + if !artifactMissing { + if err := r.Storage.VerifyArtifact(*artifact); err != nil { + r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error()) + + if err = r.Storage.Remove(*artifact); err != nil { + return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err) + } + + artifactMissing = true + } + } + + // If the artifact is missing, remove it from the object + if artifactMissing { + obj.Status.Artifact = nil + obj.Status.URL = "" + } } // Record that we do not have an artifact diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go index f7c67f0b..eb9cf102 100644 --- a/internal/controller/bucket_controller_test.go +++ b/internal/controller/bucket_controller_test.go @@ -177,17 +177,17 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/reconcile-storage/%s.txt", v), Revision: v, } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { return err } if n != len(revisions)-1 { time.Sleep(time.Second * 1) } } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, @@ -224,7 +224,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/reconcile-storage/invalid.txt"), Revision: "d", } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) return nil }, want: sreconcile.ResultSuccess, @@ -236,6 +236,68 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, + { + name: "notices empty artifact digest", + beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error { + f := "empty-digest.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/empty-digest.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, + { + name: "notices artifact digest mismatch", + beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error { + f := "digest-mismatch.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/digest-mismatch.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, { name: "updates hostname on diff from current", beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error { @@ -245,10 +307,10 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) { Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80", URL: "http://outdated.com/reconcile-storage/hostname.txt", } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index 1763da17..e47c938f 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -387,13 +387,31 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patc // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) - // Determine if the advertised artifact is still in storage var artifactMissing bool - if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { - obj.Status.Artifact = nil - artifactMissing = true - // Remove the condition as the artifact doesn't exist. - conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if artifact := obj.GetArtifact(); artifact != nil { + // Determine if the advertised artifact is still in storage + if !r.Storage.ArtifactExist(*artifact) { + artifactMissing = true + } + + // If the artifact is in storage, verify if the advertised digest still + // matches the actual artifact + if !artifactMissing { + if err := r.Storage.VerifyArtifact(*artifact); err != nil { + r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error()) + + if err = r.Storage.Remove(*artifact); err != nil { + return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err) + } + + artifactMissing = true + } + } + + // If the artifact is missing, remove it from the object + if artifactMissing { + obj.Status.Artifact = nil + } } // Record that we do not have an artifact diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 6dbf8069..ccd02519 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -1225,17 +1225,17 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/reconcile-storage/%s.txt", v), Revision: v, } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { return err } if n != len(revisions)-1 { time.Sleep(time.Second * 1) } } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, @@ -1272,7 +1272,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { Path: "/reconcile-storage/invalid.txt", Revision: "e", } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) return nil }, want: sreconcile.ResultSuccess, @@ -1284,6 +1284,68 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, + { + name: "notices empty artifact digest", + beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error { + f := "empty-digest.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/empty-digest.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, + { + name: "notices artifact digest mismatch", + beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error { + f := "digest-mismatch.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/digest-mismatch.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, { name: "updates hostname on diff from current", beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error { @@ -1293,10 +1355,10 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80", URL: "http://outdated.com/reconcile-storage/hostname.txt", } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 1b60dcae..f4222502 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -364,14 +364,32 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, sp *patch.Se // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) - // Determine if the advertised artifact is still in storage var artifactMissing bool - if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { - obj.Status.Artifact = nil - obj.Status.URL = "" - artifactMissing = true - // Remove the condition as the artifact doesn't exist. - conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if artifact := obj.GetArtifact(); artifact != nil { + // Determine if the advertised artifact is still in storage + if !r.Storage.ArtifactExist(*artifact) { + artifactMissing = true + } + + // If the artifact is in storage, verify if the advertised digest still + // matches the actual artifact + if !artifactMissing { + if err := r.Storage.VerifyArtifact(*artifact); err != nil { + r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error()) + + if err = r.Storage.Remove(*artifact); err != nil { + return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err) + } + + artifactMissing = true + } + } + + // If the artifact is missing, remove it from the object + if artifactMissing { + obj.Status.Artifact = nil + obj.Status.URL = "" + } } // Record that we do not have an artifact diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index e3ca0b43..b862b167 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -298,17 +298,17 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/reconcile-storage/%s.txt", v), Revision: v, } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { return err } if n != len(revisions)-1 { time.Sleep(time.Second * 1) } } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, @@ -345,7 +345,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { Path: "/reconcile-storage/invalid.txt", Revision: "d", } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) return nil }, want: sreconcile.ResultSuccess, @@ -357,6 +357,68 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, + { + name: "notices empty artifact digest", + beforeFunc: func(obj *helmv1.HelmChart, storage *Storage) error { + f := "empty-digest.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/empty-digest.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, + { + name: "notices artifact digest mismatch", + beforeFunc: func(obj *helmv1.HelmChart, storage *Storage) error { + f := "digest-mismatch.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/digest-mismatch.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, { name: "updates hostname on diff from current", beforeFunc: func(obj *helmv1.HelmChart, storage *Storage) error { @@ -366,10 +428,10 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) { Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80", URL: "http://outdated.com/reconcile-storage/hostname.txt", } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 835e7d61..52ee07e3 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -332,14 +332,32 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, sp *pat // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) - // Determine if the advertised artifact is still in storage var artifactMissing bool - if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { - obj.Status.Artifact = nil - obj.Status.URL = "" - artifactMissing = true - // Remove the condition as the artifact doesn't exist. - conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if artifact := obj.GetArtifact(); artifact != nil { + // Determine if the advertised artifact is still in storage + if !r.Storage.ArtifactExist(*artifact) { + artifactMissing = true + } + + // If the artifact is in storage, verify if the advertised digest still + // matches the actual artifact + if !artifactMissing { + if err := r.Storage.VerifyArtifact(*artifact); err != nil { + r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error()) + + if err = r.Storage.Remove(*artifact); err != nil { + return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err) + } + + artifactMissing = true + } + } + + // If the artifact is missing, remove it from the object + if artifactMissing { + obj.Status.Artifact = nil + obj.Status.URL = "" + } } // Record that we do not have an artifact diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 3cf6a6ec..feb7c690 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -153,17 +153,17 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/reconcile-storage/%s.txt", v), Revision: v, } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { return err } if n != len(revisions)-1 { time.Sleep(time.Second * 1) } } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, @@ -200,7 +200,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { Path: "/reconcile-storage/invalid.txt", Revision: "d", } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) return nil }, want: sreconcile.ResultSuccess, @@ -212,6 +212,68 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, + { + name: "notices empty artifact digest", + beforeFunc: func(obj *helmv1.HelmRepository, storage *Storage) error { + f := "empty-digest.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/empty-digest.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, + { + name: "notices artifact digest mismatch", + beforeFunc: func(obj *helmv1.HelmRepository, storage *Storage) error { + f := "digest-mismatch.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/reconcile-storage/digest-mismatch.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, { name: "updates hostname on diff from current", beforeFunc: func(obj *helmv1.HelmRepository, storage *Storage) error { @@ -221,10 +283,10 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) { Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80", URL: "http://outdated.com/reconcile-storage/hostname.txt", } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index f33d94e7..8dc4d197 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -910,14 +910,32 @@ func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patc // Garbage collect previous advertised artifact(s) from storage _ = r.garbageCollect(ctx, obj) - // Determine if the advertised artifact is still in storage var artifactMissing bool - if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { - obj.Status.Artifact = nil - obj.Status.URL = "" - artifactMissing = true - // Remove the condition as the artifact doesn't exist. - conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) + if artifact := obj.GetArtifact(); artifact != nil { + // Determine if the advertised artifact is still in storage + if !r.Storage.ArtifactExist(*artifact) { + artifactMissing = true + } + + // If the artifact is in storage, verify if the advertised digest still + // matches the actual artifact + if !artifactMissing { + if err := r.Storage.VerifyArtifact(*artifact); err != nil { + r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error()) + + if err = r.Storage.Remove(*artifact); err != nil { + return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err) + } + + artifactMissing = true + } + } + + // If the artifact is missing, remove it from the object + if artifactMissing { + obj.Status.Artifact = nil + obj.Status.URL = "" + } } // Record that we do not have an artifact diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index 9ec659a7..87905f6b 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -1828,7 +1828,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { tests := []struct { name string - beforeFunc func(obj *ociv1.OCIRepository) error + beforeFunc func(obj *ociv1.OCIRepository, storage *Storage) error want sreconcile.Result wantErr bool assertConditions []metav1.Condition @@ -1837,7 +1837,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { }{ { name: "garbage collects", - beforeFunc: func(obj *ociv1.OCIRepository) error { + beforeFunc: func(obj *ociv1.OCIRepository, storage *Storage) error { revisions := []string{"a", "b", "c", "d"} for n := range revisions { @@ -1846,11 +1846,11 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { Path: fmt.Sprintf("/oci-reconcile-storage/%s.txt", v), Revision: v, } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil { return err } @@ -1859,7 +1859,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { } } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") return nil }, @@ -1891,12 +1891,12 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { }, { name: "notices missing artifact in storage", - beforeFunc: func(obj *ociv1.OCIRepository) error { + beforeFunc: func(obj *ociv1.OCIRepository, storage *Storage) error { obj.Status.Artifact = &sourcev1.Artifact{ Path: "/oci-reconcile-storage/invalid.txt", Revision: "e", } - testStorage.SetArtifactURL(obj.Status.Artifact) + storage.SetArtifactURL(obj.Status.Artifact) return nil }, want: sreconcile.ResultSuccess, @@ -1908,19 +1908,81 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), }, }, + { + name: "notices empty artifact digest", + beforeFunc: func(obj *ociv1.OCIRepository, storage *Storage) error { + f := "empty-digest.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/oci-reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/oci-reconcile-storage/empty-digest.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, + { + name: "notices artifact digest mismatch", + beforeFunc: func(obj *ociv1.OCIRepository, storage *Storage) error { + f := "digest-mismatch.txt" + + obj.Status.Artifact = &sourcev1.Artifact{ + Path: fmt.Sprintf("/oci-reconcile-storage/%s.txt", f), + Revision: "fake", + } + + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { + return err + } + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil { + return err + } + + // Overwrite with a different digest + obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023" + + return nil + }, + want: sreconcile.ResultSuccess, + assertPaths: []string{ + "!/oci-reconcile-storage/digest-mismatch.txt", + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"), + }, + }, { name: "updates hostname on diff from current", - beforeFunc: func(obj *ociv1.OCIRepository) error { + beforeFunc: func(obj *ociv1.OCIRepository, storage *Storage) error { obj.Status.Artifact = &sourcev1.Artifact{ Path: "/oci-reconcile-storage/hostname.txt", Revision: "f", Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80", URL: "http://outdated.com/oci-reconcile-storage/hostname.txt", } - if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil { + if err := storage.MkdirAll(*obj.Status.Artifact); err != nil { return err } - if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { + if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil { return err } conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar") @@ -1962,7 +2024,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) { } if tt.beforeFunc != nil { - g.Expect(tt.beforeFunc(obj)).To(Succeed()) + g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed()) } g.Expect(r.Client.Create(ctx, obj)).ToNot(HaveOccurred()) diff --git a/internal/controller/storage.go b/internal/controller/storage.go index 98fb7359..ef1ac797 100644 --- a/internal/controller/storage.go +++ b/internal/controller/storage.go @@ -32,6 +32,7 @@ import ( securejoin "github.com/cyphar/filepath-securejoin" "github.com/fluxcd/go-git/v5/plumbing/format/gitignore" + "github.com/opencontainers/go-digest" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -84,7 +85,7 @@ func NewStorage(basePath string, hostname string, artifactRetentionTTL time.Dura } // NewArtifactFor returns a new v1.Artifact. -func (s *Storage) NewArtifactFor(kind string, metadata metav1.Object, revision, fileName string) v1.Artifact { +func (s Storage) NewArtifactFor(kind string, metadata metav1.Object, revision, fileName string) v1.Artifact { path := v1.ArtifactPath(kind, metadata.GetNamespace(), metadata.GetName(), fileName) artifact := v1.Artifact{ Path: path, @@ -117,13 +118,18 @@ func (s Storage) SetHostname(URL string) string { } // MkdirAll calls os.MkdirAll for the given v1.Artifact base dir. -func (s *Storage) MkdirAll(artifact v1.Artifact) error { +func (s Storage) MkdirAll(artifact v1.Artifact) error { dir := filepath.Dir(s.LocalPath(artifact)) return os.MkdirAll(dir, 0o700) } +// Remove calls os.Remove for the given v1.Artifact path. +func (s Storage) Remove(artifact v1.Artifact) error { + return os.Remove(s.LocalPath(artifact)) +} + // RemoveAll calls os.RemoveAll for the given v1.Artifact base dir. -func (s *Storage) RemoveAll(artifact v1.Artifact) (string, error) { +func (s Storage) RemoveAll(artifact v1.Artifact) (string, error) { var deletedDir string dir := filepath.Dir(s.LocalPath(artifact)) // Check if the dir exists. @@ -135,7 +141,7 @@ func (s *Storage) RemoveAll(artifact v1.Artifact) (string, error) { } // RemoveAllButCurrent removes all files for the given v1.Artifact base dir, excluding the current one. -func (s *Storage) RemoveAllButCurrent(artifact v1.Artifact) ([]string, error) { +func (s Storage) RemoveAllButCurrent(artifact v1.Artifact) ([]string, error) { deletedFiles := []string{} localPath := s.LocalPath(artifact) dir := filepath.Dir(localPath) @@ -168,7 +174,7 @@ func (s *Storage) RemoveAllButCurrent(artifact v1.Artifact) ([]string, error) { // 1. collect all artifact files with an expired ttl // 2. if we satisfy maxItemsToBeRetained, then return // 3. else, collect all artifact files till the latest n files remain, where n=maxItemsToBeRetained -func (s *Storage) getGarbageFiles(artifact v1.Artifact, totalCountLimit, maxItemsToBeRetained int, ttl time.Duration) (garbageFiles []string, _ error) { +func (s Storage) getGarbageFiles(artifact v1.Artifact, totalCountLimit, maxItemsToBeRetained int, ttl time.Duration) (garbageFiles []string, _ error) { localPath := s.LocalPath(artifact) dir := filepath.Dir(localPath) artifactFilesWithCreatedTs := make(map[time.Time]string) @@ -255,7 +261,7 @@ func (s *Storage) getGarbageFiles(artifact v1.Artifact, totalCountLimit, maxItem // GarbageCollect removes all garbage files in the artifact dir according to the provided // retention options. -func (s *Storage) GarbageCollect(ctx context.Context, artifact v1.Artifact, timeout time.Duration) ([]string, error) { +func (s Storage) GarbageCollect(ctx context.Context, artifact v1.Artifact, timeout time.Duration) ([]string, error) { delFilesChan := make(chan []string) errChan := make(chan error) // Abort if it takes more than the provided timeout duration. @@ -317,7 +323,7 @@ func stringInSlice(a string, list []string) bool { } // ArtifactExist returns a boolean indicating whether the v1.Artifact exists in storage and is a regular file. -func (s *Storage) ArtifactExist(artifact v1.Artifact) bool { +func (s Storage) ArtifactExist(artifact v1.Artifact) bool { fi, err := os.Lstat(s.LocalPath(artifact)) if err != nil { return false @@ -325,6 +331,35 @@ func (s *Storage) ArtifactExist(artifact v1.Artifact) bool { return fi.Mode().IsRegular() } +// VerifyArtifact verifies if the Digest of the v1.Artifact matches the digest +// of the file in Storage. It returns an error if the digests don't match, or +// if it can't be verified. +func (s Storage) VerifyArtifact(artifact v1.Artifact) error { + if artifact.Digest == "" { + return fmt.Errorf("artifact has no digest") + } + + d, err := digest.Parse(artifact.Digest) + if err != nil { + return fmt.Errorf("failed to parse artifact digest '%s': %w", artifact.Digest, err) + } + + f, err := os.Open(s.LocalPath(artifact)) + if err != nil { + return err + } + defer f.Close() + + verifier := d.Verifier() + if _, err = io.Copy(verifier, f); err != nil { + return err + } + if !verifier.Verified() { + return fmt.Errorf("computed digest doesn't match '%s'", d.String()) + } + return nil +} + // ArchiveFileFilter must return true if a file should not be included in the archive after inspecting the given path // and/or os.FileInfo. type ArchiveFileFilter func(p string, fi os.FileInfo) bool @@ -347,7 +382,7 @@ func SourceIgnoreFilter(ps []gitignore.Pattern, domain []string) ArchiveFileFilt // directories and any ArchiveFileFilter matches. While archiving, any environment specific data (for example, // the user and group name) is stripped from file headers. // If successful, it sets the digest and last update time on the artifact. -func (s *Storage) Archive(artifact *v1.Artifact, dir string, filter ArchiveFileFilter) (err error) { +func (s Storage) Archive(artifact *v1.Artifact, dir string, filter ArchiveFileFilter) (err error) { if f, err := os.Stat(dir); os.IsNotExist(err) || !f.IsDir() { return fmt.Errorf("invalid dir path: %s", dir) } @@ -469,7 +504,7 @@ func (s *Storage) Archive(artifact *v1.Artifact, dir string, filter ArchiveFileF // AtomicWriteFile atomically writes the io.Reader contents to the v1.Artifact path. // If successful, it sets the digest and last update time on the artifact. -func (s *Storage) AtomicWriteFile(artifact *v1.Artifact, reader io.Reader, mode os.FileMode) (err error) { +func (s Storage) AtomicWriteFile(artifact *v1.Artifact, reader io.Reader, mode os.FileMode) (err error) { localPath := s.LocalPath(*artifact) tf, err := os.CreateTemp(filepath.Split(localPath)) if err != nil { @@ -511,7 +546,7 @@ func (s *Storage) AtomicWriteFile(artifact *v1.Artifact, reader io.Reader, mode // Copy atomically copies the io.Reader contents to the v1.Artifact path. // If successful, it sets the digest and last update time on the artifact. -func (s *Storage) Copy(artifact *v1.Artifact, reader io.Reader) (err error) { +func (s Storage) Copy(artifact *v1.Artifact, reader io.Reader) (err error) { localPath := s.LocalPath(*artifact) tf, err := os.CreateTemp(filepath.Split(localPath)) if err != nil { @@ -549,7 +584,7 @@ func (s *Storage) Copy(artifact *v1.Artifact, reader io.Reader) (err error) { // CopyFromPath atomically copies the contents of the given path to the path of the v1.Artifact. // If successful, the digest and last update time on the artifact is set. -func (s *Storage) CopyFromPath(artifact *v1.Artifact, path string) (err error) { +func (s Storage) CopyFromPath(artifact *v1.Artifact, path string) (err error) { f, err := os.Open(path) if err != nil { return err @@ -564,7 +599,7 @@ func (s *Storage) CopyFromPath(artifact *v1.Artifact, path string) (err error) { } // CopyToPath copies the contents in the (sub)path of the given artifact to the given path. -func (s *Storage) CopyToPath(artifact *v1.Artifact, subPath, toPath string) error { +func (s Storage) CopyToPath(artifact *v1.Artifact, subPath, toPath string) error { // create a tmp directory to store artifact tmp, err := os.MkdirTemp("", "flux-include-") if err != nil { @@ -603,7 +638,7 @@ func (s *Storage) CopyToPath(artifact *v1.Artifact, subPath, toPath string) erro } // Symlink creates or updates a symbolic link for the given v1.Artifact and returns the URL for the symlink. -func (s *Storage) Symlink(artifact v1.Artifact, linkName string) (string, error) { +func (s Storage) Symlink(artifact v1.Artifact, linkName string) (string, error) { localPath := s.LocalPath(artifact) dir := filepath.Dir(localPath) link := filepath.Join(dir, linkName) @@ -625,14 +660,14 @@ func (s *Storage) Symlink(artifact v1.Artifact, linkName string) (string, error) } // Lock creates a file lock for the given v1.Artifact. -func (s *Storage) Lock(artifact v1.Artifact) (unlock func(), err error) { +func (s Storage) Lock(artifact v1.Artifact) (unlock func(), err error) { lockFile := s.LocalPath(artifact) + ".lock" mutex := lockedfile.MutexAt(lockFile) return mutex.Lock() } // LocalPath returns the secure local path of the given artifact (that is: relative to the Storage.BasePath). -func (s *Storage) LocalPath(artifact v1.Artifact) string { +func (s Storage) LocalPath(artifact v1.Artifact) string { if artifact.Path == "" { return "" } diff --git a/internal/controller/storage_test.go b/internal/controller/storage_test.go index bdf21b53..4d624e9f 100644 --- a/internal/controller/storage_test.go +++ b/internal/controller/storage_test.go @@ -18,8 +18,10 @@ package controller import ( "archive/tar" + "bytes" "compress/gzip" "context" + "errors" "fmt" "io" "os" @@ -265,6 +267,44 @@ func TestStorage_Archive(t *testing.T) { } } +func TestStorage_Remove(t *testing.T) { + t.Run("removes file", func(t *testing.T) { + g := NewWithT(t) + + dir := t.TempDir() + + s, err := NewStorage(dir, "", 0, 0) + g.Expect(err).ToNot(HaveOccurred()) + + artifact := sourcev1.Artifact{ + Path: filepath.Join(dir, "test.txt"), + } + g.Expect(s.MkdirAll(artifact)).To(Succeed()) + g.Expect(s.AtomicWriteFile(&artifact, bytes.NewReader([]byte("test")), 0o600)).To(Succeed()) + g.Expect(s.ArtifactExist(artifact)).To(BeTrue()) + + g.Expect(s.Remove(artifact)).To(Succeed()) + g.Expect(s.ArtifactExist(artifact)).To(BeFalse()) + }) + + t.Run("error if file does not exist", func(t *testing.T) { + g := NewWithT(t) + + dir := t.TempDir() + + s, err := NewStorage(dir, "", 0, 0) + g.Expect(err).ToNot(HaveOccurred()) + + artifact := sourcev1.Artifact{ + Path: filepath.Join(dir, "test.txt"), + } + + err = s.Remove(artifact) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue()) + }) +} + func TestStorageRemoveAllButCurrent(t *testing.T) { t.Run("bad directory in archive", func(t *testing.T) { dir := t.TempDir() @@ -718,3 +758,61 @@ func TestStorage_GarbageCollect(t *testing.T) { }) } } + +func TestStorage_VerifyArtifact(t *testing.T) { + g := NewWithT(t) + + dir := t.TempDir() + s, err := NewStorage(dir, "", 0, 0) + g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage") + + g.Expect(os.WriteFile(filepath.Join(dir, "artifact"), []byte("test"), 0o600)).To(Succeed()) + + t.Run("artifact without digest", func(t *testing.T) { + g := NewWithT(t) + + err := s.VerifyArtifact(sourcev1.Artifact{}) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError("artifact has no digest")) + }) + + t.Run("artifact with invalid digest", func(t *testing.T) { + g := NewWithT(t) + + err := s.VerifyArtifact(sourcev1.Artifact{Digest: "invalid"}) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError("failed to parse artifact digest 'invalid': invalid checksum digest format")) + }) + + t.Run("artifact with invalid path", func(t *testing.T) { + g := NewWithT(t) + + err := s.VerifyArtifact(sourcev1.Artifact{ + Digest: "sha256:9ba7a35ce8acd3557fe30680ef193ca7a36bb5dc62788f30de7122a0a5beab69", + Path: "invalid", + }) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue()) + }) + + t.Run("artifact with digest mismatch", func(t *testing.T) { + g := NewWithT(t) + + err := s.VerifyArtifact(sourcev1.Artifact{ + Digest: "sha256:9ba7a35ce8acd3557fe30680ef193ca7a36bb5dc62788f30de7122a0a5beab69", + Path: "artifact", + }) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError("computed digest doesn't match 'sha256:9ba7a35ce8acd3557fe30680ef193ca7a36bb5dc62788f30de7122a0a5beab69'")) + }) + + t.Run("artifact with digest match", func(t *testing.T) { + g := NewWithT(t) + + err := s.VerifyArtifact(sourcev1.Artifact{ + Digest: "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", + Path: "artifact", + }) + g.Expect(err).ToNot(HaveOccurred()) + }) +}