chore(crypto): Reflect the message hash in signature verification errors #3152
Conversation
This makes it easier to diagnose signture verification issues from logs or user-provided error messages.
randombit
changed the title
| @@ -115,6 +115,7 @@ pub fn verify_individual( | |||
| algorithm: AlgorithmId::MultiBls12_381, | |||
| public_key_bytes: public_key_bytes.0.to_vec(), | |||
| sig_bytes: signature_bytes.0.to_vec(), | |||
| msg_hash: Some(message.to_vec()), | |||
There was a problem hiding this comment.
IIUC, message is not a hash here, but the output of Signable::as_signed_bytes which uses SignedBytesWithoutDomainSeparator::as_signed_bytes_without_domain_separator, which depends on the actual struct that is signed.
Looking at pub trait Crypto, multi signatures are done for NotarizationContent and FinalizationContent, and both implement SignedBytesWithoutDomainSeparator by serializing self with CBOR.
Same for verify_combined.
| @@ -239,6 +239,7 @@ pub(crate) fn verify_individual_sig( | |||
| algorithm: AlgorithmId::ThresBls12_381, | |||
| public_key_bytes: PublicKeyBytes::from(public_key).0.to_vec(), | |||
| sig_bytes: IndividualSignatureBytes::from(signature).0.to_vec(), | |||
| msg_hash: Some(message.to_vec()), | |||
There was a problem hiding this comment.
Similar comment as for multi-sigs. message here is for many types not a hash. As far as I can see (based on pub trait Crypto), it is only a hash for CertificationContent, but not for CatchUpContent, CatchUpContentProtobufBytes, RandomBeaconContent, and RandomTapeContent.
Same for verify_combined_sig.
| @@ -193,6 +194,7 @@ impl CspSigner for Csp { | |||
| algorithm: algorithm_id, | |||
| public_key_bytes: signer.as_ref().to_vec(), | |||
| sig_bytes: signature.0.to_vec(), | |||
| msg_hash: Some(msg.to_vec()), | |||
There was a problem hiding this comment.
See my other comment: I believe msg is not a hash here in practise. Given that the public key here is anyway of the wrong type, including the message in the error also doesn't seem super helpful.
| @@ -341,6 +341,7 @@ pub enum CryptoError { | |||
| algorithm: AlgorithmId, | |||
| public_key_bytes: Vec<u8>, | |||
| sig_bytes: Vec<u8>, | |||
| msg_hash: Option<Vec<u8>>, | |||
There was a problem hiding this comment.
Do I understand correctly that you want to include the message hash here because including the full message would cause potentially very big log entries?
There was a problem hiding this comment.
Partially this, also partially for confidentiality reasons in case the message contains sensitive information. Additionally for ECDSA the crypto component IIRC never sees the actual message but just the hash.
|
Closing in favor of #3239 |
This makes it easier to diagnose signature verification issues from logs or user-provided error messages.