fix: Protect BcsSignable behind a sealed trait pattern

BcsSignable is a marker trait that equips a type with sensitive functions that can fail.
This commit aims to augment the amount of scrutony on these types by forcing them to be
registered in a single place using a "sealed trait" pattern.

crates/sui-types/src/batch.rs

@@ -3,9 +3,7 @@
 
 use crate::base_types::{AuthorityName, ExecutionDigests};
 use crate::committee::{Committee, EpochId};
-use crate::crypto::{
-    sha3_hash, AuthoritySignInfo, AuthoritySignature, BcsSignable, SuiAuthoritySignature,
-};
+use crate::crypto::{sha3_hash, AuthoritySignInfo, AuthoritySignature, SuiAuthoritySignature};
 use crate::error::{SuiError, SuiResult};
 use serde::{Deserialize, Serialize};
 
@@ -22,7 +20,6 @@ pub type BatchDigest = [u8; 32];
 
 #[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Hash, Default, Debug, Serialize, Deserialize)]
 pub struct TransactionBatch(pub Vec<(TxSequenceNumber, ExecutionDigests)>);
-impl BcsSignable for TransactionBatch {}
 
 #[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Default, Debug, Serialize, Deserialize)]
 pub struct AuthorityBatch {
@@ -43,8 +40,6 @@ pub struct AuthorityBatch {
     pub transactions_digest: [u8; 32],
 }
 
-impl BcsSignable for AuthorityBatch {}
-
 impl AuthorityBatch {
     pub fn digest(&self) -> BatchDigest {
         sha3_hash(self)

crates/sui-types/src/crypto.rs

@@ -600,11 +600,36 @@ where
 /// Activate the blanket implementation of `Signable` based on serde and BCS.
 /// * We use `serde_name` to extract a seed from the name of structs and enums.
 /// * We use `BCS` to generate canonical bytes suitable for hashing and signing.
-pub trait BcsSignable: Serialize + serde::de::DeserializeOwned {}
+///
+/// # Safety
+/// We protect the access to this marker trait through a "sealed trait" pattern:
+/// impls must be add added here (nowehre else) which lets us note those impls
+/// MUST be on types that comply with the `serde_name` machinery
+/// for the below implementations not to panic. One way to check they work is to write
+/// a unit test for serialization to / deserialization from signable bytes.
+///
+///
+mod bcs_signable {
+
+    pub trait BcsSignable: serde::Serialize + serde::de::DeserializeOwned {}
+    impl BcsSignable for crate::batch::TransactionBatch {}
+    impl BcsSignable for crate::batch::AuthorityBatch {}
+    impl BcsSignable for crate::messages_checkpoint::CheckpointSummary {}
+    impl BcsSignable for crate::messages_checkpoint::CheckpointContents {}
+    impl BcsSignable for crate::messages_checkpoint::CheckpointProposalSummary {}
+    impl BcsSignable for crate::messages::TransactionEffects {}
+    impl BcsSignable for crate::messages::TransactionData {}
+    impl BcsSignable for crate::messages::EpochInfo {}
+    impl BcsSignable for crate::object::Object {}
+
+    impl BcsSignable for super::bcs_signable_test::Foo {}
+    #[cfg(test)]
+    impl BcsSignable for super::bcs_signable_test::Bar {}
+}
 
 impl<T, W> Signable<W> for T
 where
-    T: BcsSignable,
+    T: bcs_signable::BcsSignable,
     W: std::io::Write,
 {
     fn write(&self, writer: &mut W) {
@@ -617,12 +642,11 @@ where
 
 impl<T> SignableBytes for T
 where
-    T: BcsSignable,
+    T: bcs_signable::BcsSignable,
 {
     fn from_signable_bytes(bytes: &[u8]) -> Result<Self, Error> {
         // Remove name tag before deserialization using BCS
-        let name = serde_name::trace_name::<Self>()
-            .ok_or_else(|| anyhow::anyhow!("Self should be a struct or an enum"))?;
+        let name = serde_name::trace_name::<Self>().expect("Self should be a struct or an enum");
         let name_byte_len = format!("{}::", name).bytes().len();
         Ok(bcs::from_bytes(&bytes[name_byte_len..])?)
     }
@@ -680,3 +704,30 @@ impl<S: AggregateAuthenticator> VerificationObligation<S> {
         Ok(())
     }
 }
+
+pub mod bcs_signable_test {
+    use serde::{Deserialize, Serialize};
+
+    #[derive(Serialize, Deserialize)]
+    pub struct Foo(pub String);
+
+    #[cfg(test)]
+    #[derive(Serialize, Deserialize)]
+    pub struct Bar(pub String);
+
+    #[cfg(test)]
+    use super::{AggregateAuthoritySignature, VerificationObligation};
+
+    #[cfg(test)]
+    pub fn get_obligation_input<T>(
+        value: &T,
+    ) -> (VerificationObligation<AggregateAuthoritySignature>, usize)
+    where
+        T: super::bcs_signable::BcsSignable,
+    {
+        let mut obligation = VerificationObligation::default();
+        // Add the obligation of the authority signature verifications.
+        let idx = obligation.add_message(value);
+        (obligation, idx)
+    }
+}

crates/sui-types/src/messages.rs

@@ -6,8 +6,8 @@ use super::{base_types::*, batch::*, committee::Committee, error::*, event::Even
 use crate::committee::{EpochId, StakeUnit};
 use crate::crypto::{
     sha3_hash, AggregateAccountSignature, AggregateAuthoritySignature, AuthoritySignInfo,
-    AuthoritySignature, AuthorityStrongQuorumSignInfo, BcsSignable, EmptySignInfo, Signable,
-    Signature, SuiAuthoritySignature, VerificationObligation,
+    AuthoritySignature, AuthorityStrongQuorumSignInfo, EmptySignInfo, Signable, Signature,
+    SuiAuthoritySignature, VerificationObligation,
 };
 use crate::gas::GasCostSummary;
 use crate::messages_checkpoint::{CheckpointFragment, CheckpointSequenceNumber};
@@ -326,10 +326,7 @@ pub struct TransactionData {
     pub gas_budget: u64,
 }
 
-impl TransactionData
-where
-    Self: BcsSignable,
-{
+impl TransactionData {
     pub fn new(
         kind: TransactionKind,
         sender: SuiAddress,
@@ -1405,8 +1402,6 @@ impl TransactionEffects {
     }
 }
 
-impl BcsSignable for TransactionEffects {}
-
 impl Display for TransactionEffects {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         let mut writer = String::new();
@@ -1762,8 +1757,6 @@ impl Display for CertifiedTransaction {
     }
 }
 
-impl BcsSignable for TransactionData {}
-
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct ConsensusOutput {
     #[serde(with = "serde_bytes")]
@@ -1833,8 +1826,6 @@ pub struct EpochInfo {
     last_checkpoint: CheckpointSequenceNumber,
 }
 
-impl BcsSignable for EpochInfo {}
-
 impl EpochInfo {
     pub fn new(
         epoch: EpochId,

crates/sui-types/src/messages_checkpoint.rs

@@ -13,9 +13,7 @@ use crate::waypoint::{Waypoint, WaypointDiff};
 use crate::{
     base_types::AuthorityName,
     committee::Committee,
-    crypto::{
-        sha3_hash, AuthoritySignature, BcsSignable, SuiAuthoritySignature, VerificationObligation,
-    },
+    crypto::{sha3_hash, AuthoritySignature, SuiAuthoritySignature, VerificationObligation},
     error::SuiError,
 };
 use serde::{Deserialize, Serialize};
@@ -233,8 +231,6 @@ impl CheckpointSummary {
     }
 }
 
-impl BcsSignable for CheckpointSummary {}
-
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct CheckpointSummaryEnvelope<S> {
     pub summary: CheckpointSummary,
@@ -388,8 +384,6 @@ pub struct CheckpointContents {
     pub transactions: Vec<ExecutionDigests>,
 }
 
-impl BcsSignable for CheckpointContents {}
-
 // TODO: We should create a type for ordered contents,
 // instead of mixing them in the same type.
 // https://github.com/MystenLabs/sui/issues/3038
@@ -441,8 +435,6 @@ impl CheckpointProposalSummary {
     }
 }
 
-impl BcsSignable for CheckpointProposalSummary {}
-
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct SignedCheckpointProposalSummary {
     pub summary: CheckpointProposalSummary,

crates/sui-types/src/object.rs

@@ -16,7 +16,7 @@ use serde::{Deserialize, Serialize};
 use serde_with::serde_as;
 use serde_with::Bytes;
 
-use crate::crypto::{sha3_hash, BcsSignable};
+use crate::crypto::sha3_hash;
 use crate::error::{ExecutionError, ExecutionErrorKind};
 use crate::error::{SuiError, SuiResult};
 use crate::move_package::MovePackage;
@@ -360,8 +360,6 @@ pub struct Object {
     pub storage_rebate: u64,
 }
 
-impl BcsSignable for Object {}
-
 impl Object {
     /// Create a new Move object
     pub fn new_move(o: MoveObject, owner: Owner, previous_transaction: TransactionDigest) -> Self {

crates/sui-types/src/signature_seed.rs

@@ -170,13 +170,14 @@ impl SignatureSeed {
     ///
     /// ```
     /// use serde::{Deserialize, Serialize};
-    /// use sui_types::crypto::BcsSignable;
     /// use sui_types::signature_seed::SignatureSeed;
+    /// use sui_types::crypto::bcs_signable_test::Foo;
     ///
-    /// #[derive(Serialize, Deserialize)]
-    /// struct Foo(String);
-    ///
-    /// impl BcsSignable for Foo {}
+    /// // The BcsSignable trait is implemented as a sealed trait, so this is equivalent to the
+    /// // following, with the BcsSignable impl. mandatorily situated in the bcs_signable module:
+    /// // #[derive(Serialize, Deserialize)]
+    /// // struct Foo(String);
+    /// // impl BcsSignable for Foo {}
     ///
     /// # fn main() {
     ///     // In production this SHOULD be a secret seed value, here we pin it for demo purposes.

crates/sui-types/src/unit_tests/base_types_tests.rs

@@ -9,26 +9,17 @@ use std::str::FromStr;
 use base64ct::{Base64, Encoding};
 use move_binary_format::file_format;
 
+use crate::crypto::bcs_signable_test::{Bar, Foo};
 use crate::crypto::{get_key_pair_from_bytes, AuthoritySignature, KeyPair, SuiAuthoritySignature};
 use crate::{
-    crypto::{get_key_pair, BcsSignable, Signature},
+    crypto::{get_key_pair, Signature},
     gas_coin::GasCoin,
     object::Object,
     SUI_FRAMEWORK_ADDRESS,
 };
 
 use super::*;
 
-#[derive(Serialize, Deserialize)]
-struct Foo(String);
-
-impl BcsSignable for Foo {}
-
-#[derive(Serialize, Deserialize)]
-struct Bar(String);
-
-impl BcsSignable for Bar {}
-
 #[test]
 fn test_signatures() {
     let (addr1, sec1) = get_key_pair();

crates/sui-types/src/unit_tests/messages_tests.rs

@@ -7,6 +7,7 @@ use std::collections::BTreeMap;
 use narwhal_crypto::traits::KeyPair;
 use roaring::RoaringBitmap;
 
+use crate::crypto::bcs_signable_test::{get_obligation_input, Foo};
 use crate::crypto::{get_key_pair, PublicKeyBytes};
 use crate::object::Owner;
 
@@ -152,10 +153,6 @@ fn test_certificates() {
     assert!(SignatureAggregator::try_new(bad_transaction, &committee).is_err());
 }
 
-#[derive(Serialize, Deserialize)]
-struct Foo(String);
-impl BcsSignable for Foo {}
-
 #[test]
 fn test_new_with_signatures() {
     let mut signatures: Vec<(AuthorityName, AuthoritySignature)> = Vec::new();
@@ -190,18 +187,6 @@ fn test_new_with_signatures() {
     );
 }
 
-fn get_obligation_input<T>(
-    value: &T,
-) -> (VerificationObligation<AggregateAuthoritySignature>, usize)
-where
-    T: BcsSignable,
-{
-    let mut obligation = VerificationObligation::default();
-    // Add the obligation of the authority signature verifications.
-    let idx = obligation.add_message(value);
-    (obligation, idx)
-}
-
 #[test]
 fn test_handle_reject_malicious_signature() {
     let message: messages_tests::Foo = Foo("some data".to_string());

crates/sui-types/src/unit_tests/signature_seed_tests.rs

@@ -1,21 +1,13 @@
 // Copyright (c) 2022, Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::crypto::BcsSignable;
-use crate::signature_seed::SignatureSeed;
-use serde::{Deserialize, Serialize};
+use crate::{crypto::bcs_signable_test::Foo, signature_seed::SignatureSeed};
 
 #[cfg(test)]
 const TEST_ID: [u8; 16] = [0u8; 16];
 #[cfg(test)]
 const TEST_DOMAIN: [u8; 16] = [1u8; 16];
 
-#[cfg(test)]
-#[derive(Serialize, Deserialize)]
-struct Foo(String);
-
-impl BcsSignable for Foo {}
-
 #[test]
 fn test_deterministic_addresses_by_id() {
     let seed = SignatureSeed::default();