From fac5ba5bae57758468728dff3172ae5f4c4fdfbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 8 May 2024 14:29:15 +0200 Subject: [PATCH] chore(crypto): Refactor the PK signing subkey constructors This makes the public key fields private to ensure that we don't accidentally swap them out. It also moves the construction of the subkeys into the master key type. --- .../matrix-sdk-crypto/src/identities/user.rs | 10 +- .../matrix-sdk-crypto/src/olm/signing/mod.rs | 122 +++++++----------- .../src/olm/signing/pk_signing.rs | 63 +++++++-- 3 files changed, 106 insertions(+), 89 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index d416194a6..bb0be89fe 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -421,9 +421,9 @@ impl ReadOnlyUserIdentity { #[cfg(test)] pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { let master_key = - identity.master_key.lock().await.as_ref().unwrap().public_key.clone().into(); + identity.master_key.lock().await.as_ref().unwrap().public_key().clone().into(); let self_signing_key = - identity.self_signing_key.lock().await.as_ref().unwrap().public_key.clone().into(); + identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone().into(); Self { user_id: identity.user_id().into(), master_key, self_signing_key } } @@ -565,11 +565,11 @@ impl ReadOnlyOwnUserIdentity { #[cfg(test)] pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { - let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key.clone(); + let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key().clone(); let self_signing_key = - identity.self_signing_key.lock().await.as_ref().unwrap().public_key.clone(); + identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone(); let user_signing_key = - identity.user_signing_key.lock().await.as_ref().unwrap().public_key.clone(); + identity.user_signing_key.lock().await.as_ref().unwrap().public_key().clone(); Self { user_id: identity.user_id().into(), diff --git a/crates/matrix-sdk-crypto/src/olm/signing/mod.rs b/crates/matrix-sdk-crypto/src/olm/signing/mod.rs index 8348ac2b7..16ac529d9 100644 --- a/crates/matrix-sdk-crypto/src/olm/signing/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/signing/mod.rs @@ -19,10 +19,9 @@ use std::sync::{ Arc, }; -use pk_signing::{MasterSigning, PickledSignings, SelfSigning, Signing, SigningError, UserSigning}; +pub use pk_signing::{MasterSigning, PickledSignings, SelfSigning, SigningError, UserSigning}; use ruma::{ api::client::keys::upload_signatures::v3::{Request as SignatureUploadRequest, SignedKeys}, - encryption::KeyUsage, events::secret::request::SecretName, DeviceKeyAlgorithm, DeviceKeyId, OwnedDeviceId, OwnedDeviceKeyId, OwnedUserId, UserId, }; @@ -174,17 +173,17 @@ impl PrivateCrossSigningIdentity { /// Get the public part of the master key, if we have one. pub async fn master_public_key(&self) -> Option { - self.master_key.lock().await.as_ref().map(|m| m.public_key.to_owned()) + self.master_key.lock().await.as_ref().map(|m| m.public_key().to_owned()) } /// Get the public part of the self-signing key, if we have one. pub async fn self_signing_public_key(&self) -> Option { - self.self_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned()) + self.self_signing_key.lock().await.as_ref().map(|k| k.public_key().to_owned()) } /// Get the public part of the user-signing key, if we have one. pub async fn user_signing_public_key(&self) -> Option { - self.user_signing_key.lock().await.as_ref().map(|k| k.public_key.to_owned()) + self.user_signing_key.lock().await.as_ref().map(|k| k.public_key().to_owned()) } /// Export the seed of the private cross signing key @@ -236,7 +235,7 @@ impl PrivateCrossSigningIdentity { let master = if let Some(master_key) = master_key { let master = MasterSigning::from_base64(self.user_id().to_owned(), master_key)?; - if public_identity.master_key() == &master.public_key { + if public_identity.master_key() == master.public_key() { Some(master) } else { return Err(SecretImportError::MismatchedPublicKeys); @@ -248,7 +247,7 @@ impl PrivateCrossSigningIdentity { let user_signing = if let Some(user_signing_key) = user_signing_key { let subkey = UserSigning::from_base64(self.user_id().to_owned(), user_signing_key)?; - if public_identity.user_signing_key() == &subkey.public_key { + if public_identity.user_signing_key() == subkey.public_key() { Ok(Some(subkey)) } else { Err(SecretImportError::MismatchedPublicKeys) @@ -260,7 +259,7 @@ impl PrivateCrossSigningIdentity { let self_signing = if let Some(self_signing_key) = self_signing_key { let subkey = SelfSigning::from_base64(self.user_id().to_owned(), self_signing_key)?; - if public_identity.self_signing_key() == &subkey.public_key { + if public_identity.self_signing_key() == subkey.public_key() { Ok(Some(subkey)) } else { Err(SecretImportError::MismatchedPublicKeys) @@ -390,35 +389,34 @@ impl PrivateCrossSigningIdentity { } } + async fn public_keys( + &self, + ) -> Result<(MasterPubkey, SelfSigningPubkey, UserSigningPubkey), SignatureError> { + let master_private_key = self.master_key.lock().await; + let master_private_key = + master_private_key.as_ref().ok_or(SignatureError::MissingSigningKey)?; + let self_signing_private_key = self.self_signing_key.lock().await; + let self_signing_private_key = + self_signing_private_key.as_ref().ok_or(SignatureError::MissingSigningKey)?; + let user_signing_private_key = self.user_signing_key.lock().await; + let user_signing_private_key = + user_signing_private_key.as_ref().ok_or(SignatureError::MissingSigningKey)?; + + let mut master = master_private_key.public_key().to_owned(); + let mut self_signing = self_signing_private_key.public_key().to_owned(); + let mut user_signing = user_signing_private_key.public_key().to_owned(); + + master_private_key.sign_subkey(master.as_mut()); + master_private_key.sign_subkey(self_signing.as_mut()); + master_private_key.sign_subkey(user_signing.as_mut()); + + Ok((master, self_signing, user_signing)) + } + pub(crate) async fn to_public_identity( &self, ) -> Result { - let master = self - .master_key - .lock() - .await - .as_ref() - .ok_or(SignatureError::MissingSigningKey)? - .public_key - .clone(); - - let self_signing = self - .self_signing_key - .lock() - .await - .as_ref() - .ok_or(SignatureError::MissingSigningKey)? - .public_key - .clone(); - - let user_signing = self - .user_signing_key - .lock() - .await - .as_ref() - .ok_or(SignatureError::MissingSigningKey)? - .public_key - .clone(); + let (master, self_signing, user_signing) = self.public_keys().await?; let identity = ReadOnlyOwnUserIdentity::new(master, self_signing, user_signing)?; identity.mark_as_verified(); @@ -516,16 +514,11 @@ impl PrivateCrossSigningIdentity { account: &Account, ) -> (Self, UploadSigningKeysRequest, SignatureUploadRequest) { let mut master = MasterSigning::new(account.user_id().into()); - let mut public_key = master.public_key.as_ref().to_owned(); account - .sign_cross_signing_key(&mut public_key) + .sign_cross_signing_key(master.public_key_mut().as_mut()) .expect("Can't sign our freshly created master key with our account"); - master.public_key = public_key - .try_into() - .expect("We can always convert our own CrossSignigKey into a MasterPubkey"); - let identity = Self::new_helper(account.user_id(), master); let signature_request = identity .sign_account(account.static_data()) @@ -538,28 +531,7 @@ impl PrivateCrossSigningIdentity { } fn new_helper(user_id: &UserId, master: MasterSigning) -> Self { - let user = Signing::new(); - let mut public_key = user.cross_signing_key(user_id.to_owned(), KeyUsage::UserSigning); - master.sign_subkey(&mut public_key); - - let user = UserSigning { - inner: user, - public_key: public_key - .try_into() - .expect("We can always create a new random UserSigningPubkey"), - }; - - let self_signing = Signing::new(); - let mut public_key = - self_signing.cross_signing_key(user_id.to_owned(), KeyUsage::SelfSigning); - master.sign_subkey(&mut public_key); - - let self_signing = SelfSigning { - inner: self_signing, - public_key: public_key - .try_into() - .expect("We can always create a new random SelfSigningPubkey"), - }; + let (user, self_signing) = master.new_subkeys(); Self { user_id: user_id.into(), @@ -648,13 +620,13 @@ impl PrivateCrossSigningIdentity { /// identity. pub(crate) async fn as_upload_request(&self) -> UploadSigningKeysRequest { let master_key = - self.master_key.lock().await.as_ref().map(|k| k.public_key.as_ref().clone()); + self.master_key.lock().await.as_ref().map(|k| k.public_key().as_ref().clone()); let user_signing_key = - self.user_signing_key.lock().await.as_ref().map(|k| k.public_key.as_ref().clone()); + self.user_signing_key.lock().await.as_ref().map(|k| k.public_key().as_ref().clone()); let self_signing_key = - self.self_signing_key.lock().await.as_ref().map(|k| k.public_key.as_ref().clone()); + self.self_signing_key.lock().await.as_ref().map(|k| k.public_key().as_ref().clone()); UploadSigningKeysRequest { master_key, self_signing_key, user_signing_key } } @@ -668,7 +640,7 @@ mod tests { use ruma::{device_id, user_id, CanonicalJsonValue, DeviceKeyAlgorithm, DeviceKeyId, UserId}; use serde_json::json; - use super::{PrivateCrossSigningIdentity, Signing}; + use super::{pk_signing::Signing, PrivateCrossSigningIdentity}; use crate::{ identities::{ReadOnlyDevice, ReadOnlyUserIdentity}, olm::{Account, SignedJsonObject, VerifyJson}, @@ -721,13 +693,13 @@ mod tests { let master_key = master_key.as_ref().unwrap(); master_key - .public_key - .verify_subkey(&identity.self_signing_key.lock().await.as_ref().unwrap().public_key) + .public_key() + .verify_subkey(identity.self_signing_key.lock().await.as_ref().unwrap().public_key()) .unwrap(); master_key - .public_key - .verify_subkey(&identity.user_signing_key.lock().await.as_ref().unwrap().public_key) + .public_key() + .verify_subkey(identity.user_signing_key.lock().await.as_ref().unwrap().public_key()) .unwrap(); } @@ -758,7 +730,7 @@ mod tests { let master = identity.master_key.lock().await; let master = master.as_ref().unwrap(); - let public_key = master.public_key.as_ref(); + let public_key = master.public_key().as_ref(); let signatures = &public_key.signatures; let canonical_json = public_key.to_canonical_json().unwrap(); @@ -767,11 +739,11 @@ mod tests { .expect("The account should have signed the master key"); master - .public_key + .public_key() .has_signed_raw(signatures, &canonical_json) .expect("The master key should have self-signed"); - assert!(!master.public_key.signatures().is_empty()); + assert!(!master.public_key().signatures().is_empty()); } #[async_test] @@ -787,7 +759,7 @@ mod tests { self_signing.sign_device(&mut device_keys).unwrap(); device.update_device(&device_keys).unwrap(); - let public_key = &self_signing.public_key; + let public_key = &self_signing.public_key(); public_key.verify_device(&device).unwrap() } @@ -814,6 +786,6 @@ mod tests { bob_public.master_key = Arc::new(master.try_into().unwrap()); - user_signing.public_key.verify_master_key(bob_public.master_key()).unwrap(); + user_signing.public_key().verify_master_key(bob_public.master_key()).unwrap(); } } diff --git a/crates/matrix-sdk-crypto/src/olm/signing/pk_signing.rs b/crates/matrix-sdk-crypto/src/olm/signing/pk_signing.rs index 5f98397fc..1adea5199 100644 --- a/crates/matrix-sdk-crypto/src/olm/signing/pk_signing.rs +++ b/crates/matrix-sdk-crypto/src/olm/signing/pk_signing.rs @@ -72,8 +72,8 @@ impl SignJson for Signing { #[derive(PartialEq, Debug)] pub struct MasterSigning { - pub inner: Signing, - pub public_key: MasterPubkey, + inner: Signing, + public_key: MasterPubkey, } #[derive(Deserialize, Serialize)] @@ -115,6 +115,43 @@ impl MasterSigning { key } + pub(crate) fn new_subkeys(&self) -> (UserSigning, SelfSigning) { + let user = Signing::new(); + let mut public_key = + user.cross_signing_key(self.public_key.user_id().to_owned(), KeyUsage::UserSigning); + + self.sign_subkey(&mut public_key); + + let user = UserSigning { + inner: user, + public_key: public_key + .try_into() + .expect("We can always create a new random UserSigningPubkey"), + }; + + let self_signing = Signing::new(); + let mut public_key = self_signing + .cross_signing_key(self.public_key.user_id().to_owned(), KeyUsage::SelfSigning); + self.sign_subkey(&mut public_key); + + let self_signing = SelfSigning { + inner: self_signing, + public_key: public_key + .try_into() + .expect("We can always create a new random SelfSigningPubkey"), + }; + + (user, self_signing) + } + + pub fn public_key(&self) -> &MasterPubkey { + &self.public_key + } + + pub(crate) fn public_key_mut(&mut self) -> &mut MasterPubkey { + &mut self.public_key + } + pub fn pickle(&self) -> PickledMasterSigning { let pickle = self.inner.pickle(); let public_key = self.public_key.clone(); @@ -168,6 +205,10 @@ impl UserSigning { PickledUserSigning { pickle, public_key } } + pub fn public_key(&self) -> &UserSigningPubkey { + &self.public_key + } + pub fn export_seed(&self) -> String { self.inner.to_base64() } @@ -223,12 +264,16 @@ impl UserSigning { } impl SelfSigning { - pub fn pickle(&self) -> PickledSelfSigning { + pub(crate) fn pickle(&self) -> PickledSelfSigning { let pickle = self.inner.pickle(); let public_key = self.public_key.clone(); PickledSelfSigning { pickle, public_key } } + pub fn public_key(&self) -> &SelfSigningPubkey { + &self.public_key + } + pub fn export_seed(&self) -> String { self.inner.to_base64() } @@ -243,7 +288,7 @@ impl SelfSigning { Ok(Self { inner, public_key }) } - pub fn sign_device(&self, device_keys: &mut DeviceKeys) -> Result<(), SignatureError> { + pub(crate) fn sign_device(&self, device_keys: &mut DeviceKeys) -> Result<(), SignatureError> { let serialized = serde_json::to_value(&device_keys)?; let signature = self.inner.sign_json(serialized)?; @@ -259,7 +304,7 @@ impl SelfSigning { Ok(()) } - pub fn from_pickle(pickle: PickledSelfSigning) -> Result { + pub(crate) fn from_pickle(pickle: PickledSelfSigning) -> Result { let inner = Signing::from_pickle(pickle.pickle)?; Ok(Self { inner, public_key: pickle.public_key }) @@ -268,14 +313,14 @@ impl SelfSigning { #[derive(PartialEq, Debug)] pub struct SelfSigning { - pub inner: Signing, - pub public_key: SelfSigningPubkey, + inner: Signing, + public_key: SelfSigningPubkey, } #[derive(PartialEq, Debug)] pub struct UserSigning { - pub inner: Signing, - pub public_key: UserSigningPubkey, + inner: Signing, + public_key: UserSigningPubkey, } #[derive(Serialize, Deserialize)]