Skip to content

Commit 9be6175

Browse files
wkingwking
committed
pkg/cvo/sync_worker: Make expected/actual version mismatch fatal
If, for example, the configured ClusterVersion spec.upstream advertised a given image as 4.0.1, but the version baked into the release's own metadata was 1.0.0-abc, report VerifyPayloadVersionFailed and continue to apply the previous release image, instead of optimistically moving on to apply the new release image. This protects users from compromised or man-in-the-middled upstreams who attempt downgrade and similar attacks by misrepresenting a recommended update. Addressing a FIXME from 58356de (*: Port from Update to Release for ClusterVersion status, 2020-07-24, #419).
1 parent 16d3d84 commit 9be6175

File tree

2 files changed

+19
-31
lines changed

2 files changed

+19
-31
lines changed

‎pkg/cvo/cvo_scenarios_test.go‎

Lines changed: 19 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,8 @@ func TestCVO_StartupAndSync(t *testing.T) {
178178
// Step 3: Given an operator image, begin synchronizing
179179
//
180180
o.release.Image = "image/image:1"
181-
o.release.Version = "4.0.1"
182-
desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"}
181+
o.release.Version = "1.0.0-abc"
182+
desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}
183183
//
184184
client.ClearActions()
185185
err = o.sync(ctx, o.queueKey())
@@ -205,13 +205,13 @@ func TestCVO_StartupAndSync(t *testing.T) {
205205
ObservedGeneration: 1,
206206
Desired: desired,
207207
History: []configv1.UpdateHistory{
208-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime},
208+
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime},
209209
},
210210
Conditions: []configv1.ClusterOperatorStatusCondition{
211211
{Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse},
212212
// cleared failing status and set progressing
213213
{Type: ClusterStatusFailing, Status: configv1.ConditionFalse},
214-
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"},
214+
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"},
215215
{Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse},
216216
},
217217
},
@@ -221,8 +221,7 @@ func TestCVO_StartupAndSync(t *testing.T) {
221221
Generation: 1,
222222
Step: "RetrievePayload",
223223
Initial: true,
224-
// the desired version is briefly incorrect (user provided) until we retrieve the image
225-
Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"},
224+
Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"},
226225
},
227226
SyncWorkerStatus{
228227
Generation: 1,
@@ -314,9 +313,7 @@ func TestCVO_StartupAndSync(t *testing.T) {
314313
},
315314
VersionHash: "6GC9TkkG9PA=",
316315
History: []configv1.UpdateHistory{
317-
// Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO)
318316
{State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
319-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
320317
},
321318
Conditions: []configv1.ClusterOperatorStatusCondition{
322319
{Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"},
@@ -493,8 +490,8 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) {
493490
// Step 3: Given an operator image, begin synchronizing
494491
//
495492
o.release.Image = "image/image:1"
496-
o.release.Version = "4.0.1"
497-
desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"}
493+
o.release.Version = "1.0.0-abc"
494+
desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}
498495
//
499496
client.ClearActions()
500497
err = o.sync(ctx, o.queueKey())
@@ -520,23 +517,22 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) {
520517
Desired: desired,
521518
ObservedGeneration: 1,
522519
History: []configv1.UpdateHistory{
523-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime},
520+
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime},
524521
},
525522
Conditions: []configv1.ClusterOperatorStatusCondition{
526523
{Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse},
527524
// cleared failing status and set progressing
528525
{Type: ClusterStatusFailing, Status: configv1.ConditionFalse},
529-
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"},
526+
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"},
530527
{Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse},
531528
},
532529
},
533530
})
534531
verifyAllStatus(t, worker.StatusCh(),
535532
SyncWorkerStatus{
536-
Step: "RetrievePayload",
537-
Initial: true,
538-
// the desired version is briefly incorrect (user provided) until we retrieve the image
539-
Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"},
533+
Step: "RetrievePayload",
534+
Initial: true,
535+
Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"},
540536
Generation: 1,
541537
},
542538
SyncWorkerStatus{
@@ -629,9 +625,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) {
629625
},
630626
VersionHash: "6GC9TkkG9PA=",
631627
History: []configv1.UpdateHistory{
632-
// Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO)
633628
{State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
634-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
635629
},
636630
Conditions: []configv1.ClusterOperatorStatusCondition{
637631
{Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"},
@@ -798,8 +792,8 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) {
798792
// Step 3: Given an operator image, begin synchronizing
799793
//
800794
o.release.Image = "image/image:1"
801-
o.release.Version = "4.0.1"
802-
desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"}
795+
o.release.Version = "1.0.0-abc"
796+
desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}
803797
//
804798
client.ClearActions()
805799
err = o.sync(ctx, o.queueKey())
@@ -825,23 +819,22 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) {
825819
Desired: desired,
826820
ObservedGeneration: 1,
827821
History: []configv1.UpdateHistory{
828-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime},
822+
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime},
829823
},
830824
Conditions: []configv1.ClusterOperatorStatusCondition{
831825
{Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse},
832826
// cleared failing status and set progressing
833827
{Type: ClusterStatusFailing, Status: configv1.ConditionFalse},
834-
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"},
828+
{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"},
835829
{Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse},
836830
},
837831
},
838832
})
839833
verifyAllStatus(t, worker.StatusCh(),
840834
SyncWorkerStatus{
841-
Step: "RetrievePayload",
842-
Initial: true,
843-
// the desired version is briefly incorrect (user provided) until we retrieve the image
844-
Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"},
835+
Step: "RetrievePayload",
836+
Initial: true,
837+
Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"},
845838
Generation: 1,
846839
},
847840
SyncWorkerStatus{
@@ -934,9 +927,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) {
934927
},
935928
VersionHash: "6GC9TkkG9PA=",
936929
History: []configv1.UpdateHistory{
937-
// Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO)
938930
{State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
939-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
940931
},
941932
Conditions: []configv1.ClusterOperatorStatusCondition{
942933
{Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"},
@@ -2094,7 +2085,6 @@ func TestCVO_RestartAndReconcile(t *testing.T) {
20942085
History: []configv1.UpdateHistory{
20952086
// TODO: this is wrong, should be single partial entry
20962087
{State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
2097-
{State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
20982088
{State: configv1.PartialUpdate, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
20992089
{State: configv1.PartialUpdate, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime},
21002090
},

‎pkg/cvo/sync_worker.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,6 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in
539539
} else if payloadUpdate.Release.Version != work.Desired.Version {
540540
err = fmt.Errorf("release image version %s does not match the expected upstream version %s", payloadUpdate.Release.Version, work.Desired.Version)
541541
w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "VerifyPayloadVersionFailed", "verifying payload failed version=%q image=%q failure=%v", work.Desired.Version, work.Desired.Image, err)
542-
/* FIXME: Ignore for now. I will make this fatal in a follow-up pivot
543542
reporter.Report(SyncWorkerStatus{
544543
Generation: work.Generation,
545544
Failure: err,
@@ -550,7 +549,6 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in
550549
Verified: info.Verified,
551550
})
552551
return err
553-
*/
554552
}
555553

556554
// need to make sure the payload is only set when the preconditions have been successful

0 commit comments

Comments
 (0)