Skip to content

Commit 3c87ad6

Browse files
hiddecohiddeco
committed
controller: verify digest of artifact in storage
This commits adds verification of the digest of the artifact in storage to all reconcilers which manage artifacts. When the artifact does not have a digest or if it mismatches with the file in storage, the file is removed from the storage and status of the object. This hardens the storage against potential tampering, in addition to resolving an issue where users upgrading from a (much) older version of the controller would run into an error after the checksum field was removed from the API. This would cause the controller to not advertise any checksum at all, while not producing a new one until a new revision was detected. Resulting in fetch failures for consumers while they would try to verify the digest of the advertised artifact. While not strictly part of this exercise, some of the tests were altered to prepare the storage used in test cases to become isolated by strictly using the `storage` provided via the callback. Actually isolating this has however been left as a task at a later moment. Signed-off-by: Hidde Beydals <[email protected]>
1 parent 67e9c94 commit 3c87ad6

12 files changed

+514
-70
lines changed

‎internal/controller/bucket_controller.go‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -363,14 +363,32 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, sp *patch.Seria
363363
// Garbage collect previous advertised artifact(s) from storage
364364
_ = r.garbageCollect(ctx, obj)
365365

366-
// Determine if the advertised artifact is still in storage
367366
var artifactMissing bool
368-
if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) {
369-
obj.Status.Artifact = nil
370-
obj.Status.URL = ""
371-
artifactMissing = true
372-
// Remove the condition as the artifact doesn't exist.
373-
conditions.Delete(obj, sourcev1.ArtifactInStorageCondition)
367+
if artifact := obj.GetArtifact(); artifact != nil {
368+
// Determine if the advertised artifact is still in storage
369+
if !r.Storage.ArtifactExist(*artifact) {
370+
artifactMissing = true
371+
}
372+
373+
// If the artifact is in storage, verify if the advertised digest still
374+
// matches the actual artifact
375+
if !artifactMissing {
376+
if err := r.Storage.VerifyArtifact(*artifact); err != nil {
377+
r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error())
378+
379+
if err = r.Storage.Remove(*artifact); err != nil {
380+
return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err)
381+
}
382+
383+
artifactMissing = true
384+
}
385+
}
386+
387+
// If the artifact is missing, remove it from the object
388+
if artifactMissing {
389+
obj.Status.Artifact = nil
390+
obj.Status.URL = ""
391+
}
374392
}
375393

376394
// Record that we do not have an artifact

