diff --git a/FORK.md b/FORK.md index d0f1db37..077e1582 100644 --- a/FORK.md +++ b/FORK.md @@ -192,8 +192,8 @@ Signing is unaffected either way. - `cargo fmt --all -- --check` — clean. - `cargo clippy --all-features --all-targets` — clean. -- `cargo test --all-features -- --test-threads 1` — 68 unit + 24 integration tests - pass (10 ignored, all hardware-dependent). +- `cargo test --all-features -- --test-threads 1` — 68 unit + 28 integration tests + pass (10 ignored, all hardware-dependent), printing nothing but their results. - The published image is pulled and checked on every release: it reports its own version, keeps `tmkms` as the entrypoint, and links the USB libraries the `yubihsm`/`ledger` backends need. diff --git a/tests/integration.rs b/tests/integration.rs index 2b098f6c..0f7d5b9f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -5,11 +5,17 @@ use cometbft_p2p::{self as p2p, IdentitySecret, PublicKey, ReadMsg, SecretConnec use rand::Rng; use signature::Verifier; use std::{ - fs, + fs::{self, File}, io::{self, Cursor, Write}, net::{TcpListener, TcpStream}, - os::unix::net::{UnixListener, UnixStream}, - process::{Child, Command}, + os::unix::{ + fs::PermissionsExt, + net::{UnixListener, UnixStream}, + }, + path::Path, + process::{Child, Command, ExitStatus}, + thread, + time::{Duration, Instant}, }; use tempfile::{NamedTempFile, TempDir}; use tmkms::{ @@ -31,6 +37,16 @@ const KMS_EXE_PATH: &str = "target/debug/tmkms"; const SIGNING_ED25519_KEY_PATH: &str = "tests/support/signing_ed25519.key"; const SIGNING_SECP256K1_KEY_PATH: &str = "tests/support/signing_secp256k1.key"; +/// Path to the KMS's identity key for the secret connection +const SECRET_CONNECTION_KEY_PATH: &str = "tests/support/secret_connection.key"; + +/// Name of the KMS's log inside the KMS's directory +const KMS_LOG_FILE: &str = "kms.log"; + +/// How long a KMS may take to connect, answer or exit before its test fails instead +/// of hanging +const KMS_TIMEOUT: Duration = Duration::from_secs(10); + enum KmsSocket { /// TCP socket type Tcp(TcpStream), @@ -75,9 +91,9 @@ struct KmsProcess { /// A socket to KMS process socket: KmsSocket, - /// Directory holding this process's double-sign protection state file, - /// kept alive (and cleaned up) for the lifetime of the process - _state_dir: TempDir, + /// Directory holding this process's keys, double-sign protection state file and + /// log, kept alive (and cleaned up) for the lifetime of the process + kms_dir: TempDir, } impl KmsProcess { @@ -85,54 +101,111 @@ impl KmsProcess { pub fn create_tcp(key_type: &KeyType) -> Self { // Generate a random port and a config file let port: u16 = rand::thread_rng().gen_range(60000..=65535); - let state_dir = tempfile::tempdir().unwrap(); - let config = KmsProcess::create_tcp_config(port, key_type, &state_dir); + let kms_dir = tempfile::tempdir().unwrap(); + let config = KmsProcess::create_tcp_config(port, key_type, &kms_dir); // Listen on a random port let listener = TcpListener::bind(format!("{}:{}", "127.0.0.1", port)).unwrap(); + listener.set_nonblocking(true).unwrap(); - let args = &["start", "-c", config.path().to_str().unwrap()]; - let process = Command::new(KMS_EXE_PATH).args(args).spawn().unwrap(); + let mut process = KmsProcess::spawn(&config, &kms_dir); - let (socket, _) = listener.accept().unwrap(); + let socket = wait_for_connection(&mut process, &kms_dir, || { + listener.accept().map(|(socket, _)| socket) + }); + socket.set_nonblocking(false).unwrap(); + socket.set_read_timeout(Some(KMS_TIMEOUT)).unwrap(); Self { process, socket: KmsSocket::Tcp(socket), - _state_dir: state_dir, + kms_dir, } } /// Spawn the KMS process and connect to the Unix listener pub fn create_unix(key_type: &KeyType) -> Self { + Self::create_unix_with(key_type, |_| ()) + } + + /// Like `create_unix`, with `prepare` changing the KMS's directory before the KMS starts + fn create_unix_with(key_type: &KeyType, prepare: impl FnOnce(&Path)) -> Self { // Create a random socket path and a config file let mut rng = rand::thread_rng(); let letter: char = rng.gen_range(b'a'..=b'z') as char; let number: u32 = rng.gen_range(0..=999999); let socket_path = format!("/tmp/tmkms-{letter}{number:06}.sock"); - let state_dir = tempfile::tempdir().unwrap(); - let config = KmsProcess::create_unix_config(&socket_path, key_type, &state_dir); + let kms_dir = tempfile::tempdir().unwrap(); + let config = KmsProcess::create_unix_config(&socket_path, key_type, &kms_dir); + prepare(kms_dir.path()); // Start listening for connections via the Unix socket let listener = UnixListener::bind(socket_path).unwrap(); + listener.set_nonblocking(true).unwrap(); // Fire up the KMS process and allow it to connect to our Unix socket - let args = &["start", "-c", config.path().to_str().unwrap()]; - let process = Command::new(KMS_EXE_PATH).args(args).spawn().unwrap(); + let mut process = KmsProcess::spawn(&config, &kms_dir); - let (socket, _) = listener.accept().unwrap(); + let socket = wait_for_connection(&mut process, &kms_dir, || { + listener.accept().map(|(socket, _)| socket) + }); + socket.set_nonblocking(false).unwrap(); + socket.set_read_timeout(Some(KMS_TIMEOUT)).unwrap(); Self { process, socket: KmsSocket::Unix(socket), - _state_dir: state_dir, + kms_dir, } } + /// Start a KMS on `config`, logging to a file in `kms_dir` for the test to check. + /// It logs only warnings and errors, so a healthy KMS logs nothing. + fn spawn(config: &NamedTempFile, kms_dir: &TempDir) -> Child { + let log = File::create(kms_dir.path().join(KMS_LOG_FILE)).unwrap(); + + Command::new(KMS_EXE_PATH) + .args(["start", "-c", config.path().to_str().unwrap()]) + .env("RUST_LOG", "warn") + .stdout(log.try_clone().unwrap()) + .stderr(log) + .spawn() + .unwrap() + } + + /// Everything the KMS has logged so far + fn log(&self) -> String { + kms_log(&self.kms_dir) + } + + /// Wait for the KMS to exit on its own, and return how it exited + fn wait_for_exit(&mut self) -> ExitStatus { + let deadline = Instant::now() + KMS_TIMEOUT; + loop { + if let Some(status) = self.process.try_wait().unwrap() { + return status; + } + assert!( + Instant::now() < deadline, + "KMS still running after {KMS_TIMEOUT:?}" + ); + thread::sleep(Duration::from_millis(10)); + } + } + + /// Stop the KMS and return everything it logged + fn stop(&mut self) -> String { + self.process.kill().unwrap(); + self.process.wait().unwrap(); + self.log() + } + /// Create a config file for a TCP KMS and return its path - fn create_tcp_config(port: u16, key_type: &KeyType, state_dir: &TempDir) -> NamedTempFile { + fn create_tcp_config(port: u16, key_type: &KeyType, kms_dir: &TempDir) -> NamedTempFile { let mut config_file = NamedTempFile::new().unwrap(); let pub_key = test_ed25519_signing_keypair().verifying_key(); let peer_id = PublicKey::from(pub_key).peer_id(); - let state_path = state_dir.path().join("state.json").display().to_string(); + let state_path = seeded_state_file(kms_dir); + let secret_key = owner_only_copy(SECRET_CONNECTION_KEY_PATH, kms_dir); + let signing_key = owner_only_copy(signing_key_path(key_type), kms_dir); writeln!( config_file, @@ -143,19 +216,18 @@ impl KmsProcess { state_file = "{state_path}" [[validator]] - addr = "tcp://{}@127.0.0.1:{}" + addr = "tcp://{peer_id}@127.0.0.1:{port}" chain_id = "test_chain_id" max_height = "500000" reconnect = false - secret_key = "tests/support/secret_connection.key" + secret_key = "{secret_key}" [[providers.softsign]] chain_ids = ["test_chain_id"] key_format = "base64" - path = "{}" - key_type = "{}" - "#, - &peer_id.to_string(), port, signing_key_path(key_type), key_type + path = "{signing_key}" + key_type = "{key_type}" + "# ).unwrap(); config_file @@ -165,11 +237,11 @@ impl KmsProcess { fn create_unix_config( socket_path: &str, key_type: &KeyType, - state_dir: &TempDir, + kms_dir: &TempDir, ) -> NamedTempFile { let mut config_file = NamedTempFile::new().unwrap(); - let key_path = signing_key_path(key_type); - let state_path = state_dir.path().join("state.json").display().to_string(); + let key_path = owner_only_copy(signing_key_path(key_type), kms_dir); + let state_path = seeded_state_file(kms_dir); writeln!( config_file, r#" @@ -182,6 +254,7 @@ impl KmsProcess { addr = "unix://{socket_path}" chain_id = "test_chain_id" max_height = "500000" + reconnect = false [[providers.softsign]] chain_ids = ["test_chain_id"] @@ -219,6 +292,56 @@ impl KmsProcess { } } +impl Drop for KmsProcess { + fn drop(&mut self) { + self.process.kill().unwrap(); + self.process.wait().unwrap(); + + // The test harness prints this with the failure, so a failing test shows what + // its KMS logged + if thread::panicking() { + eprintln!("--- {KMS_LOG_FILE} ---\n{}", self.log()); + } + } +} + +/// Everything the KMS keeping its files in `kms_dir` has logged so far +fn kms_log(kms_dir: &TempDir) -> String { + fs::read_to_string(kms_dir.path().join(KMS_LOG_FILE)).unwrap() +} + +/// Wait for the KMS to connect, polling `accept` on a non-blocking listener. A KMS that +/// exits first, or doesn't connect within `KMS_TIMEOUT`, fails the test with its log. +fn wait_for_connection( + process: &mut Child, + kms_dir: &TempDir, + mut accept: impl FnMut() -> io::Result, +) -> S { + let deadline = Instant::now() + KMS_TIMEOUT; + + loop { + match accept() { + Ok(socket) => return socket, + Err(e) if e.kind() == io::ErrorKind::WouldBlock => (), + Err(e) => panic!("couldn't accept the KMS's connection: {e}"), + } + + let failure = match process.try_wait().unwrap() { + Some(status) => format!("KMS exited ({status}) before connecting"), + None if Instant::now() < deadline => { + thread::sleep(Duration::from_millis(10)); + continue; + } + None => { + process.kill().unwrap(); + process.wait().unwrap(); + format!("KMS didn't connect within {KMS_TIMEOUT:?}") + } + }; + panic!("{failure}\n--- {KMS_LOG_FILE} ---\n{}", kms_log(kms_dir)); + } +} + /// A struct to hold protocol integration tests contexts struct ProtocolTester { tcp_device: KmsProcess, @@ -228,29 +351,38 @@ struct ProtocolTester { } impl ProtocolTester { - pub fn apply(key_type: &KeyType, functor: F) - where - F: FnOnce(ProtocolTester), - { + /// Start a TCP and a UNIX KMS and connect to both + pub fn start(key_type: &KeyType) -> Self { let tcp_device = KmsProcess::create_tcp(key_type); let tcp_connection = tcp_device.create_connection(); let unix_device = KmsProcess::create_unix(key_type); let unix_connection = unix_device.create_connection(); - functor(Self { + Self { tcp_device, tcp_connection, unix_device, unix_connection, - }); + } + } + + /// Run `test` against a TCP and a UNIX KMS, then check that neither logged anything: + /// passing tests must print nothing, so a healthy KMS must log nothing + pub fn apply(key_type: &KeyType, test: F) + where + F: FnOnce(&mut ProtocolTester), + { + let mut pt = Self::start(key_type); + test(&mut pt); + + for kms in [&mut pt.tcp_device, &mut pt.unix_device] { + assert_eq!(kms.stop(), "", "a healthy KMS logged something"); + } } } impl Drop for ProtocolTester { fn drop(&mut self) { - self.tcp_device.process.kill().unwrap(); - self.unix_device.process.kill().unwrap(); - match fs::remove_file("test_chain_id_priv_validator_state.json") { Err(ref e) if e.kind() != io::ErrorKind::NotFound => { panic!("{}", e); @@ -301,6 +433,128 @@ fn signing_key_path(key_type: &KeyType) -> &'static str { } } +/// Create the state file a fresh KMS would, with the KMS's own code, so that it loads +/// the file instead of warning that double-sign protection has no history +fn seeded_state_file(dir: &TempDir) -> String { + let path = dir.path().join("state.json"); + tmkms::chain::State::load_state(&path).unwrap(); + path.display().to_string() +} + +/// Copy a key fixture into `dir` as a file only its owner can read, the way a real +/// deployment keeps it, and return the copy's path +fn owner_only_copy(fixture: &str, dir: &TempDir) -> String { + let path = dir.path().join(Path::new(fixture).file_name().unwrap()); + fs::copy(fixture, &path).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o600)).unwrap(); + path.display().to_string() +} + +/// A KMS that dies before connecting fails its test with the KMS's own error, rather +/// than leaving the test waiting for a connection that never comes +#[test] +#[should_panic(expected = "couldn't read key from")] +fn a_kms_that_dies_before_connecting_fails_its_test_with_its_log() { + KmsProcess::create_unix_with(&KeyType::Consensus, |kms_dir| { + fs::remove_file(kms_dir.join("signing_ed25519.key")).unwrap(); + }); +} + +/// The harness keeps two warnings out of every other test, so this one checks that a +/// KMS still gives them: about a key file others can read, and about a state file it +/// had to create +#[test] +fn kms_warns_about_a_readable_key_and_a_missing_state_file() { + let mut kms = KmsProcess::create_unix_with(&KeyType::Consensus, |kms_dir| { + let key = kms_dir.join("signing_ed25519.key"); + fs::set_permissions(key, fs::Permissions::from_mode(0o644)).unwrap(); + fs::remove_file(kms_dir.join("state.json")).unwrap(); + }); + let key = kms.kms_dir.path().join("signing_ed25519.key"); + let state = kms.kms_dir.path().join("state.json"); + let log = kms.stop(); + + assert!( + log.contains(&format!( + "{} is readable by users other than its owner (mode 0644): \ + restrict it with `chmod 600`", + key.display() + )), + "{log}" + ); + assert!( + log.contains(&format!( + "[test_chain_id] no state file found: created {} at height 0, \ + so double signing protection has no history", + state.display() + )), + "{log}" + ); +} + +/// A KMS that finds no state file warns that double-sign protection has no history, +/// so every config the harness writes must point at a state file that already exists +#[test] +fn generated_configs_point_at_an_existing_state_file() { + for key_type in [KeyType::Account, KeyType::Consensus] { + let (tcp_dir, unix_dir) = (tempfile::tempdir().unwrap(), tempfile::tempdir().unwrap()); + let configs = [ + KmsProcess::create_tcp_config(60000, &key_type, &tcp_dir), + KmsProcess::create_unix_config("/tmp/tmkms-unused.sock", &key_type, &unix_dir), + ]; + + for config in configs { + let config: toml::Table = + toml::from_str(&fs::read_to_string(config.path()).unwrap()).unwrap(); + + for chain in config["chain"].as_array().unwrap() { + let path = chain["state_file"].as_str().unwrap(); + assert!(Path::new(path).exists(), "{path} does not exist yet"); + } + } + } +} + +/// The KMS warns about any key file others can read, and git stores the committed +/// fixtures as 0644, so every config the harness writes must point at owner-only +/// copies instead +#[test] +fn generated_configs_point_at_owner_only_key_files() { + let dir = tempfile::tempdir().unwrap(); + + for key_type in [KeyType::Account, KeyType::Consensus] { + let configs = [ + KmsProcess::create_tcp_config(60000, &key_type, &dir), + KmsProcess::create_unix_config("/tmp/tmkms-unused.sock", &key_type, &dir), + ]; + + for config in configs { + let config: toml::Table = + toml::from_str(&fs::read_to_string(config.path()).unwrap()).unwrap(); + let secret_keys = config["validator"] + .as_array() + .unwrap() + .iter() + .filter_map(|validator| validator.get("secret_key")); + let signing_keys = config["providers"]["softsign"] + .as_array() + .unwrap() + .iter() + .map(|provider| &provider["path"]); + + for path in secret_keys.chain(signing_keys) { + let path = path.as_str().unwrap(); + let mode = fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "{path} is readable by others (mode {mode:04o})" + ); + } + } + } +} + /// Extract the actual length of an amino message pub fn extract_actual_len(buf: &[u8]) -> Result { let mut buff = Cursor::new(buf); @@ -330,7 +584,7 @@ fn handle_and_sign_proposal(key_type: KeyType) { nanos: dt.timestamp_subsec_nanos() as i32, }; - ProtocolTester::apply(&key_type, |mut pt| { + ProtocolTester::apply(&key_type, |pt| { let proposal = proto::types::v1beta1::Proposal { r#type: ConsensusMsgType::Proposal.into(), height: 12345, @@ -350,10 +604,10 @@ fn handle_and_sign_proposal(key_type: KeyType) { send_request( proto::privval::message::Sum::SignProposalRequest(request), - &mut pt, + pt, ); - let response = match read_response(&mut pt) { + let response = match read_response(pt) { proto::privval::message::Sum::SignedProposalResponse(resp) => resp, other => panic!("unexpected message type in response: {other:?}"), }; @@ -404,7 +658,7 @@ fn handle_and_sign_vote(key_type: KeyType) { nanos: dt.timestamp_subsec_nanos() as i32, }; - ProtocolTester::apply(&key_type, |mut pt| { + ProtocolTester::apply(&key_type, |pt| { let vote_msg = proto::types::v1beta1::Vote { r#type: 0x01, height: 12345, @@ -435,9 +689,9 @@ fn handle_and_sign_vote(key_type: KeyType) { // skip_extension_signing: false, }; - send_request(proto::privval::message::Sum::SignVoteRequest(vote), &mut pt); + send_request(proto::privval::message::Sum::SignVoteRequest(vote), pt); - let request = match read_response(&mut pt) { + let request = match read_response(pt) { proto::privval::message::Sum::SignedVoteResponse(resp) => resp, other => panic!("unexpected message type in response: {other:?}"), }; @@ -472,17 +726,17 @@ fn handle_and_sign_vote(key_type: KeyType) { } #[test] -#[should_panic] fn test_exceed_max_height_account() { exceed_max_height(KeyType::Account) } #[test] -#[should_panic] fn test_exceed_max_height_consensus() { exceed_max_height(KeyType::Consensus) } +/// Asked to sign above its `max_height`, the KMS drops the connection without answering +/// and exits with an error naming the stop height fn exceed_max_height(key_type: KeyType) { let chain_id = "test_chain_id"; @@ -492,71 +746,52 @@ fn exceed_max_height(key_type: KeyType) { nanos: dt.timestamp_subsec_nanos() as i32, }; - ProtocolTester::apply(&key_type, |mut pt| { - let vote_msg = proto::types::v1beta1::Vote { - r#type: 0x01, - height: 500001, - round: 2, - timestamp: Some(t), - block_id: Some(proto::types::v1beta1::BlockId { - hash: b"some hash00000000000000000000000".to_vec(), - part_set_header: Some(proto::types::v1beta1::PartSetHeader { - total: 1000000, - hash: b"parts_hash0000000000000000000000".to_vec(), - }), + // Not `apply`: each KMS logs the error it is made to hit + let mut pt = ProtocolTester::start(&key_type); + + let vote_msg = proto::types::v1beta1::Vote { + r#type: 0x01, + height: 500001, + round: 2, + timestamp: Some(t), + block_id: Some(proto::types::v1beta1::BlockId { + hash: b"some hash00000000000000000000000".to_vec(), + part_set_header: Some(proto::types::v1beta1::PartSetHeader { + total: 1000000, + hash: b"parts_hash0000000000000000000000".to_vec(), }), - validator_address: vec![ - 0xa3, 0xb2, 0xcc, 0xdd, 0x71, 0x86, 0xf1, 0x68, 0x5f, 0x21, 0xf2, 0x48, 0x2a, 0xf4, - 0xfb, 0x34, 0x46, 0xa8, 0x4b, 0x35, - ], - validator_index: 56789, - signature: vec![], - // extension: vec![], - // extension_signature: vec![], - }; - - let signable_msg = ConsensusMsg::try_from(vote_msg.clone()).unwrap(); - - let vote = proto::privval::v1beta1::SignVoteRequest { - vote: Some(vote_msg), - chain_id: chain_id.into(), - // skip_extension_signing: false, - }; - - send_request(proto::privval::message::Sum::SignVoteRequest(vote), &mut pt); - - let response = match read_response(&mut pt) { - proto::privval::message::Sum::SignedVoteResponse(resp) => resp, - other => panic!("unexpected message type in response: {other:?}"), - }; + }), + validator_address: vec![ + 0xa3, 0xb2, 0xcc, 0xdd, 0x71, 0x86, 0xf1, 0x68, 0x5f, 0x21, 0xf2, 0x48, 0x2a, 0xf4, + 0xfb, 0x34, 0x46, 0xa8, 0x4b, 0x35, + ], + validator_index: 56789, + signature: vec![], + // extension: vec![], + // extension_signature: vec![], + }; - let signable_bytes = signable_msg - .canonical_bytes(chain_id.parse().unwrap()) - .unwrap(); + let vote = proto::privval::v1beta1::SignVoteRequest { + vote: Some(vote_msg), + chain_id: chain_id.into(), + // skip_extension_signing: false, + }; - let vote_msg = response - .vote - .expect("vote should be embedded int the response but none was found"); + send_request(proto::privval::message::Sum::SignVoteRequest(vote), &mut pt); - let sig: Vec = vote_msg.signature; - assert_ne!(sig.len(), 0); + for (connection, kms) in [ + (&mut pt.tcp_connection, &mut pt.tcp_device), + (&mut pt.unix_connection, &mut pt.unix_device), + ] { + let response = connection.read_msg(); + assert!(response.is_err(), "KMS answered: {response:?}"); - let r = match key_type { - KeyType::Account => { - let signature = k256::ecdsa::Signature::try_from(sig.as_slice()).unwrap(); - test_secp256k1_keypair() - .1 - .verify(&signable_bytes, &signature) - } - KeyType::Consensus => { - let signature = ed25519::Signature::try_from(sig.as_slice()).unwrap(); - test_ed25519_signing_keypair() - .verifying_key() - .verify(&signable_bytes, &signature) - } - }; - assert!(r.is_ok()); - }); + assert_eq!(kms.wait_for_exit().code(), Some(1)); + assert!(kms.log().contains( + "requested signature above stop height: \ + attempted to sign at height 500001 which is greater than 500000" + )); + } } #[test] @@ -572,17 +807,14 @@ fn test_handle_and_sign_get_publickey_consensus() { fn handle_and_sign_get_publickey(key_type: KeyType) { let chain_id = "test_chain_id"; - ProtocolTester::apply(&key_type, |mut pt| { + ProtocolTester::apply(&key_type, |pt| { let request = proto::privval::v1beta1::PubKeyRequest { chain_id: chain_id.into(), }; - send_request( - proto::privval::message::Sum::PubKeyRequest(request), - &mut pt, - ); + send_request(proto::privval::message::Sum::PubKeyRequest(request), pt); - let response = match read_response(&mut pt) { + let response = match read_response(pt) { proto::privval::message::Sum::PubKeyResponse(resp) => resp, other => panic!("unexpected message type in response: {other:?}"), }; @@ -606,10 +838,10 @@ fn handle_and_sign_get_publickey(key_type: KeyType) { fn test_handle_and_sign_ping_pong() { let key_type = KeyType::Consensus; - ProtocolTester::apply(&key_type, |mut pt| { + ProtocolTester::apply(&key_type, |pt| { let request = proto::privval::v1beta1::PingRequest {}; - send_request(proto::privval::message::Sum::PingRequest(request), &mut pt); - read_response(&mut pt); + send_request(proto::privval::message::Sum::PingRequest(request), pt); + read_response(pt); }); }