Skip to content

Commit

Permalink
Refactor VerifiedStruct and Verifiable (openmls#1472)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@keks @kkohbrok
keks and kkohbrok authored Jan 10, 2024
1 parent 281e59f commit 3291f0d
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 148 deletions.
95 changes: 32 additions & 63 deletions openmls/src/ciphersuite/signable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,32 +53,6 @@ pub trait SignedStruct<T> {
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<T> {
/// 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 {
Expand Down Expand Up @@ -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
Expand All @@ -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<Vec<u8>, tls_codec::Error>;

Expand All @@ -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<T>(
fn verify(
self,
crypto: &impl OpenMlsCrypto,
pk: &OpenMlsSignaturePublicKey,
) -> Result<T, SignatureError>
where
T: VerifiedStruct<Self>,
{
verify(crypto, &self, pk)?;
Ok(T::from_verifiable(self, T::SealingType::default()))
}
) -> Result<Self::VerifiedStruct, SignatureError>;

/// Verifies the payload against the given `credential`.
/// The signature is fetched via the [`Verifiable::signature()`] function and
Expand All @@ -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)
}
21 changes: 14 additions & 7 deletions openmls/src/ciphersuite/tests/kat_crypto_basics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ impl SignedStruct<SignWithLabelTest> for MySignature {
}

impl Verifiable for ParsedSignWithLabel {
type VerifiedStruct = ();

fn unsigned_payload(&self) -> Result<Vec<u8>, tls_codec::Error> {
Ok(self.content.clone())
}
Expand All @@ -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::VerifiedStruct, crate::ciphersuite::signable::SignatureError> {
self.verify_no_out(crypto, pk)?;
Ok(())
}
}

// Dummy implementation
impl VerifiedStruct<ParsedSignWithLabel> for () {
type SealingType = u8;

fn from_verifiable(_: ParsedSignWithLabel, _seal: Self::SealingType) -> Self {}
}
impl VerifiedStruct for () {}

impl Signable for ParsedSignWithLabel {
type SignedOutput = MySignature;
Expand Down Expand Up @@ -294,7 +301,7 @@ pub fn run_test_vector(
// verify signature
parsed
.clone()
.verify::<()>(
.verify(
provider.crypto(),
&OpenMlsSignaturePublicKey::new(
public.clone().into(),
Expand All @@ -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(),
Expand Down
31 changes: 15 additions & 16 deletions openmls/src/framing/mls_auth_content_in.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -194,6 +188,8 @@ impl VerifiableAuthenticatedContentIn {
}

impl Verifiable for VerifiableAuthenticatedContentIn {
type VerifiedStruct = AuthenticatedContentIn;

fn unsigned_payload(&self) -> Result<Vec<u8>, tls_codec::Error> {
self.tbs.tls_serialize_detached()
}
Expand All @@ -205,20 +201,23 @@ impl Verifiable for VerifiableAuthenticatedContentIn {
fn label(&self) -> &str {
"FramedContentTBS"
}
}

impl VerifiedStruct<VerifiableAuthenticatedContentIn> 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::VerifiedStruct, signable::SignatureError> {
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<FramedContentTbsIn> for AuthenticatedContentIn {
fn from_payload(tbs: FramedContentTbsIn, signature: Signature) -> Self {
let auth = FramedContentAuthData {
Expand Down
33 changes: 17 additions & 16 deletions openmls/src/key_packages/key_package_in.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -31,6 +31,8 @@ impl VerifiableKeyPackage {
}

impl Verifiable for VerifiableKeyPackage {
type VerifiedStruct = KeyPackage;

fn unsigned_payload(&self) -> Result<Vec<u8>, tls_codec::Error> {
self.payload.tls_serialize_detached()
}
Expand All @@ -42,23 +44,22 @@ impl Verifiable for VerifiableKeyPackage {
fn label(&self) -> &str {
SIGNATURE_KEY_PACKAGE_LABEL
}
}

impl VerifiedStruct<VerifiableKeyPackage> 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::VerifiedStruct, SignatureError> {
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.
///
Expand Down Expand Up @@ -143,7 +144,7 @@ impl KeyPackageIn {

let leaf_node = match leaf_node {
VerifiableLeafNode::KeyPackage(leaf_node) => leaf_node
.verify::<LeafNode>(crypto, signature_key)
.verify(crypto, signature_key)
.map_err(|_| KeyPackageVerifyError::InvalidLeafNodeSignature)?,
_ => return Err(KeyPackageVerifyError::InvalidLeafNodeSourceType),
};
Expand All @@ -168,7 +169,7 @@ impl KeyPackageIn {

// Verify the KeyPackage signature
let key_package = VerifiableKeyPackage::new(key_package_tbs, self.signature)
.verify::<KeyPackage>(crypto, signature_key)
.verify(crypto, signature_key)
.map_err(|_| KeyPackageVerifyError::InvalidSignature)?;

// Extension included in the extensions or leaf_node.extensions fields
Expand Down
26 changes: 13 additions & 13 deletions openmls/src/messages/group_info.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ impl SignedStruct<GroupInfoTBS> for GroupInfo {
}

impl Verifiable for VerifiableGroupInfo {
type VerifiedStruct = GroupInfo;

fn unsigned_payload(&self) -> Result<Vec<u8>, tls_codec::Error> {
self.payload.tls_serialize_detached()
}
Expand All @@ -283,20 +285,18 @@ impl Verifiable for VerifiableGroupInfo {
fn label(&self) -> &str {
SIGNATURE_GROUP_INFO_LABEL
}
}

impl VerifiedStruct<VerifiableGroupInfo> 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::VerifiedStruct, crate::ciphersuite::signable::SignatureError> {
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 {}
Loading

0 comments on commit 3291f0d

Please sign in to comment.