‎internal/controller/bucket_controller_test.go‎

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -177,17 +177,17 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
177177
Path: fmt.Sprintf("/reconcile-storage/%s.txt", v),
178178
Revision: v,
179179
}
180-
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
180+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
181181
return err
182182
}
183-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil {
183+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil {
184184
return err
185185
}
186186
if n != len(revisions)-1 {
187187
time.Sleep(time.Second * 1)
188188
}
189189
}
190-
testStorage.SetArtifactURL(obj.Status.Artifact)
190+
storage.SetArtifactURL(obj.Status.Artifact)
191191
conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar")
192192
return nil
193193
},
@@ -224,7 +224,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
224224
Path: fmt.Sprintf("/reconcile-storage/invalid.txt"),
225225
Revision: "d",
226226
}
227-
testStorage.SetArtifactURL(obj.Status.Artifact)
227+
storage.SetArtifactURL(obj.Status.Artifact)
228228
return nil
229229
},
230230
want: sreconcile.ResultSuccess,
@@ -236,6 +236,68 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
236236
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
237237
},
238238
},
239+
{
240+
name: "notices empty artifact digest",
241+
beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error {
242+
f := "empty-digest.txt"
243+
244+
obj.Status.Artifact = &sourcev1.Artifact{
245+
Path: fmt.Sprintf("/reconcile-storage/%s.txt", f),
246+
Revision: "fake",
247+
}
248+
249+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
250+
return err
251+
}
252+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil {
253+
return err
254+
}
255+
256+
// Overwrite with a different digest
257+
obj.Status.Artifact.Digest = ""
258+
259+
return nil
260+
},
261+
want: sreconcile.ResultSuccess,
262+
assertPaths: []string{
263+
"!/reconcile-storage/empty-digest.txt",
264+
},
265+
assertConditions: []metav1.Condition{
266+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
267+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
268+
},
269+
},
270+
{
271+
name: "notices artifact digest mismatch",
272+
beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error {
273+
f := "digest-mismatch.txt"
274+
275+
obj.Status.Artifact = &sourcev1.Artifact{
276+
Path: fmt.Sprintf("/reconcile-storage/%s.txt", f),
277+
Revision: "fake",
278+
}
279+
280+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
281+
return err
282+
}
283+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil {
284+
return err
285+
}
286+
287+
// Overwrite with a different digest
288+
obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023"
289+
290+
return nil
291+
},
292+
want: sreconcile.ResultSuccess,
293+
assertPaths: []string{
294+
"!/reconcile-storage/digest-mismatch.txt",
295+
},
296+
assertConditions: []metav1.Condition{
297+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
298+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
299+
},
300+
},
239301
{
240302
name: "updates hostname on diff from current",
241303
beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error {
@@ -245,10 +307,10 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
245307
Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
246308
URL: "http://outdated.com/reconcile-storage/hostname.txt",
247309
}
248-
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
310+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
249311
return err
250312
}
251-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil {
313+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil {
252314
return err
253315
}
254316
conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar")

‎internal/controller/gitrepository_controller.go‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -387,13 +387,31 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patc
387387
// Garbage collect previous advertised artifact(s) from storage
388388
_ = r.garbageCollect(ctx, obj)
389389

390-
// Determine if the advertised artifact is still in storage
391390
var artifactMissing bool
392-
if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) {
393-
obj.Status.Artifact = nil
394-
artifactMissing = true
395-
// Remove the condition as the artifact doesn't exist.
396-
conditions.Delete(obj, sourcev1.ArtifactInStorageCondition)
391+
if artifact := obj.GetArtifact(); artifact != nil {
392+
// Determine if the advertised artifact is still in storage
393+
if !r.Storage.ArtifactExist(*artifact) {
394+
artifactMissing = true
395+
}
396+
397+
// If the artifact is in storage, verify if the advertised digest still
398+
// matches the actual artifact
399+
if !artifactMissing {
400+
if err := r.Storage.VerifyArtifact(*artifact); err != nil {
401+
r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error())
402+
403+
if err = r.Storage.Remove(*artifact); err != nil {
404+
return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err)
405+
}
406+
407+
artifactMissing = true
408+
}
409+
}
410+
411+
// If the artifact is missing, remove it from the object
412+
if artifactMissing {
413+
obj.Status.Artifact = nil
414+
}
397415
}
398416

399417
// Record that we do not have an artifact

