From 4883e1448210d0efd9a397b41cc184772401ec35 Mon Sep 17 00:00:00 2001
From: maksimryndin <maksim.ryndin@gmail.com>
Date: Sun, 11 Feb 2024 10:59:10 +0100
Subject: [PATCH] refactor pvf security module (#3047)
resolve https://github.com/paritytech/polkadot-sdk/issues/2321
- [x] refactor `security` module into a conditionally compiled
- [x] rename `amd64` into x86-64 for consistency with conditional
compilation guards and remove reference to a particular vendor
- [x] run unit tests and zombienet
---------
Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com>
---
polkadot/node/core/pvf/README.md | 6 +-
polkadot/node/core/pvf/common/Cargo.toml | 2 +
polkadot/node/core/pvf/common/src/lib.rs | 30 +++++
polkadot/node/core/pvf/src/host.rs | 26 +++-
polkadot/node/core/pvf/src/lib.rs | 20 +++
polkadot/node/core/pvf/src/security.rs | 160 ++++++++---------------
6 files changed, 132 insertions(+), 112 deletions(-)
diff --git a/polkadot/node/core/pvf/README.md b/polkadot/node/core/pvf/README.md
index 796e17c05fa..5304b0720b2 100644
--- a/polkadot/node/core/pvf/README.md
+++ b/polkadot/node/core/pvf/README.md
@@ -13,7 +13,7 @@ See also:
Running `cargo test` in the `pvf/` directory will run unit and integration
tests.
-**Note:** some tests run only under Linux, amd64, and/or with the
+**Note:** some tests run only under Linux, x86-64, and/or with the
`ci-only-tests` feature enabled.
See the general [Testing][testing] instructions for more information on
@@ -34,8 +34,8 @@ RUST_LOG=parachain::pvf=trace zombienet --provider=native spawn zombienet_tests/
## Testing on Linux
Some of the PVF functionality, especially related to security, is Linux-only,
-and some is amd64-only. If you touch anything security-related, make sure to
-test on Linux amd64! If you're on a Mac, you can either run a VM or you can hire
+and some is x86-64-only. If you touch anything security-related, make sure to
+test on Linux x86-64! If you're on a Mac, you can either run a VM or you can hire
a VPS and use the open-source tool [EternalTerminal][et] to connect to it.[^et]
[^et]: Unlike ssh, ET preserves your session across disconnects, and unlike
diff --git a/polkadot/node/core/pvf/common/Cargo.toml b/polkadot/node/core/pvf/common/Cargo.toml
index 583db0b29f4..631d4335b52 100644
--- a/polkadot/node/core/pvf/common/Cargo.toml
+++ b/polkadot/node/core/pvf/common/Cargo.toml
@@ -35,6 +35,8 @@ sp-tracing = { path = "../../../../../substrate/primitives/tracing" }
[target.'cfg(target_os = "linux")'.dependencies]
landlock = "0.3.0"
nix = { version = "0.27.1", features = ["sched"] }
+
+[target.'cfg(all(target_os = "linux", target_arch = "x86_64"))'.dependencies]
seccompiler = "0.4.0"
[dev-dependencies]
diff --git a/polkadot/node/core/pvf/common/src/lib.rs b/polkadot/node/core/pvf/common/src/lib.rs
index d891c06bf2a..15097dbd3af 100644
--- a/polkadot/node/core/pvf/common/src/lib.rs
+++ b/polkadot/node/core/pvf/common/src/lib.rs
@@ -86,3 +86,33 @@ pub fn framed_recv_blocking(r: &mut (impl Read + Unpin)) -> io::Result<Vec<u8>>
r.read_exact(&mut buf)?;
Ok(buf)
}
+
+#[cfg(all(test, not(feature = "test-utils")))]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn default_secure_status() {
+ let status = SecurityStatus::default();
+ assert!(
+ !status.secure_validator_mode,
+ "secure_validator_mode is false for default security status"
+ );
+ assert!(
+ !status.can_enable_landlock,
+ "can_enable_landlock is false for default security status"
+ );
+ assert!(
+ !status.can_enable_seccomp,
+ "can_enable_seccomp is false for default security status"
+ );
+ assert!(
+ !status.can_unshare_user_namespace_and_change_root,
+ "can_unshare_user_namespace_and_change_root is false for default security status"
+ );
+ assert!(
+ !status.can_do_secure_clone,
+ "can_do_secure_clone is false for default security status"
+ );
+ }
+}
diff --git a/polkadot/node/core/pvf/src/host.rs b/polkadot/node/core/pvf/src/host.rs
index e215f11b91d..ae9fdc7d2de 100644
--- a/polkadot/node/core/pvf/src/host.rs
+++ b/polkadot/node/core/pvf/src/host.rs
@@ -24,7 +24,7 @@ use crate::{
artifacts::{ArtifactId, ArtifactPathId, ArtifactState, Artifacts},
execute::{self, PendingExecutionRequest},
metrics::Metrics,
- prepare, security, Priority, SecurityStatus, ValidationError, LOG_TARGET,
+ prepare, Priority, SecurityStatus, ValidationError, LOG_TARGET,
};
use always_assert::never;
use futures::{
@@ -225,10 +225,32 @@ pub async fn start(
// Run checks for supported security features once per host startup. If some checks fail, warn
// if Secure Validator Mode is disabled and return an error otherwise.
- let security_status = match security::check_security_status(&config).await {
+ #[cfg(target_os = "linux")]
+ let security_status = match crate::security::check_security_status(&config).await {
Ok(ok) => ok,
Err(err) => return Err(SubsystemError::Context(err)),
};
+ #[cfg(not(target_os = "linux"))]
+ let security_status = if config.secure_validator_mode {
+ gum::error!(
+ target: LOG_TARGET,
+ "{}{}{}",
+ crate::SECURE_MODE_ERROR,
+ crate::SECURE_LINUX_NOTE,
+ crate::IGNORE_SECURE_MODE_TIP
+ );
+ return Err(SubsystemError::Context(
+ "could not enable Secure Validator Mode for non-Linux; check logs".into(),
+ ));
+ } else {
+ gum::warn!(
+ target: LOG_TARGET,
+ "{}{}",
+ crate::SECURE_MODE_WARNING,
+ crate::SECURE_LINUX_NOTE,
+ );
+ SecurityStatus::default()
+ };
let (to_host_tx, to_host_rx) = mpsc::channel(HOST_MESSAGE_QUEUE_SIZE);
diff --git a/polkadot/node/core/pvf/src/lib.rs b/polkadot/node/core/pvf/src/lib.rs
index 92263281eea..462498fa8f6 100644
--- a/polkadot/node/core/pvf/src/lib.rs
+++ b/polkadot/node/core/pvf/src/lib.rs
@@ -97,6 +97,7 @@ mod host;
mod metrics;
mod prepare;
mod priority;
+#[cfg(target_os = "linux")]
mod security;
mod worker_interface;
@@ -135,3 +136,22 @@ pub fn get_worker_version(worker_path: &Path) -> std::io::Result<String> {
.trim()
.to_string())
}
+
+// Trying to run securely and some mandatory errors occurred.
+pub(crate) const SECURE_MODE_ERROR: &'static str =
+ "🚨 Your system cannot securely run a validator. \
+\nRunning validation of malicious PVF code has a higher risk of compromising this machine.";
+// Some errors occurred when running insecurely, or some optional errors occurred when running
+// securely.
+pub(crate) const SECURE_MODE_WARNING: &'static str = "🚨 Some security issues have been detected. \
+\nRunning validation of malicious PVF code has a higher risk of compromising this machine.";
+// Message to be printed only when running securely and mandatory errors occurred.
+pub(crate) const IGNORE_SECURE_MODE_TIP: &'static str =
+"\nYou can ignore this error with the `--insecure-validator-i-know-what-i-do` \
+command line argument if you understand and accept the risks of running insecurely. \
+With this flag, security features are enabled on a best-effort basis, but not mandatory. \
+\nMore information: https://wiki.polkadot.network/docs/maintain-guides-secure-validator#secure-validator-mode";
+// Only Linux supports security features
+#[cfg(not(target_os = "linux"))]
+pub(crate) const SECURE_LINUX_NOTE: &'static str = "\nSecure mode is enabled only for Linux \
+\nand a full secure mode is enabled only for Linux x86-64.";
diff --git a/polkadot/node/core/pvf/src/security.rs b/polkadot/node/core/pvf/src/security.rs
index f62a232abf2..733ef18bcad 100644
--- a/polkadot/node/core/pvf/src/security.rs
+++ b/polkadot/node/core/pvf/src/security.rs
@@ -169,20 +169,6 @@ impl fmt::Display for SecureModeError {
/// Print an error if Secure Validator Mode and some mandatory errors occurred, warn otherwise.
fn print_secure_mode_error_or_warning(security_status: &FullSecurityStatus) {
- // Trying to run securely and some mandatory errors occurred.
- const SECURE_MODE_ERROR: &'static str = "🚨 Your system cannot securely run a validator. \
- \nRunning validation of malicious PVF code has a higher risk of compromising this machine.";
- // Some errors occurred when running insecurely, or some optional errors occurred when running
- // securely.
- const SECURE_MODE_WARNING: &'static str = "🚨 Some security issues have been detected. \
- \nRunning validation of malicious PVF code has a higher risk of compromising this machine.";
- // Message to be printed only when running securely and mandatory errors occurred.
- const IGNORE_SECURE_MODE_TIP: &'static str =
- "\nYou can ignore this error with the `--insecure-validator-i-know-what-i-do` \
- command line argument if you understand and accept the risks of running insecurely. \
- With this flag, security features are enabled on a best-effort basis, but not mandatory. \
- \nMore information: https://wiki.polkadot.network/docs/maintain-guides-secure-validator#secure-validator-mode";
-
let all_errs_allowed = security_status.all_errs_allowed();
let errs_string = security_status.errs_string();
@@ -190,16 +176,16 @@ fn print_secure_mode_error_or_warning(security_status: &FullSecurityStatus) {
gum::warn!(
target: LOG_TARGET,
"{}{}",
- SECURE_MODE_WARNING,
+ crate::SECURE_MODE_WARNING,
errs_string,
);
} else {
gum::error!(
target: LOG_TARGET,
"{}{}{}",
- SECURE_MODE_ERROR,
+ crate::SECURE_MODE_ERROR,
errs_string,
- IGNORE_SECURE_MODE_TIP
+ crate::IGNORE_SECURE_MODE_TIP
);
}
}
@@ -210,29 +196,25 @@ fn print_secure_mode_error_or_warning(security_status: &FullSecurityStatus) {
/// to running the check in a worker, we try it... in a worker. The expected return status is 0 on
/// success and -1 on failure.
async fn check_can_unshare_user_namespace_and_change_root(
- #[cfg_attr(not(target_os = "linux"), allow(unused_variables))]
prepare_worker_program_path: &Path,
- #[cfg_attr(not(target_os = "linux"), allow(unused_variables))] cache_path: &Path,
+ cache_path: &Path,
) -> SecureModeResult {
- cfg_if::cfg_if! {
- if #[cfg(target_os = "linux")] {
- let cache_dir_tempdir = tempfile::Builder::new()
- .prefix("check-can-unshare-")
- .tempdir_in(cache_path)
- .map_err(|err| SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(
- format!("could not create a temporary directory in {:?}: {}", cache_path, err)
- ))?;
- spawn_process_for_security_check(
- prepare_worker_program_path,
- "--check-can-unshare-user-namespace-and-change-root",
- &[cache_dir_tempdir.path()],
- ).await.map_err(|err| SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(err))
- } else {
- Err(SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(
- "only available on Linux".into()
+ let cache_dir_tempdir = tempfile::Builder::new()
+ .prefix("check-can-unshare-")
+ .tempdir_in(cache_path)
+ .map_err(|err| {
+ SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(format!(
+ "could not create a temporary directory in {:?}: {}",
+ cache_path, err
))
- }
- }
+ })?;
+ spawn_process_for_security_check(
+ prepare_worker_program_path,
+ "--check-can-unshare-user-namespace-and-change-root",
+ &[cache_dir_tempdir.path()],
+ )
+ .await
+ .map_err(|err| SecureModeError::CannotUnshareUserNamespaceAndChangeRoot(err))
}
/// Check if landlock is supported and return an error if not.
@@ -240,25 +222,15 @@ async fn check_can_unshare_user_namespace_and_change_root(
/// We do this check by spawning a new process and trying to sandbox it. To get as close as possible
/// to running the check in a worker, we try it... in a worker. The expected return status is 0 on
/// success and -1 on failure.
-async fn check_landlock(
- #[cfg_attr(not(target_os = "linux"), allow(unused_variables))]
- prepare_worker_program_path: &Path,
-) -> SecureModeResult {
- cfg_if::cfg_if! {
- if #[cfg(target_os = "linux")] {
- let abi = polkadot_node_core_pvf_common::worker::security::landlock::LANDLOCK_ABI as u8;
- spawn_process_for_security_check(
- prepare_worker_program_path,
- "--check-can-enable-landlock",
- std::iter::empty::<&str>(),
- ).await.map_err(|err| SecureModeError::CannotEnableLandlock { err, abi })
- } else {
- Err(SecureModeError::CannotEnableLandlock {
- err: "only available on Linux".into(),
- abi: 0,
- })
- }
- }
+async fn check_landlock(prepare_worker_program_path: &Path) -> SecureModeResult {
+ let abi = polkadot_node_core_pvf_common::worker::security::landlock::LANDLOCK_ABI as u8;
+ spawn_process_for_security_check(
+ prepare_worker_program_path,
+ "--check-can-enable-landlock",
+ std::iter::empty::<&str>(),
+ )
+ .await
+ .map_err(|err| SecureModeError::CannotEnableLandlock { err, abi })
}
/// Check if seccomp is supported and return an error if not.
@@ -266,39 +238,23 @@ async fn check_landlock(
/// We do this check by spawning a new process and trying to sandbox it. To get as close as possible
/// to running the check in a worker, we try it... in a worker. The expected return status is 0 on
/// success and -1 on failure.
-async fn check_seccomp(
- #[cfg_attr(not(all(target_os = "linux", target_arch = "x86_64")), allow(unused_variables))]
- prepare_worker_program_path: &Path,
-) -> SecureModeResult {
- cfg_if::cfg_if! {
- if #[cfg(target_os = "linux")] {
- cfg_if::cfg_if! {
- if #[cfg(target_arch = "x86_64")] {
- spawn_process_for_security_check(
- prepare_worker_program_path,
- "--check-can-enable-seccomp",
- std::iter::empty::<&str>(),
- ).await.map_err(|err| SecureModeError::CannotEnableSeccomp(err))
- } else {
- Err(SecureModeError::CannotEnableSeccomp(
- "only supported on CPUs from the x86_64 family (usually Intel or AMD)".into()
- ))
- }
- }
- } else {
- cfg_if::cfg_if! {
- if #[cfg(target_arch = "x86_64")] {
- Err(SecureModeError::CannotEnableSeccomp(
- "only supported on Linux".into()
- ))
- } else {
- Err(SecureModeError::CannotEnableSeccomp(
- "only supported on Linux and on CPUs from the x86_64 family (usually Intel or AMD).".into()
- ))
- }
- }
- }
- }
+
+#[cfg(target_arch = "x86_64")]
+async fn check_seccomp(prepare_worker_program_path: &Path) -> SecureModeResult {
+ spawn_process_for_security_check(
+ prepare_worker_program_path,
+ "--check-can-enable-seccomp",
+ std::iter::empty::<&str>(),
+ )
+ .await
+ .map_err(|err| SecureModeError::CannotEnableSeccomp(err))
+}
+
+#[cfg(not(target_arch = "x86_64"))]
+async fn check_seccomp(_: &Path) -> SecureModeResult {
+ Err(SecureModeError::CannotEnableSeccomp(
+ "only supported on CPUs from the x86_64 family (usually Intel or AMD)".into(),
+ ))
}
/// Check if we can call `clone` with all sandboxing flags, and return an error if not.
@@ -306,26 +262,16 @@ async fn check_seccomp(
/// We do this check by spawning a new process and trying to sandbox it. To get as close as possible
/// to running the check in a worker, we try it... in a worker. The expected return status is 0 on
/// success and -1 on failure.
-async fn check_can_do_secure_clone(
- #[cfg_attr(not(target_os = "linux"), allow(unused_variables))]
- prepare_worker_program_path: &Path,
-) -> SecureModeResult {
- cfg_if::cfg_if! {
- if #[cfg(target_os = "linux")] {
- spawn_process_for_security_check(
- prepare_worker_program_path,
- "--check-can-do-secure-clone",
- std::iter::empty::<&str>(),
- ).await.map_err(|err| SecureModeError::CannotDoSecureClone(err))
- } else {
- Err(SecureModeError::CannotDoSecureClone(
- "only available on Linux".into()
- ))
- }
- }
+async fn check_can_do_secure_clone(prepare_worker_program_path: &Path) -> SecureModeResult {
+ spawn_process_for_security_check(
+ prepare_worker_program_path,
+ "--check-can-do-secure-clone",
+ std::iter::empty::<&str>(),
+ )
+ .await
+ .map_err(|err| SecureModeError::CannotDoSecureClone(err))
}
-#[cfg(target_os = "linux")]
async fn spawn_process_for_security_check<I, S>(
prepare_worker_program_path: &Path,
check_arg: &'static str,
--
GitLab