fix: avoid shell command execution in NodeUpgradeJob

Signed-off-by: Chuanhao Jin <15221580643@163.com>
This commit is contained in:
Chuanhao Jin 2026-07-14 20:37:52 +08:00
parent 1853a18186
commit ea2eba3348
8 changed files with 317 additions and 93 deletions

View file

@ -22,14 +22,13 @@ import (
"fmt"
"net/http"
"reflect"
"strings"
"github.com/blang/semver"
admissionv1 "k8s.io/api/admission/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
"github.com/kubeedge/api/apis/operations/v1alpha1"
"github.com/kubeedge/kubeedge/pkg/util/validation"
)
func serveNodeUpgradeJob(w http.ResponseWriter, r *http.Request) {
@ -49,7 +48,6 @@ func admitNodeUpgradeJob(review admissionv1.AdmissionReview) *admissionv1.Admiss
if _, _, err := deserializer.Decode(raw, nil, &upgrade); err != nil {
return admissionResponse(fmt.Errorf("validation failed with error: %v", err))
}
return admissionResponse(validateNodeUpgradeJob(&upgrade))
case admissionv1.Update:
@ -58,7 +56,6 @@ func admitNodeUpgradeJob(review admissionv1.AdmissionReview) *admissionv1.Admiss
if _, _, err := deserializer.Decode(review.Request.Object.Raw, nil, &newUpgrade); err != nil {
return admissionResponse(fmt.Errorf("validation failed with error: %v", err))
}
oldUpgrade := v1alpha1.NodeUpgradeJob{}
if _, _, err := deserializer.Decode(review.Request.OldObject.Raw, nil, &oldUpgrade); err != nil {
return admissionResponse(fmt.Errorf("validation failed with error: %v", err))
@ -82,22 +79,19 @@ func admitNodeUpgradeJob(review admissionv1.AdmissionReview) *admissionv1.Admiss
}
func validateNodeUpgradeJob(upgrade *v1alpha1.NodeUpgradeJob) error {
// version must be valid
if !strings.HasPrefix(upgrade.Spec.Version, "v") {
return fmt.Errorf("version must begin with prefix 'v'")
if !validation.ValidateVersion(upgrade.Spec.Version) {
return fmt.Errorf("invalid version %s", upgrade.Spec.Version)
}
_, err := semver.Parse(strings.TrimPrefix(upgrade.Spec.Version, "v"))
if err != nil {
return fmt.Errorf("version is not a semver compatible version: %v", err)
// Image is a optional field.
if upgrade.Spec.Image != "" && !validation.ValidateImageRepo(upgrade.Spec.Image) {
return fmt.Errorf("invalid image repo %s", upgrade.Spec.Image)
}
// we must specify NodeNames or LabelSelector, and we can only specify only one
if len(upgrade.Spec.NodeNames) == 0 && upgrade.Spec.LabelSelector == nil {
return fmt.Errorf("both NodeNames and LabelSelector are NOT specified")
return fmt.Errorf("both NodeNames and LabelSelctor are NOT specified")
}
if len(upgrade.Spec.NodeNames) != 0 && upgrade.Spec.LabelSelector != nil {
return fmt.Errorf("both NodeNames and LabelSelector are specified")
return fmt.Errorf("both NodeNames and LabelSelctor are specified")
}
return nil
@ -151,7 +145,7 @@ func generateNodeUpgradeJobPatch(spec v1alpha1.NodeUpgradeJobSpec) []patchValue
// mutate .spec.concurrency to default value 1 if not specified
if spec.Concurrency == 0 {
patch = append(patch, patchValue{
Op: "add",
Op: "replace",
Path: "/spec/concurrency",
Value: 1,
})
@ -160,7 +154,7 @@ func generateNodeUpgradeJobPatch(spec v1alpha1.NodeUpgradeJobSpec) []patchValue
if spec.TimeoutSeconds == nil {
var defaultTimeoutSeconds uint32 = 300
patch = append(patch, patchValue{
Op: "add",
Op: "replace",
Path: "/spec/timeoutSeconds",
Value: &defaultTimeoutSeconds,
})

View file

@ -61,7 +61,7 @@ func TestAdmitNodeUpgradeJob(t *testing.T) {
},
},
expectedAllowed: false,
expectedError: "version must begin with prefix 'v'",
expectedError: "invalid version 1.0.0",
},
{
name: "Invalid Semver",
@ -73,7 +73,19 @@ func TestAdmitNodeUpgradeJob(t *testing.T) {
},
},
expectedAllowed: false,
expectedError: "version is not a semver compatible version",
expectedError: "invalid version v1.0",
},
{
name: "Invalid image",
operation: admissionv1.Create,
upgrade: &v1alpha1.NodeUpgradeJob{
Spec: v1alpha1.NodeUpgradeJobSpec{
Version: "v1.0.0",
Image: "invalid-image",
},
},
expectedAllowed: false,
expectedError: "invalid image repo invalid-image",
},
{
name: "No NodeNames and LabelSelector",
@ -84,7 +96,7 @@ func TestAdmitNodeUpgradeJob(t *testing.T) {
},
},
expectedAllowed: false,
expectedError: "both NodeNames and LabelSelector are NOT specified",
expectedError: "both NodeNames and LabelSelctor are NOT specified",
},
{
name: "Both NodeNames and LabelSelector",
@ -97,7 +109,7 @@ func TestAdmitNodeUpgradeJob(t *testing.T) {
},
},
expectedAllowed: false,
expectedError: "both NodeNames and LabelSelector are specified",
expectedError: "both NodeNames and LabelSelctor are specified",
},
{
name: "Valid Update",
@ -197,7 +209,17 @@ func TestValidateNodeUpgradeJob(t *testing.T) {
NodeNames: []string{"node1"},
},
},
expectedErr: "version must begin with prefix 'v'",
expectedErr: "invalid version 1.0.0",
},
{
name: "Invalid image",
upgrade: &v1alpha1.NodeUpgradeJob{
Spec: v1alpha1.NodeUpgradeJobSpec{
Version: "v1.0.0",
Image: "invalid-image",
},
},
expectedErr: "invalid image repo invalid-image",
},
{
name: "Invalid version (not semver compatible)",
@ -207,7 +229,7 @@ func TestValidateNodeUpgradeJob(t *testing.T) {
NodeNames: []string{"node1"},
},
},
expectedErr: "version is not a semver compatible version",
expectedErr: "invalid version v1.0",
},
{
name: "Missing both NodeNames and LabelSelector",
@ -216,7 +238,7 @@ func TestValidateNodeUpgradeJob(t *testing.T) {
Version: "v1.0.0",
},
},
expectedErr: "both NodeNames and LabelSelector are NOT specified",
expectedErr: "both NodeNames and LabelSelctor are NOT specified",
},
{
name: "Both NodeNames and LabelSelector specified",
@ -227,7 +249,7 @@ func TestValidateNodeUpgradeJob(t *testing.T) {
LabelSelector: &metav1.LabelSelector{},
},
},
expectedErr: "both NodeNames and LabelSelector are specified",
expectedErr: "both NodeNames and LabelSelctor are specified",
},
{
name: "Valid upgrade job",
@ -322,10 +344,10 @@ func TestMutatingNodeUpgradeJob(t *testing.T) {
err = json.Unmarshal(response.Patch, &patch)
assert.NoError(err)
assert.Len(patch, 2)
assert.Equal("add", patch[0]["op"])
assert.Equal("replace", patch[0]["op"])
assert.Equal("/spec/concurrency", patch[0]["path"])
assert.Equal(float64(1), patch[0]["value"])
assert.Equal("add", patch[1]["op"])
assert.Equal("replace", patch[1]["op"])
assert.Equal("/spec/timeoutSeconds", patch[1]["path"])
assert.Equal(float64(300), patch[1]["value"])
}
@ -356,19 +378,19 @@ func TestGenerateNodeUpgradeJobPatch(t *testing.T) {
},
expectedPatch: []patchValue{
{
Op: "add",
Op: "replace",
Path: "/spec/concurrency",
Value: 1,
},
{
Op: "add",
Op: "replace",
Path: "/spec/timeoutSeconds",
Value: func() *uint32 { v := uint32(300); return &v }(),
},
},
},
{
name: "TimeoutSeconds specified",
name: "Concurrency specified",
spec: v1alpha1.NodeUpgradeJobSpec{
Version: "v1.0.0",
NodeNames: []string{"node1"},
@ -376,14 +398,14 @@ func TestGenerateNodeUpgradeJobPatch(t *testing.T) {
},
expectedPatch: []patchValue{
{
Op: "add",
Op: "replace",
Path: "/spec/concurrency",
Value: 1,
},
},
},
{
name: "Concurrency specified",
name: "TimeoutSeconds specified",
spec: v1alpha1.NodeUpgradeJobSpec{
Version: "v1.0.0",
NodeNames: []string{"node1"},
@ -391,7 +413,7 @@ func TestGenerateNodeUpgradeJobPatch(t *testing.T) {
},
expectedPatch: []patchValue{
{
Op: "add",
Op: "replace",
Path: "/spec/timeoutSeconds",
Value: func() *uint32 { v := uint32(300); return &v }(),
},

View file

@ -20,9 +20,10 @@ import (
"context"
"encoding/json"
"fmt"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"time"
"github.com/go-logr/logr"
@ -38,6 +39,7 @@ import (
taskmsg "github.com/kubeedge/kubeedge/pkg/nodetask/message"
upgradeedge "github.com/kubeedge/kubeedge/pkg/upgrade/edge"
"github.com/kubeedge/kubeedge/pkg/util/execs"
"github.com/kubeedge/kubeedge/pkg/util/validation"
)
func newNodeUpgradeJobRunner() *ActionRunner {
@ -131,6 +133,15 @@ func (nodeUpgradeJobActionHandler) checkItems(
}
}
if !validation.ValidateVersion(spec.Version) {
resp.err = fmt.Errorf("invalid version %s", spec.Version)
return resp
}
if spec.Image != "" && !validation.ValidateImageRepo(spec.Image) {
resp.err = fmt.Errorf("invalid image repo %s", spec.Image)
return resp
}
// Pull installation-package image.
cfg := options.GetEdgeCoreConfig()
ctrcli, err := containers.NewContainerRuntime(
@ -241,18 +252,43 @@ func (h *nodeUpgradeJobActionHandler) upgrade(
resp.interrupt = true // No upgrade yet, no need to roll back.
return resp
}
var cmdline strings.Builder
cmdline.WriteString("keadm upgrade edge --force --toVersion " + spec.Version)
if spec.Image != "" {
cmdline.WriteString(" --image " + spec.Image)
if !validation.ValidateVersion(spec.Version) {
resp.err = fmt.Errorf("invalid version %s", spec.Version)
resp.interrupt = true // No upgrade yet, no need to roll back.
return resp
}
cmdline.WriteString(" >> /tmp/keadm.log 2>&1")
cmd := execs.NewCommand(cmdline.String())
h.logger.V(2).Info("run upgrade cmd", "cmd", cmdline.String())
resp.err = cmd.Exec()
if spec.Image != "" && !validation.ValidateImageRepo(spec.Image) {
resp.err = fmt.Errorf("invalid image repo %s", spec.Image)
resp.interrupt = true // No upgrade yet, no need to roll back.
return resp
}
logFile, err := os.OpenFile("/tmp/keadm.log", os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
if err != nil {
resp.err = fmt.Errorf("failed to open keadm log file: %w", err)
resp.interrupt = true // No upgrade yet, no need to roll back.
return resp
}
defer logFile.Close()
args := buildNodeUpgradeJobCommandArgs(spec)
cmd := exec.Command("keadm", args...)
cmd.Stdout = logFile
cmd.Stderr = logFile
h.logger.V(2).Info("run upgrade cmd", "cmd", "keadm", "args", args)
resp.err = cmd.Run()
return resp
}
func buildNodeUpgradeJobCommandArgs(spec *operationsv1alpha2.NodeUpgradeJobSpec) []string {
args := []string{"upgrade", "edge", "--force", "--toVersion", spec.Version}
if spec.Image != "" {
args = append(args, "--image", spec.Image)
}
return args
}
func (h *nodeUpgradeJobActionHandler) rollback(
_ctx context.Context,
_jobname, _nodename string,

View file

@ -259,51 +259,50 @@ func TestNodeUpgradeJobBackup(t *testing.T) {
}
func TestNodeUpgradeJobUpgrade(t *testing.T) {
ctx := context.TODO()
specser := &cachedSpecSerializer{}
h := nodeUpgradeJobActionHandler{
logger: klog.Background(),
}
t.Run("get spec failed", func(t *testing.T) {
resp := h.upgrade(ctx, "", "", specser)
require.ErrorContains(t, resp.Error(), "failed to conv spec to NodeUpgradeJobSpec, actual type <nil>")
require.True(t, resp.NeedInterrupt())
})
t.Run("standard upgrade command", func(t *testing.T) {
patches := gomonkey.NewPatches()
defer patches.Reset()
patches.ApplyMethod(reflect.TypeOf((*execs.Command)(nil)), "Exec",
func(cmd *execs.Command) error {
assert.Equal(t, "bash -c keadm upgrade edge --force --toVersion 1.21.0 >> /tmp/keadm.log 2>&1", cmd.GetCommand())
return nil
})
specser.spec = &operationsv1alpha2.NodeUpgradeJobSpec{
Version: "1.21.0",
t.Run("standard upgrade command args", func(t *testing.T) {
spec := &operationsv1alpha2.NodeUpgradeJobSpec{
Version: "v1.21.0",
}
resp := h.upgrade(ctx, "", "", specser)
require.NoError(t, resp.Error())
args := buildNodeUpgradeJobCommandArgs(spec)
assert.Equal(t, []string{
"upgrade", "edge",
"--force",
"--toVersion", "v1.21.0",
}, args)
})
t.Run("custom image repository upgrade command", func(t *testing.T) {
patches := gomonkey.NewPatches()
defer patches.Reset()
patches.ApplyMethod(reflect.TypeOf((*execs.Command)(nil)), "Exec",
func(cmd *execs.Command) error {
assert.Equal(t, "bash -c keadm upgrade edge --force --toVersion 1.21.0 --image custom.com/kubeedge/installation-package >> /tmp/keadm.log 2>&1", cmd.GetCommand())
return nil
})
specser.spec = &operationsv1alpha2.NodeUpgradeJobSpec{
Version: "1.21.0",
t.Run("custom image repository upgrade command args", func(t *testing.T) {
spec := &operationsv1alpha2.NodeUpgradeJobSpec{
Version: "v1.21.0",
Image: "custom.com/kubeedge/installation-package",
}
resp := h.upgrade(ctx, "", "", specser)
require.NoError(t, resp.Error())
args := buildNodeUpgradeJobCommandArgs(spec)
assert.Equal(t, []string{
"upgrade", "edge",
"--force",
"--toVersion", "v1.21.0",
"--image", "custom.com/kubeedge/installation-package",
}, args)
})
t.Run("malicious fields are kept as argv values", func(t *testing.T) {
spec := &operationsv1alpha2.NodeUpgradeJobSpec{
Version: "v1.21.0; touch /tmp/pwned",
Image: "custom.com/kubeedge/installation-package$(touch /tmp/pwned)",
}
args := buildNodeUpgradeJobCommandArgs(spec)
assert.Equal(t, []string{
"upgrade", "edge",
"--force",
"--toVersion", "v1.21.0; touch /tmp/pwned",
"--image", "custom.com/kubeedge/installation-package$(touch /tmp/pwned)",
}, args)
})
}

View file

@ -21,6 +21,7 @@ import (
"encoding/json"
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
@ -191,21 +192,42 @@ func upgrade(taskReq types.NodeTaskRequest) (event fsm.Event) {
func keadmUpgrade(upgradeReq commontypes.NodeUpgradeJobRequest, opts *options.EdgeCoreOptions) error {
klog.Infof("Begin to run upgrade command")
upgradeCmd := fmt.Sprintf("keadm upgrade edge --upgradeID %s --historyID %s --fromVersion %s --toVersion %s --config %s --image %s > /tmp/keadm.log 2>&1",
upgradeReq.UpgradeID, upgradeReq.HistoryID, version.Get(), upgradeReq.Version, opts.ConfigFile, upgradeReq.Image)
// run upgrade cmd to upgrade edge node
// use nohup command to start a child progress
command := fmt.Sprintf("nohup %s &", upgradeCmd)
cmd := exec.Command("bash", "-c", command)
s, err := cmd.CombinedOutput()
logFile, err := os.OpenFile("/tmp/keadm.log", os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)
if err != nil {
return fmt.Errorf("run upgrade command %s failed: %v, %s", command, err, s)
return fmt.Errorf("failed to open keadm log file: %w", err)
}
klog.Infof("!!! Finish upgrade from Version %s to %s ...", version.Get(), upgradeReq.Version)
defer logFile.Close()
args := buildKeadmUpgradeArgs(upgradeReq, opts)
cmd := exec.Command("nohup", append([]string{"keadm"}, args...)...)
cmd.Stdout = logFile
cmd.Stderr = logFile
if err := cmd.Start(); err != nil {
return fmt.Errorf("failed to start keadm upgrade command: %w", err)
}
if err := cmd.Process.Release(); err != nil {
return fmt.Errorf("failed to release keadm upgrade process: %w", err)
}
klog.Infof("Started keadm upgrade from Version %s to %s ...", version.Get().String(), upgradeReq.Version)
return nil
}
func buildKeadmUpgradeArgs(upgradeReq commontypes.NodeUpgradeJobRequest, opts *options.EdgeCoreOptions) []string {
return []string{
"upgrade", "edge",
"--upgradeID", upgradeReq.UpgradeID,
"--historyID", upgradeReq.HistoryID,
"--fromVersion", version.Get().String(),
"--toVersion", upgradeReq.Version,
"--config", opts.ConfigFile,
"--image", upgradeReq.Image,
}
}
func prepareKeadm(upgradeReq *commontypes.NodeUpgradeJobRequest) error {
ctx := context.Background()
config := options.GetEdgeCoreConfig()

View file

@ -0,0 +1,54 @@
/*
Copyright 2026 The KubeEdge Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package taskexecutor
import (
"reflect"
"testing"
commontypes "github.com/kubeedge/kubeedge/common/types"
"github.com/kubeedge/kubeedge/edge/cmd/edgecore/app/options"
"github.com/kubeedge/kubeedge/pkg/version"
)
func TestBuildKeadmUpgradeArgsDoesNotUseShell(t *testing.T) {
upgradeReq := commontypes.NodeUpgradeJobRequest{
UpgradeID: "upgrade-1; touch /tmp/pwned",
HistoryID: "history-1$(touch /tmp/pwned)",
Version: "v1.23.1; rm -rf /",
Image: "kubeedge/installation-package; touch /tmp/pwned",
}
opts := &options.EdgeCoreOptions{
ConfigFile: "/etc/kubeedge/config/edgecore.yaml; touch /tmp/pwned",
}
args := buildKeadmUpgradeArgs(upgradeReq, opts)
want := []string{
"upgrade", "edge",
"--upgradeID", upgradeReq.UpgradeID,
"--historyID", upgradeReq.HistoryID,
"--fromVersion", version.Get().String(),
"--toVersion", upgradeReq.Version,
"--config", opts.ConfigFile,
"--image", upgradeReq.Image,
}
if !reflect.DeepEqual(args, want) {
t.Fatalf("unexpected args: got %v, want %v", args, want)
}
}

View file

@ -19,6 +19,14 @@ package validation
import (
"fmt"
"net"
"regexp"
)
// Regexps
var (
regexpImageRepo = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._:-]*/[A-Za-z0-9][A-Za-z0-9._-]*(/[A-Za-z0-9][A-Za-z0-9._-]*)*(@sha256:[A-Fa-f0-9]{64}|:[A-Za-z0-9_][A-Za-z0-9._-]{0,127})?$`)
regexpVersion = regexp.MustCompile(`^v\d+\.\d+\.\d+(-[a-zA-Z0-9_.]+(?:-[a-zA-Z0-9_.]+)*)?$`)
)
// IsValidIP tests that the argument is a valid IP address.
@ -42,3 +50,11 @@ func IsValidPortNum(port int) []string {
func InclusiveRangeError(lo, hi int) string {
return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi)
}
func ValidateImageRepo(image string) bool {
return regexpImageRepo.MatchString(image)
}
func ValidateVersion(version string) bool {
return regexpVersion.MatchString(version)
}

View file

@ -45,7 +45,7 @@ func TestIsValidIP(t *testing.T) {
t.Run(c.Name, func(t *testing.T) {
v := IsValidIP(c.IP)
get := len(v) == 0
assert.Equal(get, c.Expect)
assert.Equal(c.Expect, get)
})
}
}
@ -83,7 +83,7 @@ func TestIsValidPortNum(t *testing.T) {
for _, c := range cases {
t.Run(c.Name, func(t *testing.T) {
v := IsValidPortNum(c.Port)
assert.Equal(v, c.Expect)
assert.Equal(c.Expect, v)
})
}
}
@ -93,5 +93,86 @@ func TestInclusiveRangeError(t *testing.T) {
result := InclusiveRangeError(1, 65535)
expect := "must be between 1 and 65535, inclusive"
assert.Equal(result, expect)
assert.Equal(expect, result)
}
func TestValidateImageRepo(t *testing.T) {
cases := []struct {
imageRepo string
want bool
}{
{
imageRepo: "installation-package",
want: false,
},
{
imageRepo: "kubeedge/installation-package",
want: true,
},
{
imageRepo: "kubeedge/installation-package;bash",
want: false,
},
{
imageRepo: "_kubeedge/installation-package",
want: false,
},
{
imageRepo: "aaa.bbb.ccc/kubeedge/installation-package",
want: true,
},
{
imageRepo: "registry.example.com:5000/kubeedge/installation-package",
want: true,
},
{
imageRepo: "kubeedge/installation-package:v1.23.1",
want: true,
},
{
imageRepo: "kubeedge/installation-package@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
want: true,
},
{
imageRepo: "kubeedge/installation-package;touch /tmp/pwned",
want: false,
},
{
imageRepo: "kubeedge/installation-package$(touch /tmp/pwned)",
want: false,
},
{
imageRepo: "kubeedge/installation-package`touch /tmp/pwned`",
want: false,
},
{
imageRepo: "kubeedge/installation-package\n touch /tmp/pwned",
want: false,
},
}
for _, c := range cases {
t.Run(c.imageRepo, func(t *testing.T) {
assert.Equal(t, c.want, ValidateImageRepo(c.imageRepo))
})
}
}
func TestValidateVersion(t *testing.T) {
cases := []struct {
version string
want bool
}{
{version: "v1.0.0", want: true},
{version: "V1.0.0", want: false},
{version: "1.0.0", want: false},
{version: "v1.0", want: false},
{version: "v1.0.0;bash", want: false},
{version: "v1.0.0-rc1", want: true},
{version: "v1.0.0-rc1.1", want: true},
}
for _, c := range cases {
t.Run(c.version, func(t *testing.T) {
assert.Equal(t, c.want, ValidateVersion(c.version))
})
}
}