‎internal/controller/gitrepository_controller_test.go‎

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1225,17 +1225,17 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
12251225
Path: fmt.Sprintf("/reconcile-storage/%s.txt", v),
12261226
Revision: v,
12271227
}
1228-
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
1228+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
12291229
return err
12301230
}
1231-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil {
1231+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o640); err != nil {
12321232
return err
12331233
}
12341234
if n != len(revisions)-1 {
12351235
time.Sleep(time.Second * 1)
12361236
}
12371237
}
1238-
testStorage.SetArtifactURL(obj.Status.Artifact)
1238+
storage.SetArtifactURL(obj.Status.Artifact)
12391239
conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar")
12401240
return nil
12411241
},
@@ -1272,7 +1272,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
12721272
Path: "/reconcile-storage/invalid.txt",
12731273
Revision: "e",
12741274
}
1275-
testStorage.SetArtifactURL(obj.Status.Artifact)
1275+
storage.SetArtifactURL(obj.Status.Artifact)
12761276
return nil
12771277
},
12781278
want: sreconcile.ResultSuccess,
@@ -1284,6 +1284,68 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
12841284
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
12851285
},
12861286
},
1287+
{
1288+
name: "notices empty artifact digest",
1289+
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
1290+
f := "empty-digest.txt"
1291+
1292+
obj.Status.Artifact = &sourcev1.Artifact{
1293+
Path: fmt.Sprintf("/reconcile-storage/%s.txt", f),
1294+
Revision: "fake",
1295+
}
1296+
1297+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
1298+
return err
1299+
}
1300+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil {
1301+
return err
1302+
}
1303+
1304+
// Overwrite with a different digest
1305+
obj.Status.Artifact.Digest = ""
1306+
1307+
return nil
1308+
},
1309+
want: sreconcile.ResultSuccess,
1310+
assertPaths: []string{
1311+
"!/reconcile-storage/empty-digest.txt",
1312+
},
1313+
assertConditions: []metav1.Condition{
1314+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
1315+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
1316+
},
1317+
},
1318+
{
1319+
name: "notices artifact digest mismatch",
1320+
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
1321+
f := "digest-mismatch.txt"
1322+
1323+
obj.Status.Artifact = &sourcev1.Artifact{
1324+
Path: fmt.Sprintf("/reconcile-storage/%s.txt", f),
1325+
Revision: "fake",
1326+
}
1327+
1328+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
1329+
return err
1330+
}
1331+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(f), 0o600); err != nil {
1332+
return err
1333+
}
1334+
1335+
// Overwrite with a different digest
1336+
obj.Status.Artifact.Digest = "sha256:6c329d5322473f904e2f908a51c12efa0ca8aa4201dd84f2c9d203a6ab3e9023"
1337+
1338+
return nil
1339+
},
1340+
want: sreconcile.ResultSuccess,
1341+
assertPaths: []string{
1342+
"!/reconcile-storage/digest-mismatch.txt",
1343+
},
1344+
assertConditions: []metav1.Condition{
1345+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
1346+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: disappeared from storage"),
1347+
},
1348+
},
12871349
{
12881350
name: "updates hostname on diff from current",
12891351
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
@@ -1293,10 +1355,10 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
12931355
Digest: "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
12941356
URL: "http://outdated.com/reconcile-storage/hostname.txt",
12951357
}
1296-
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
1358+
if err := storage.MkdirAll(*obj.Status.Artifact); err != nil {
12971359
return err
12981360
}
1299-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil {
1361+
if err := storage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o640); err != nil {
13001362
return err
13011363
}
13021364
conditions.MarkTrue(obj, meta.ReadyCondition, "foo", "bar")

‎internal/controller/helmchart_controller.go‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -364,14 +364,32 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, sp *patch.Se
364364
// Garbage collect previous advertised artifact(s) from storage
365365
_ = r.garbageCollect(ctx, obj)
366366

367-
// Determine if the advertised artifact is still in storage
368367
var artifactMissing bool
369-
if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) {
370-
obj.Status.Artifact = nil
371-
obj.Status.URL = ""
372-
artifactMissing = true
373-
// Remove the condition as the artifact doesn't exist.
374-
conditions.Delete(obj, sourcev1.ArtifactInStorageCondition)
368+
if artifact := obj.GetArtifact(); artifact != nil {
369+
// Determine if the advertised artifact is still in storage
370+
if !r.Storage.ArtifactExist(*artifact) {
371+
artifactMissing = true
372+
}
373+
374+
// If the artifact is in storage, verify if the advertised digest still
375+
// matches the actual artifact
376+
if !artifactMissing {
377+
if err := r.Storage.VerifyArtifact(*artifact); err != nil {
378+
r.Eventf(obj, corev1.EventTypeWarning, "ArtifactVerificationFailed", "failed to verify integrity of artifact: %s", err.Error())
379+
380+
if err = r.Storage.Remove(*artifact); err != nil {
381+
return sreconcile.ResultEmpty, fmt.Errorf("failed to remove artifact after digest mismatch: %w", err)
382+
}
383+
384+
artifactMissing = true
385+
}
386+
}
387+
388+
// If the artifact is missing, remove it from the object
389+
if artifactMissing {
390+
obj.Status.Artifact = nil
391+
obj.Status.URL = ""
392+
}
375393
}
376394

377395
// Record that we do not have an artifact

0 commit comments

Comments
 (0)