diff --git a/Cargo.lock b/Cargo.lock index 81b7f91b5b..e1b9a1f432 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6665,11 +6665,13 @@ dependencies = [ "protobuf", "protocols", "resource", + "rstest 0.18.2", "runtime-spec", "serde_json", "shim-interface", "slog", "slog-scope", + "tempfile", "tokio", "tracing", "tracing-opentelemetry 0.18.0", diff --git a/docs/how-to/containerd-kata.md b/docs/how-to/containerd-kata.md index b0b06f024f..c567ed0a99 100644 --- a/docs/how-to/containerd-kata.md +++ b/docs/how-to/containerd-kata.md @@ -235,7 +235,7 @@ This `ConfigPath` option is optional. If you want to use a different configurati > **Note:** In this example, the specified `ConfigPath` is valid in Kubernetes/Containerd workflow with containerd v1.7+ but doesn't work with ctr and nerdctl. -If you do not specify it, `shimv2` first tries to get the configuration file from the environment variable `KATA_CONF_FILE`. If you want to adopt this way, you should first create a shell script as `containerd-shim-kata-v2` which is placed under the path of `/usr/local/bin/`. The following is an example of the shell script `containerd-shim-kata-qemu-v2` which specifies the configuration file with `KATA_CONF_FILE` +If you do not specify it, `shimv2` first tries to get the configuration file from the environment variable `KATA_CONF_FILE`. For security, this value is accepted only when it resolves to one of Kata's shipped default configuration files. If you want to adopt this way, you should first create a shell script as `containerd-shim-kata-v2` which is placed under the path of `/usr/local/bin/`. The following is an example of the shell script `containerd-shim-kata-qemu-v2` which specifies the configuration file with `KATA_CONF_FILE` > **Note:** Just use containerd 2.x configuration as an example, the configuration for containerd 1.7.x is similar, just replace `io.containerd.cri.v1.runtime` with `io.containerd.grpc.v1.cri` diff --git a/docs/how-to/how-to-use-kata-containers-with-firecracker.md b/docs/how-to/how-to-use-kata-containers-with-firecracker.md index ee10f728ee..ca0aab5dc9 100644 --- a/docs/how-to/how-to-use-kata-containers-with-firecracker.md +++ b/docs/how-to/how-to-use-kata-containers-with-firecracker.md @@ -224,7 +224,7 @@ Next, we need to configure containerd. Add a file in your path (e.g. `/usr/local #!/bin/bash KATA_CONF_FILE=/etc/kata-containers/configuration-fc.toml /usr/local/bin/containerd-shim-kata-v2 $@ ``` -> **Note:** You may need to edit the paths of the configuration file and the `containerd-shim-kata-v2` to correspond to your setup. +> **Note:** You may need to edit the paths of the configuration file and the `containerd-shim-kata-v2` to correspond to your setup. `KATA_CONF_FILE` is only accepted when it resolves to one of Kata's shipped default configuration files. Make it executable: diff --git a/src/libs/kata-types/src/annotations/mod.rs b/src/libs/kata-types/src/annotations/mod.rs index ad91964dae..5bac8ea476 100644 --- a/src/libs/kata-types/src/annotations/mod.rs +++ b/src/libs/kata-types/src/annotations/mod.rs @@ -38,8 +38,6 @@ pub const KATA_ANNO_PREFIX: &str = "io.katacontainers."; pub const KATA_ANNO_CFG_PREFIX: &str = "io.katacontainers.config."; /// Prefix for Kata container annotations pub const KATA_ANNO_CONTAINER_PREFIX: &str = "io.katacontainers.container."; -/// The annotation key to fetch runtime configuration file. - // OCI section /// The annotation key to fetch the OCI configuration file path. pub const BUNDLE_PATH_KEY: &str = "io.katacontainers.pkg.oci.bundle_path"; diff --git a/src/runtime-rs/crates/runtimes/Cargo.toml b/src/runtime-rs/crates/runtimes/Cargo.toml index d3526e8822..bfad277853 100644 --- a/src/runtime-rs/crates/runtimes/Cargo.toml +++ b/src/runtime-rs/crates/runtimes/Cargo.toml @@ -5,6 +5,10 @@ authors = { workspace = true } edition = { workspace = true } license = { workspace = true } +[dev-dependencies] +rstest = { workspace = true } +tempfile = { workspace = true } + [dependencies] anyhow = { workspace = true } lazy_static = { workspace = true } diff --git a/src/runtime-rs/crates/runtimes/src/manager.rs b/src/runtime-rs/crates/runtimes/src/manager.rs index cf1e860b9f..7ec4ab7ecf 100644 --- a/src/runtime-rs/crates/runtimes/src/manager.rs +++ b/src/runtime-rs/crates/runtimes/src/manager.rs @@ -817,9 +817,18 @@ impl Env for RootlessEnv { fn load_config(an: &HashMap, option: &Option>) -> Result { const KATA_CONF_FILE: &str = "KATA_CONF_FILE"; let annotation = Annotation::new(an.clone()); + // Clone a logger from global logger to ensure the logs in this function get flushed when drop + let logger = slog::Logger::clone(&slog_scope::logger()); let config_path = if let Ok(path) = std::env::var(KATA_CONF_FILE) { - path + if is_shipped_kata_config_path(&path) { + path + } else { + return Err(anyhow!( + "invalid KATA_CONF_FILE {:?}: only shipped Kata configuration files are accepted", + path + )); + } } else if let Some(option) = option { // Parse the containerd runtime options protobuf message to extract the config path. // The options are passed as a serialized runtimeoptions.v1.Options protobuf message @@ -841,9 +850,6 @@ fn load_config(an: &HashMap, option: &Option>) -> Result String::from("") }; - // Clone a logger from global logger to ensure the logs in this function get flushed when drop - let logger = slog::Logger::clone(&slog_scope::logger()); - info!(logger, "get config path {:?}", &config_path); let (mut toml_config, _) = TomlConfig::load_from_file(&config_path).context(format!( "load TOML config failed (tried {:?})", @@ -859,6 +865,21 @@ fn load_config(an: &HashMap, option: &Option>) -> Result Ok(toml_config) } +fn is_shipped_kata_config_path(config_path: &str) -> bool { + config_path_matches_defaults(config_path, TomlConfig::get_default_config_file_list()) +} + +fn config_path_matches_defaults(config_path: &str, default_config_paths: Vec) -> bool { + let Ok(resolved_config_path) = std::fs::canonicalize(config_path) else { + return false; + }; + + default_config_paths + .into_iter() + .filter_map(|path| std::fs::canonicalize(path).ok()) + .any(|path| path == resolved_config_path) +} + // this update the agent-specfic kernel parameters into hypervisor's bootinfo // the agent inside the VM will read from file cmdline to get the params and function fn update_agent_kernel_params(config: &mut TomlConfig) -> Result<()> { @@ -1001,6 +1022,7 @@ fn configure_non_root_hypervisor(config: &mut Hypervisor) -> Result<()> { mod tests { use super::*; use common::types::ShutdownRequest; + use rstest::rstest; use tokio::sync::mpsc::channel; // A ShutdownContainer RPC that arrives before any runtime instance was @@ -1036,4 +1058,50 @@ mod tests { assert_eq!(effective_log_level(true, "trace"), "trace"); assert_eq!(effective_log_level(true, "warn"), "warn"); } + + #[derive(Debug)] + enum ConfigPathCase { + Shipped, + NonShipped, + NonExistent, + Empty, + } + + #[rstest] + #[case::shipped_config_is_accepted(ConfigPathCase::Shipped, true)] + #[case::non_shipped_config_is_rejected(ConfigPathCase::NonShipped, false)] + #[case::non_existent_path_is_rejected(ConfigPathCase::NonExistent, false)] + #[case::empty_path_is_rejected(ConfigPathCase::Empty, false)] + fn test_config_path_matches_defaults( + #[case] path_case: ConfigPathCase, + #[case] expected: bool, + ) { + let tmpdir = tempfile::tempdir().unwrap(); + let shipped_path = tmpdir.path().join("shipped.toml"); + let non_shipped_path = tmpdir.path().join("malicious.toml"); + std::fs::write(&shipped_path, b"[hypervisor.qemu]\n").unwrap(); + std::fs::write(&non_shipped_path, b"[hypervisor.qemu]\n").unwrap(); + + // Only the shipped path is treated as a default config location. + let default_config_paths = vec![shipped_path.clone()]; + + let config_path = match path_case { + ConfigPathCase::Shipped => shipped_path.to_string_lossy().to_string(), + ConfigPathCase::NonShipped => non_shipped_path.to_string_lossy().to_string(), + ConfigPathCase::NonExistent => tmpdir + .path() + .join("nonexistent.toml") + .to_string_lossy() + .to_string(), + ConfigPathCase::Empty => String::new(), + }; + + assert_eq!( + config_path_matches_defaults(&config_path, default_config_paths), + expected, + "case {:?}: unexpected result for path {:?}", + path_case, + config_path, + ); + } } diff --git a/src/runtime/README.md b/src/runtime/README.md index 2e36133e61..cd6eba5f75 100644 --- a/src/runtime/README.md +++ b/src/runtime/README.md @@ -65,7 +65,8 @@ The shimv2 runtime looks for its configuration in the following places (in order [shimv2](/docs/design/architecture/README.md#shim-v2-architecture) options passed to the runtime. -- The value of the `KATA_CONF_FILE` environment variable. +- The value of the `KATA_CONF_FILE` environment variable, if it resolves to one + of the shipped default configuration files. - The [default configuration paths](#stateless-systems). diff --git a/src/runtime/pkg/containerd-shim-v2/create.go b/src/runtime/pkg/containerd-shim-v2/create.go index 17dad46c54..104dd6a009 100644 --- a/src/runtime/pkg/containerd-shim-v2/create.go +++ b/src/runtime/pkg/containerd-shim-v2/create.go @@ -290,7 +290,14 @@ func loadRuntimeConfig(s *service, r *taskAPI.CreateTaskRequest) (*oci.RuntimeCo // Try to get the config file from the env KATA_CONF_FILE if configPath == "" { - configPath = os.Getenv("KATA_CONF_FILE") + envConfigPath := os.Getenv("KATA_CONF_FILE") + if envConfigPath != "" { + if isShippedKataConfigPath(envConfigPath) { + configPath = envConfigPath + } else { + return nil, fmt.Errorf("invalid KATA_CONF_FILE %q: only shipped Kata configuration files are accepted", envConfigPath) + } + } } _, runtimeConfig, err := katautils.LoadConfiguration(configPath, false) @@ -306,6 +313,26 @@ func loadRuntimeConfig(s *service, r *taskAPI.CreateTaskRequest) (*oci.RuntimeCo return &runtimeConfig, nil } +func isShippedKataConfigPath(configPath string) bool { + resolvedConfigPath, err := katautils.ResolvePath(configPath) + if err != nil { + return false + } + + for _, defaultConfigPath := range katautils.GetDefaultConfigFilePaths() { + resolvedDefaultPath, err := katautils.ResolvePath(defaultConfigPath) + if err != nil { + continue + } + + if resolvedConfigPath == resolvedDefaultPath { + return true + } + } + + return false +} + func checkAndMount(s *service, r *taskAPI.CreateTaskRequest) (bool, error) { if len(r.Rootfs) == 1 { m := r.Rootfs[0] diff --git a/src/runtime/pkg/containerd-shim-v2/create_test.go b/src/runtime/pkg/containerd-shim-v2/create_test.go index 5da89c9aef..1af6a46c95 100644 --- a/src/runtime/pkg/containerd-shim-v2/create_test.go +++ b/src/runtime/pkg/containerd-shim-v2/create_test.go @@ -19,9 +19,11 @@ import ( crioption "github.com/containerd/cri-containerd/pkg/api/runtimeoptions/v1" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/kata-containers/kata-containers/src/runtime/pkg/device/config" ktu "github.com/kata-containers/kata-containers/src/runtime/pkg/katatestutils" + "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils" vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/compatoci" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/vcmock" @@ -406,6 +408,18 @@ func TestCreateLoadRuntimeConfig(t *testing.T) { _, err = loadRuntimeConfig(s, r) assert.Error(err) + // existing but non-shipped config should be rejected + maliciousConfig := path.Join(tmpdir, "malicious.toml") + err = os.WriteFile(maliciousConfig, []byte("[hypervisor.qemu]\n"), os.FileMode(0640)) + assert.NoError(err) + err = os.Setenv("KATA_CONF_FILE", maliciousConfig) + assert.NoError(err) + option.ConfigPath = "" + r.Options, err = protobuf.MarshalAnyToProto(option) + assert.NoError(err) + _, err = loadRuntimeConfig(s, r) + assert.Error(err) + // 1. shimv2 create task option option.ConfigPath = config r.Options, err = protobuf.MarshalAnyToProto(option) @@ -422,3 +436,61 @@ func TestCreateLoadRuntimeConfig(t *testing.T) { _, err = loadRuntimeConfig(s, r) assert.NoError(err) } + +func TestIsShippedKataConfigPath(t *testing.T) { + tmpdir := t.TempDir() + + // Setup: Create test config files + shippedConfigPath := path.Join(tmpdir, "shipped.toml") + maliciousConfigPath := path.Join(tmpdir, "malicious.toml") + nonExistentPath := path.Join(tmpdir, "nonexistent.toml") + + require.NoError(t, os.WriteFile(shippedConfigPath, []byte("[hypervisor.qemu]\n"), 0640)) + require.NoError(t, os.WriteFile(maliciousConfigPath, []byte("[hypervisor.qemu]\n"), 0640)) + + // Configure shipped path + defaultConfigPaths := katautils.GetDefaultConfigFilePaths() + defer katautils.SetConfigOptions("", defaultConfigPaths[1], defaultConfigPaths[0]) + katautils.SetConfigOptions("", shippedConfigPath, "") + + tests := []struct { + name string + path string + expected bool + reason string + }{ + { + name: "shipped config is accepted", + path: shippedConfigPath, + expected: true, + reason: "path matches configured default", + }, + { + name: "malicious config is rejected", + path: maliciousConfigPath, + expected: false, + reason: "path does not match any default", + }, + { + name: "non-existent path is rejected", + path: nonExistentPath, + expected: false, + reason: "path cannot be resolved", + }, + { + name: "empty path is rejected", + path: "", + expected: false, + reason: "empty path is invalid", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isShippedKataConfigPath(tt.path) + assert.Equal(t, tt.expected, result, + "isShippedKataConfigPath(%q) = %v, want %v: %s", + tt.path, result, tt.expected, tt.reason) + }) + } +}