From d4c82f1020448cfc3c05e74ec5412a9d748cfb00 Mon Sep 17 00:00:00 2001 From: TzZtzt Date: Tue, 23 Jan 2024 21:19:36 +0800 Subject: [PATCH] refactor: safe exec commands (#3699) * Move security command to pkg/utils/security Signed-off-by: trafalgarzzz * Fix build Signed-off-by: trafalgarzzz * Fix unit tests Signed-off-by: trafalgarzzz * Move Commands to cmdguard utils package Signed-off-by: trafalgarzzz * Rename exec pipes Signed-off-by: trafalgarzzz * Add unit tests for exec pipes Signed-off-by: trafalgarzzz * Add unit tests for exec pipes Signed-off-by: trafalgarzzz * Remove unused helm utility functions Signed-off-by: trafalgarzzz * Fix illegal sequence to pass Juicefs's health check Signed-off-by: trafalgarzzz * Rename allowedEnvs to allowedExpressions Signed-off-by: trafalgarzzz --------- Signed-off-by: trafalgarzzz --- pkg/csi/plugins/nodeserver.go | 5 +- pkg/ddc/alluxio/operations/base.go | 8 +- pkg/ddc/efc/operations/base.go | 10 +- pkg/ddc/goosefs/operations/base.go | 4 +- pkg/ddc/jindo/operations/base.go | 4 +- pkg/ddc/jindocache/operations/base.go | 6 + pkg/ddc/jindofsx/operations/base.go | 4 +- pkg/ddc/juicefs/operations/base.go | 6 +- pkg/ddc/thin/operations/base.go | 4 +- pkg/utils/{ => cmdguard}/exec.go | 66 ++++-- pkg/utils/cmdguard/exec_pipes.go | 190 ++++++++++++++++++ .../exec_pipes_test.go} | 125 +++++++++--- pkg/utils/{ => cmdguard}/exec_test.go | 122 +++++++++-- pkg/utils/helm/get.go | 17 -- pkg/utils/helm/helm.go | 103 ---------- pkg/utils/helm/helm_test.go | 133 ------------ pkg/utils/helm/utils.go | 23 ++- pkg/utils/helm/utils_test.go | 6 +- pkg/utils/home.go | 8 +- pkg/utils/kubeclient/exec.go | 5 - pkg/utils/kubectl/configmap.go | 4 +- pkg/utils/mount.go | 3 +- pkg/utils/shell_pipes.go | 156 -------------- sdk/python | 2 +- 24 files changed, 499 insertions(+), 515 deletions(-) rename pkg/utils/{ => cmdguard}/exec.go (54%) create mode 100644 pkg/utils/cmdguard/exec_pipes.go rename pkg/utils/{shell_pipes_test.go => cmdguard/exec_pipes_test.go} (57%) rename pkg/utils/{ => cmdguard}/exec_test.go (52%) delete mode 100644 pkg/utils/helm/get.go delete mode 100644 pkg/utils/helm/helm.go delete mode 100644 pkg/utils/helm/helm_test.go delete mode 100644 pkg/utils/shell_pipes.go diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go index 3363a0ecf53..e3af1068318 100644 --- a/pkg/csi/plugins/nodeserver.go +++ b/pkg/csi/plugins/nodeserver.go @@ -30,6 +30,7 @@ import ( "github.com/fluid-cloudnative/fluid/pkg/common" "github.com/fluid-cloudnative/fluid/pkg/ddc/base" "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/dataset/volume" "github.com/pkg/errors" v1 "k8s.io/api/core/v1" @@ -161,7 +162,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } else { args = append(args, mountPath, targetPath) } - command, err := utils.SimpleCommand("mount", args...) + command, err := cmdguard.Command("mount", args...) if err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } @@ -511,7 +512,7 @@ func checkMountInUse(volumeName string) (bool, error) { // TODO: refer to https://github.com/kubernetes-sigs/alibaba-cloud-csi-driver/blob/4fcb743220371de82d556ab0b67b08440b04a218/pkg/oss/utils.go#L72 // for a better implementation - command, err := utils.SimpleCommand("/usr/local/bin/check_bind_mounts.sh", volumeName) + command, err := cmdguard.Command("/usr/local/bin/check_bind_mounts.sh", volumeName) if err != nil { return inUse, err } diff --git a/pkg/ddc/alluxio/operations/base.go b/pkg/ddc/alluxio/operations/base.go index 9fa4107abb8..558aa124785 100644 --- a/pkg/ddc/alluxio/operations/base.go +++ b/pkg/ddc/alluxio/operations/base.go @@ -25,8 +25,9 @@ import ( "strings" "time" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" - securityutil "github.com/fluid-cloudnative/fluid/pkg/utils/security" + securityutils "github.com/fluid-cloudnative/fluid/pkg/utils/security" "github.com/go-logr/logr" "github.com/fluid-cloudnative/fluid/pkg/utils" @@ -505,7 +506,7 @@ func (a AlluxioFileUtils) exec(command []string, verbose bool) (stdout string, s select { case <-ch: - a.log.Info("execute in time", "command", securityutil.FilterCommand(command)) + a.log.Info("execute in time", "command", securityutils.FilterCommand(command)) case <-ctx.Done(): err = fmt.Errorf("timeout when executing %v", command) } @@ -515,7 +516,8 @@ func (a AlluxioFileUtils) exec(command []string, verbose bool) (stdout string, s // execWithoutTimeout func (a AlluxioFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) + if err != nil { return } diff --git a/pkg/ddc/efc/operations/base.go b/pkg/ddc/efc/operations/base.go index 479d11e3b7a..ae1ca9dddcd 100644 --- a/pkg/ddc/efc/operations/base.go +++ b/pkg/ddc/efc/operations/base.go @@ -23,8 +23,9 @@ import ( "time" "github.com/fluid-cloudnative/fluid/pkg/common" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" - securityutil "github.com/fluid-cloudnative/fluid/pkg/utils/security" + securityutils "github.com/fluid-cloudnative/fluid/pkg/utils/security" "github.com/go-logr/logr" ) @@ -57,7 +58,7 @@ func (a EFCFileUtils) exec(command []string, verbose bool) (stdout string, stder select { case <-ch: - a.log.Info("execute in time", "command", securityutil.FilterCommand(command)) + a.log.Info("execute in time", "command", securityutils.FilterCommand(command)) case <-ctx.Done(): err = fmt.Errorf("timeout when executing %v", command) } @@ -67,6 +68,11 @@ func (a EFCFileUtils) exec(command []string, verbose bool) (stdout string, stder // execWithoutTimeout func (a EFCFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { + err = cmdguard.ValidateCommandSlice(command) + if err != nil { + return + } + stdout, stderr, err = kubeclient.ExecCommandInContainer(a.podName, a.container, a.namespace, command) if err != nil { a.log.Info("Stdout", "Command", command, "Stdout", stdout) diff --git a/pkg/ddc/goosefs/operations/base.go b/pkg/ddc/goosefs/operations/base.go index 905fb01bc94..5ce4878793c 100644 --- a/pkg/ddc/goosefs/operations/base.go +++ b/pkg/ddc/goosefs/operations/base.go @@ -25,7 +25,7 @@ import ( "strings" "time" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" "github.com/go-logr/logr" ) @@ -502,7 +502,7 @@ func (a GooseFSFileUtils) exec(command []string, verbose bool) (stdout string, s // execWithoutTimeout func (a GooseFSFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) if err != nil { return } diff --git a/pkg/ddc/jindo/operations/base.go b/pkg/ddc/jindo/operations/base.go index f4419f0c649..f4a64e3534f 100644 --- a/pkg/ddc/jindo/operations/base.go +++ b/pkg/ddc/jindo/operations/base.go @@ -22,7 +22,7 @@ import ( "strings" "time" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" "github.com/go-logr/logr" ) @@ -67,7 +67,7 @@ func (a JindoFileUtils) exec(command []string, verbose bool) (stdout string, std // execWithoutTimeout func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) if err != nil { return } diff --git a/pkg/ddc/jindocache/operations/base.go b/pkg/ddc/jindocache/operations/base.go index 3622c61af6b..ea3a3c786ac 100644 --- a/pkg/ddc/jindocache/operations/base.go +++ b/pkg/ddc/jindocache/operations/base.go @@ -22,6 +22,7 @@ import ( "strings" "time" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" "github.com/go-logr/logr" ) @@ -87,6 +88,11 @@ func (a JindoFileUtils) execWithTimeOut(command []string, verbose bool, second i // execWithoutTimeout func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { + err = cmdguard.ValidateCommandSlice(command) + if err != nil { + return + } + stdout, stderr, err = kubeclient.ExecCommandInContainer(a.podName, a.container, a.namespace, command) if err != nil { a.log.Info("Stdout", "Command", command, "Stdout", stdout) diff --git a/pkg/ddc/jindofsx/operations/base.go b/pkg/ddc/jindofsx/operations/base.go index 28051041e5e..0948beabc40 100644 --- a/pkg/ddc/jindofsx/operations/base.go +++ b/pkg/ddc/jindofsx/operations/base.go @@ -22,7 +22,7 @@ import ( "strings" "time" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" "github.com/go-logr/logr" ) @@ -88,7 +88,7 @@ func (a JindoFileUtils) execWithTimeOut(command []string, verbose bool, second i // execWithoutTimeout func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) if err != nil { return } diff --git a/pkg/ddc/juicefs/operations/base.go b/pkg/ddc/juicefs/operations/base.go index d32a8850d35..af9e9d92b78 100644 --- a/pkg/ddc/juicefs/operations/base.go +++ b/pkg/ddc/juicefs/operations/base.go @@ -26,7 +26,7 @@ import ( "github.com/go-logr/logr" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" ) @@ -239,7 +239,7 @@ func (j JuiceFileUtils) DeleteCacheDir(dir string) (err error) { // GetStatus get status of volume func (j JuiceFileUtils) GetStatus(source string) (status string, err error) { var ( - command = []string{"/bin/sh", "-c", fmt.Sprintf("juicefs status %s", source)} + command = []string{"sh", "-c", fmt.Sprintf("juicefs status %s", source)} stdout string stderr string ) @@ -327,7 +327,7 @@ func (j JuiceFileUtils) exec(command []string) (stdout string, stderr string, er // execWithoutTimeout func (j JuiceFileUtils) execWithoutTimeout(command []string) (stdout string, stderr string, err error) { // validate the pipe command with white list - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) if err != nil { return } diff --git a/pkg/ddc/thin/operations/base.go b/pkg/ddc/thin/operations/base.go index 21b4c70fb72..b40ce71cded 100644 --- a/pkg/ddc/thin/operations/base.go +++ b/pkg/ddc/thin/operations/base.go @@ -23,7 +23,7 @@ import ( "strings" "time" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient" "github.com/go-logr/logr" ) @@ -157,7 +157,7 @@ func (t ThinFileUtils) exec(command []string, verbose bool) (stdout string, stde // execWithoutTimeout func (t ThinFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) { // validate the pipe command with white list - err = utils.ValidateCommandSlice(command) + err = cmdguard.ValidateCommandSlice(command) if err != nil { return } diff --git a/pkg/utils/exec.go b/pkg/utils/cmdguard/exec.go similarity index 54% rename from pkg/utils/exec.go rename to pkg/utils/cmdguard/exec.go index ae05eec1d0e..cb052cba50c 100644 --- a/pkg/utils/exec.go +++ b/pkg/utils/cmdguard/exec.go @@ -14,31 +14,42 @@ See the License for the specific language governing permissions and limitations under the License. */ -package utils +package cmdguard import ( "fmt" "os/exec" "path/filepath" "strings" + + "github.com/go-logr/logr" + ctrl "sigs.k8s.io/controller-runtime" ) -func init() { - allowPathlist = buildPathList(allowPathlist) -} +var log logr.Logger -// allowPathlist of safe commands -var allowPathlist = map[string]bool{ +// allowedPathList is a whitelist of safe commands +var allowedPathList = map[string]bool{ // "helm": true, "kubectl": true, "ddc-helm": true, // add other commands as needed } +var shellCommandList = map[string]bool{ + "bash": true, + "sh": true, +} + // illegalChars to check -// var illegalChars = []string{"&", "|", ";", "$", "'", "`", "(", ")", ">>"} var illegalChars = []rune{'&', '|', ';', '$', '\'', '`', '(', ')', '>'} +func init() { + allowedPathList = buildPathList(allowedPathList) + shellCommandList = buildPathList(shellCommandList) + log = ctrl.Log.WithName("utils.security") +} + // buildPathList is a function that builds a map of paths for the given pathList. func buildPathList(pathList map[string]bool) (targetPath map[string]bool) { targetPath = make(map[string]bool) @@ -47,7 +58,6 @@ func buildPathList(pathList map[string]bool) (targetPath map[string]bool) { path, err := exec.LookPath(name) if err != nil { log.Info("Failed to find path %s due to %v", path, err) - } else { targetPath[path] = enabled } @@ -57,19 +67,37 @@ func buildPathList(pathList map[string]bool) (targetPath map[string]bool) { return } -// SimpleCommand checks the args before creating *exec.Cmd -func SimpleCommand(name string, arg ...string) (cmd *exec.Cmd, err error) { - if allowPathlist[name] { - cmd = exec.Command(name, arg...) - } else { - err = checkCommandArgs(arg...) - if err != nil { - return - } - cmd = exec.Command(name, arg...) +// Command checks the args before creating *exec.Cmd +func Command(name string, arg ...string) (cmd *exec.Cmd, err error) { + commandSlice := append([]string{name}, arg...) + + if err = ValidateCommandSlice(commandSlice); err != nil { + return } - return + return exec.Command(name, arg...), nil +} + +// ValidateCommandSlice validates all the commands in the commandSlice. +// - For command in allowedPathList, it passes validation without further checks. +// - For a possible shell command, it calls ValidateShellCommandSlice() for detailed checks. +// - For any other command, it checks all the command args to ensure no illegal chars exists. +func ValidateCommandSlice(commandSlice []string) (err error) { + if len(commandSlice) == 0 { + return nil + } + + mainCmd := commandSlice[0] + if allowedPathList[mainCmd] { + return nil + } + + if shellCommandList[mainCmd] { + return ValidateShellCommandSlice(commandSlice) + } + + // For any other mainCmd, check illegal chars in command args + return checkCommandArgs(commandSlice[1:]...) } // CheckCommandArgs is check string is valid in args diff --git a/pkg/utils/cmdguard/exec_pipes.go b/pkg/utils/cmdguard/exec_pipes.go new file mode 100644 index 00000000000..6c47d7f891f --- /dev/null +++ b/pkg/utils/cmdguard/exec_pipes.go @@ -0,0 +1,190 @@ +/* +Copyright 2024 The Fluid 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 cmdguard + +import ( + "fmt" + "os/exec" + "strings" + + "github.com/pkg/errors" +) + +type CommandValidater func(str string, pattern string) bool + +var ( + PrefixMatch CommandValidater = func(str, pattern string) bool { return strings.HasPrefix(str, pattern) } + ExactMatch CommandValidater = func(str, pattern string) bool { return str == pattern } +) + +// Make sure the shell command is allowed +var AllowedShellCommands = map[string]CommandValidater{ + "bash -c": ExactMatch, + "sh -c": ExactMatch, +} + +// allowedFirstCommands is a global map that contains all allowed prefix for the first command in a shell pipe. +var allowedFirstCommands = map[string]CommandValidater{ + "ls": PrefixMatch, + "df": PrefixMatch, + "mount": PrefixMatch, + "alluxio": PrefixMatch, + "goosefs": PrefixMatch, + "kubectl": PrefixMatch, + "ddc-helm": PrefixMatch, +} + +// AllowedPipedCommands is a map that contains all allowed piped commands and their validater functions. +var allowedPipedCommands = map[string]CommandValidater{ + "grep": PrefixMatch, + "wc -l": ExactMatch, // means exact match (wc -l is exactly the allowed command) + // Add more commands as you see fit +} + +// Define illegal sequences that may lead to command injection attack +var illegalSequences = []string{"&", ";", "$", "'", "`", "(", ")", "||", ">>"} + +var allowedExpressions = []string{ + "${METAURL}", // JuiceFS community's metaurl +} + +// ShellCommand is a safe wrapper of exec.Command that checks potential risks in the command. +// It requires the command follows the format like ["bash", "-c", ""] and each part +// of the command must be valid. If no shell command is needed, use security.Command instead. +func ShellCommand(name string, arg ...string) (cmd *exec.Cmd, err error) { + var commands = append([]string{name}, arg...) + + err = ValidateShellCommandSlice(commands) + if err != nil { + return nil, err + } + + return exec.Command(name, arg...), nil +} + +// ValidateShellCommandSlice takes in a slice of shell commands and returns an error if any are invalid. +// The function looks specifically for pipe commands (i.e., commands that contain a '|'). +// If a pipe command is found in the slice, ValidatePipeCommandSlice is called for further validation. +func ValidateShellCommandSlice(shellCommandSlice []string) (err error) { + shellCommand, shellScript, err := splitShellCommand(shellCommandSlice) + if err != nil { + return errors.Wrapf(err, "failed to split shell pipe command %v", shellCommandSlice) + } + + if err := validateShellCommand(shellCommand); err != nil { + return errors.Wrapf(err, "failed to validate shell command [%s]", shellCommand) + } + + if strings.Contains(shellScript, "|") { + // shellScript is possible a shell pipeline + if err := validateShellPipeString(shellScript); err != nil { + return errors.Wrapf(err, "failed to validate shell script [%s]", shellScript) + } + } else { + if err := checkIllegalSequence(shellScript); err != nil { + return errors.Wrap(err, "failed to pass illegal sequence check") + } + } + + return +} + +func splitShellCommand(shellCommandSlice []string) (shellCommand string, pipedCommands string, err error) { + // A shell pipeline command slice is REQUIRED to have 3 parts of commands, e.g. "bash", "-c", "" + if len(shellCommandSlice) != 3 { + err = fmt.Errorf("invalid shell command slice. Expected num of slice is exactly 3, received %d", len(shellCommandSlice)) + return + } + + // We assume -c always directly follows the shell command + shellCommand = strings.Join(strings.Fields(shellCommandSlice[0]+" "+shellCommandSlice[1]), " ") + + return shellCommand, shellCommandSlice[2], nil +} + +func validateShellCommand(shellCommand string) (err error) { + if _, ok := AllowedShellCommands[shellCommand]; !ok { + return fmt.Errorf("unknown shell command: %s", shellCommand) + } + + return nil +} + +// validateShellPipeString function checks whether the input command string is safe to execute. +// It checks whether all parts of a pipeline command start with any command prefixes defined in AllowedCommands +// It also checks for any illegal sequences that may lead to command injection attack. +func validateShellPipeString(pipedCommandStr string) error { + // Separate parts of pipeline command + pipelineCommands := strings.Split(pipedCommandStr, "|") + + // Check each part of pipeline command + for i, cmd := range pipelineCommands { + cmd = strings.Join( + strings.Fields( + strings.TrimSpace(cmd)), " ") + + if i > 0 { + // Check whether command starts with any allowed command prefix + validCmd := isValidCommand(cmd, allowedPipedCommands) + + // If none of the allowed command prefix is found, throw error + if !validCmd { + return fmt.Errorf("full pipeline command not supported: part %d contains unsupported command '%s', the whole command %s", i+1, cmd, pipedCommandStr) + } + } else { + validCmd := isValidCommand(cmd, allowedFirstCommands) + // If none of the allowed command prefix is found, throw error + if !validCmd { + return fmt.Errorf("full pipeline command not supported: part %d contains unsupported command '%s', the whole command %s", i+1, cmd, pipedCommandStr) + } + } + + if err := checkIllegalSequence(cmd); err != nil { + return errors.Wrap(err, "failed to pass illegal sequence check") + } + } + + // If no error found, return nil + return nil +} + +// Defining a function to check if the command is valid +func isValidCommand(cmd string, allowedCommands map[string]CommandValidater) bool { + for allowedCmd, validaterFn := range allowedCommands { + if validaterFn(cmd, allowedCmd) { + return true + } + } + + return false +} + +func checkIllegalSequence(script string) error { + scriptToCheck := script + for _, allowedEnv := range allowedExpressions { + scriptToCheck = strings.ReplaceAll(scriptToCheck, allowedEnv, "ALLOWED_ENV") + } + + // TODO: Simply check illegal sequence for now. Better filtered with a allowed list in future. + for _, illegalSeq := range illegalSequences { + if strings.Contains(scriptToCheck, illegalSeq) { + return fmt.Errorf("unsafe shell script %s, illegal sequence detected: %s", script, illegalSeq) + } + } + + return nil +} diff --git a/pkg/utils/shell_pipes_test.go b/pkg/utils/cmdguard/exec_pipes_test.go similarity index 57% rename from pkg/utils/shell_pipes_test.go rename to pkg/utils/cmdguard/exec_pipes_test.go index 6a40a66c976..391ab09d466 100644 --- a/pkg/utils/shell_pipes_test.go +++ b/pkg/utils/cmdguard/exec_pipes_test.go @@ -1,4 +1,4 @@ -package utils +package cmdguard import ( "os/exec" @@ -42,14 +42,14 @@ func TestValidateShellPipeString(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := ValidateShellPipeString(tt.args.command); (err != nil) != tt.wantErr { + if err := validateShellPipeString(tt.args.command); (err != nil) != tt.wantErr { t.Errorf("Testcase '%s' ValidateShellPipeString() error = %v, wantErr %v", tt.name, err, tt.wantErr) } }) } } -func TestPipeCommand(t *testing.T) { +func TestShellCommand(t *testing.T) { type args struct { name string arg []string @@ -68,7 +68,7 @@ func TestPipeCommand(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotCmd, err := PipeCommand(tt.args.name, tt.args.arg...) + gotCmd, err := ShellCommand(tt.args.name, tt.args.arg...) if (err != nil) != tt.wantErr { t.Errorf("Testcase '%s': PipeCommand() error = %v, wantErr %v", tt.name, err, tt.wantErr) return @@ -83,49 +83,120 @@ func TestPipeCommand(t *testing.T) { } } -func TestValidatePipeCommandSlice(t *testing.T) { +func TestIsValidCommand(t *testing.T) { + type args struct { + cmd string + allowedCommands map[string]CommandValidater + } + tests := []struct { + name string + args args + want bool + }{ + {name: "valid bash command", args: args{cmd: "bash", allowedCommands: map[string]CommandValidater{"bash": ExactMatch}}, want: true}, + {name: "valid sh command", args: args{cmd: "sh", allowedCommands: map[string]CommandValidater{"bash": ExactMatch, "sh": ExactMatch}}, want: true}, + {name: "invalid zsh command", args: args{cmd: "zsh", allowedCommands: map[string]CommandValidater{"bash": ExactMatch}}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isValidCommand(tt.args.cmd, tt.args.allowedCommands); got != tt.want { + t.Errorf("Testcase '%s': isValidCommand() = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +func Test_splitShellCommand(t *testing.T) { type args struct { shellCommandSlice []string } tests := []struct { - name string - args args - wantErr bool + name string + args args + wantShellCommand string + wantPipedCommands string + wantErr bool }{ - {name: "valid bash command", args: args{shellCommandSlice: []string{"bash", "-c", "ls"}}, wantErr: false}, - {name: "valid sh command", args: args{shellCommandSlice: []string{"sh", "-c", "ls"}}, wantErr: false}, - {name: "unknown shell command", args: args{shellCommandSlice: []string{"zsh", "-c", "ls"}}, wantErr: true}, - {name: "invalid bash command", args: args{shellCommandSlice: []string{"bash", "-c", "wrong_command"}}, wantErr: true}, - {name: "insufficient arguments", args: args{shellCommandSlice: []string{"bash", "-c"}}, wantErr: true}, - {name: "empty command slice", args: args{shellCommandSlice: []string{}}, wantErr: true}, + { + name: "valid shell command", + args: args{shellCommandSlice: []string{" bash ", " -c", "echo foobar | grep foo"}}, + wantShellCommand: "bash -c", + wantPipedCommands: "echo foobar | grep foo", + wantErr: false, + }, + { + name: "empty shell command", + args: args{shellCommandSlice: []string{}}, + wantShellCommand: "", + wantPipedCommands: "", + wantErr: true, + }, + { + name: "invalid command without shell", + args: args{shellCommandSlice: []string{"echo foobar | grep foo"}}, + wantShellCommand: "", + wantPipedCommands: "", + wantErr: true, + }, + { + name: "valid command without shell", + args: args{shellCommandSlice: []string{"test", "hello", "--help"}}, + wantShellCommand: "test hello", + wantPipedCommands: "--help", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := ValidatePipeCommandSlice(tt.args.shellCommandSlice); (err != nil) != tt.wantErr { - t.Errorf("Testcase '%s': ValidatePipeCommandSlice() error = %v, wantErr %v", tt.name, err, tt.wantErr) + gotShellCommand, gotPipedCommands, err := splitShellCommand(tt.args.shellCommandSlice) + if (err != nil) != tt.wantErr { + t.Errorf("splitShellCommand() error = %v, wantErr %v", err, tt.wantErr) + return + } + if gotShellCommand != tt.wantShellCommand { + t.Errorf("splitShellCommand() gotShellCommand = %v, want %v", gotShellCommand, tt.wantShellCommand) + } + if gotPipedCommands != tt.wantPipedCommands { + t.Errorf("splitShellCommand() gotPipedCommands = %v, want %v", gotPipedCommands, tt.wantPipedCommands) } }) } } -func TestIsValidCommand(t *testing.T) { +func Test_validateShellCommand(t *testing.T) { type args struct { - cmd string - allowedCommands map[string]bool + shellCommand string } tests := []struct { - name string - args args - want bool + name string + args args + wantErr bool }{ - {name: "valid bash command", args: args{cmd: "bash", allowedCommands: map[string]bool{"bash": true}}, want: true}, - {name: "valid sh command", args: args{cmd: "sh", allowedCommands: map[string]bool{"bash": true, "sh": true}}, want: true}, - {name: "invalid zsh command", args: args{cmd: "zsh", allowedCommands: map[string]bool{"bash": true}}, want: false}, + { + name: "bash shell", + args: args{shellCommand: "bash -c"}, + wantErr: false, + }, + { + name: "sh shell", + args: args{shellCommand: "sh -c"}, + wantErr: false, + }, + { + name: "zsh shell(invalid)", + args: args{shellCommand: "zsh -c"}, + wantErr: true, + }, + { + name: "bash command", + args: args{shellCommand: "bash -s"}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := isValidCommand(tt.args.cmd, tt.args.allowedCommands); got != tt.want { - t.Errorf("Testcase '%s': isValidCommand() = %v, want %v", tt.name, got, tt.want) + if err := validateShellCommand(tt.args.shellCommand); (err != nil) != tt.wantErr { + t.Errorf("validateShellCommand() error = %v, wantErr %v", err, tt.wantErr) } }) } diff --git a/pkg/utils/exec_test.go b/pkg/utils/cmdguard/exec_test.go similarity index 52% rename from pkg/utils/exec_test.go rename to pkg/utils/cmdguard/exec_test.go index 036cb4a6eec..3c1e79ea933 100644 --- a/pkg/utils/exec_test.go +++ b/pkg/utils/cmdguard/exec_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package utils +package cmdguard import ( "fmt" @@ -64,7 +64,7 @@ func TestCheckCommandArgs(t *testing.T) { } } -func TestSimpleCommand(t *testing.T) { +func TestCommand(t *testing.T) { type args struct { name string arg []string @@ -76,15 +76,12 @@ func TestSimpleCommand(t *testing.T) { wantErr bool }{ { - name: "Allow path list", + name: "Allowed path list", args: args{ name: "kubectl", - arg: []string{"Hello", "World"}, - }, - wantCmd: &exec.Cmd{ - Path: "echo", - Args: []string{"Hello", "World"}, + arg: []string{"create", "configmap"}, }, + wantCmd: exec.Command("kubectl", "create", "configmap"), wantErr: false, }, { name: "Valid command arguments", @@ -92,10 +89,7 @@ func TestSimpleCommand(t *testing.T) { name: "echo", arg: []string{"Hello", "World"}, }, - wantCmd: &exec.Cmd{ - Path: "echo", - Args: []string{"Hello", "World"}, - }, + wantCmd: exec.Command("echo", "Hello", "World"), wantErr: false, }, { @@ -107,25 +101,84 @@ func TestSimpleCommand(t *testing.T) { wantCmd: nil, wantErr: true, }, + { + name: "Valid shell command", + args: args{ + name: "bash", + arg: []string{"-c", "echo hello world"}, + }, + wantCmd: exec.Command("bash", "-c", "echo hello world"), + wantErr: false, + }, + { + name: "Invalid shell command", + args: args{ + name: "sh", + arg: []string{"/entrypoint.sh"}, + }, + wantCmd: nil, + wantErr: true, + }, + { + name: "Valid shell pipeline command", + args: args{ + name: "sh", + arg: []string{"-c", "ls -lh | grep -c test"}, + }, + wantCmd: exec.Command("sh", "-c", "ls -lh | grep -c test"), + wantErr: false, + }, + { + name: "Invalid shell pipeline command (invalid pipelined command)", + args: args{ + name: "bash", + arg: []string{"-c", "du -sh ./ | xargs echo"}, + }, + wantCmd: nil, + wantErr: true, + }, + { + name: "Invalid shell pipeline command (invalid first command)", + args: args{ + name: "bash", + arg: []string{"-c", "echo hello | grep hello"}, + }, + wantCmd: nil, + wantErr: true, + }, + { + name: "Invalid shell pipeline command (illegal sequence)", + args: args{ + name: "bash", + arg: []string{"-c", "ls -lh $(cat myfile) | grep hello"}, + }, + wantCmd: nil, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, err := SimpleCommand(tt.args.name, tt.args.arg...) + cmd, err := Command(tt.args.name, tt.args.arg...) if (err != nil) != tt.wantErr { - t.Errorf("SimpleCommand() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("Command() error = %v, wantErr %v", err, tt.wantErr) return } if err != nil { return } - tt.wantCmd = exec.Command(tt.args.name, tt.args.arg...) - if !reflect.DeepEqual(tt.wantCmd.Args, cmd.Args) { - t.Errorf("SimpleCommand() = %v, want %v", tt.args.arg, cmd.Args) - } - if !reflect.DeepEqual(tt.wantCmd.Path, cmd.Path) { - t.Errorf("SimpleCommand() = %v, want %v", tt.args.arg, cmd.Args) + + if !reflect.DeepEqual(tt.wantCmd, cmd) { + t.Fatalf("Command() = %v, want %v", cmd, tt.wantCmd) } + + // tt.wantCmd = exec.Command(tt.args.name, tt.args.arg...) + // if !reflect.DeepEqual(tt.wantCmd.Args, cmd.Args) { + // t.Errorf("SimpleCommand() = %v, want %v", tt.args.arg, cmd.Args) + // } + // if !reflect.DeepEqual(tt.wantCmd.Path, cmd.Path) { + // t.Errorf("SimpleCommand() = %v, want %v", tt.args.arg, cmd.Args) + // } }) } } @@ -186,3 +239,32 @@ func Test_buildPathList(t *testing.T) { }) } } + +func TestValidateCommandSlice(t *testing.T) { + type args struct { + commandSlice []string + } + tests := []struct { + name string + args args + wantErr bool + }{ + {name: "allowed path list", args: args{[]string{"kubectl", "create", "foobar"}}, wantErr: false}, + {name: "valid command args", args: args{[]string{"echo", "hello"}}, wantErr: false}, + {name: "invalid command args", args: args{[]string{"echo", "$MYENV"}}, wantErr: true}, + {name: "valid shell command", args: args{[]string{"sh", "-c", "echo test"}}, wantErr: false}, + {name: "invalid shell command", args: args{[]string{"sh", "-c", "echo $MYENV"}}, wantErr: true}, + {name: "invalid shell command", args: args{[]string{"sh", "myscript.sh"}}, wantErr: true}, + {name: "valid shell piped command", args: args{[]string{"bash", "-c", "ls -lh ./ | wc -l"}}, wantErr: false}, + {name: "invalid shell piped command(invalid pipe command)", args: args{[]string{"bash", "-c", "ls -lh ./ | xargs echo"}}, wantErr: true}, + {name: "invalid shell piped command(invalid first command)", args: args{[]string{"bash", "-c", "echo foobar | grep foo"}}, wantErr: true}, + {name: "invalid shell piped command(illegal sequence)", args: args{[]string{"bash", "-c", "du -sh ./ | grep $(foo) | wc -l"}}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateCommandSlice(tt.args.commandSlice); (err != nil) != tt.wantErr { + t.Errorf("ValidateCommandSlice() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/pkg/utils/helm/get.go b/pkg/utils/helm/get.go deleted file mode 100644 index f5702f866ee..00000000000 --- a/pkg/utils/helm/get.go +++ /dev/null @@ -1,17 +0,0 @@ -/* -Copyright 2023 The Fluid Author. - -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 helm diff --git a/pkg/utils/helm/helm.go b/pkg/utils/helm/helm.go deleted file mode 100644 index 3be00a45448..00000000000 --- a/pkg/utils/helm/helm.go +++ /dev/null @@ -1,103 +0,0 @@ -/* - -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 helm - -import ( - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - - "github.com/go-logr/logr" - ctrl "sigs.k8s.io/controller-runtime" - - "github.com/fluid-cloudnative/fluid/pkg/utils" -) - -var log logr.Logger - -func init() { - log = ctrl.Log.WithName("helm") -} - -var helmCmd = []string{"ddc-helm"} - -// GenerateValueFile generates value file. -// It returns the name of the value file and error -func GenerateValueFile(values interface{}) (valueFileName string, err error) { - // 1. generate the template file - valueFile, err := os.CreateTemp(os.TempDir(), "values") - if err != nil { - log.Error(err, "Failed to create tmp file", "tmpfile", valueFile.Name()) - return "", err - } - - valueFileName = valueFile.Name() - log.V(1).Info("Save the values file", "fileName", valueFileName) - - // 2. dump the object into the template file - err = utils.ToYaml(values, valueFile) - return valueFileName, err -} - -// GetChartVersion checks the chart version by given the chart directory -// helm inspect chart /charts/tf-horovod -func GetChartVersion(chart string) (version string, err error) { - binary, err := exec.LookPath(helmCmd[0]) - if err != nil { - return "", err - } - - // 1. check if the chart file exists, if it's it's unix path, then check if it's exist - // if strings.HasPrefix(chart, "/") { - if _, err = os.Stat(chart); err != nil { - return "", err - } - // } - - // 2. prepare the arguments - args := []string{binary, "inspect", "chart", chart, - "|", "grep", "version:"} - log.V(1).Info("Exec bash -c", "args", args) - - // cmd := exec.Command("bash", "-c", strings.Join(args, " ")) - cmd, err := utils.PipeCommand("bash", "-c", strings.Join(args, " ")) - if err != nil { - return "", err - } - out, err := cmd.Output() - if err != nil { - return "", err - } - - lines := strings.Split(string(out), "\n") - if len(lines) == 0 { - return "", fmt.Errorf("failed to find version when executing %s, result is %s", args, out) - } - fields := strings.Split(lines[0], ":") - if len(fields) != 2 { - return "", fmt.Errorf("failed to find version when executing %s, result is %s", args, out) - } - - version = strings.TrimSpace(fields[1]) - return version, nil -} - -// GetChartName extracts the last element of the chart's path as the chart's name -func GetChartName(chart string) string { - return filepath.Base(chart) -} diff --git a/pkg/utils/helm/helm_test.go b/pkg/utils/helm/helm_test.go deleted file mode 100644 index 35e9dc8a1c5..00000000000 --- a/pkg/utils/helm/helm_test.go +++ /dev/null @@ -1,133 +0,0 @@ -package helm - -import ( - "errors" - "os" - "os/exec" - "testing" - - "github.com/brahma-adshonor/gohook" -) - -func TestGenerateValueFile(t *testing.T) { - _, err := GenerateValueFile("test-value") - if err != nil { - t.Errorf("fail to exec the function") - return - } -} - -func TestGetChartVersion(t *testing.T) { - LookPathCommon := func(file string) (string, error) { - return "helm", nil - } - LookPathErr := func(file string) (string, error) { - return "", errors.New("fail to run the command") - } - StatCommon := func(name string) (os.FileInfo, error) { - return nil, nil - } - StatErr := func(name string) (os.FileInfo, error) { - return nil, errors.New("fail to run the command") - } - OutputCommon := func(cmd *exec.Cmd) ([]byte, error) { - return []byte("fluid:v0.6.0"), nil - } - OutputErr := func(cmd *exec.Cmd) ([]byte, error) { - return nil, errors.New("fail to run the command") - } - - wrappedUnhookOutput := func() { - err := gohook.UnHook((*exec.Cmd).Output) - if err != nil { - t.Fatal(err.Error()) - } - } - wrappedUnhookStat := func() { - err := gohook.UnHook(os.Stat) - if err != nil { - t.Fatal(err.Error()) - } - } - wrappedUnhookLookPath := func() { - err := gohook.UnHook(exec.LookPath) - if err != nil { - t.Fatal(err.Error()) - } - } - - err := gohook.Hook(exec.LookPath, LookPathErr, nil) - if err != nil { - t.Fatal(err.Error()) - } - _, err = GetChartVersion("fluid:v0.6.0") - if err == nil { - t.Errorf("fail to catch the error") - } - wrappedUnhookLookPath() - - err = gohook.Hook(exec.LookPath, LookPathCommon, nil) - if err != nil { - t.Fatal(err.Error()) - } - err = gohook.Hook(os.Stat, StatErr, nil) - if err != nil { - t.Fatal(err.Error()) - } - _, err = GetChartVersion("fluid:v0.6.0") - if err == nil { - t.Errorf("fail to catch the error") - } - wrappedUnhookStat() - - err = gohook.Hook(os.Stat, StatCommon, nil) - if err != nil { - t.Fatal(err.Error()) - } - err = gohook.Hook((*exec.Cmd).Output, OutputErr, nil) - if err != nil { - t.Fatal(err.Error()) - } - _, err = GetChartVersion("fluid:v0.6.0") - if err == nil { - t.Errorf("fail to catch the error") - } - wrappedUnhookOutput() - - err = gohook.Hook((*exec.Cmd).Output, OutputCommon, nil) - if err != nil { - t.Fatal(err.Error()) - } - version, err := GetChartVersion("fluid:v0.6.0") - if err != nil { - t.Errorf("fail to exec the function due to %v", err) - } - if version != "v0.6.0" { - t.Errorf("fail to get the version of the helm due to %v", err) - } - wrappedUnhookOutput() - wrappedUnhookStat() - wrappedUnhookLookPath() -} - -func TestGetChartName(t *testing.T) { - var testCases = []struct { - chartName string - expectedResult string - }{ - { - chartName: "", - expectedResult: ".", - }, - { - chartName: "/chart/fluid", - expectedResult: "fluid", - }, - } - - for _, test := range testCases { - if name := GetChartName(test.chartName); name != test.expectedResult { - t.Errorf("fail to get chart name") - } - } -} diff --git a/pkg/utils/helm/utils.go b/pkg/utils/helm/utils.go index 2e853554b8b..40008842fab 100644 --- a/pkg/utils/helm/utils.go +++ b/pkg/utils/helm/utils.go @@ -26,9 +26,20 @@ import ( "syscall" "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" + "github.com/go-logr/logr" "github.com/pkg/errors" + ctrl "sigs.k8s.io/controller-runtime" ) +var log logr.Logger + +func init() { + log = ctrl.Log.WithName("helm") +} + +var helmCmd = []string{"ddc-helm"} + // InstallRelease installs the release with cmd: helm install -f values.yaml chart_name, support helm v3 func InstallRelease(name string, namespace string, valueFile string, chartName string) error { defer utils.TimeTrack(time.Now(), "Helm.InstallRelease", "name", name, "namespace", namespace) @@ -55,7 +66,7 @@ func InstallRelease(name string, namespace string, valueFile string, chartName s // return syscall.Exec(cmd, args, env) // 5. execute the command - cmd, err := utils.SimpleCommand(binary, args...) + cmd, err := cmdguard.Command(binary, args...) if err != nil { return err } @@ -85,7 +96,7 @@ func CheckRelease(name, namespace string) (exist bool, err error) { return exist, err } - cmd, err := utils.SimpleCommand(helmCmd[0], "status", name, "-n", namespace) + cmd, err := cmdguard.Command(helmCmd[0], "status", name, "-n", namespace) if err != nil { return exist, err } @@ -153,7 +164,7 @@ func DeleteRelease(name, namespace string) error { } args := []string{"uninstall", name, "-n", namespace} - cmd, err := utils.SimpleCommand(binary, args...) + cmd, err := cmdguard.Command(binary, args...) if err != nil { return err } @@ -180,7 +191,7 @@ func ListReleases(namespace string) (releases []string, err error) { return releases, err } - cmd, err := utils.SimpleCommand(helmCmd[0], "list", "-q", "-n", namespace) + cmd, err := cmdguard.Command(helmCmd[0], "list", "-q", "-n", namespace) if err != nil { return releases, err } @@ -203,7 +214,7 @@ func ListReleaseMap(namespace string) (releaseMap map[string]string, err error) return releaseMap, err } - cmd, err := utils.SimpleCommand(helmCmd[0], "list", "-n", namespace) + cmd, err := cmdguard.Command(helmCmd[0], "list", "-n", namespace) if err != nil { return releaseMap, err } @@ -240,7 +251,7 @@ func ListAllReleasesWithDetail(namespace string) (releaseMap map[string][]string return releaseMap, err } - cmd, err := utils.SimpleCommand(helmCmd[0], "list", "--all", "-n", namespace) + cmd, err := cmdguard.Command(helmCmd[0], "list", "--all", "-n", namespace) if err != nil { return releaseMap, err } diff --git a/pkg/utils/helm/utils_test.go b/pkg/utils/helm/utils_test.go index 8a8b7164052..e5e5cadbd47 100644 --- a/pkg/utils/helm/utils_test.go +++ b/pkg/utils/helm/utils_test.go @@ -336,12 +336,12 @@ func TestListReleases(t *testing.T) { t.Errorf("fail to exec the function ListRelease") } wrappedUnhookOutput() - wrappedUnhookLookPath() _, err = ListReleases("def$ault") if err == nil { t.Errorf("fail to catch the error") } + wrappedUnhookLookPath() } func TestListReleaseMap(t *testing.T) { @@ -407,12 +407,12 @@ func TestListReleaseMap(t *testing.T) { t.Errorf("fail to split the strout") } wrappedUnhookOutput() - wrappedUnhookLookPath() _, err = ListReleaseMap("def$ault") if err == nil { t.Errorf("fail to catch the error") } + wrappedUnhookLookPath() } func TestListAllReleasesWithDetail(t *testing.T) { @@ -478,12 +478,12 @@ func TestListAllReleasesWithDetail(t *testing.T) { t.Errorf("fail to split the strout") } wrappedUnhookOutput() - wrappedUnhookLookPath() _, err = ListAllReleasesWithDetail("def$ault") if err == nil { t.Errorf("fail to catch the error") } + wrappedUnhookLookPath() } func TestDeleteReleaseIfExists(t *testing.T) { diff --git a/pkg/utils/home.go b/pkg/utils/home.go index 0c557e52a40..fc77a92c232 100644 --- a/pkg/utils/home.go +++ b/pkg/utils/home.go @@ -20,6 +20,7 @@ import ( "bytes" "errors" "os" + "os/exec" "os/user" "runtime" "strings" @@ -53,10 +54,9 @@ func homeUnix() (home string, err error) { // If that fails, try the shell var stdout bytes.Buffer - cmd, err := SimpleCommand("sh", "-c", "eval echo ~$USER") - if err != nil { - return - } + // cmd, err := security.Command("sh", "-c", "eval echo ~$USER") + cmd := exec.Command("sh", "-c", "eval echo ~$USER") + cmd.Stdout = &stdout err = cmd.Run() if err != nil { diff --git a/pkg/utils/kubeclient/exec.go b/pkg/utils/kubeclient/exec.go index a0455f6ca80..34c7017e975 100644 --- a/pkg/utils/kubeclient/exec.go +++ b/pkg/utils/kubeclient/exec.go @@ -151,11 +151,6 @@ func ExecCommandInContainerWithFullOutput(podName string, containerName string, }) } -// ExecShellInContainer executes shell command or script in the specified container and return stdout, stderr and error -func ExecShellInContainer(podName string, containerName string, namespace string, cmd string) (stdout string, stderr string, err error) { - return ExecCommandInContainer(podName, containerName, namespace, []string{"/bin/sh", "-c", cmd}) -} - // A wrapper function of ExecCommandInContainerWithFullOutput func ExecCommandInContainer(podName string, containerName string, namespace string, cmd []string) (stdout string, stderr string, err error) { return ExecCommandInContainerWithFullOutput(podName, containerName, namespace, cmd) diff --git a/pkg/utils/kubectl/configmap.go b/pkg/utils/kubectl/configmap.go index 5038649943c..7892ed56ab7 100644 --- a/pkg/utils/kubectl/configmap.go +++ b/pkg/utils/kubectl/configmap.go @@ -21,7 +21,7 @@ import ( "os/exec" "strings" - "github.com/fluid-cloudnative/fluid/pkg/utils" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/go-logr/logr" ctrl "sigs.k8s.io/controller-runtime" ) @@ -69,7 +69,7 @@ func kubectl(args []string) ([]byte, error) { // return syscall.Exec(cmd, args, env) // 2. execute the command - cmd, err := utils.SimpleCommand(binary, args...) + cmd, err := cmdguard.Command(binary, args...) if err != nil { return nil, err } diff --git a/pkg/utils/mount.go b/pkg/utils/mount.go index 480a8300d8a..c2930d9cd08 100644 --- a/pkg/utils/mount.go +++ b/pkg/utils/mount.go @@ -23,6 +23,7 @@ import ( "os/exec" "strings" + "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" "github.com/fluid-cloudnative/fluid/pkg/utils/validation" "github.com/golang/glog" corev1 "k8s.io/api/core/v1" @@ -47,7 +48,7 @@ func CheckMountReadyAndSubPathExist(fluidPath string, mountType string, subPath return errors.New("target is not specified for checking the mount") } args := []string{fluidPath, mountType, subPath} - command, err := SimpleCommand("/usr/local/bin/check_mount.sh", args...) + command, err := cmdguard.Command("/usr/local/bin/check_mount.sh", args...) if err != nil { return } diff --git a/pkg/utils/shell_pipes.go b/pkg/utils/shell_pipes.go deleted file mode 100644 index f79f016aa37..00000000000 --- a/pkg/utils/shell_pipes.go +++ /dev/null @@ -1,156 +0,0 @@ -/* -Copyright 2024 The Fluid 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 utils - -import ( - "fmt" - "os/exec" - "strings" -) - -func PipeCommand(name string, arg ...string) (cmd *exec.Cmd, err error) { - // prepare the slice for ValidatePipeCommandSlice - var commands []string - commands = append(commands, name) - commands = append(commands, arg...) - - // validate commands - err = ValidatePipeCommandSlice(commands) - if err != nil { - return nil, err - } - - return exec.Command(name, arg...), nil -} - -// ValidateCommandSlice takes in a slice of shell commands and returns an error if any are invalid. -// The function looks specifically for pipe commands (i.e., commands that contain a '|'). -// If a pipe command is found in the slice, ValidatePipeCommandSlice is called for further validation. -func ValidateCommandSlice(shellCommandSlice []string) (err error) { - isPossiblePipeCommand := false - - for _, command := range shellCommandSlice { - if strings.Contains(command, "|") { - isPossiblePipeCommand = true - } - } - - if isPossiblePipeCommand { - err = ValidatePipeCommandSlice(shellCommandSlice) - } // else { - // // Todo: need handle no PossiblePipeCommand - // } - return -} - -func ValidatePipeCommandSlice(shellCommandSlice []string) (err error) { - // Make sure the shell command is allowed - var AllowedShellCommands = map[string]bool{ - "bash -c": true, - "sh -c": true, - } - - // check if shellCommandSlice has enough arguments - if len(shellCommandSlice) < 3 { - return fmt.Errorf("insufficient arguments. Expected at least 3, received %d", len(shellCommandSlice)) - } - // We assume -c always directly follows the shell command - shellCommand := strings.Join(strings.Fields(shellCommandSlice[0]+" "+shellCommandSlice[1]), " ") - if _, ok := AllowedShellCommands[shellCommand]; !ok { - return fmt.Errorf("unknown shell command: %s", shellCommand) - } - - for _, command := range shellCommandSlice[2:] { - if err := ValidateShellPipeString(command); err != nil { - return err - } - } - return -} - -// ValidateShellPipeString function checks whether the input command string is safe to execute. -// It checks whether all parts of a pipeline command start with any command prefixes defined in AllowedCommands -// It also checks for any illegal sequences that may lead to command injection attack. -func ValidateShellPipeString(command string) error { - // Define illegal sequences that may lead to command injection attack - illegalSequences := []string{"&", ";", "$", "'", "`", "(", ")", "||", ">>"} - // Separate parts of pipeline command - pipelineCommands := strings.Split(command, "|") - - // AllowedCommands is a global map that contains all allowed command prefixes. - var AllowedCommands = map[string]bool{ - "ls": false, - "df": false, - "mount": false, - "alluxio": false, - "goosefs": false, - "kubectl": false, - "helm": false, - } - - // AllowedPipeCommands is a map that contains all allowed pipe command prefixes. - var allowedPipeCommands = map[string]bool{ - "grep": false, // false means partial match - "wc -l": true, // true means full match (wc -l is exactly the allowed command) - // Add more commands as you see fit - } - - // Check each part of pipeline command - for i, cmd := range pipelineCommands { - cmd = strings.Join( - strings.Fields( - strings.TrimSpace(cmd)), " ") - - if i > 0 { - // Check whether command starts with any allowed command prefix - validCmd := isValidCommand(cmd, allowedPipeCommands) - - // If none of the allowed command prefix is found, throw error - if !validCmd { - return fmt.Errorf("full pipeline command not supported: part %d contains unsupported command '%s', the whole command %s", i+1, cmd, command) - } - } else { - validCmd := isValidCommand(cmd, AllowedCommands) - // If none of the allowed command prefix is found, throw error - if !validCmd { - return fmt.Errorf("full pipeline command not supported: part %d contains unsupported command '%s', the whole command %s", i+1, cmd, command) - } - } - - // Check for illegal sequences in command - for _, illegalSeq := range illegalSequences { - if strings.Contains(cmd, illegalSeq) { - return fmt.Errorf("unsafe pipeline command %s, illegal sequence detected: %s in part %d: '%s'", command, illegalSeq, i+1, cmd) - } - } - } - - // If no error found, return nil - return nil -} - -// Defining a function to check if the command is valid -func isValidCommand(cmd string, allowedCommands map[string]bool) bool { - for cmdPrefix, exactMatch := range allowedCommands { - if exactMatch && cmd == cmdPrefix { - return true - } else if !exactMatch && strings.HasPrefix(cmd, cmdPrefix) { - return true - } - } - return false -} diff --git a/sdk/python b/sdk/python index 3056a5e14ee..91bc786e628 160000 --- a/sdk/python +++ b/sdk/python @@ -1 +1 @@ -Subproject commit 3056a5e14ee6aa0edbb0699f8bca59cc9dfa41e2 +Subproject commit 91bc786e62833c3b4718997b03bbb8266959827b