refactor pvf security module (#3047)

resolve #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 <[email protected]>
paritytech · Feb 11, 2024 · 4883e14 · 4883e14
1 parent edd95b3
commit 4883e14
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 112 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -169,37 +169,23 @@ 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();
 
 	if all_errs_allowed {
 		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,122 +196,82 @@ 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.
 ///
 /// 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.
 ///
 /// 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.
 ///
 /// 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,