diff --git a/changelogs/unreleased/10574-Kaizhe b/changelogs/unreleased/10574-Kaizhe new file mode 100644 index 000000000..e8672a119 --- /dev/null +++ b/changelogs/unreleased/10574-Kaizhe @@ -0,0 +1 @@ +Fix panic in ChangeImageNameAction when a restored container image field is not a string diff --git a/pkg/restore/actions/change_image_name_action.go b/pkg/restore/actions/change_image_name_action.go index 69e9e5f33..f9b04e3b6 100644 --- a/pkg/restore/actions/change_image_name_action.go +++ b/pkg/restore/actions/change_image_name_action.go @@ -161,14 +161,24 @@ func (a *ChangeImageNameAction) replaceImageName(obj *unstructured.Unstructured, } for i, container := range containers { log.Infoln("container:", container) - if image, ok := container.(map[string]any)["image"]; ok { - imageName := image.(string) - if exists, newImageName, err := a.isImageReplaceRuleExist(log, imageName, config); exists && err == nil { - needUpdateObj = true - log.Infof("Updating item's image from %s to %s", imageName, newImageName) - container.(map[string]any)["image"] = newImageName - containers[i] = container - } + containerMap, ok := container.(map[string]any) + if !ok { + log.Warnf("skipping container: container entry is not a map (got %T)", container) + continue + } + imageName, found, err := unstructured.NestedString(containerMap, "image") + if err != nil { + log.Warnf("skipping container: image field is not a string: %v", err) + continue + } + if !found || imageName == "" { + continue + } + if exists, newImageName, err := a.isImageReplaceRuleExist(log, imageName, config); exists && err == nil { + needUpdateObj = true + log.Infof("Updating item's image from %s to %s", imageName, newImageName) + containerMap["image"] = newImageName + containers[i] = containerMap } } if needUpdateObj { diff --git a/pkg/restore/actions/change_image_name_action_test.go b/pkg/restore/actions/change_image_name_action_test.go index 530c8453d..ef69d8bf2 100644 --- a/pkg/restore/actions/change_image_name_action_test.go +++ b/pkg/restore/actions/change_image_name_action_test.go @@ -177,3 +177,90 @@ func TestChangeImageRepositoryActionExecute(t *testing.T) { }) } } + +// TestReplaceImageNameSkipsNonStringImageField is a regression test for a +// panic on a crafted/malformed backup: replaceImageName operates on generic +// unstructured content decoded from a backup tarball, which is untrusted +// input not validated against the Pod schema before this code runs, so the +// "image" field is not guaranteed to be a string. Uses a hand-built +// unstructured object (rather than the table-driven cases above, which all +// go through a typed corev1api.Pod and so can never produce a non-string +// image field) to exercise that path directly. +func TestReplaceImageNameSkipsNonStringImageField(t *testing.T) { + a := NewChangeImageNameAction(logrus.StandardLogger(), nil) + + obj := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]any{ + "name": "pod1", + "namespace": "default", + }, + "spec": map[string]any{ + "containers": []any{ + map[string]any{ + "name": "container1", + "image": float64(5000), + }, + }, + }, + }, + } + + configMap := builder.ForConfigMap("velero", "change-image-name"). + ObjectMeta(builder.WithLabels("velero.io/plugin-config", "", "velero.io/change-image-name", "RestoreItemAction")). + Data("specific", "1.1.1.1:5000,2.2.2.2:3000"). + Result() + + require.NotPanics(t, func() { + err := a.replaceImageName(obj, configMap, "spec", "containers") + require.NoError(t, err) + }) + + containers, _, err := unstructured.NestedSlice(obj.UnstructuredContent(), "spec", "containers") + require.NoError(t, err) + require.Len(t, containers, 1) + // the non-string image field must be left untouched, not modified or + // coerced into a string. + assert.Equal(t, float64(5000), containers[0].(map[string]any)["image"]) +} + +// TestReplaceImageNameSkipsNonMapContainerEntry is a regression test for the +// same untrusted-content trust boundary as +// TestReplaceImageNameSkipsNonStringImageField, but for the container list +// entry itself rather than its "image" field: a crafted/malformed backup's +// spec.containers entry is not guaranteed to be a map at all. +func TestReplaceImageNameSkipsNonMapContainerEntry(t *testing.T) { + a := NewChangeImageNameAction(logrus.StandardLogger(), nil) + + obj := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]any{ + "name": "pod1", + "namespace": "default", + }, + "spec": map[string]any{ + "containers": []any{"not-a-map"}, + }, + }, + } + + configMap := builder.ForConfigMap("velero", "change-image-name"). + ObjectMeta(builder.WithLabels("velero.io/plugin-config", "", "velero.io/change-image-name", "RestoreItemAction")). + Data("specific", "1.1.1.1:5000,2.2.2.2:3000"). + Result() + + require.NotPanics(t, func() { + err := a.replaceImageName(obj, configMap, "spec", "containers") + require.NoError(t, err) + }) + + containers, _, err := unstructured.NestedSlice(obj.UnstructuredContent(), "spec", "containers") + require.NoError(t, err) + require.Len(t, containers, 1) + // the non-map container entry must be left untouched. + assert.Equal(t, "not-a-map", containers[0]) +}