Skip to content

Commit

Permalink
pkg/cvo/sync_worker: Make expected/actual version mismatch fatal
Browse files Browse the repository at this point in the history 
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).
  • Loading branch information
@wking
wking committed Aug 9, 2020
1 parent 16d3d84 commit 9be6175
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 31 deletions.
48 changes: 19 additions & 29 deletions pkg/cvo/cvo_scenarios_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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},
},
},
Expand All @@ -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,
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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())
Expand All @@ -520,23 +517,22 @@ 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},
},
},
})
verifyAllStatus(t, worker.StatusCh(),
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"},
Step: "RetrievePayload",
Initial: true,
Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"},
Generation: 1,
},
SyncWorkerStatus{
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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())
Expand All @@ -825,23 +819,22 @@ 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},
},
},
})
verifyAllStatus(t, worker.StatusCh(),
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"},
Step: "RetrievePayload",
Initial: true,
Actual: configv1.Release{Version: "1.0.0-abc", Image: "image/image:1"},
Generation: 1,
},
SyncWorkerStatus{
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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},
},
Expand Down
2 changes: 0 additions & 2 deletions pkg/cvo/sync_worker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down

0 comments on commit 9be6175

Please sign in to comment.