From c53b9b2a87ece2ac700d7099a44a67eaefe2fe13 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 9 Aug 2020 13:35:03 -0700 Subject: [PATCH] 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 58356deee6 (*: Port from Update to Release for ClusterVersion status, 2020-07-24, #419). --- pkg/cvo/cvo_scenarios_test.go | 40 +++++++++++++---------------------- pkg/cvo/sync_worker.go | 2 -- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 1b5086ebb..7417bed30 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -178,8 +178,8 @@ func TestCVO_StartupAndSync(t *testing.T) { // Step 3: Given an operator image, begin synchronizing // o.release.Image = "image/image:1" - o.release.Version = "4.0.1" - desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"} + o.release.Version = "1.0.0-abc" + desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"} // client.ClearActions() err = o.sync(ctx, o.queueKey()) @@ -205,13 +205,13 @@ func TestCVO_StartupAndSync(t *testing.T) { ObservedGeneration: 1, Desired: desired, History: []configv1.UpdateHistory{ - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, + {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, // cleared failing status and set progressing {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, @@ -221,8 +221,7 @@ func TestCVO_StartupAndSync(t *testing.T) { Generation: 1, Step: "RetrievePayload", Initial: true, - // the desired version is briefly incorrect (user provided) until we retrieve the image - Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"}, + Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}, }, SyncWorkerStatus{ Generation: 1, @@ -314,9 +313,7 @@ func TestCVO_StartupAndSync(t *testing.T) { }, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ - // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, @@ -493,8 +490,8 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { // Step 3: Given an operator image, begin synchronizing // o.release.Image = "image/image:1" - o.release.Version = "4.0.1" - desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"} + o.release.Version = "1.0.0-abc" + desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"} // client.ClearActions() err = o.sync(ctx, o.queueKey()) @@ -520,13 +517,13 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Desired: desired, ObservedGeneration: 1, History: []configv1.UpdateHistory{ - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, + {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, // cleared failing status and set progressing {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, @@ -535,8 +532,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { SyncWorkerStatus{ Step: "RetrievePayload", Initial: true, - // the desired version is briefly incorrect (user provided) until we retrieve the image - Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"}, + Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}, Generation: 1, }, SyncWorkerStatus{ @@ -629,9 +625,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { }, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ - // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, @@ -798,8 +792,8 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { // Step 3: Given an operator image, begin synchronizing // o.release.Image = "image/image:1" - o.release.Version = "4.0.1" - desired := configv1.Release{Version: "4.0.1", Image: "image/image:1"} + o.release.Version = "1.0.0-abc" + desired := configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"} // client.ClearActions() err = o.sync(ctx, o.queueKey()) @@ -825,13 +819,13 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Desired: desired, ObservedGeneration: 1, History: []configv1.UpdateHistory{ - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime}, + {State: configv1.PartialUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, // cleared failing status and set progressing {Type: ClusterStatusFailing, Status: configv1.ConditionFalse}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 1.0.0-abc"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, @@ -840,8 +834,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { SyncWorkerStatus{ Step: "RetrievePayload", Initial: true, - // the desired version is briefly incorrect (user provided) until we retrieve the image - Actual: configv1.Release{Version: "4.0.1", Image: "image/image:1"}, + Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"}, Generation: 1, }, SyncWorkerStatus{ @@ -934,9 +927,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { }, VersionHash: "6GC9TkkG9PA=", History: []configv1.UpdateHistory{ - // Because image and operator had mismatched versions, we get two entries (which shouldn't happen unless there is a bug in the CVO) {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, @@ -2094,7 +2085,6 @@ func TestCVO_RestartAndReconcile(t *testing.T) { History: []configv1.UpdateHistory{ // TODO: this is wrong, should be single partial entry {State: configv1.CompletedUpdate, Image: "image/image:1", Version: "1.0.0-abc", Verified: true, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, - {State: configv1.PartialUpdate, Image: "image/image:1", Version: "4.0.1", StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, {State: configv1.PartialUpdate, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, {State: configv1.PartialUpdate, StartedTime: defaultStartedTime, CompletionTime: &defaultCompletionTime}, }, diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index dcd676a90..891d72370 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -539,7 +539,6 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in } else if payloadUpdate.Release.Version != work.Desired.Version { err = fmt.Errorf("release image version %s does not match the expected upstream version %s", payloadUpdate.Release.Version, work.Desired.Version) w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "VerifyPayloadVersionFailed", "verifying payload failed version=%q image=%q failure=%v", work.Desired.Version, work.Desired.Image, err) - /* FIXME: Ignore for now. I will make this fatal in a follow-up pivot reporter.Report(SyncWorkerStatus{ Generation: work.Generation, Failure: err, @@ -550,7 +549,6 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in Verified: info.Verified, }) return err - */ } // need to make sure the payload is only set when the preconditions have been successful