Merge pull request #10069 from microsoft/danmihai1/exec-args

genpolicy: validate each exec command line arg

src/tools/genpolicy/genpolicy-settings.json

@@ -299,7 +299,7 @@
             "^$(cpath)/"
         ],
         "ExecProcessRequest": {
-            "commands": [],
+            "allowed_commands": [],
             "regex": []
         },
         "CloseStdinRequest": false,

src/tools/genpolicy/rules.rego

@@ -1111,12 +1111,9 @@ CreateSandboxRequest {
 ExecProcessRequest {
     print("ExecProcessRequest 1: input =", input)
 
-    i_command = concat(" ", input.process.Args)
-    print("ExecProcessRequest 1: i_command =", i_command)
-
-    some p_command in policy_data.request_defaults.ExecProcessRequest.commands
+    some p_command in policy_data.request_defaults.ExecProcessRequest.allowed_commands
     print("ExecProcessRequest 1: p_command =", p_command)
-    p_command == i_command
+    p_command == input.process.Args
 
     print("ExecProcessRequest 1: true")
 }

src/tools/genpolicy/src/policy.rs

@@ -314,8 +314,12 @@ pub struct CreateContainerRequestDefaults {
 /// ExecProcessRequest settings from genpolicy-settings.json.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct ExecProcessRequestDefaults {
+    /// Allow these commands to be executed. This field has been deprecated - use allowed_commands instead.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub commands: Option<Vec<String>>,
+
     /// Allow these commands to be executed.
-    commands: Vec<String>,
+    pub allowed_commands: Vec<Vec<String>>,
 
     /// Allow commands matching these regexes to be executed.
     regex: Vec<String>,

src/tools/genpolicy/src/settings.rs

@@ -73,6 +73,7 @@ impl Settings {
         if let Ok(file) = File::open(json_settings_path) {
             let settings: Self = serde_json::from_reader(file).unwrap();
             debug!("settings = {:?}", &settings);
+            Self::validate_settings(&settings);
             settings
         } else {
             panic!("Cannot open file {}. Please copy it to the current directory or specify the path to it using the -j parameter.",
@@ -87,4 +88,13 @@ impl Settings {
             &self.other_container
         }
     }
+
+    fn validate_settings(settings: &Self) {
+        if let Some(commands) = &settings.request_defaults.ExecProcessRequest.commands {
+            if !commands.is_empty() {
+                panic!("The settings field <request_defaults.ExecProcessRequest.commands> has been deprecated. \
+                    Please use <request_defaults.ExecProcessRequest.allowed_commands> instead.");
+            }
+        }
+    }
 }

tests/integration/kubernetes/k8s-exec.bats

@@ -60,7 +60,7 @@ EOF
 
 	## Case for return value
 	### Command return non-zero code
-	run bash -c "kubectl exec -i $pod_name -- sh <<-EOF
+	run bash -c "kubectl exec -i $pod_name -- "$sh_command" <<-EOF
 exit 123
 EOF"
 	echo "run status: $status" 1>&2

tests/integration/kubernetes/k8s-policy-pod.bats

@@ -17,8 +17,8 @@ setup() {
 	get_pod_config_dir
 	policy_settings_dir="$(create_tmp_policy_settings_dir "${pod_config_dir}")"
 	
-	exec_command="printenv data-3"
-	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command}"
+	exec_command=(printenv data-3)
+	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command[@]}"
 	add_requests_to_policy_settings "${policy_settings_dir}" "ReadStreamRequest"
 
 	correct_configmap_yaml="${pod_config_dir}/k8s-policy-configmap.yaml"
@@ -63,10 +63,8 @@ wait_for_pod_ready() {
 }
 
 @test "Able to read env variables sourced from configmap using envFrom" {
-	kubectl create -f "${correct_configmap_yaml}"
-	kubectl create -f "${correct_pod_yaml}"
-	kubectl wait --for=condition=Ready "--timeout=${timeout}" pod "${pod_name}"
-	expected_env_var=$(kubectl exec "${pod_name}" -- printenv data-3)
+	wait_for_pod_ready
+	expected_env_var=$(kubectl exec "${pod_name}" -- ${exec_command[@]})
 	[ "$expected_env_var" = "value-3" ] || fail "expected_env_var is not equal to value-3"
 }
 

tests/integration/kubernetes/tests_common.sh

@@ -236,23 +236,15 @@ add_exec_to_policy_settings() {
 	auto_generate_policy_enabled || return 0
 
 	local -r settings_dir="$1"
-
-	# TODO: teach genpolicy to work with an array of args, instead of joining the args here.
 	shift
-	if [ "${#@}" -gt "1" ]; then
-		# Join all the exec args.
-		local allowed_exec=$(printf '%s ' "${@}")
 
-		# Remove the trailing space character.
-		allowed_exec="${allowed_exec::-1}"
-	else
-		local -r allowed_exec="$1"
-	fi
+	# Create a JSON array of strings containing all the args of the command to be allowed.
+	local exec_args=$(printf "%s\n" "$@" | jq -R | jq -sc)
 
 	# Change genpolicy settings to allow kubectl to exec the command specified by the caller.
-	info "${settings_dir}/genpolicy-settings.json: allowing exec: ${allowed_exec}"
-	jq --arg allowed_exec "${allowed_exec}" \
-		'.request_defaults.ExecProcessRequest.commands |= . + [$allowed_exec]' \
+	local jq_command=".request_defaults.ExecProcessRequest.allowed_commands |= . + [${exec_args}]"
+	info "${settings_dir}/genpolicy-settings.json: executing jq command: ${jq_command}"
+	jq "${jq_command}" \
 		"${settings_dir}/genpolicy-settings.json" > \
 		"${settings_dir}/new-genpolicy-settings.json"
 	mv "${settings_dir}/new-genpolicy-settings.json" \
@@ -281,29 +273,28 @@ add_requests_to_policy_settings() {
 # Change genpolicy settings to allow executing on the Guest VM the commands
 # used by "kubectl cp" from the Host to the Guest.
 add_copy_from_host_to_policy_settings() {
-	declare -r genpolicy_settings_dir="$1"
+	local -r genpolicy_settings_dir="$1"
 
-	exec_command="test -d /tmp"
-	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command}"
-	exec_command="tar -xmf - -C /tmp"
-	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command}"
+	local exec_command=(test -d /tmp)
+	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command[@]}"
+	exec_command=(tar -xmf - -C /tmp)
+	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command[@]}"
 }
 
 # Change genpolicy settings to allow executing on the Guest VM the commands
 # used by "kubectl cp" from the Guest to the Host.
 add_copy_from_guest_to_policy_settings() {
-	declare -r genpolicy_settings_dir="$1"
-	declare -r copied_file="$2"
+	local -r genpolicy_settings_dir="$1"
+	local -r copied_file="$2"
 
-	exec_command="tar cf - ${copied_file}"
-	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command}"
+	exec_command=(tar cf - "${copied_file}")
+	add_exec_to_policy_settings "${policy_settings_dir}" "${exec_command[@]}"
 }
 
-# Change genpolicy settings to allow "kubectl exec" to execute a command
-# and to read console output from a test pod.
+# Change genpolicy settings to use a pod namespace different than "default".
 set_namespace_to_policy_settings() {
-	declare -r settings_dir="$1"
-	declare -r namespace="$2"
+	local -r settings_dir="$1"
+	local -r namespace="$2"
 
 	auto_generate_policy_enabled || return 0