From 3291f0dac42f87f3ec5a47aee81c9cef9573ae74 Mon Sep 17 00:00:00 2001 From: Jan Winkelmann <146678+keks@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:55:42 +0100 Subject: [PATCH] Refactor VerifiedStruct and Verifiable (#1472) The current version of the two traits is the way it is because it attempted to prevent the caller from being able to convert to the Type implementing VerifiedStruct without checking the signature. However, the approach that was taken does not work. This commit refactors the two traits and performs the conversion in the same method as the verification. Unfortunately, this means that the verify method has to be defined per impl instead of being able to rely on a default one. There probably is no straightforward way around that, so this simple solution is preferred. Co-authored-by: Konrad Kohbrok --- openmls/src/ciphersuite/signable.rs | 95 +++++++------------ .../ciphersuite/tests/kat_crypto_basics.rs | 21 ++-- openmls/src/framing/mls_auth_content_in.rs | 31 +++--- openmls/src/key_packages/key_package_in.rs | 33 +++---- openmls/src/messages/group_info.rs | 26 ++--- openmls/src/treesync/node/leaf_node.rs | 71 +++++++------- 6 files changed, 129 insertions(+), 148 deletions(-) diff --git a/openmls/src/ciphersuite/signable.rs b/openmls/src/ciphersuite/signable.rs index ce04e2badf..52b885fb43 100644 --- a/openmls/src/ciphersuite/signable.rs +++ b/openmls/src/ciphersuite/signable.rs @@ -53,32 +53,6 @@ pub trait SignedStruct { fn from_payload(payload: T, signature: Signature) -> Self; } -/// This trait must be implemented by all structs that contain a verified -/// self-signature. -pub trait VerifiedStruct { - /// This type is used to prevent users of the trait from bypassing `verify` - /// by simply calling `from_verifiable`. `Seal` should be a dummy type - /// defined in a private module as follows: - /// ``` - /// mod private_mod { - /// pub struct Seal; - /// - /// impl Default for Seal { - /// fn default() -> Self { - /// Seal {} - /// } - /// } - /// } - /// ``` - type SealingType: Default; - - /// Build a verified struct version from the payload struct. This function - /// is only meant to be called by the implementation of the `Verifiable` - /// trait corresponding to this `VerifiedStruct`. - #[doc(hidden)] - fn from_verifiable(verifiable: T, _seal: Self::SealingType) -> Self; -} - /// The `Signable` trait is implemented by all struct that are being signed. /// The implementation has to provide the `unsigned_payload` function. pub trait Signable: Sized { @@ -117,6 +91,10 @@ pub trait Signable: Sized { } } +/// This marker trait must be implemented by all structs that contain a verified +/// self-signature. +pub trait VerifiedStruct {} + /// The verifiable trait must be implemented by any struct that is signed with /// a credential. The actual `verify` method is provided. /// The `unsigned_payload` and `signature` functions have to be implemented for @@ -127,6 +105,10 @@ pub trait Signable: Sized { /// struct implementing them aren't well defined. Not that both traits define an /// `unsigned_payload` function. pub trait Verifiable: Sized { + /// The type used for representing the verified data. Must implement the marker trait + /// [`VerifiedStruct`]. + type VerifiedStruct: VerifiedStruct; + /// Return the unsigned, serialized payload that should be verified. fn unsigned_payload(&self) -> Result, tls_codec::Error>; @@ -137,22 +119,17 @@ pub trait Verifiable: Sized { fn label(&self) -> &str; /// Verifies the payload against the given `credential`. - /// The signature is fetched via the [`Verifiable::signature()`] function and - /// the payload via [`Verifiable::unsigned_payload()`]. + /// Usually this is implemented by first checking that `self.verify_no_out()` + /// does not return an error, and then converting the value into + /// `Self::VerifiedStruct`. /// /// Returns `Ok(Self::VerifiedOutput)` if the signature is valid and /// `CredentialError::InvalidSignature` otherwise. - fn verify( + fn verify( self, crypto: &impl OpenMlsCrypto, pk: &OpenMlsSignaturePublicKey, - ) -> Result - where - T: VerifiedStruct, - { - verify(crypto, &self, pk)?; - Ok(T::from_verifiable(self, T::SealingType::default())) - } + ) -> Result; /// Verifies the payload against the given `credential`. /// The signature is fetched via the [`Verifiable::signature()`] function and @@ -165,32 +142,24 @@ pub trait Verifiable: Sized { crypto: &impl OpenMlsCrypto, pk: &OpenMlsSignaturePublicKey, ) -> Result<(), SignatureError> { - verify(crypto, self, pk) + let payload = self + .unsigned_payload() + .map_err(|_| SignatureError::VerificationError)?; + let sign_content = SignContent::new(self.label(), payload.into()); + let payload = match sign_content.tls_serialize_detached() { + Ok(p) => p, + Err(e) => { + log::error!("Serializing SignContent failed, {:?}", e); + return Err(SignatureError::VerificationError); + } + }; + crypto + .verify_signature( + pk.signature_scheme(), + &payload, + pk.as_slice(), + self.signature().value(), + ) + .map_err(|_| SignatureError::VerificationError) } } - -fn verify( - crypto: &impl OpenMlsCrypto, - verifiable: &impl Verifiable, - pk: &OpenMlsSignaturePublicKey, -) -> Result<(), SignatureError> { - let payload = verifiable - .unsigned_payload() - .map_err(|_| SignatureError::VerificationError)?; - let sign_content = SignContent::new(verifiable.label(), payload.into()); - let payload = match sign_content.tls_serialize_detached() { - Ok(p) => p, - Err(e) => { - log::error!("Serializing SignContent failed, {:?}", e); - return Err(SignatureError::VerificationError); - } - }; - crypto - .verify_signature( - pk.signature_scheme(), - &payload, - pk.as_slice(), - verifiable.signature().value(), - ) - .map_err(|_| SignatureError::VerificationError) -} diff --git a/openmls/src/ciphersuite/tests/kat_crypto_basics.rs b/openmls/src/ciphersuite/tests/kat_crypto_basics.rs index 625e508e33..1fdecc601b 100644 --- a/openmls/src/ciphersuite/tests/kat_crypto_basics.rs +++ b/openmls/src/ciphersuite/tests/kat_crypto_basics.rs @@ -154,6 +154,8 @@ impl SignedStruct for MySignature { } impl Verifiable for ParsedSignWithLabel { + type VerifiedStruct = (); + fn unsigned_payload(&self) -> Result, tls_codec::Error> { Ok(self.content.clone()) } @@ -165,14 +167,19 @@ impl Verifiable for ParsedSignWithLabel { fn label(&self) -> &str { &self.label } + + fn verify( + self, + crypto: &impl openmls_traits::crypto::OpenMlsCrypto, + pk: &crate::ciphersuite::OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(()) + } } // Dummy implementation -impl VerifiedStruct for () { - type SealingType = u8; - - fn from_verifiable(_: ParsedSignWithLabel, _seal: Self::SealingType) -> Self {} -} +impl VerifiedStruct for () {} impl Signable for ParsedSignWithLabel { type SignedOutput = MySignature; @@ -294,7 +301,7 @@ pub fn run_test_vector( // verify signature parsed .clone() - .verify::<()>( + .verify( provider.crypto(), &OpenMlsSignaturePublicKey::new( public.clone().into(), @@ -307,7 +314,7 @@ pub fn run_test_vector( // verify own signature parsed.signature = my_signature.0; parsed - .verify::<()>( + .verify( provider.crypto(), &OpenMlsSignaturePublicKey::new(public.into(), ciphersuite.signature_algorithm()) .unwrap(), diff --git a/openmls/src/framing/mls_auth_content_in.rs b/openmls/src/framing/mls_auth_content_in.rs index c9db02a412..426a2f4bf5 100644 --- a/openmls/src/framing/mls_auth_content_in.rs +++ b/openmls/src/framing/mls_auth_content_in.rs @@ -24,12 +24,6 @@ use crate::{ #[cfg(doc)] use super::{PrivateMessageIn, PublicMessageIn}; -/// Private module to ensure protection of [`AuthenticatedContent`]. -mod private_mod { - #[derive(Default)] - pub(crate) struct Seal; -} - /// 6 Message Framing /// /// ```c @@ -194,6 +188,8 @@ impl VerifiableAuthenticatedContentIn { } impl Verifiable for VerifiableAuthenticatedContentIn { + type VerifiedStruct = AuthenticatedContentIn; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { self.tbs.tls_serialize_detached() } @@ -205,20 +201,23 @@ impl Verifiable for VerifiableAuthenticatedContentIn { fn label(&self) -> &str { "FramedContentTBS" } -} -impl VerifiedStruct for AuthenticatedContentIn { - fn from_verifiable(v: VerifiableAuthenticatedContentIn, _seal: Self::SealingType) -> Self { - AuthenticatedContentIn { - wire_format: v.tbs.wire_format, - content: v.tbs.content, - auth: v.auth, - } + fn verify( + self, + crypto: &impl OpenMlsCrypto, + pk: &OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(AuthenticatedContentIn { + wire_format: self.tbs.wire_format, + content: self.tbs.content, + auth: self.auth, + }) } - - type SealingType = private_mod::Seal; } +impl VerifiedStruct for AuthenticatedContentIn {} + impl SignedStruct for AuthenticatedContentIn { fn from_payload(tbs: FramedContentTbsIn, signature: Signature) -> Self { let auth = FramedContentAuthData { diff --git a/openmls/src/key_packages/key_package_in.rs b/openmls/src/key_packages/key_package_in.rs index ab0efad7bb..0b0d9ad3f7 100644 --- a/openmls/src/key_packages/key_package_in.rs +++ b/openmls/src/key_packages/key_package_in.rs @@ -5,7 +5,7 @@ use crate::{ ciphersuite::{signable::*, *}, credentials::*, extensions::Extensions, - treesync::node::leaf_node::{LeafNode, LeafNodeIn, VerifiableLeafNode}, + treesync::node::leaf_node::{LeafNodeIn, VerifiableLeafNode}, versions::ProtocolVersion, }; use openmls_traits::{crypto::OpenMlsCrypto, types::Ciphersuite}; @@ -31,6 +31,8 @@ impl VerifiableKeyPackage { } impl Verifiable for VerifiableKeyPackage { + type VerifiedStruct = KeyPackage; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { self.payload.tls_serialize_detached() } @@ -42,23 +44,22 @@ impl Verifiable for VerifiableKeyPackage { fn label(&self) -> &str { SIGNATURE_KEY_PACKAGE_LABEL } -} -impl VerifiedStruct for KeyPackage { - type SealingType = private_mod::Seal; - - fn from_verifiable(verifiable: VerifiableKeyPackage, _seal: Self::SealingType) -> Self { - Self { - payload: verifiable.payload, - signature: verifiable.signature, - } + fn verify( + self, + crypto: &impl OpenMlsCrypto, + pk: &OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + + Ok(KeyPackage { + payload: self.payload, + signature: self.signature, + }) } } -mod private_mod { - #[derive(Default)] - pub struct Seal; -} +impl VerifiedStruct for KeyPackage {} /// The unsigned payload of a key package. /// @@ -143,7 +144,7 @@ impl KeyPackageIn { let leaf_node = match leaf_node { VerifiableLeafNode::KeyPackage(leaf_node) => leaf_node - .verify::(crypto, signature_key) + .verify(crypto, signature_key) .map_err(|_| KeyPackageVerifyError::InvalidLeafNodeSignature)?, _ => return Err(KeyPackageVerifyError::InvalidLeafNodeSourceType), }; @@ -168,7 +169,7 @@ impl KeyPackageIn { // Verify the KeyPackage signature let key_package = VerifiableKeyPackage::new(key_package_tbs, self.signature) - .verify::(crypto, signature_key) + .verify(crypto, signature_key) .map_err(|_| KeyPackageVerifyError::InvalidSignature)?; // Extension included in the extensions or leaf_node.extensions fields diff --git a/openmls/src/messages/group_info.rs b/openmls/src/messages/group_info.rs index 98dbf7fec1..4aa4d297ad 100644 --- a/openmls/src/messages/group_info.rs +++ b/openmls/src/messages/group_info.rs @@ -272,6 +272,8 @@ impl SignedStruct for GroupInfo { } impl Verifiable for VerifiableGroupInfo { + type VerifiedStruct = GroupInfo; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { self.payload.tls_serialize_detached() } @@ -283,20 +285,18 @@ impl Verifiable for VerifiableGroupInfo { fn label(&self) -> &str { SIGNATURE_GROUP_INFO_LABEL } -} -impl VerifiedStruct for GroupInfo { - type SealingType = private_mod::Seal; - - fn from_verifiable(v: VerifiableGroupInfo, _seal: Self::SealingType) -> Self { - Self { - payload: v.payload, - signature: v.signature, - } + fn verify( + self, + crypto: &impl OpenMlsCrypto, + pk: &crate::ciphersuite::OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(GroupInfo { + payload: self.payload, + signature: self.signature, + }) } } -mod private_mod { - #[derive(Default)] - pub struct Seal; -} +impl VerifiedStruct for GroupInfo {} diff --git a/openmls/src/treesync/node/leaf_node.rs b/openmls/src/treesync/node/leaf_node.rs index 5277a566a1..b50aa7bf67 100644 --- a/openmls/src/treesync/node/leaf_node.rs +++ b/openmls/src/treesync/node/leaf_node.rs @@ -35,12 +35,6 @@ mod codec; pub use capabilities::*; -/// Private module to ensure protection. -mod private_mod { - #[derive(Default)] - pub(crate) struct Seal; -} - pub(crate) struct NewLeafNodeParams { pub(crate) config: CryptoConfig, pub(crate) credential_with_key: CredentialWithKey, @@ -790,6 +784,8 @@ impl VerifiableKeyPackageLeafNode { } impl Verifiable for VerifiableKeyPackageLeafNode { + type VerifiedStruct = LeafNode; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { self.payload.tls_serialize_detached() } @@ -801,19 +797,22 @@ impl Verifiable for VerifiableKeyPackageLeafNode { fn label(&self) -> &str { LEAF_NODE_SIGNATURE_LABEL } -} -impl VerifiedStruct for LeafNode { - fn from_verifiable(verifiable: VerifiableKeyPackageLeafNode, _seal: Self::SealingType) -> Self { - Self { - payload: verifiable.payload, - signature: verifiable.signature, - } + fn verify( + self, + crypto: &impl openmls_traits::crypto::OpenMlsCrypto, + pk: &crate::ciphersuite::OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(LeafNode { + payload: self.payload, + signature: self.signature, + }) } - - type SealingType = private_mod::Seal; } +impl VerifiedStruct for LeafNode {} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct VerifiableUpdateLeafNode { payload: LeafNodePayload, @@ -832,6 +831,8 @@ impl VerifiableUpdateLeafNode { } impl Verifiable for VerifiableUpdateLeafNode { + type VerifiedStruct = LeafNode; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { let tree_info_tbs = match &self.tree_position { Some(tree_position) => TreeInfoTbs::Commit(tree_position.clone()), @@ -851,17 +852,18 @@ impl Verifiable for VerifiableUpdateLeafNode { fn label(&self) -> &str { LEAF_NODE_SIGNATURE_LABEL } -} -impl VerifiedStruct for LeafNode { - fn from_verifiable(verifiable: VerifiableUpdateLeafNode, _seal: Self::SealingType) -> Self { - Self { - payload: verifiable.payload, - signature: verifiable.signature, - } + fn verify( + self, + crypto: &impl openmls_traits::crypto::OpenMlsCrypto, + pk: &crate::ciphersuite::OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(LeafNode { + payload: self.payload, + signature: self.signature, + }) } - - type SealingType = private_mod::Seal; } #[derive(Debug, Clone, PartialEq, Eq)] @@ -882,6 +884,8 @@ impl VerifiableCommitLeafNode { } impl Verifiable for VerifiableCommitLeafNode { + type VerifiedStruct = LeafNode; + fn unsigned_payload(&self) -> Result, tls_codec::Error> { let tree_info_tbs = match &self.tree_position { Some(tree_position) => TreeInfoTbs::Commit(tree_position.clone()), @@ -902,17 +906,18 @@ impl Verifiable for VerifiableCommitLeafNode { fn label(&self) -> &str { LEAF_NODE_SIGNATURE_LABEL } -} -impl VerifiedStruct for LeafNode { - fn from_verifiable(verifiable: VerifiableCommitLeafNode, _seal: Self::SealingType) -> Self { - Self { - payload: verifiable.payload, - signature: verifiable.signature, - } + fn verify( + self, + crypto: &impl openmls_traits::crypto::OpenMlsCrypto, + pk: &crate::ciphersuite::OpenMlsSignaturePublicKey, + ) -> Result { + self.verify_no_out(crypto, pk)?; + Ok(LeafNode { + payload: self.payload, + signature: self.signature, + }) } - - type SealingType = private_mod::Seal; } impl Signable for LeafNodeTbs {