diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts index 390a0b91c6bf..a72e5908ce1e 100644 --- a/apps/desktop/desktop_native/napi/index.d.ts +++ b/apps/desktop/desktop_native/napi/index.d.ts @@ -482,6 +482,11 @@ export declare namespace sshagent_v2 { privateKey: string name: string cipherId: string + /** + * SHA-256 host-key fingerprints this key is restricted to offering. Empty means + * unrestricted. + */ + destinationFingerprints: Array } } diff --git a/apps/desktop/desktop_native/napi/src/sshagent_v2.rs b/apps/desktop/desktop_native/napi/src/sshagent_v2.rs index 63d9d7689398..f1c2d5eb4cbe 100644 --- a/apps/desktop/desktop_native/napi/src/sshagent_v2.rs +++ b/apps/desktop/desktop_native/napi/src/sshagent_v2.rs @@ -30,6 +30,9 @@ pub mod sshagent_v2 { pub private_key: String, pub name: String, pub cipher_id: String, + /// SHA-256 host-key fingerprints this key is restricted to offering. Empty means + /// unrestricted. + pub destination_fingerprints: Vec, } /// SSH public key data @@ -217,6 +220,7 @@ pub mod sshagent_v2 { private_key_pem: k.private_key, name: k.name, cipher_id: k.cipher_id, + destination_fingerprints: k.destination_fingerprints, }) .collect(); diff --git a/apps/desktop/desktop_native/ssh_agent/src/server/connection.rs b/apps/desktop/desktop_native/ssh_agent/src/server/connection.rs index 6f8a84f03520..d189abc73cad 100644 --- a/apps/desktop/desktop_native/ssh_agent/src/server/connection.rs +++ b/apps/desktop/desktop_native/ssh_agent/src/server/connection.rs @@ -188,7 +188,9 @@ async fn handle_message( }; match message { - AgentMessage::RequestIdentities => handle_list_request(keystore, auth_policy).await, + AgentMessage::RequestIdentities => { + handle_list_request(session_bind_state, keystore, auth_policy).await + } AgentMessage::SignRequest { public_key, data, @@ -213,6 +215,7 @@ async fn handle_message( } async fn handle_list_request( + session_bind_state: &SessionBindState, keystore: &Arc, auth_policy: &Arc, ) -> Vec { @@ -231,7 +234,13 @@ async fn handle_list_request( return failure(); } - match keystore.get_all_public_keys_and_names() { + let host_fingerprint = if session_bind_state.host_fingerprint.is_empty() { + None + } else { + Some(session_bind_state.host_fingerprint.as_str()) + }; + + match keystore.get_all_public_keys_and_names(host_fingerprint) { Ok(keys) => build_identities_answer(keys), Err(error) => { error!(%error, "Failed to retrieve keys from keystore"); @@ -394,7 +403,7 @@ mod tests { keystore .expect_get_all_public_keys_and_names() .once() - .returning(|| { + .returning(|_host_fingerprint| { Ok(vec![( PublicKey { alg: "ssh-ed25519".to_string(), @@ -441,7 +450,7 @@ mod tests { keystore .expect_get_all_public_keys_and_names() .once() - .returning(|| Err(anyhow::anyhow!("keystore error"))); + .returning(|_host_fingerprint| Err(anyhow::anyhow!("keystore error"))); let auth_policy = Arc::new(AlwaysAllowPolicy); let response = super::handle_message( @@ -456,6 +465,64 @@ mod tests { assert_eq!(response, vec![FAILURE]); } + #[tokio::test] + async fn list_request_without_session_bind_passes_none_host_fingerprint_to_keystore() { + let mut keystore = MockKeyStore::new(); + keystore + .expect_get_all_public_keys_and_names() + // `.with(eq(...))` requires the predicate's `Borrow` impl to hold for any lifetime, + // which a plain `&str` reference can't satisfy against this mocked method's named + // lifetime; `.withf` sidesteps that by matching directly on the real, non-'static arg. + .withf(|host_fingerprint| host_fingerprint.is_none()) + .once() + .returning(|_host_fingerprint| Ok(vec![])); + let auth_policy = Arc::new(AlwaysAllowPolicy); + + let response = super::handle_message( + &[REQUEST_IDENTITIES], + None, + &SessionBindState::default(), + &Arc::new(keystore), + &auth_policy, + ) + .await; + + assert_eq!(response[0], IDENTITIES_ANSWER); + } + + #[tokio::test] + async fn list_request_host_fingerprint_propagates_to_keystore() { + use ssh_key::{private::Ed25519Keypair, rand_core::OsRng}; + + let keypair = Ed25519Keypair::random(&mut OsRng); + let bind_payload = make_session_bind_payload_ed25519(&keypair, &[0x42u8; 32], false); + let ext_payload = make_extension_payload(b"session-bind@openssh.com", &bind_payload); + + let mut state = SessionBindState::default(); + super::handle_extension_message(&ext_payload, &mut state); + let expected_fingerprint = state.host_fingerprint.clone(); + assert!(!expected_fingerprint.is_empty()); + + let mut keystore = MockKeyStore::new(); + keystore + .expect_get_all_public_keys_and_names() + .withf(move |host_fingerprint| *host_fingerprint == Some(expected_fingerprint.as_str())) + .once() + .returning(|_host_fingerprint| Ok(vec![])); + let auth_policy = Arc::new(AlwaysAllowPolicy); + + let response = super::handle_message( + &[REQUEST_IDENTITIES], + None, + &state, + &Arc::new(keystore), + &auth_policy, + ) + .await; + + assert_eq!(response[0], IDENTITIES_ANSWER); + } + #[tokio::test] async fn sign_request_when_authorized_key_found_returns_sign_response() { use ssh_key::{private::Ed25519Keypair, rand_core::OsRng}; diff --git a/apps/desktop/desktop_native/ssh_agent/src/storage/keydata.rs b/apps/desktop/desktop_native/ssh_agent/src/storage/keydata.rs index f4d1f9a70fc6..14a819fa4997 100644 --- a/apps/desktop/desktop_native/ssh_agent/src/storage/keydata.rs +++ b/apps/desktop/desktop_native/ssh_agent/src/storage/keydata.rs @@ -28,6 +28,33 @@ pub trait QueryableKeyData: Send + Sync { fn cipher_id(&self) -> &String; } +/// Classifies how a key relates to a verified destination host-key fingerprint for a connection. +/// +/// This exists to serve two distinct, related purposes on the identities returned by +/// `SSH_AGENTC_REQUEST_IDENTITIES`: +/// * **Filtering** — whether a key is offered at all (`NoMatch` is omitted; the other two variants +/// are offered). +/// * **Prioritization** — among offered keys, `ExplicitMatch` keys are listed before `Unrestricted` +/// keys, so OpenSSH tries a destination-matching key first. +/// +/// This is an identity-offering optimization, not an authorization or security boundary: it only +/// changes which identities are *offered* and in what order, never whether signing is authorized +/// (that remains governed by session-bind verification and the auth policy independently of this +/// classification). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum DestinationMatch { + /// `host_fingerprint` is present in this key's `destination_fingerprints`. Offered, and + /// listed ahead of `Unrestricted` keys. + ExplicitMatch, + /// This key has no configured `destination_fingerprints` (or no verified `host_fingerprint` + /// was available for the connection, in which case every key is classified uniformly as + /// `Unrestricted` so the original order is preserved unchanged). Offered, but not + /// prioritized ahead of other offered keys. + Unrestricted, + /// This key is restricted to other destinations. Not offered for this connection. + NoMatch, +} + /// An intermediary struct representing an SSH key from the vault, /// before its private key has been parsed. pub struct UnparsedSSHKeyData { @@ -37,6 +64,8 @@ pub struct UnparsedSSHKeyData { pub name: String, /// Vault cipher ID associated with the key pair pub cipher_id: String, + /// SHA-256 host-key fingerprints this key is restricted to offering. Empty means unrestricted. + pub destination_fingerprints: Vec, } /// Represents an SSH key and its associated metadata. @@ -50,6 +79,8 @@ pub struct SSHKeyData { pub(super) name: String, /// Vault cipher ID associated with the key pair pub(super) cipher_id: String, + /// SHA-256 host-key fingerprints this key is restricted to offering. Empty means unrestricted. + pub(super) destination_fingerprints: Vec, } impl SSHKeyData { @@ -61,18 +92,22 @@ impl SSHKeyData { /// * `public_key` - The public key component /// * `name` - A human-readable name for the key /// * `cipher_id` - The vault cipher identifier associated with this key + /// * `destination_fingerprints` - SHA-256 host-key fingerprints this key is restricted to + /// offering. Empty means unrestricted. #[must_use] pub fn new( private_key: PrivateKey, public_key: PublicKey, name: String, cipher_id: String, + destination_fingerprints: Vec, ) -> Self { Self { private_key, public_key, name, cipher_id, + destination_fingerprints, } } @@ -82,7 +117,12 @@ impl SSHKeyData { /// /// Returns an error if the PEM string cannot be parsed, the public key blob cannot be /// encoded, or the key algorithm is unsupported. - pub fn from_private_key_pem(pem: &str, name: String, cipher_id: String) -> Result { + pub fn from_private_key_pem( + pem: &str, + name: String, + cipher_id: String, + destination_fingerprints: Vec, + ) -> Result { let ssh_key = ssh_key::PrivateKey::from_openssh(pem) .map_err(|e| anyhow!("Failed to parse private key: {e}"))?; @@ -99,6 +139,7 @@ impl SSHKeyData { PublicKey { alg, blob }, name, cipher_id, + destination_fingerprints, )) } @@ -110,9 +151,14 @@ impl SSHKeyData { .into_iter() .filter_map(|k| { let cipher_id = k.cipher_id.clone(); - Self::from_private_key_pem(&k.private_key_pem, k.name, k.cipher_id) - .inspect_err(|error| warn!(%error, %cipher_id, "Skipping un-parseable key")) - .ok() + Self::from_private_key_pem( + &k.private_key_pem, + k.name, + k.cipher_id, + k.destination_fingerprints, + ) + .inspect_err(|error| warn!(%error, %cipher_id, "Skipping un-parseable key")) + .ok() }) .collect(); @@ -133,6 +179,26 @@ impl SSHKeyData { pub fn private_key(&self) -> &PrivateKey { &self.private_key } + + /// Classifies this key against the given verified destination host-key fingerprint. See + /// [`DestinationMatch`] for how filtering and prioritization are derived from the result. + #[must_use] + pub(super) fn destination_match(&self, host_fingerprint: Option<&str>) -> DestinationMatch { + let Some(host_fingerprint) = host_fingerprint else { + return DestinationMatch::Unrestricted; + }; + if self.destination_fingerprints.is_empty() { + DestinationMatch::Unrestricted + } else if self + .destination_fingerprints + .iter() + .any(|fp| fp == host_fingerprint) + { + DestinationMatch::ExplicitMatch + } else { + DestinationMatch::NoMatch + } + } } impl QueryableKeyData for SSHKeyData { @@ -239,6 +305,7 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAE3NrLXRlc3RAZXhhbXBsZS5jb20BAgMEBQY= TEST_SK_ED25519_PEM, "sk-test".to_string(), "cipher-sk-1".to_string(), + vec![], ); assert!(result.is_err(), "sk-ssh-ed25519 key type must be rejected"); @@ -247,34 +314,39 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAE3NrLXRlc3RAZXhhbXBsZS5jb20BAgMEBQY= #[test] fn from_private_key_pem_ed25519_sets_correct_algorithm_string() { let data = - SSHKeyData::from_private_key_pem(TEST_ED25519_PEM, "k".into(), "id".into()).unwrap(); + SSHKeyData::from_private_key_pem(TEST_ED25519_PEM, "k".into(), "id".into(), vec![]) + .unwrap(); assert_eq!(data.public_key().alg(), "ssh-ed25519"); } #[test] fn from_private_key_pem_ecdsa_p256_sets_correct_algorithm_string() { let data = - SSHKeyData::from_private_key_pem(TEST_ECDSA_P256_PEM, "k".into(), "id".into()).unwrap(); + SSHKeyData::from_private_key_pem(TEST_ECDSA_P256_PEM, "k".into(), "id".into(), vec![]) + .unwrap(); assert_eq!(data.public_key().alg(), "ecdsa-sha2-nistp256"); } #[test] fn from_private_key_pem_ecdsa_p384_sets_correct_algorithm_string() { let data = - SSHKeyData::from_private_key_pem(TEST_ECDSA_P384_PEM, "k".into(), "id".into()).unwrap(); + SSHKeyData::from_private_key_pem(TEST_ECDSA_P384_PEM, "k".into(), "id".into(), vec![]) + .unwrap(); assert_eq!(data.public_key().alg(), "ecdsa-sha2-nistp384"); } #[test] fn from_private_key_pem_ecdsa_p521_sets_correct_algorithm_string() { let data = - SSHKeyData::from_private_key_pem(TEST_ECDSA_P521_PEM, "k".into(), "id".into()).unwrap(); + SSHKeyData::from_private_key_pem(TEST_ECDSA_P521_PEM, "k".into(), "id".into(), vec![]) + .unwrap(); assert_eq!(data.public_key().alg(), "ecdsa-sha2-nistp521"); } #[test] fn from_private_key_pem_rsa_sets_correct_algorithm_string() { - let data = SSHKeyData::from_private_key_pem(TEST_RSA_PEM, "k".into(), "id".into()).unwrap(); + let data = SSHKeyData::from_private_key_pem(TEST_RSA_PEM, "k".into(), "id".into(), vec![]) + .unwrap(); assert_eq!(data.public_key().alg(), "ssh-rsa"); } @@ -283,6 +355,7 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAE3NrLXRlc3RAZXhhbXBsZS5jb20BAgMEBQY= private_key_pem: pem.to_string(), name: name.to_string(), cipher_id: format!("cipher-{name}"), + destination_fingerprints: vec![], } } @@ -306,4 +379,47 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAE3NrLXRlc3RAZXhhbXBsZS5jb20BAgMEBQY= assert!(parsed.is_empty()); } + + fn keydata_with_destinations(destinations: &[&str]) -> SSHKeyData { + SSHKeyData::from_private_key_pem( + TEST_ED25519_PEM, + "k".into(), + "id".into(), + destinations.iter().map(|s| s.to_string()).collect(), + ) + .unwrap() + } + + #[test] + fn destination_match_no_host_fingerprint_is_unrestricted_even_when_restricted() { + let key = keydata_with_destinations(&["SHA256:other"]); + assert_eq!(key.destination_match(None), DestinationMatch::Unrestricted); + } + + #[test] + fn destination_match_unrestricted_key_is_unrestricted_for_any_host() { + let key = keydata_with_destinations(&[]); + assert_eq!( + key.destination_match(Some("SHA256:anything")), + DestinationMatch::Unrestricted + ); + } + + #[test] + fn destination_match_matching_destination_is_explicit_match() { + let key = keydata_with_destinations(&["SHA256:a", "SHA256:b"]); + assert_eq!( + key.destination_match(Some("SHA256:b")), + DestinationMatch::ExplicitMatch + ); + } + + #[test] + fn destination_match_non_matching_destination_is_no_match() { + let key = keydata_with_destinations(&["SHA256:a", "SHA256:b"]); + assert_eq!( + key.destination_match(Some("SHA256:c")), + DestinationMatch::NoMatch + ); + } } diff --git a/apps/desktop/desktop_native/ssh_agent/src/storage/keystore.rs b/apps/desktop/desktop_native/ssh_agent/src/storage/keystore.rs index 465b3cdf0c28..1b468cf2304a 100644 --- a/apps/desktop/desktop_native/ssh_agent/src/storage/keystore.rs +++ b/apps/desktop/desktop_native/ssh_agent/src/storage/keystore.rs @@ -10,9 +10,12 @@ use std::sync::{ use anyhow::Result; use secure_memory::{EncryptedMemoryStore, SecureMemoryStore}; -use crate::crypto::{PrivateKey, PublicKey, QueryableKeyData, SSHKeyData}; #[cfg(test)] use crate::storage::keydata::MockQueryableKeyData; +use crate::{ + crypto::{PrivateKey, PublicKey, QueryableKeyData, SSHKeyData}, + storage::keydata::DestinationMatch, +}; /// Securely store and retrieve SSH key data. /// @@ -45,10 +48,32 @@ pub trait KeyStore: Send + Sync { /// * `Err(_)` if an error occurred during retrieval fn get_private_key(&self, public_key: &PublicKey) -> Result>; + /// # Arguments + /// + /// * `host_fingerprint` - The verified SHA-256 fingerprint of the destination host key for the + /// current connection, if session-bind information is available. + /// + /// **Filtering**: when `None`, all keys are returned regardless of their configured + /// destinations (fallback behavior — original keystore order, unmodified). When `Some`, + /// keys restricted to other destinations are omitted entirely. + /// + /// **Prioritization**: among the keys that are returned, ones explicitly configured for + /// `host_fingerprint` are ordered before unrestricted keys, so OpenSSH tries a + /// destination-matching key first. This is an identity-offering optimization, not an + /// authorization boundary — it never affects whether signing is authorized. The relative + /// order within each group (explicit matches among themselves, unrestricted keys among + /// themselves) matches their order in the underlying keystore. + /// /// # Returns /// - /// A vector of tuples containing each key's public key and human-readable name. - fn get_all_public_keys_and_names(&self) -> Result>; + /// A vector of tuples containing each offered key's public key and human-readable name. + // The lifetime is needed for `mockall::automock` to expand this method correctly; plain + // elision compiles fine without the attribute but fails with E0106 once it's applied. + #[allow(clippy::needless_lifetimes)] + fn get_all_public_keys_and_names<'a>( + &self, + host_fingerprint: Option<&'a str>, + ) -> Result>; /// Atomically replaces all keys in the keystore. fn replace(&self, keys: Vec) -> Result<()>; @@ -112,17 +137,35 @@ impl KeyStore for InMemoryEncryptedKeyStore { .transpose() } - fn get_all_public_keys_and_names(&self) -> Result> { - self.secure_memory + #[allow(clippy::needless_lifetimes)] + fn get_all_public_keys_and_names<'a>( + &self, + host_fingerprint: Option<&'a str>, + ) -> Result> { + // Two buckets, filled in a single pass over the keystore's existing order: this makes the + // partition stable (each bucket keeps the relative order its members already had) and + // preserves the original collect::>() semantics of returning on the *first* + // unparseable key, since `?` short-circuits at the same point in the same iteration order. + let mut explicit_matches = Vec::new(); + let mut unrestricted = Vec::new(); + + for bytes in self + .secure_memory .lock() .expect("Mutex is not poisoned") .to_vec()? - .into_iter() - .map(|bytes| { - SSHKeyData::try_from(bytes) - .map(|key_data| (key_data.public_key().clone(), key_data.name().clone())) - }) - .collect::, _>>() + { + let key_data = SSHKeyData::try_from(bytes)?; + let entry = (key_data.public_key().clone(), key_data.name().clone()); + match key_data.destination_match(host_fingerprint) { + DestinationMatch::ExplicitMatch => explicit_matches.push(entry), + DestinationMatch::Unrestricted => unrestricted.push(entry), + DestinationMatch::NoMatch => {} + } + } + + explicit_matches.extend(unrestricted); + Ok(explicit_matches) } fn get_private_key(&self, public_key: &PublicKey) -> Result> { @@ -175,6 +218,14 @@ mod tests { use crate::crypto::{PrivateKey, QueryableKeyData}; fn create_test_keydata_ed25519(name: &str, cipher_id: &str) -> SSHKeyData { + create_test_keydata_ed25519_with_destinations(name, cipher_id, &[]) + } + + fn create_test_keydata_ed25519_with_destinations( + name: &str, + cipher_id: &str, + destinations: &[&str], + ) -> SSHKeyData { let ed25519_keypair = Ed25519Keypair::random(&mut OsRng); let ssh_key = ssh_key::PrivateKey::new( ssh_key::private::KeypairData::Ed25519(ed25519_keypair.clone()), @@ -191,6 +242,7 @@ mod tests { }, name.to_string(), cipher_id.to_string(), + destinations.iter().map(|s| s.to_string()).collect(), ) } @@ -209,6 +261,7 @@ mod tests { }, name.to_string(), cipher_id.to_string(), + vec![], ) } @@ -216,7 +269,7 @@ mod tests { fn test_new_creates_empty_store() { let ks = InMemoryEncryptedKeyStore::new(); - let result = ks.get_all_public_keys_and_names(); + let result = ks.get_all_public_keys_and_names(None); assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 0); } @@ -251,6 +304,7 @@ mod tests { public_key.clone(), "updated-name".to_string(), "updated-cipher".to_string(), + vec![], ); // insert second key with same public key @@ -302,7 +356,7 @@ mod tests { ks.replace(vec![key1, key2]).unwrap(); - let result = ks.get_all_public_keys_and_names().unwrap(); + let result = ks.get_all_public_keys_and_names(None).unwrap(); assert_eq!(result.len(), 2); let names: Vec = result.iter().map(|(_, n)| n.clone()).collect(); assert!(names.contains(&"key1".to_string())); @@ -318,7 +372,7 @@ mod tests { let new_key = create_test_keydata_rsa("new-key", "cipher-new"); ks.replace(vec![new_key]).unwrap(); - let result = ks.get_all_public_keys_and_names().unwrap(); + let result = ks.get_all_public_keys_and_names(None).unwrap(); assert_eq!(result.len(), 1); assert_eq!(result[0].1, "new-key"); } @@ -332,7 +386,7 @@ mod tests { ks.replace(vec![key1]).unwrap(); ks.replace(vec![key2]).unwrap(); - let result = ks.get_all_public_keys_and_names().unwrap(); + let result = ks.get_all_public_keys_and_names(None).unwrap(); assert_eq!(result.len(), 1); assert_eq!(result[0].1, "key2"); } @@ -345,7 +399,7 @@ mod tests { ks.replace(vec![]).unwrap(); - let result = ks.get_all_public_keys_and_names().unwrap(); + let result = ks.get_all_public_keys_and_names(None).unwrap(); assert_eq!(result.len(), 0); } @@ -382,7 +436,7 @@ mod tests { #[test] fn test_get_all_empty_store() { let ks = InMemoryEncryptedKeyStore::new(); - let result = ks.get_all_public_keys_and_names(); + let result = ks.get_all_public_keys_and_names(None); assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 0); @@ -403,7 +457,7 @@ mod tests { ks.insert(key2).unwrap(); ks.insert(key3).unwrap(); - let result = ks.get_all_public_keys_and_names().unwrap(); + let result = ks.get_all_public_keys_and_names(None).unwrap(); assert_eq!(result.len(), 3); let names: Vec = result.iter().map(|(_, name)| name.clone()).collect(); @@ -418,4 +472,364 @@ mod tests { assert!(public_keys.contains(&pub_key2)); assert!(public_keys.contains(&pub_key3)); } + + #[test] + fn test_get_all_unrestricted_key_is_always_returned() { + let ks = InMemoryEncryptedKeyStore::new(); + let key = create_test_keydata_ed25519_with_destinations("key", "cipher", &[]); + ks.insert(key).unwrap(); + + let result = ks + .get_all_public_keys_and_names(Some("SHA256:host")) + .unwrap(); + assert_eq!(result.len(), 1); + } + + #[test] + fn test_get_all_key_with_matching_destination_is_returned() { + let ks = InMemoryEncryptedKeyStore::new(); + let key = create_test_keydata_ed25519_with_destinations("key", "cipher", &["SHA256:host"]); + ks.insert(key).unwrap(); + + let result = ks + .get_all_public_keys_and_names(Some("SHA256:host")) + .unwrap(); + assert_eq!(result.len(), 1); + } + + #[test] + fn test_get_all_key_with_only_non_matching_destinations_is_excluded() { + let ks = InMemoryEncryptedKeyStore::new(); + let key = create_test_keydata_ed25519_with_destinations("key", "cipher", &["SHA256:other"]); + ks.insert(key).unwrap(); + + let result = ks + .get_all_public_keys_and_names(Some("SHA256:host")) + .unwrap(); + assert_eq!(result.len(), 0); + } + + #[test] + fn test_get_all_key_with_multiple_destinations_one_matching_is_returned() { + let ks = InMemoryEncryptedKeyStore::new(); + let key = create_test_keydata_ed25519_with_destinations( + "key", + "cipher", + &["SHA256:other", "SHA256:host"], + ); + ks.insert(key).unwrap(); + + let result = ks + .get_all_public_keys_and_names(Some("SHA256:host")) + .unwrap(); + assert_eq!(result.len(), 1); + } + + #[test] + fn test_get_all_mixed_restricted_and_unrestricted_keys_returns_correct_subset() { + let ks = InMemoryEncryptedKeyStore::new(); + let unrestricted = + create_test_keydata_ed25519_with_destinations("unrestricted", "cipher-1", &[]); + let matching = + create_test_keydata_ed25519_with_destinations("matching", "cipher-2", &["SHA256:host"]); + let non_matching = create_test_keydata_ed25519_with_destinations( + "non-matching", + "cipher-3", + &["SHA256:other"], + ); + ks.insert(unrestricted).unwrap(); + ks.insert(matching).unwrap(); + ks.insert(non_matching).unwrap(); + + let result = ks + .get_all_public_keys_and_names(Some("SHA256:host")) + .unwrap(); + let names: Vec = result.iter().map(|(_, name)| name.clone()).collect(); + + assert_eq!(result.len(), 2); + assert!(names.contains(&"unrestricted".to_string())); + assert!(names.contains(&"matching".to_string())); + assert!(!names.contains(&"non-matching".to_string())); + } + + #[test] + fn test_get_all_without_host_fingerprint_returns_everything_regardless_of_destinations() { + let ks = InMemoryEncryptedKeyStore::new(); + let unrestricted = + create_test_keydata_ed25519_with_destinations("unrestricted", "cipher-1", &[]); + let restricted = create_test_keydata_ed25519_with_destinations( + "restricted", + "cipher-2", + &["SHA256:other"], + ); + ks.insert(unrestricted).unwrap(); + ks.insert(restricted).unwrap(); + + let result = ks.get_all_public_keys_and_names(None).unwrap(); + assert_eq!(result.len(), 2); + } + + /// Names of the keys returned for `host_fingerprint`, in order. + fn names_in_order( + ks: &InMemoryEncryptedKeyStore, + host_fingerprint: Option<&str>, + ) -> Vec { + ks.get_all_public_keys_and_names(host_fingerprint) + .unwrap() + .into_iter() + .map(|(_, name)| name) + .collect() + } + + /// `baseline`, restricted to the names in `keep`, preserving `baseline`'s relative order. + fn filter_preserving_order(baseline: &[String], keep: &[&str]) -> Vec { + baseline + .iter() + .filter(|name| keep.contains(&name.as_str())) + .cloned() + .collect() + } + + #[test] + fn test_get_all_explicit_match_ordered_before_unrestricted() { + let ks = InMemoryEncryptedKeyStore::new(); + let unrestricted = + create_test_keydata_ed25519_with_destinations("unrestricted", "cipher-1", &[]); + let matching = + create_test_keydata_ed25519_with_destinations("matching", "cipher-2", &["SHA256:host"]); + ks.insert(unrestricted).unwrap(); + ks.insert(matching).unwrap(); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!( + result, + vec!["matching".to_string(), "unrestricted".to_string()] + ); + } + + #[test] + fn test_get_all_unrestricted_keys_still_returned_alongside_explicit_match() { + // Prioritization must not turn into exclusive selection: once any explicit match exists, + // unrestricted keys must still be present in the result, not dropped. + let ks = InMemoryEncryptedKeyStore::new(); + let matching = + create_test_keydata_ed25519_with_destinations("matching", "cipher-1", &["SHA256:host"]); + let unrestricted = + create_test_keydata_ed25519_with_destinations("unrestricted", "cipher-2", &[]); + ks.insert(matching).unwrap(); + ks.insert(unrestricted).unwrap(); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result.len(), 2); + assert!(result.contains(&"unrestricted".to_string())); + } + + #[test] + fn test_get_all_restricted_non_matching_key_still_omitted_under_prioritization() { + let ks = InMemoryEncryptedKeyStore::new(); + let matching = + create_test_keydata_ed25519_with_destinations("matching", "cipher-1", &["SHA256:host"]); + let non_matching = create_test_keydata_ed25519_with_destinations( + "non-matching", + "cipher-2", + &["SHA256:other"], + ); + ks.insert(matching).unwrap(); + ks.insert(non_matching).unwrap(); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result, vec!["matching".to_string()]); + } + + #[test] + fn test_get_all_multiple_explicit_matches_preserve_relative_order() { + let ks = InMemoryEncryptedKeyStore::new(); + let names = ["match-a", "match-b", "match-c"]; + for (i, name) in names.iter().enumerate() { + ks.insert(create_test_keydata_ed25519_with_destinations( + name, + &format!("cipher-{i}"), + &["SHA256:host"], + )) + .unwrap(); + } + let baseline = names_in_order(&ks, None); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result, filter_preserving_order(&baseline, &names)); + } + + #[test] + fn test_get_all_multiple_unrestricted_keys_preserve_relative_order() { + let ks = InMemoryEncryptedKeyStore::new(); + let names = ["unres-a", "unres-b", "unres-c"]; + for (i, name) in names.iter().enumerate() { + ks.insert(create_test_keydata_ed25519_with_destinations( + name, + &format!("cipher-{i}"), + &[], + )) + .unwrap(); + } + let baseline = names_in_order(&ks, None); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result, filter_preserving_order(&baseline, &names)); + } + + #[test] + fn test_get_all_stable_partition_with_mixed_matches_unrestricted_and_non_matching() { + // Mirrors the ordering example from the feature spec: two explicit matches, two + // unrestricted keys, and one restricted-but-non-matching key, inserted in a shuffled + // arrangement. The result must be [explicit matches in original relative order] followed + // by [unrestricted keys in original relative order], with the non-matching key omitted. + let ks = InMemoryEncryptedKeyStore::new(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "match-2", + "cipher-1", + &["SHA256:host"], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "unrestricted-1", + "cipher-2", + &[], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "match-1", + "cipher-3", + &["SHA256:host"], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "restricted-non-match", + "cipher-4", + &["SHA256:other"], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "unrestricted-2", + "cipher-5", + &[], + )) + .unwrap(); + let baseline = names_in_order(&ks, None); + + let result = names_in_order(&ks, Some("SHA256:host")); + + let expected_matches = filter_preserving_order(&baseline, &["match-1", "match-2"]); + let expected_unrestricted = + filter_preserving_order(&baseline, &["unrestricted-1", "unrestricted-2"]); + assert_eq!(result.len(), 4); + assert_eq!(&result[..2], expected_matches.as_slice()); + assert_eq!(&result[2..], expected_unrestricted.as_slice()); + assert!(!result.contains(&"restricted-non-match".to_string())); + } + + #[test] + fn test_get_all_no_explicit_matches_preserves_original_order_among_unrestricted() { + let ks = InMemoryEncryptedKeyStore::new(); + let names = ["key-a", "key-b", "key-c"]; + for (i, name) in names.iter().enumerate() { + ks.insert(create_test_keydata_ed25519_with_destinations( + name, + &format!("cipher-{i}"), + &[], + )) + .unwrap(); + } + let baseline = names_in_order(&ks, None); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result, baseline); + } + + #[test] + fn test_get_all_every_key_matches_preserves_original_order() { + let ks = InMemoryEncryptedKeyStore::new(); + let names = ["key-a", "key-b", "key-c"]; + for (i, name) in names.iter().enumerate() { + ks.insert(create_test_keydata_ed25519_with_destinations( + name, + &format!("cipher-{i}"), + &["SHA256:host"], + )) + .unwrap(); + } + let baseline = names_in_order(&ks, None); + + let result = names_in_order(&ks, Some("SHA256:host")); + + assert_eq!(result, baseline); + } + + #[test] + fn test_get_all_without_host_fingerprint_matches_raw_keystore_order_exactly() { + // The fallback path must not reorder anything: it should reproduce the exact sequence the + // underlying store yields, independent of any configured destinations. + let ks = InMemoryEncryptedKeyStore::new(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "restricted", + "cipher-1", + &["SHA256:other"], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "unrestricted", + "cipher-2", + &[], + )) + .unwrap(); + ks.insert(create_test_keydata_ed25519_with_destinations( + "matching", + "cipher-3", + &["SHA256:host"], + )) + .unwrap(); + + let raw_order: Vec = ks + .secure_memory + .lock() + .expect("Mutex is not poisoned") + .to_vec() + .unwrap() + .into_iter() + .map(|bytes| SSHKeyData::try_from(bytes).unwrap().name().clone()) + .collect(); + + let result = names_in_order(&ks, None); + + assert_eq!(result, raw_order); + } + + #[test] + fn test_get_all_propagates_parse_error_for_malformed_stored_entry() { + // A malformed entry (however it got into the store) must still surface as an error rather + // than being silently skipped — this preserves the pre-existing + // collect::, _>>() short-circuit-on-first-error behavior. + let ks = InMemoryEncryptedKeyStore::new(); + let valid = + create_test_keydata_ed25519_with_destinations("valid", "cipher-1", &["SHA256:host"]); + ks.insert(valid).unwrap(); + + let malformed_key = PublicKey { + alg: "ssh-ed25519".to_string(), + blob: vec![0, 1, 2, 3], + }; + ks.secure_memory + .lock() + .expect("Mutex is not poisoned") + .put(malformed_key, b"not valid rkyv bytes"); + + let result = ks.get_all_public_keys_and_names(Some("SHA256:host")); + + assert!(result.is_err()); + } } diff --git a/apps/desktop/desktop_native/ssh_agent/src/storage/serialization.rs b/apps/desktop/desktop_native/ssh_agent/src/storage/serialization.rs index 74e07c060309..fb9b49df8b7b 100644 --- a/apps/desktop/desktop_native/ssh_agent/src/storage/serialization.rs +++ b/apps/desktop/desktop_native/ssh_agent/src/storage/serialization.rs @@ -28,6 +28,7 @@ struct SSHKeyDataSerializable { public_key: PublicKey, name: String, cipher_id: String, + destination_fingerprints: Vec, } impl TryFrom for SSHKeyData { @@ -42,6 +43,7 @@ impl TryFrom for SSHKeyData { public_key: key_data.public_key, name: key_data.name, cipher_id: key_data.cipher_id, + destination_fingerprints: key_data.destination_fingerprints, }) } } @@ -77,6 +79,7 @@ impl TryFrom for Vec { public_key: key_data.public_key, name: key_data.name, cipher_id: key_data.cipher_id, + destination_fingerprints: key_data.destination_fingerprints, }; Ok(rkyv::to_bytes::(&serializable)?.to_vec()) @@ -139,6 +142,7 @@ invalid_base64_data!!! }, name: "test-key".to_string(), cipher_id: "test-cipher-123".to_string(), + destination_fingerprints: vec![], } } @@ -157,6 +161,7 @@ invalid_base64_data!!! }, name: "test-rsa-key".to_string(), cipher_id: "test-cipher-456".to_string(), + destination_fingerprints: vec![], } } @@ -175,6 +180,7 @@ invalid_base64_data!!! }, name: "test-ecdsa-p256-key".to_string(), cipher_id: "test-cipher-ecdsa".to_string(), + destination_fingerprints: vec![], } } @@ -252,6 +258,7 @@ invalid_base64_data!!! }, name: "test".to_string(), cipher_id: "cipher-123".to_string(), + destination_fingerprints: vec![], }; let result = SSHKeyData::try_from(serializable); @@ -269,6 +276,7 @@ invalid_base64_data!!! }, name: "test".to_string(), cipher_id: "cipher-123".to_string(), + destination_fingerprints: vec![], }; SSHKeyData::try_from(serializable).unwrap(); @@ -301,6 +309,21 @@ invalid_base64_data!!! assert_eq!(restored.private_key(), original.private_key()); } + #[test] + fn test_keydata_destination_fingerprints_survive_roundtrip() { + let mut original = create_test_keydata_ed25519(); + original.destination_fingerprints = + vec!["SHA256:aaaa".to_string(), "SHA256:bbbb".to_string()]; + + let bytes: Vec = original.clone().try_into().unwrap(); + let restored: SSHKeyData = bytes.try_into().unwrap(); + + assert_eq!( + restored.destination_fingerprints, + original.destination_fingerprints + ); + } + #[test] fn test_keydata_rsa_to_from_bytes() { let original = create_test_keydata_rsa(); diff --git a/apps/desktop/desktop_native/ssh_agent/tests/common.rs b/apps/desktop/desktop_native/ssh_agent/tests/common.rs index 21b63323ca4a..5f3d82047724 100644 --- a/apps/desktop/desktop_native/ssh_agent/tests/common.rs +++ b/apps/desktop/desktop_native/ssh_agent/tests/common.rs @@ -109,6 +109,7 @@ pub fn test_ed25519_key() -> SSHKeyData { TEST_ED25519_PEM, "Test Key".to_string(), "cipher-test-1".to_string(), + vec![], ) .expect("test PEM should be valid") } @@ -127,6 +128,18 @@ pub fn test_rsa_key() -> SSHKeyData { TEST_RSA_PEM, "Test RSA Key".to_string(), "cipher-rsa-1".to_string(), + vec![], + ) + .expect("test RSA PEM should be valid") +} + +/// Like [`test_rsa_key`], but restricted to the given destination host-key fingerprints. +pub fn test_rsa_key_with_destinations(name: &str, destinations: &[&str]) -> SSHKeyData { + SSHKeyData::from_private_key_pem( + TEST_RSA_PEM, + name.to_string(), + "cipher-rsa-1".to_string(), + destinations.iter().map(|s| s.to_string()).collect(), ) .expect("test RSA PEM should be valid") } @@ -145,6 +158,7 @@ pub fn test_ecdsa_p256_key() -> SSHKeyData { TEST_ECDSA_P256_PEM, "Test ECDSA P-256 Key".to_string(), "cipher-ecdsa-p256".to_string(), + vec![], ) .expect("test ECDSA P-256 PEM should be valid") } @@ -163,6 +177,7 @@ pub fn test_ecdsa_p384_key() -> SSHKeyData { TEST_ECDSA_P384_PEM, "Test ECDSA P-384 Key".to_string(), "cipher-ecdsa-p384".to_string(), + vec![], ) .expect("test ECDSA P-384 PEM should be valid") } @@ -181,6 +196,7 @@ pub fn test_ecdsa_p521_key() -> SSHKeyData { TEST_ECDSA_P521_PEM, "Test ECDSA P-521 Key".to_string(), "cipher-ecdsa-p521".to_string(), + vec![], ) .expect("test ECDSA P-521 PEM should be valid") } @@ -273,17 +289,40 @@ where /// Parses the human-readable name of the first key from an IDENTITIES_ANSWER body. pub fn parse_first_key_name(response: &[u8]) -> String { + parse_key_names(response) + .into_iter() + .next() + .expect("at least one key in response") +} + +/// Parses the human-readable names of every key in an IDENTITIES_ANSWER body, in the order the +/// server returned them. +pub fn parse_key_names(response: &[u8]) -> Vec { // byte 0: type; bytes 1-4: count; then for each key: [4-byte blob_len][blob][4-byte // name_len][name] - let blob_len = u32::from_be_bytes(response[5..9].try_into().expect("4-byte slice")) as usize; - let name_offset = 9 + blob_len; - let name_len = u32::from_be_bytes( - response[name_offset..name_offset + 4] - .try_into() - .expect("4-byte slice"), - ) as usize; - String::from_utf8(response[name_offset + 4..name_offset + 4 + name_len].to_vec()) - .expect("valid UTF-8 key name") + let count = u32::from_be_bytes(response[1..5].try_into().expect("4-byte slice")) as usize; + let mut offset = 5; + let mut names = Vec::with_capacity(count); + for _ in 0..count { + let blob_len = u32::from_be_bytes( + response[offset..offset + 4] + .try_into() + .expect("4-byte slice"), + ) as usize; + offset += 4 + blob_len; + let name_len = u32::from_be_bytes( + response[offset..offset + 4] + .try_into() + .expect("4-byte slice"), + ) as usize; + offset += 4; + names.push( + String::from_utf8(response[offset..offset + name_len].to_vec()) + .expect("valid UTF-8 key name"), + ); + offset += name_len; + } + names } mockall::mock! { @@ -335,12 +374,26 @@ fn make_session_bind_payload( /// Builds a framed EXTENSION message containing a valid session-bind payload. pub fn framed_session_bind_extension(is_forwarding: bool) -> Vec { - use ssh_key::{private::Ed25519Keypair, rand_core::OsRng}; + session_bind_extension_with_fingerprint(is_forwarding).0 +} + +/// Like [`framed_session_bind_extension`], but also returns the SHA-256 fingerprint (in the same +/// `SHA256:...` format the agent computes) of the host key used to sign the bind, so tests can +/// configure destination-restricted keys that do or don't match it. +pub fn session_bind_extension_with_fingerprint(is_forwarding: bool) -> (Vec, String) { + use ssh_key::{private::Ed25519Keypair, rand_core::OsRng, HashAlg}; let keypair = Ed25519Keypair::random(&mut OsRng); let session_id = [0x42u8; 32]; let bind_payload = make_session_bind_payload(&keypair, &session_id, is_forwarding); + let private_key = ssh_key::PrivateKey::new(ssh_key::private::KeypairData::Ed25519(keypair), "") + .expect("key generation not to fail."); + let fingerprint = private_key + .public_key() + .fingerprint(HashAlg::Sha256) + .to_string(); + let name = b"session-bind@openssh.com"; let mut msg = vec![27u8]; // SSH2_AGENTC_EXTENSION write_ssh_string(&mut msg, name); @@ -348,7 +401,7 @@ pub fn framed_session_bind_extension(is_forwarding: bool) -> Vec { let mut framed = (msg.len() as u32).to_be_bytes().to_vec(); framed.extend(msg); - framed + (framed, fingerprint) } /// Builds a framed EXTENSION message with the session-bind name but a garbage payload. diff --git a/apps/desktop/desktop_native/ssh_agent/tests/server_unix.rs b/apps/desktop/desktop_native/ssh_agent/tests/server_unix.rs index 61d80f754545..3671f8da1dd0 100644 --- a/apps/desktop/desktop_native/ssh_agent/tests/server_unix.rs +++ b/apps/desktop/desktop_native/ssh_agent/tests/server_unix.rs @@ -10,10 +10,11 @@ use common::{ agent_with_keys, always_approving_agent, always_denying_agent, framed_invalid_session_bind_extension, framed_request_identities, framed_session_bind_extension, framed_sign_request, init_tracing, parse_first_key_name, - parse_sign_response_algorithm, read_framed_response, test_ecdsa_p256_key, - test_ecdsa_p256_key_blob, test_ecdsa_p384_key, test_ecdsa_p384_key_blob, test_ecdsa_p521_key, - test_ecdsa_p521_key_blob, test_ed25519_key, test_ed25519_key_blob, test_rsa_key, - test_rsa_key_blob, unsupported_dsa_key_blob, MockApprovalRequester, + parse_key_names, parse_sign_response_algorithm, read_framed_response, + session_bind_extension_with_fingerprint, test_ecdsa_p256_key, test_ecdsa_p256_key_blob, + test_ecdsa_p384_key, test_ecdsa_p384_key_blob, test_ecdsa_p521_key, test_ecdsa_p521_key_blob, + test_ed25519_key, test_ed25519_key_blob, test_rsa_key, test_rsa_key_blob, + test_rsa_key_with_destinations, unsupported_dsa_key_blob, MockApprovalRequester, }; use ssh_agent::{BitwardenSSHAgent, InMemoryEncryptedKeyStore}; @@ -544,6 +545,159 @@ async fn test_session_bind_is_forwarding_reaches_approval_layer() { agent.stop(); } +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_filtered_by_bound_destination_host_fingerprint() { + setup(); + // "Test Key" (ed25519) is unrestricted, so it's always offered. "Restricted RSA Key" is + // restricted to a destination that never matches this connection's session-bind fingerprint, + // so it must be omitted once the connection is bound. + let mut agent = agent_with_keys(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Restricted RSA Key", &["SHA256:some-other-host"]), + ]); + agent.start().unwrap(); + + let mut stream = UnixStream::connect(test_socket_path()).await.unwrap(); + let (bind_frame, _fingerprint) = session_bind_extension_with_fingerprint(false); + stream.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut stream).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + // Session-bind and REQUEST_IDENTITIES must be sent over the same connection: the verified + // host fingerprint is per-connection state, not global to the agent. + stream + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut stream).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 1, "restricted key must be filtered out"); + assert_eq!(parse_first_key_name(&response), "Test Key"); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_includes_key_matching_bound_destination_host_fingerprint() { + setup(); + let mut agent = agent_with_keys(vec![test_ed25519_key()]); + agent.start().unwrap(); + + // Bind first so the matching fingerprint is known, then insert an RSA key restricted to it. + let mut stream = UnixStream::connect(test_socket_path()).await.unwrap(); + let (bind_frame, fingerprint) = session_bind_extension_with_fingerprint(false); + stream.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut stream).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + agent + .replace(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Matching RSA Key", &[fingerprint.as_str()]), + ]) + .unwrap(); + + stream + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut stream).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 2, "both unrestricted and matching keys are offered"); + assert_eq!( + parse_key_names(&response), + vec!["Matching RSA Key".to_string(), "Test Key".to_string()], + "the destination-matching key must be listed before the unrestricted key" + ); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_prioritizes_matching_destination_over_multiple_unrestricted_keys() { + setup(); + // Two unrestricted keys (distinct algorithms, so distinct public keys) are inserted before + // the destination-matching key, so a naive implementation that merely preserves + // insertion/keystore order (without prioritizing) would list the match last. It must still be + // listed first. + let mut agent = agent_with_keys(vec![test_ed25519_key()]); + agent.start().unwrap(); + + let mut stream = UnixStream::connect(test_socket_path()).await.unwrap(); + let (bind_frame, fingerprint) = session_bind_extension_with_fingerprint(false); + stream.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut stream).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + agent + .replace(vec![ + test_ecdsa_p256_key(), + test_ecdsa_p384_key(), + test_rsa_key_with_destinations("Matching Key", &[fingerprint.as_str()]), + ]) + .unwrap(); + + stream + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut stream).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let names = parse_key_names(&response); + assert_eq!(names.len(), 3, "no key should be dropped"); + assert_eq!( + names[0], "Matching Key", + "the destination-matching key must be listed first" + ); + assert_eq!( + names[1..].iter().collect::>(), + [ + "Test ECDSA P-256 Key".to_string(), + "Test ECDSA P-384 Key".to_string() + ] + .iter() + .collect::>(), + "both unrestricted keys must still be offered" + ); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_unbound_connection_returns_all_keys_regardless_of_destinations() { + setup(); + // A separate, unbound connection carries no session-bind state, so it must fall back to + // returning every key, including ones restricted to a specific destination — this is the same + // reason `ssh-add -L` always shows every identity regardless of the live SSH connection. + let mut agent = agent_with_keys(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Restricted RSA Key", &["SHA256:some-other-host"]), + ]); + agent.start().unwrap(); + + let mut stream = UnixStream::connect(test_socket_path()).await.unwrap(); + stream + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut stream).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 2, "unbound connection must see every key"); + + agent.stop(); +} + // The three tests below exercise the is_initialized() gating logic end-to-end through a real // socket connection. BitwardenSSHAgent::new() wires up BitwardenAuthPolicy internally, so passing // a MockApprovalRequester is sufficient — no manual policy construction is needed. diff --git a/apps/desktop/desktop_native/ssh_agent/tests/server_windows.rs b/apps/desktop/desktop_native/ssh_agent/tests/server_windows.rs index 6a1db827e801..5fde34e1d8c3 100644 --- a/apps/desktop/desktop_native/ssh_agent/tests/server_windows.rs +++ b/apps/desktop/desktop_native/ssh_agent/tests/server_windows.rs @@ -8,10 +8,11 @@ use common::{ agent_with_keys, always_approving_agent, always_denying_agent, framed_invalid_session_bind_extension, framed_request_identities, framed_session_bind_extension, framed_sign_request, init_tracing, parse_first_key_name, - parse_sign_response_algorithm, read_framed_response, test_ecdsa_p256_key, - test_ecdsa_p256_key_blob, test_ecdsa_p384_key, test_ecdsa_p384_key_blob, test_ecdsa_p521_key, - test_ecdsa_p521_key_blob, test_ed25519_key, test_ed25519_key_blob, test_rsa_key, - test_rsa_key_blob, unsupported_dsa_key_blob, MockApprovalRequester, + parse_key_names, parse_sign_response_algorithm, read_framed_response, + session_bind_extension_with_fingerprint, test_ecdsa_p256_key, test_ecdsa_p256_key_blob, + test_ecdsa_p384_key, test_ecdsa_p384_key_blob, test_ecdsa_p521_key, test_ecdsa_p521_key_blob, + test_ed25519_key, test_ed25519_key_blob, test_rsa_key, test_rsa_key_blob, + test_rsa_key_with_destinations, unsupported_dsa_key_blob, MockApprovalRequester, }; use ssh_agent::{BitwardenSSHAgent, InMemoryEncryptedKeyStore}; @@ -517,3 +518,156 @@ async fn test_session_bind_is_forwarding_reaches_approval_layer() { agent.stop(); } + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_filtered_by_bound_destination_host_fingerprint() { + setup(); + // "Test Key" (ed25519) is unrestricted, so it's always offered. "Restricted RSA Key" is + // restricted to a destination that never matches this connection's session-bind fingerprint, + // so it must be omitted once the connection is bound. + let mut agent = agent_with_keys(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Restricted RSA Key", &["SHA256:some-other-host"]), + ]); + agent.start().unwrap(); + + let mut client = ClientOptions::new().open(PIPE_NAME).unwrap(); + let (bind_frame, _fingerprint) = session_bind_extension_with_fingerprint(false); + client.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut client).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + // Session-bind and REQUEST_IDENTITIES must be sent over the same connection: the verified + // host fingerprint is per-connection state, not global to the agent. + client + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut client).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 1, "restricted key must be filtered out"); + assert_eq!(parse_first_key_name(&response), "Test Key"); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_includes_key_matching_bound_destination_host_fingerprint() { + setup(); + let mut agent = agent_with_keys(vec![test_ed25519_key()]); + agent.start().unwrap(); + + // Bind first so the matching fingerprint is known, then insert an RSA key restricted to it. + let mut client = ClientOptions::new().open(PIPE_NAME).unwrap(); + let (bind_frame, fingerprint) = session_bind_extension_with_fingerprint(false); + client.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut client).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + agent + .replace(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Matching RSA Key", &[fingerprint.as_str()]), + ]) + .unwrap(); + + client + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut client).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 2, "both unrestricted and matching keys are offered"); + assert_eq!( + parse_key_names(&response), + vec!["Matching RSA Key".to_string(), "Test Key".to_string()], + "the destination-matching key must be listed before the unrestricted key" + ); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_prioritizes_matching_destination_over_multiple_unrestricted_keys() { + setup(); + // Two unrestricted keys (distinct algorithms, so distinct public keys) are inserted before + // the destination-matching key, so a naive implementation that merely preserves + // insertion/keystore order (without prioritizing) would list the match last. It must still be + // listed first. + let mut agent = agent_with_keys(vec![test_ed25519_key()]); + agent.start().unwrap(); + + let mut client = ClientOptions::new().open(PIPE_NAME).unwrap(); + let (bind_frame, fingerprint) = session_bind_extension_with_fingerprint(false); + client.write_all(&bind_frame).await.unwrap(); + let bind_response = read_framed_response(&mut client).await; + assert_eq!(bind_response[0], 6, "session-bind should succeed"); + + agent + .replace(vec![ + test_ecdsa_p256_key(), + test_ecdsa_p384_key(), + test_rsa_key_with_destinations("Matching Key", &[fingerprint.as_str()]), + ]) + .unwrap(); + + client + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut client).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let names = parse_key_names(&response); + assert_eq!(names.len(), 3, "no key should be dropped"); + assert_eq!( + names[0], "Matching Key", + "the destination-matching key must be listed first" + ); + assert_eq!( + names[1..].iter().collect::>(), + [ + "Test ECDSA P-256 Key".to_string(), + "Test ECDSA P-384 Key".to_string() + ] + .iter() + .collect::>(), + "both unrestricted keys must still be offered" + ); + + agent.stop(); +} + +#[serial] +#[tokio::test(flavor = "multi_thread")] +async fn test_list_keys_unbound_connection_returns_all_keys_regardless_of_destinations() { + setup(); + // A separate, unbound connection carries no session-bind state, so it must fall back to + // returning every key, including ones restricted to a specific destination — this is the same + // reason `ssh-add -L` always shows every identity regardless of the live SSH connection. + let mut agent = agent_with_keys(vec![ + test_ed25519_key(), + test_rsa_key_with_destinations("Restricted RSA Key", &["SHA256:some-other-host"]), + ]); + agent.start().unwrap(); + + let mut client = ClientOptions::new().open(PIPE_NAME).unwrap(); + client + .write_all(&framed_request_identities()) + .await + .unwrap(); + let response = read_framed_response(&mut client).await; + + assert_eq!(response[0], 12, "expected IDENTITIES_ANSWER type byte"); + let count = u32::from_be_bytes(response[1..5].try_into().unwrap()); + assert_eq!(count, 2, "unbound connection must see every key"); + + agent.stop(); +} diff --git a/apps/desktop/src/app/services/services.module.ts b/apps/desktop/src/app/services/services.module.ts index 1160a4476b21..d8ed894d4951 100644 --- a/apps/desktop/src/app/services/services.module.ts +++ b/apps/desktop/src/app/services/services.module.ts @@ -145,6 +145,7 @@ import { UnlockService } from "@bitwarden/unlock"; import { CipherFormGenerationService, DefaultSshImportPromptService, + SshAgentDestinationSettingsService, SshImportPromptService, VaultFilterServiceAbstraction, VaultFilterService, @@ -162,6 +163,8 @@ import { DesktopAutofillService } from "../../autofill/services/desktop-autofill import { DesktopAutotypeMvpService } from "../../autofill/services/desktop-autotype-mvp.service"; import { DesktopAutotypeDefaultSettingPolicy } from "../../autofill/services/desktop-autotype-policy.service"; import { DesktopFido2UserInterfaceService } from "../../autofill/services/desktop-fido2-user-interface.service"; +import { DesktopSshAgentDestinationSettingsService } from "../../autofill/services/desktop-ssh-agent-destination-settings.service"; +import { SshAgentDestinationsService } from "../../autofill/services/ssh-agent-destinations.service"; import { DesktopBiometricsService } from "../../key-management/biometrics/desktop.biometrics.service"; import { RendererBiometricsService } from "../../key-management/biometrics/renderer-biometrics.service"; import { ElectronKeyService } from "../../key-management/electron-key.service"; @@ -424,6 +427,15 @@ const safeProviders: SafeProvider[] = [ provide: DesktopAutofillSettingsService, deps: [StateProvider], }), + safeProvider({ + provide: SshAgentDestinationsService, + deps: [StateProvider], + }), + safeProvider({ + provide: SshAgentDestinationSettingsService, + useClass: DesktopSshAgentDestinationSettingsService, + deps: [SshAgentDestinationsService], + }), safeProvider({ provide: DesktopAutofillService, deps: [ diff --git a/apps/desktop/src/autofill/main/main-ssh-agent.service.ts b/apps/desktop/src/autofill/main/main-ssh-agent.service.ts index 641cee079fcc..105b2ac025b9 100644 --- a/apps/desktop/src/autofill/main/main-ssh-agent.service.ts +++ b/apps/desktop/src/autofill/main/main-ssh-agent.service.ts @@ -162,14 +162,11 @@ export class MainSshAgentService { } private registerV2IpcHandlers() { - ipcMain.handle( - SSH_AGENT_IPC_CHANNELS.REPLACE, - async (_, keys: { name: string; privateKey: string; cipherId: string }[]) => { - if (this.agentStateV2 != null && this.agentStateV2.isRunning()) { - this.agentStateV2.replace(keys); - } - }, - ); + ipcMain.handle(SSH_AGENT_IPC_CHANNELS.REPLACE, async (_, keys: sshagent_v2.SshKeyData[]) => { + if (this.agentStateV2 != null && this.agentStateV2.isRunning()) { + this.agentStateV2.replace(keys); + } + }); ipcMain.handle( SSH_AGENT_IPC_CHANNELS.SIGN_REQUEST_RESPONSE, diff --git a/apps/desktop/src/autofill/preload.ts b/apps/desktop/src/autofill/preload.ts index 385a2174bf2a..e862624068f3 100644 --- a/apps/desktop/src/autofill/preload.ts +++ b/apps/desktop/src/autofill/preload.ts @@ -1,5 +1,7 @@ import { ipcRenderer } from "electron"; +import type { sshagent_v2 } from "@bitwarden/desktop-napi"; + import { DesktopAutofillPreload } from "./desktop-autofill.preload"; import { AutotypeConfig } from "./models/autotype-config"; import { AutotypeMatchError } from "./models/autotype-errors"; @@ -10,7 +12,9 @@ const sshAgent = { init: async (useV2: boolean) => { await ipcRenderer.invoke(SSH_AGENT_IPC_CHANNELS.INIT, { useV2 }); }, - replace: (keys: { name: string; privateKey: string; cipherId: string }[]): Promise => + // The V1 REPLACE handler shares this channel and ignores the extra destinationFingerprints + // field; only V2 (session-bind-aware destination filtering) reads it. + replace: (keys: sshagent_v2.SshKeyData[]): Promise => ipcRenderer.invoke(SSH_AGENT_IPC_CHANNELS.REPLACE, keys), signRequestResponse: async (requestId: number, accepted: boolean) => { await ipcRenderer.invoke(SSH_AGENT_IPC_CHANNELS.SIGN_REQUEST_RESPONSE, { requestId, accepted }); diff --git a/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.spec.ts b/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.spec.ts new file mode 100644 index 000000000000..5c97fad056da --- /dev/null +++ b/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.spec.ts @@ -0,0 +1,61 @@ +import { firstValueFrom, of } from "rxjs"; + +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { CipherId } from "@bitwarden/common/types/guid"; + +import { DesktopSshAgentDestinationSettingsService } from "./desktop-ssh-agent-destination-settings.service"; +import { SshAgentDestinationsService } from "./ssh-agent-destinations.service"; + +describe("DesktopSshAgentDestinationSettingsService", () => { + let service: DesktopSshAgentDestinationSettingsService; + let mockSshAgentDestinationsService: { + destinationFingerprints$: ReturnType; + setDestinationFingerprints: jest.Mock; + }; + + const cipherId = Utils.newGuid() as CipherId; + + beforeEach(() => { + mockSshAgentDestinationsService = { + destinationFingerprints$: of({}), + setDestinationFingerprints: jest.fn().mockResolvedValue(undefined), + }; + + service = new DesktopSshAgentDestinationSettingsService( + mockSshAgentDestinationsService as unknown as SshAgentDestinationsService, + ); + }); + + describe("destinationFingerprints$", () => { + it("projects the fingerprints configured for the given cipher", async () => { + const otherId = Utils.newGuid() as CipherId; + mockSshAgentDestinationsService.destinationFingerprints$ = of({ + [cipherId]: ["SHA256:aaaa"], + [otherId]: ["SHA256:bbbb"], + }); + + const result = await firstValueFrom(service.destinationFingerprints$(cipherId)); + + expect(result).toEqual(["SHA256:aaaa"]); + }); + + it("emits an empty array when the cipher has no configured fingerprints", async () => { + mockSshAgentDestinationsService.destinationFingerprints$ = of({}); + + const result = await firstValueFrom(service.destinationFingerprints$(cipherId)); + + expect(result).toEqual([]); + }); + }); + + describe("setDestinationFingerprints", () => { + it("delegates to the underlying service", async () => { + await service.setDestinationFingerprints(cipherId, ["SHA256:aaaa"]); + + expect(mockSshAgentDestinationsService.setDestinationFingerprints).toHaveBeenCalledWith( + cipherId, + ["SHA256:aaaa"], + ); + }); + }); +}); diff --git a/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.ts b/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.ts new file mode 100644 index 000000000000..ac676dbc9d5f --- /dev/null +++ b/apps/desktop/src/autofill/services/desktop-ssh-agent-destination-settings.service.ts @@ -0,0 +1,28 @@ +import { map, Observable } from "rxjs"; + +import { CipherId } from "@bitwarden/common/types/guid"; +import { SshAgentDestinationSettingsService } from "@bitwarden/vault"; + +import { SshAgentDestinationsService } from "./ssh-agent-destinations.service"; + +/** + * Desktop implementation of {@link SshAgentDestinationSettingsService}, backed by the existing + * {@link SshAgentDestinationsService} local-state service. This is the only concrete provider of + * the abstraction today — other clients don't provide it, so the shared cipher form UI stays + * hidden there. + */ +export class DesktopSshAgentDestinationSettingsService extends SshAgentDestinationSettingsService { + constructor(private sshAgentDestinationsService: SshAgentDestinationsService) { + super(); + } + + destinationFingerprints$(cipherId: CipherId): Observable { + return this.sshAgentDestinationsService.destinationFingerprints$.pipe( + map((all) => all[cipherId] ?? []), + ); + } + + setDestinationFingerprints(cipherId: CipherId, fingerprints: string[]): Promise { + return this.sshAgentDestinationsService.setDestinationFingerprints(cipherId, fingerprints); + } +} diff --git a/apps/desktop/src/autofill/services/ssh-agent-destinations.service.spec.ts b/apps/desktop/src/autofill/services/ssh-agent-destinations.service.spec.ts new file mode 100644 index 000000000000..bfd137764657 --- /dev/null +++ b/apps/desktop/src/autofill/services/ssh-agent-destinations.service.spec.ts @@ -0,0 +1,60 @@ +import { firstValueFrom } from "rxjs"; + +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { FakeStateProvider, mockAccountServiceWith } from "@bitwarden/common/spec"; +import { CipherId, UserId } from "@bitwarden/common/types/guid"; + +import { SshAgentDestinationsService } from "./ssh-agent-destinations.service"; + +describe("SshAgentDestinationsService", () => { + let service: SshAgentDestinationsService; + let stateProvider: FakeStateProvider; + + const userId = Utils.newGuid() as UserId; + const cipherId = Utils.newGuid() as CipherId; + + beforeEach(() => { + stateProvider = new FakeStateProvider(mockAccountServiceWith(userId)); + service = new SshAgentDestinationsService(stateProvider); + }); + + it("defaults to an empty record when nothing has been set", async () => { + const result = await firstValueFrom(service.destinationFingerprints$); + expect(result).toEqual({}); + }); + + it("stores fingerprints set for a cipher and makes them observable", async () => { + await service.setDestinationFingerprints(cipherId, ["SHA256:aaaa", "SHA256:bbbb"]); + + const result = await firstValueFrom(service.destinationFingerprints$); + expect(result).toEqual({ [cipherId]: ["SHA256:aaaa", "SHA256:bbbb"] }); + }); + + it("preserves fingerprints for other ciphers when updating one cipher", async () => { + const otherCipherId = Utils.newGuid() as CipherId; + await service.setDestinationFingerprints(otherCipherId, ["SHA256:other"]); + await service.setDestinationFingerprints(cipherId, ["SHA256:mine"]); + + const result = await firstValueFrom(service.destinationFingerprints$); + expect(result).toEqual({ + [otherCipherId]: ["SHA256:other"], + [cipherId]: ["SHA256:mine"], + }); + }); + + it("overwrites previous fingerprints when set again for the same cipher", async () => { + await service.setDestinationFingerprints(cipherId, ["SHA256:old"]); + await service.setDestinationFingerprints(cipherId, ["SHA256:new"]); + + const result = await firstValueFrom(service.destinationFingerprints$); + expect(result).toEqual({ [cipherId]: ["SHA256:new"] }); + }); + + it("removes the cipher entry when set to an empty array", async () => { + await service.setDestinationFingerprints(cipherId, ["SHA256:aaaa"]); + await service.setDestinationFingerprints(cipherId, []); + + const result = await firstValueFrom(service.destinationFingerprints$); + expect(result).toEqual({}); + }); +}); diff --git a/apps/desktop/src/autofill/services/ssh-agent-destinations.service.ts b/apps/desktop/src/autofill/services/ssh-agent-destinations.service.ts new file mode 100644 index 000000000000..f5e286fb6e9b --- /dev/null +++ b/apps/desktop/src/autofill/services/ssh-agent-destinations.service.ts @@ -0,0 +1,46 @@ +import { map } from "rxjs"; + +import { + AUTOFILL_SETTINGS_DISK, + StateProvider, + UserKeyDefinition, +} from "@bitwarden/common/platform/state"; +import { CipherId } from "@bitwarden/common/types/guid"; + +const SSH_AGENT_DESTINATION_FINGERPRINTS = UserKeyDefinition.record( + AUTOFILL_SETTINGS_DISK, + "sshAgentDestinationFingerprints", + { + deserializer: (fingerprints: string[]) => fingerprints, + clearOn: [], + }, +); + +/** + * Desktop-local, per-cipher SSH-agent destination host-key fingerprints. + * + * When a key has one or more configured fingerprints, the native SSH agent only offers it for + * connections whose verified `session-bind@openssh.com` destination host key matches one of them. + * This preference is never synced — it exists only on this Desktop installation. + */ +export class SshAgentDestinationsService { + private state = this.stateProvider.getActive(SSH_AGENT_DESTINATION_FINGERPRINTS); + + destinationFingerprints$ = this.state.state$.pipe(map((value) => value ?? {})); + + constructor(private stateProvider: StateProvider) {} + + async setDestinationFingerprints(cipherId: CipherId, fingerprints: string[]): Promise { + await this.state.update((current) => { + const updated = { ...(current ?? {}) }; + + if (fingerprints.length === 0) { + delete updated[cipherId]; + } else { + updated[cipherId] = fingerprints; + } + + return updated; + }); + } +} diff --git a/apps/desktop/src/autofill/services/ssh-agent.service.spec.ts b/apps/desktop/src/autofill/services/ssh-agent.service.spec.ts index afce4b1eb447..a1f74909c215 100644 --- a/apps/desktop/src/autofill/services/ssh-agent.service.spec.ts +++ b/apps/desktop/src/autofill/services/ssh-agent.service.spec.ts @@ -1,7 +1,7 @@ import { BehaviorSubject, EMPTY, Subject, of } from "rxjs"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; -import { UserId } from "@bitwarden/common/types/guid"; +import { CipherId, UserId } from "@bitwarden/common/types/guid"; import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; @@ -38,6 +38,7 @@ describe("SshAgentService", () => { let accountSubject: BehaviorSubject<{ id: UserId } | null>; let enabledSubject: BehaviorSubject; let cipherViewsSubject: BehaviorSubject; + let destinationsSubject: BehaviorSubject>; let authStatusPerUser: Map>; let mockIsLoaded: jest.Mock; @@ -59,6 +60,7 @@ describe("SshAgentService", () => { accountSubject = new BehaviorSubject<{ id: UserId } | null>(null); enabledSubject = new BehaviorSubject(false); cipherViewsSubject = new BehaviorSubject(null); + destinationsSubject = new BehaviorSubject>({}); authStatusPerUser = new Map(); mockIsLoaded = jest.fn().mockResolvedValue(false); @@ -106,6 +108,10 @@ describe("SshAgentService", () => { }; const mockAccountService = { activeAccount$: accountSubject.asObservable() }; const mockConfigService = { getFeatureFlag: jest.fn().mockResolvedValue(true) }; + const mockSshAgentDestinationsService = { + destinationFingerprints$: destinationsSubject.asObservable(), + setDestinationFingerprints: jest.fn().mockResolvedValue(undefined), + }; service = new SshAgentService( mockCipherService as any, @@ -118,6 +124,7 @@ describe("SshAgentService", () => { mockDesktopSettingsService as any, mockAccountService as any, mockConfigService as any, + mockSshAgentDestinationsService as any, ); await service.init(); @@ -137,7 +144,7 @@ describe("SshAgentService", () => { expect(mockInit).toHaveBeenCalledWith(true); expect(mockReplace).toHaveBeenCalledWith([ - { name: "My Key", privateKey: "pem", cipherId: "c1" }, + { name: "My Key", privateKey: "pem", cipherId: "c1", destinationFingerprints: [] }, ]); }); @@ -219,7 +226,7 @@ describe("SshAgentService", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "User2 Key", privateKey: "pem2", cipherId: "c2" }, + { name: "User2 Key", privateKey: "pem2", cipherId: "c2", destinationFingerprints: [] }, ]); }); @@ -259,8 +266,8 @@ describe("SshAgentService", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "Key A", privateKey: "pem1", cipherId: "c1" }, - { name: "Key B", privateKey: "pem2", cipherId: "c2" }, + { name: "Key A", privateKey: "pem1", cipherId: "c1", destinationFingerprints: [] }, + { name: "Key B", privateKey: "pem2", cipherId: "c2", destinationFingerprints: [] }, ]); }); @@ -280,7 +287,7 @@ describe("SshAgentService", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "Key A", privateKey: "pem1", cipherId: "c1" }, + { name: "Key A", privateKey: "pem1", cipherId: "c1", destinationFingerprints: [] }, ]); }); @@ -303,7 +310,7 @@ describe("SshAgentService", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "Key A", privateKey: "pem1", cipherId: "c1" }, + { name: "Key A", privateKey: "pem1", cipherId: "c1", destinationFingerprints: [] }, ]); }); @@ -335,7 +342,7 @@ describe("SshAgentService", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "New Name", privateKey: "pem1", cipherId: "c1" }, + { name: "New Name", privateKey: "pem1", cipherId: "c1", destinationFingerprints: [] }, ]); }); @@ -448,7 +455,7 @@ describe("SshAgentService", () => { expect(mockReplace).toHaveBeenCalledTimes(2); expect(mockReplace).toHaveBeenLastCalledWith([ - { name: "Renamed", privateKey: "pem", cipherId: "c1" }, + { name: "Renamed", privateKey: "pem", cipherId: "c1", destinationFingerprints: [] }, ]); }); @@ -514,7 +521,47 @@ describe("SshAgentService", () => { await flush(); await flush(); - expect(mockReplace).toHaveBeenCalledWith([{ name: "Key", privateKey: "pem", cipherId: "c1" }]); + expect(mockReplace).toHaveBeenCalledWith([ + { name: "Key", privateKey: "pem", cipherId: "c1", destinationFingerprints: [] }, + ]); + }); + + it("when a cipher's destination fingerprints change with no other change, re-pushes keys", async () => { + enabledSubject.next(true); + accountSubject.next({ id: "user-1" as UserId }); + cipherViewsSubject.next([makeSshCipher("c1", "Key", "pem")]); + authSubjectFor("user-1").next(AuthenticationStatus.Unlocked); + await flush(); + + mockReplace.mockClear(); + + destinationsSubject.next({ c1: ["SHA256:host"] } as Record); + await flush(); + + expect(mockReplace).toHaveBeenCalledWith([ + { + name: "Key", + privateKey: "pem", + cipherId: "c1", + destinationFingerprints: ["SHA256:host"], + }, + ]); + }); + + it("when destination fingerprints are unchanged, does not re-push keys", async () => { + destinationsSubject.next({ c1: ["SHA256:host"] } as Record); + enabledSubject.next(true); + accountSubject.next({ id: "user-1" as UserId }); + cipherViewsSubject.next([makeSshCipher("c1", "Key", "pem")]); + authSubjectFor("user-1").next(AuthenticationStatus.Unlocked); + await flush(); + + mockReplace.mockClear(); + + destinationsSubject.next({ c1: ["SHA256:host"] } as Record); + await flush(); + + expect(mockReplace).not.toHaveBeenCalled(); }); }); @@ -573,6 +620,7 @@ describe("SshAgentService – sign request authorization", () => { } as any, { activeAccount$: accountSubject.asObservable() } as any, { getFeatureFlag: jest.fn().mockResolvedValue(true) } as any, + { destinationFingerprints$: of({}) } as any, ); await service.init(); @@ -830,6 +878,7 @@ describe("SshAgentService – list keys request", () => { } as any, { activeAccount$: accountSubject.asObservable() } as any, { getFeatureFlag: jest.fn().mockResolvedValue(true) } as any, + { destinationFingerprints$: of({}) } as any, ); await service.init(); @@ -849,7 +898,7 @@ describe("SshAgentService – list keys request", () => { await flush(); expect(mockReplace).toHaveBeenCalledWith([ - { name: "My Key", privateKey: "pem", cipherId: "c1" }, + { name: "My Key", privateKey: "pem", cipherId: "c1", destinationFingerprints: [] }, ]); expect(mockListRequestResponse).toHaveBeenCalledWith(LIST_REQUEST_ID, true); }); @@ -983,6 +1032,7 @@ describe("SshAgentService – concurrent sign requests", () => { } as any, { activeAccount$: of({ id: "user-1" as UserId }) } as any, { getFeatureFlag: jest.fn().mockResolvedValue(true) } as any, + { destinationFingerprints$: of({}) } as any, ); await service.init(); @@ -1089,6 +1139,7 @@ describe("SshAgentService – concurrent list keys requests", () => { } as any, { activeAccount$: of({ id: "user-1" as UserId }) } as any, { getFeatureFlag: jest.fn().mockResolvedValue(true) } as any, + { destinationFingerprints$: of({}) } as any, ); await service.init(); diff --git a/apps/desktop/src/autofill/services/ssh-agent.service.ts b/apps/desktop/src/autofill/services/ssh-agent.service.ts index 5771063ae831..30275f1553bd 100644 --- a/apps/desktop/src/autofill/services/ssh-agent.service.ts +++ b/apps/desktop/src/autofill/services/ssh-agent.service.ts @@ -30,7 +30,7 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { CommandDefinition, MessageListener } from "@bitwarden/common/platform/messaging"; -import { UserId } from "@bitwarden/common/types/guid"; +import { CipherId, UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; @@ -41,6 +41,12 @@ import { ApproveSshRequestComponent } from "../components/approve-ssh-request"; import { SSH_AGENT_IPC_CHANNELS } from "../models/ipc-channels"; import { SshAgentPromptType } from "../models/ssh-agent-setting"; +import { SshAgentDestinationsService } from "./ssh-agent-destinations.service"; + +function fingerprintsEqual(a: string[] | undefined, b: string[]): boolean { + return (a ?? []).length === b.length && (a ?? []).every((fp, i) => fp === b[i]); +} + @Injectable({ providedIn: "root", }) @@ -66,6 +72,7 @@ export class SshAgentService implements OnDestroy { private desktopSettingsService: DesktopSettingsService, private accountService: AccountService, private configService: ConfigService, + private sshAgentDestinationsService: SshAgentDestinationsService, ) {} async init() { @@ -174,7 +181,10 @@ export class SshAgentService implements OnDestroy { // V1, delete with PM-30758: isListRequest is not present in v2. if (isListRequest) { - await ipc.autofill.sshAgent.replace(this.toAgentKeys(ciphers)); + const destinations = await firstValueFrom( + this.sshAgentDestinationsService.destinationFingerprints$, + ); + await ipc.autofill.sshAgent.replace(this.toAgentKeys(ciphers, destinations)); await ipc.autofill.sshAgent.signRequestResponse(requestId, true); return; } @@ -341,7 +351,10 @@ export class SshAgentService implements OnDestroy { concatMap(async ([message, ciphers]) => { const requestId = message.requestId as number; try { - await ipc.autofill.sshAgent.replace(this.toAgentKeys(ciphers ?? [])); + const destinations = await firstValueFrom( + this.sshAgentDestinationsService.destinationFingerprints$, + ); + await ipc.autofill.sshAgent.replace(this.toAgentKeys(ciphers ?? [], destinations)); } catch (e) { // Refuse the request rather than leaving the agent's list callback unresolved, which // would hang the SSH client that is waiting on it. @@ -393,12 +406,19 @@ export class SshAgentService implements OnDestroy { // When locked, cipherViews$ emits null (caught by the filter below), // so replace() is not called and existing keys are left in the native store. return from(this.ensureAgentRunning(useV2)).pipe( - // Subscribe to live cipher data for the active account. - switchMap(() => this.cipherService.cipherViews$(account.id)), + // Subscribe to live cipher data and destination preferences for the active + // account. Combined so that a destination-only change (no cipher change) still + // triggers a re-push. + switchMap(() => + combineLatest([ + this.cipherService.cipherViews$(account.id), + this.sshAgentDestinationsService.destinationFingerprints$, + ]), + ), // Skip emissions before cipher data is available (e.g. during initial decrypt). - filter((views) => views != null), + filter(([views]) => views != null), // Project to the SSH key fields needed by the agent. - map((views) => this.toAgentKeys(views)), + map(([views, destinations]) => this.toAgentKeys(views, destinations)), // Skip re-push when the SSH key set hasn't actually changed. distinctUntilChanged((prev, curr) => { // if the length is different, replace keys @@ -406,12 +426,23 @@ export class SshAgentService implements OnDestroy { return false; } const prevMap = new Map( - prev.map((k) => [k.cipherId, { privateKey: k.privateKey, name: k.name }]), + prev.map((k) => [ + k.cipherId, + { + privateKey: k.privateKey, + name: k.name, + destinationFingerprints: k.destinationFingerprints, + }, + ]), ); - // if any has either private key changed or the name changed, replace keys + // if the private key, name, or destination fingerprints changed, replace keys return curr.every((k) => { const p = prevMap.get(k.cipherId); - return p?.privateKey === k.privateKey && p?.name === k.name; + return ( + p?.privateKey === k.privateKey && + p?.name === k.name && + fingerprintsEqual(p?.destinationFingerprints, k.destinationFingerprints) + ); }); }), concatMap(async (keys) => { @@ -475,10 +506,16 @@ export class SshAgentService implements OnDestroy { private toAgentKeys( ciphers: CipherView[], - ): { name: string; privateKey: string; cipherId: string }[] { + destinations: Record = {}, + ): { name: string; privateKey: string; cipherId: string; destinationFingerprints: string[] }[] { return ciphers .filter((c) => c.type === CipherType.SshKey && !c.isDeleted && !c.isArchived) - .map((c) => ({ name: c.name, privateKey: c.sshKey.privateKey, cipherId: c.id })); + .map((c) => ({ + name: c.name, + privateKey: c.sshKey.privateKey, + cipherId: c.id, + destinationFingerprints: destinations[c.id as CipherId] ?? [], + })); } private async rememberAuthorization( diff --git a/apps/desktop/src/locales/en/messages.json b/apps/desktop/src/locales/en/messages.json index edf9392c86a3..44245d709566 100644 --- a/apps/desktop/src/locales/en/messages.json +++ b/apps/desktop/src/locales/en/messages.json @@ -641,6 +641,21 @@ "sshFingerprint": { "message": "Fingerprint" }, + "sshAgentDestinations": { + "message": "SSH agent destinations" + }, + "sshAgentDestinationsDescription": { + "message": "When destination information is available, only offer this key for these SSH host key fingerprints. Leave empty to offer this key for any destination." + }, + "sshAgentDestinationFingerprint": { + "message": "SSH host key fingerprint" + }, + "addSshAgentDestination": { + "message": "Add destination" + }, + "invalidSshAgentDestinationFingerprint": { + "message": "Enter a valid SHA256 host key fingerprint, starting with \"SHA256:\"" + }, "sshKeyAlgorithm": { "message": "Key type" }, diff --git a/libs/vault/src/cipher-form/abstractions/ssh-agent-destination-settings.service.ts b/libs/vault/src/cipher-form/abstractions/ssh-agent-destination-settings.service.ts new file mode 100644 index 000000000000..a3e24d46f74b --- /dev/null +++ b/libs/vault/src/cipher-form/abstractions/ssh-agent-destination-settings.service.ts @@ -0,0 +1,28 @@ +import { Observable } from "rxjs"; + +import { CipherId } from "@bitwarden/common/types/guid"; + +/** + * Optional, platform-specific settings for SSH-agent destination host-key fingerprints. + * + * Not every client provides this — it is currently implemented on Desktop only, backed by + * client-local storage. Consumers must inject it with `@Optional()` and treat `undefined` as + * "this platform doesn't support destination filtering," not as an error. + * + * This is an identity-offering optimization, not an authorization or security boundary: it + * restricts which stored SSH keys the agent *offers* for a given destination, it does not + * authorize or block a server from being connected to. + */ +export abstract class SshAgentDestinationSettingsService { + /** + * The destination host-key fingerprints currently configured for the given cipher. + * Emits an empty array when the key is unrestricted (offered for any destination). + */ + abstract destinationFingerprints$(cipherId: CipherId): Observable; + + /** + * Replaces the destination host-key fingerprints configured for the given cipher. + * Passing an empty array clears any restriction, making the key unrestricted again. + */ + abstract setDestinationFingerprints(cipherId: CipherId, fingerprints: string[]): Promise; +} diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index 4c646908acc7..a7628027f031 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -29,6 +29,12 @@ export type CipherForm = { cardDetails?: CardDetailsSectionComponent["cardDetailsForm"]; identityDetails?: IdentitySectionComponent["identityForm"]; sshKeyDetails?: SshKeySectionComponent["sshKeyForm"]; + /** + * Only participates in the parent form's validity — an invalid destination fingerprint blocks + * Save. Values here are never patched into the cipher; see + * {@link SshKeySectionComponent.destinationsForm}. + */ + sshAgentDestinations?: SshKeySectionComponent["destinationsForm"]; bankAccountDetails?: BankAccountSectionComponent["bankAccountForm"]; driversLicenseDetails?: DriversLicenseSectionComponent["driversLicenseForm"]; passportDetails?: PassportSectionComponent["passportForm"]; @@ -89,4 +95,12 @@ export abstract class CipherFormContainer { * This can be used for child forms to react to changes in the form status. */ formStatusChange$: Observable<"enabled" | "disabled">; + + /** + * Emits the saved {@link CipherView} after a successful save, including for a newly-created + * cipher (which has no `id` until this point). Child forms that need to persist data keyed by + * cipher ID, but that isn't part of the synced cipher itself, can use this to defer persistence + * until an ID exists. + */ + abstract readonly cipherSaved$: Observable; } diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.spec.ts b/libs/vault/src/cipher-form/components/cipher-form.component.spec.ts index e1dd9c54834b..8f2d67eb5f39 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.spec.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.spec.ts @@ -97,6 +97,19 @@ describe("CipherFormComponent", () => { expect(mockCipherArchiveService.userCanArchive$).toHaveBeenCalledWith("user-id"); }); + it("cipherSaved$ emits the same saved cipher as the cipherSaved output", async () => { + const savedCipher = new CipherView(); + savedCipher.id = "saved-cipher-id"; + mockAddEditFormService.saveCipher = jest.fn().mockResolvedValue(savedCipher); + + const cipherSaved$Spy = jest.fn(); + component.cipherSaved$.subscribe(cipherSaved$Spy); + + await component.submit(); + + expect(cipherSaved$Spy).toHaveBeenCalledWith(savedCipher); + }); + it("shows an error toast and aborts when the policy applies but there are no eligible orgs", async () => { component.config = { mode: "add", diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.ts b/libs/vault/src/cipher-form/components/cipher-form.component.ts index 0330403d7928..e59521a861f8 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -129,6 +129,9 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref @Output() cipherSaved = new EventEmitter(); + /** {@inheritDoc CipherFormContainer.cipherSaved$} */ + readonly cipherSaved$ = this.cipherSaved.asObservable(); + private formReadySubject = new Subject(); // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals diff --git a/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.spec.ts b/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.spec.ts new file mode 100644 index 000000000000..552154774ed0 --- /dev/null +++ b/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.spec.ts @@ -0,0 +1,61 @@ +import { FormControl } from "@angular/forms"; + +import { + normalizeSshAgentDestinationFingerprints, + sshAgentDestinationFingerprintValidator, +} from "./ssh-agent-destination-fingerprint.util"; + +describe("sshAgentDestinationFingerprintValidator", () => { + const ERROR_MESSAGE = "Enter a valid SHA256 host key fingerprint"; + const validator = sshAgentDestinationFingerprintValidator(ERROR_MESSAGE); + + it("returns null for an empty value", () => { + expect(validator(new FormControl(""))).toBeNull(); + }); + + it("returns null for a whitespace-only value", () => { + expect(validator(new FormControl(" "))).toBeNull(); + }); + + it("returns null for a value with the SHA256: prefix", () => { + expect(validator(new FormControl("SHA256:abcd1234"))).toBeNull(); + }); + + it("returns the given message for a value missing the SHA256: prefix", () => { + expect(validator(new FormControl("abcd1234"))).toEqual({ + invalidSshAgentDestinationFingerprint: { message: ERROR_MESSAGE }, + }); + }); + + it("returns the given message for a value with a different digest prefix", () => { + expect(validator(new FormControl("MD5:abcd1234"))).toEqual({ + invalidSshAgentDestinationFingerprint: { message: ERROR_MESSAGE }, + }); + }); +}); + +describe("normalizeSshAgentDestinationFingerprints", () => { + it("trims leading and trailing whitespace", () => { + expect(normalizeSshAgentDestinationFingerprints([" SHA256:aaaa "])).toEqual(["SHA256:aaaa"]); + }); + + it("drops empty and whitespace-only values", () => { + expect(normalizeSshAgentDestinationFingerprints(["SHA256:aaaa", "", " "])).toEqual([ + "SHA256:aaaa", + ]); + }); + + it("removes exact duplicates, keeping the first occurrence's position", () => { + expect( + normalizeSshAgentDestinationFingerprints(["SHA256:aaaa", "SHA256:bbbb", "SHA256:aaaa"]), + ).toEqual(["SHA256:aaaa", "SHA256:bbbb"]); + }); + + it("returns an empty array for an all-empty input", () => { + expect(normalizeSshAgentDestinationFingerprints(["", " ", null, undefined])).toEqual([]); + }); + + it("returns an empty array for an empty input", () => { + expect(normalizeSshAgentDestinationFingerprints([])).toEqual([]); + }); +}); diff --git a/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.ts b/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.ts new file mode 100644 index 000000000000..e1956116ce22 --- /dev/null +++ b/libs/vault/src/cipher-form/components/sshkey-section/ssh-agent-destination-fingerprint.util.ts @@ -0,0 +1,41 @@ +import { AbstractControl, ValidationErrors, ValidatorFn } from "@angular/forms"; + +const SHA256_PREFIX = "SHA256:"; + +/** + * Builds a validator for a single destination host-key fingerprint control. + * + * An empty value is valid — it represents a row the user hasn't finished filling in yet, and is + * dropped by {@link normalizeSshAgentDestinationFingerprints} before persistence, not treated as + * a real (empty) fingerprint. + * + * `errorMessage` is provided by the caller (typically `I18nService.t(...)`) rather than hard-coded + * here, so this file — and the rest of `libs/vault` — never bakes in an English string. The + * `{ message }` shape is what `BitErrorComponent`'s fallback rendering expects for error keys it + * doesn't recognize natively, letting `bit-form-field` render the error automatically. + */ +export function sshAgentDestinationFingerprintValidator(errorMessage: string): ValidatorFn { + return (control: AbstractControl): ValidationErrors | null => { + const value = (control.value ?? "").trim(); + if (value.length === 0) { + return null; + } + + return value.startsWith(SHA256_PREFIX) + ? null + : { invalidSshAgentDestinationFingerprint: { message: errorMessage } }; + }; +} + +/** + * Normalizes raw destination host-key fingerprint form values before persistence: + * trims whitespace, drops empty entries, and removes exact duplicates (first occurrence wins). + * + * Assumes every non-empty value has already passed {@link sshAgentDestinationFingerprintValidator}. + */ +export function normalizeSshAgentDestinationFingerprints( + values: (string | null | undefined)[], +): string[] { + const trimmed = values.map((value) => (value ?? "").trim()).filter((value) => value.length > 0); + return Array.from(new Set(trimmed)); +} diff --git a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.html b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.html index c90ab29fe29a..ff7060067cb6 100644 --- a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.html +++ b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.html @@ -38,3 +38,53 @@

+ +@if (showDestinationSettings) { +
+ +

+ {{ "sshAgentDestinations" | i18n }} +

+ + +

+ {{ "sshAgentDestinationsDescription" | i18n }} +

+
+ @for (control of fingerprints.controls; track $index; let i = $index) { + + {{ "sshAgentDestinationFingerprint" | i18n }} + + + + } +
+ + + +
+} diff --git a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.spec.ts b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.spec.ts index 289ca370477e..546f5d3a593a 100644 --- a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.spec.ts +++ b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.spec.ts @@ -1,5 +1,6 @@ import { NO_ERRORS_SCHEMA } from "@angular/core"; import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { AbstractControl, FormGroup } from "@angular/forms"; import { By } from "@angular/platform-browser"; import { mock } from "jest-mock-extended"; import { BehaviorSubject, Subject } from "rxjs"; @@ -8,10 +9,12 @@ import { ClientType } from "@bitwarden/common/enums"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; +import { CipherId } from "@bitwarden/common/types/guid"; import { SshKeyView } from "@bitwarden/common/vault/models/view/ssh-key.view"; import { generate_ssh_key } from "@bitwarden/sdk-internal"; import { SshImportPromptService } from "../../../services/ssh-import-prompt.service"; +import { SshAgentDestinationSettingsService } from "../../abstractions/ssh-agent-destination-settings.service"; import { CipherFormContainer } from "../../cipher-form-container"; import { SshKeySectionComponent } from "./sshkey-section.component"; @@ -230,4 +233,272 @@ describe("SshKeySectionComponent", () => { expect(component.sshKeyForm.get("publicKey")?.value).toBe("b"); expect(component.sshKeyForm.get("keyFingerprint")?.value).toBe("c"); }); + + it("does not render the SSH agent destinations section when the optional service is unavailable", async () => { + platformUtilsService.getClientType.mockReturnValue(ClientType.Desktop); + (generate_ssh_key as unknown as jest.Mock).mockReturnValue({ + privateKey: "genPriv", + publicKey: "genPub", + fingerprint: "genFp", + }); + + await component.ngOnInit(); + fixture.detectChanges(); + + expect(component.showDestinationSettings).toBe(false); + expect(fixture.debugElement.query(By.css('[data-testid="add-destination-button"]'))).toBeNull(); + }); +}); + +describe("SshKeySectionComponent - SSH agent destinations", () => { + const CIPHER_ID = "cipher-1" as CipherId; + + let fixture: ComponentFixture; + let component: SshKeySectionComponent; + const mockI18nService = mock(); + + let cipherFormContainer: { + registerChildForm: jest.Mock; + patchCipher: jest.Mock; + getInitialCipherView: jest.Mock; + formStatusChange$: Subject; + cipherSaved$: Subject<{ id: string; sshKey?: unknown }>; + }; + + let destinationFingerprintsSubject: BehaviorSubject; + let destinationSettings: { + destinationFingerprints$: jest.Mock; + setDestinationFingerprints: jest.Mock; + }; + + let sdkService: { client$: BehaviorSubject }; + let sshImportPromptService: { importSshKeyFromClipboard: jest.Mock }; + let platformUtilsService: { getClientType: jest.Mock }; + + async function setup(originalCipherView: { edit: boolean; id?: string; sshKey: null } | null) { + (generate_ssh_key as unknown as jest.Mock).mockReturnValue({ + privateKey: "genPriv", + publicKey: "genPub", + fingerprint: "genFp", + }); + + await TestBed.configureTestingModule({ + imports: [SshKeySectionComponent], + providers: [ + { provide: I18nService, useValue: mockI18nService }, + { provide: CipherFormContainer, useValue: cipherFormContainer }, + { provide: SdkService, useValue: sdkService }, + { provide: SshImportPromptService, useValue: sshImportPromptService }, + { provide: PlatformUtilsService, useValue: platformUtilsService }, + { provide: SshAgentDestinationSettingsService, useValue: destinationSettings }, + ], + schemas: [NO_ERRORS_SCHEMA], + }).compileComponents(); + + fixture = TestBed.createComponent(SshKeySectionComponent); + component = fixture.componentInstance; + fixture.componentRef.setInput("originalCipherView", originalCipherView); + + await component.ngOnInit(); + fixture.detectChanges(); + } + + beforeEach(() => { + (generate_ssh_key as unknown as jest.Mock).mockReset(); + mockI18nService.t.mockImplementation((key: string) => key); + + cipherFormContainer = { + registerChildForm: jest.fn(), + patchCipher: jest.fn(), + getInitialCipherView: jest.fn().mockReturnValue(null), + formStatusChange$: new Subject(), + cipherSaved$: new Subject(), + }; + + destinationFingerprintsSubject = new BehaviorSubject([]); + destinationSettings = { + destinationFingerprints$: jest.fn().mockReturnValue(destinationFingerprintsSubject), + setDestinationFingerprints: jest.fn().mockResolvedValue(undefined), + }; + + sdkService = { client$: new BehaviorSubject({}) }; + sshImportPromptService = { importSshKeyFromClipboard: jest.fn() }; + platformUtilsService = { getClientType: jest.fn().mockReturnValue(ClientType.Desktop) }; + }); + + it("renders the section and loads configured fingerprints for an existing cipher", async () => { + destinationFingerprintsSubject.next(["SHA256:aaaa", "SHA256:bbbb"]); + + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + expect(destinationSettings.destinationFingerprints$).toHaveBeenCalledWith(CIPHER_ID); + expect(component.fingerprints.value).toEqual(["SHA256:aaaa", "SHA256:bbbb"]); + expect( + fixture.debugElement.query(By.css('[data-testid="add-destination-button"]')), + ).not.toBeNull(); + // Loading existing state must not persist it back. + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + }); + + it("registers the destinations form so an invalid fingerprint blocks the parent cipher form, and fixing it restores validity", async () => { + // Mirrors what CipherFormComponent really does in registerChildForm: attach the child group + // to the parent FormGroup so its validity is aggregated. + const cipherForm = new FormGroup({}); + cipherFormContainer.registerChildForm = jest.fn((name: string, group: AbstractControl) => { + cipherForm.setControl(name, group); + }); + + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + expect(cipherFormContainer.registerChildForm).toHaveBeenCalledWith( + "sshAgentDestinations", + component.destinationsForm, + ); + expect(cipherForm.valid).toBe(true); + + // A blank placeholder row must not block the form. + component.addDestinationRow(); + expect(cipherForm.valid).toBe(true); + + // An invalid non-empty fingerprint must block it. + component.fingerprints.at(0).setValue("not-a-valid-fingerprint"); + expect(cipherForm.valid).toBe(false); + + // Fixing the value restores validity. + component.fingerprints.at(0).setValue("SHA256:aaaa"); + expect(cipherForm.valid).toBe(true); + + // Removing an invalid row also restores validity. + component.fingerprints.at(0).setValue("still-invalid"); + expect(cipherForm.valid).toBe(false); + component.removeDestinationRow(0); + expect(cipherForm.valid).toBe(true); + }); + + it("never patches destination data into the cipher", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + cipherFormContainer.patchCipher.mockClear(); + + component.addDestinationRow("SHA256:aaaa"); + component.removeDestinationRow(0); + + expect(cipherFormContainer.patchCipher).not.toHaveBeenCalled(); + }); + + it("does not persist while the user is editing rows (no save yet)", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow("SHA256:aaaa"); + component.addDestinationRow("SHA256:bbbb"); + component.removeDestinationRow(0); + + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + }); + + it("leaves persisted destinations unchanged when there's no successful save (cancel)", async () => { + destinationFingerprintsSubject.next(["SHA256:existing"]); + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow("SHA256:aaaa"); + component.removeDestinationRow(0); + + // No cipherSaved$ emission — simulates Cancel/closing the form. + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + }); + + it("persists normalized destinations after a successful save for an existing cipher", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow(" SHA256:aaaa "); + component.addDestinationRow("SHA256:aaaa"); + component.addDestinationRow("SHA256:bbbb"); + + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + + expect(destinationSettings.setDestinationFingerprints).toHaveBeenCalledTimes(1); + expect(destinationSettings.setDestinationFingerprints).toHaveBeenCalledWith(CIPHER_ID, [ + "SHA256:aaaa", + "SHA256:bbbb", + ]); + }); + + it("persists an empty array when all destinations are removed and then saved", async () => { + destinationFingerprintsSubject.next(["SHA256:aaaa"]); + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.removeDestinationRow(0); + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + + expect(destinationSettings.setDestinationFingerprints).toHaveBeenCalledWith(CIPHER_ID, []); + }); + + it("does not persist an invalid fingerprint even if a save event fires", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow("not-a-valid-fingerprint"); + fixture.detectChanges(); + + expect(component.fingerprints.at(0).invalid).toBe(true); + expect(fixture.debugElement.query(By.css('[data-testid="remove-destination"]'))).not.toBeNull(); + + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + }); + + it("shows exactly one translated validation error for an invalid, touched fingerprint, and clears it once fixed", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow("not-a-valid-fingerprint"); + // bit-form-field only renders the error once the control has been touched (matches real + // blur behavior); markAsTouched simulates that here. + component.fingerprints.at(0).markAsTouched(); + fixture.detectChanges(); + + expect(component.fingerprints.at(0).invalid).toBe(true); + let errors = fixture.debugElement.queryAll(By.css("bit-error")); + expect(errors.length).toBe(1); + expect(errors[0].nativeElement.textContent).toContain("invalidSshAgentDestinationFingerprint"); + + component.fingerprints.at(0).setValue("SHA256:aaaa"); + fixture.detectChanges(); + + expect(component.fingerprints.at(0).invalid).toBe(false); + errors = fixture.debugElement.queryAll(By.css("bit-error")); + expect(errors.length).toBe(0); + }); + + it("persists pending fingerprints for a new cipher once it receives an ID", async () => { + await setup({ edit: true, sshKey: null }); + + component.addDestinationRow("SHA256:aaaa"); + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + + expect(destinationSettings.setDestinationFingerprints).toHaveBeenCalledWith(CIPHER_ID, [ + "SHA256:aaaa", + ]); + }); + + it("persists again on every subsequent successful save, keeping later edits pending until then", async () => { + await setup({ edit: true, id: CIPHER_ID, sshKey: null }); + + component.addDestinationRow("SHA256:aaaa"); + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + destinationSettings.setDestinationFingerprints.mockClear(); + + // Edits after the first save must stay pending... + component.addDestinationRow("SHA256:bbbb"); + expect(destinationSettings.setDestinationFingerprints).not.toHaveBeenCalled(); + + // ...until the next successful save persists them. + cipherFormContainer.cipherSaved$.next({ id: CIPHER_ID }); + expect(destinationSettings.setDestinationFingerprints).toHaveBeenCalledWith(CIPHER_ID, [ + "SHA256:aaaa", + "SHA256:bbbb", + ]); + }); }); diff --git a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts index 51017663938b..f6596b3acf53 100644 --- a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts +++ b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts @@ -1,21 +1,24 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore import { CommonModule } from "@angular/common"; -import { Component, computed, input, OnInit } from "@angular/core"; +import { Component, computed, input, OnInit, Optional } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { FormBuilder, ReactiveFormsModule } from "@angular/forms"; +import { FormArray, FormBuilder, FormControl, ReactiveFormsModule } from "@angular/forms"; import { firstValueFrom } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { ClientType } from "@bitwarden/common/enums"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; +import { CipherId } from "@bitwarden/common/types/guid"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { SshKeyView } from "@bitwarden/common/vault/models/view/ssh-key.view"; import { CardComponent, FormFieldModule, IconButtonModule, + LinkModule, SectionHeaderComponent, SelectModule, TypographyModule, @@ -23,8 +26,14 @@ import { import { generate_ssh_key } from "@bitwarden/sdk-internal"; import { SshImportPromptService } from "../../../services/ssh-import-prompt.service"; +import { SshAgentDestinationSettingsService } from "../../abstractions/ssh-agent-destination-settings.service"; import { CipherFormContainer } from "../../cipher-form-container"; +import { + normalizeSshAgentDestinationFingerprints, + sshAgentDestinationFingerprintValidator, +} from "./ssh-agent-destination-fingerprint.util"; + // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ @@ -38,6 +47,7 @@ import { CipherFormContainer } from "../../cipher-form-container"; SelectModule, SectionHeaderComponent, IconButtonModule, + LinkModule, JslibModule, CommonModule, ], @@ -66,12 +76,35 @@ export class SshKeySectionComponent implements OnInit { ); }); + /** + * Whether the "SSH agent destinations" section should render. Only true on platforms that + * provide {@link SshAgentDestinationSettingsService} (currently Desktop only). + */ + readonly showDestinationSettings = this.destinationSettings != null; + + /** + * Local form for destination host-key fingerprints. Registered with + * {@link CipherFormContainer.registerChildForm} so an invalid fingerprint blocks Save like any + * other form section — but its values are never patched into the cipher (no `patchCipher` call + * here). Destination data is Desktop-local and reaches storage only through + * {@link SshAgentDestinationSettingsService}, on save, never as part of the synced cipher. + */ + destinationsForm = this.formBuilder.group({ + fingerprints: new FormArray>([]), + }); + + get fingerprints(): FormArray> { + return this.destinationsForm.controls.fingerprints; + } + constructor( private cipherFormContainer: CipherFormContainer, private formBuilder: FormBuilder, private sdkService: SdkService, private sshImportPromptService: SshImportPromptService, private platformUtilsService: PlatformUtilsService, + private i18nService: I18nService, + @Optional() private destinationSettings?: SshAgentDestinationSettingsService, ) { this.cipherFormContainer.registerChildForm("sshKeyDetails", this.sshKeyForm); this.sshKeyForm.valueChanges.pipe(takeUntilDestroyed()).subscribe((value) => { @@ -84,6 +117,24 @@ export class SshKeySectionComponent implements OnInit { return cipher; }); }); + + if (this.destinationSettings) { + // Registering (not patching) makes an invalid fingerprint block Save via the parent + // cipherForm's aggregated validity, without the destination values ever entering the + // synced cipher. + this.cipherFormContainer.registerChildForm("sshAgentDestinations", this.destinationsForm); + + // Destinations follow the form's own Save/Cancel semantics: edits only touch local form + // state, and are persisted exactly once per successful save — for a new cipher this is the + // first time an ID exists; for an existing cipher, every subsequent save persists again. + // Editing keystrokes never call the agent-destinations service directly, so the native + // agent's key list isn't reloaded on every keystroke. + this.cipherFormContainer.cipherSaved$.pipe(takeUntilDestroyed()).subscribe((savedCipher) => { + if (savedCipher?.id != null) { + this.persistDestinationFingerprints(savedCipher.id as CipherId); + } + }); + } } async ngOnInit() { @@ -95,6 +146,17 @@ export class SshKeySectionComponent implements OnInit { } else { await this.generateSshKey(); } + + if (this.destinationSettings) { + const cipherId = (prefillCipher?.id ?? this.originalCipherView()?.id) as CipherId | undefined; + + if (cipherId != null) { + const fingerprints = await firstValueFrom( + this.destinationSettings.destinationFingerprints$(cipherId), + ); + fingerprints.forEach((fingerprint) => this.addDestinationRow(fingerprint)); + } + } } /** Set form initial form values from the current cipher */ @@ -128,4 +190,32 @@ export class SshKeySectionComponent implements OnInit { keyFingerprint: sshKey.fingerprint, }); } + + /** Adds a destination fingerprint row. */ + addDestinationRow(value = "") { + this.fingerprints.push( + this.formBuilder.control(value, { + nonNullable: true, + validators: [ + sshAgentDestinationFingerprintValidator( + this.i18nService.t("invalidSshAgentDestinationFingerprint"), + ), + ], + }), + ); + } + + removeDestinationRow(index: number) { + this.fingerprints.removeAt(index); + } + + /** Persists the current fingerprint set for `cipherId`, unless a row is invalid. */ + private persistDestinationFingerprints(cipherId: CipherId) { + if (this.destinationSettings == null || this.fingerprints.invalid) { + return; + } + + const normalized = normalizeSshAgentDestinationFingerprints(this.fingerprints.value); + void this.destinationSettings.setDestinationFingerprints(cipherId, normalized); + } } diff --git a/libs/vault/src/cipher-form/index.ts b/libs/vault/src/cipher-form/index.ts index 89c0c8f34bb2..b048b107033d 100644 --- a/libs/vault/src/cipher-form/index.ts +++ b/libs/vault/src/cipher-form/index.ts @@ -6,6 +6,7 @@ export { OptionalInitialValues, } from "./abstractions/cipher-form-config.service"; export { TotpCaptureService } from "./abstractions/totp-capture.service"; +export { SshAgentDestinationSettingsService } from "./abstractions/ssh-agent-destination-settings.service"; export { CipherFormGenerationService } from "./abstractions/cipher-form-generation.service"; export { DefaultCipherFormConfigService } from "./services/default-cipher-form-config.service"; export { CipherFormGeneratorComponent } from "./components/cipher-generator/cipher-form-generator.component";