From a37f10fc87a735242cba8c388a559fb500f96ebc Mon Sep 17 00:00:00 2001 From: Dan Mihai Date: Thu, 25 Jul 2024 16:57:17 +0000 Subject: [PATCH] genpolicy: validate each exec command line arg Generate policy that validates each exec command line argument, instead of joining those args and validating the resulting string. Joining the args ignored the fact that some of the args might include space characters. The older format from genpolicy-settings.json was similar to: "ExecProcessRequest": { "commands": [ "sh -c cat /proc/self/status" ], "regex": [] }, That format will not be supported anymore. genpolicy will detect if its users are trying to use the older "commands" field and will exit with a relevant error message in that case. The new settings format is: "ExecProcessRequest": { "allowed_commands": [ [ "sh", "-c", "cat /proc/self/status" ] ], "regex": [] }, Signed-off-by: Dan Mihai --- src/tools/genpolicy/genpolicy-settings.json | 2 +- src/tools/genpolicy/rules.rego | 7 +--- src/tools/genpolicy/src/policy.rs | 6 ++- src/tools/genpolicy/src/settings.rs | 10 +++++ tests/integration/kubernetes/tests_common.sh | 43 ++++++++------------ 5 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/tools/genpolicy/genpolicy-settings.json b/src/tools/genpolicy/genpolicy-settings.json index 8a612a594..309e6a076 100644 --- a/src/tools/genpolicy/genpolicy-settings.json +++ b/src/tools/genpolicy/genpolicy-settings.json @@ -299,7 +299,7 @@ "^$(cpath)/" ], "ExecProcessRequest": { - "commands": [], + "allowed_commands": [], "regex": [] }, "CloseStdinRequest": false, diff --git a/src/tools/genpolicy/rules.rego b/src/tools/genpolicy/rules.rego index 6d60c7773..8704695cd 100644 --- a/src/tools/genpolicy/rules.rego +++ b/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") } diff --git a/src/tools/genpolicy/src/policy.rs b/src/tools/genpolicy/src/policy.rs index 448fc7e31..9c5995f60 100644 --- a/src/tools/genpolicy/src/policy.rs +++ b/src/tools/genpolicy/src/policy.rs @@ -313,8 +313,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>, + /// Allow these commands to be executed. - commands: Vec, + pub allowed_commands: Vec>, /// Allow commands matching these regexes to be executed. regex: Vec, diff --git a/src/tools/genpolicy/src/settings.rs b/src/tools/genpolicy/src/settings.rs index 71b9615fe..80d1ac40f 100644 --- a/src/tools/genpolicy/src/settings.rs +++ b/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 has been deprecated. \ + Please use instead."); + } + } + } } diff --git a/tests/integration/kubernetes/tests_common.sh b/tests/integration/kubernetes/tests_common.sh index ca48de34f..6ed0e42ac 100644 --- a/tests/integration/kubernetes/tests_common.sh +++ b/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