Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions apps/desktop/desktop_native/napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>
}
}

Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/desktop_native/napi/src/sshagent_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
}

/// SSH public key data
Expand Down Expand Up @@ -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();

Expand Down
75 changes: 71 additions & 4 deletions apps/desktop/desktop_native/ssh_agent/src/server/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,9 @@ async fn handle_message<K: KeyStore, A: AuthPolicy>(
};

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,
Expand All @@ -213,6 +215,7 @@ async fn handle_message<K: KeyStore, A: AuthPolicy>(
}

async fn handle_list_request<K: KeyStore, A: AuthPolicy>(
session_bind_state: &SessionBindState,
keystore: &Arc<K>,
auth_policy: &Arc<A>,
) -> Vec<u8> {
Expand All @@ -231,7 +234,13 @@ async fn handle_list_request<K: KeyStore, A: AuthPolicy>(
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");
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(
Expand All @@ -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};
Expand Down
134 changes: 125 additions & 9 deletions apps/desktop/desktop_native/ssh_agent/src/storage/keydata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<String>,
}

/// Represents an SSH key and its associated metadata.
Expand All @@ -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<String>,
}

impl SSHKeyData {
Expand All @@ -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<String>,
) -> Self {
Self {
private_key,
public_key,
name,
cipher_id,
destination_fingerprints,
}
}

Expand All @@ -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<Self> {
pub fn from_private_key_pem(
pem: &str,
name: String,
cipher_id: String,
destination_fingerprints: Vec<String>,
) -> Result<Self> {
let ssh_key = ssh_key::PrivateKey::from_openssh(pem)
.map_err(|e| anyhow!("Failed to parse private key: {e}"))?;

Expand All @@ -99,6 +139,7 @@ impl SSHKeyData {
PublicKey { alg, blob },
name,
cipher_id,
destination_fingerprints,
))
}

Expand All @@ -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();

Expand All @@ -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 {
Expand Down Expand Up @@ -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");
Expand All @@ -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");
}

Expand All @@ -283,6 +355,7 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAE3NrLXRlc3RAZXhhbXBsZS5jb20BAgMEBQY=
private_key_pem: pem.to_string(),
name: name.to_string(),
cipher_id: format!("cipher-{name}"),
destination_fingerprints: vec![],
}
}

Expand All @@ -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
);
}
}
Loading
Loading