From 59381da037f74df2b5f5b62ab5ad3a412e78d243 Mon Sep 17 00:00:00 2001 From: lovesh Date: Fri, 6 Mar 2020 01:52:16 +0530 Subject: [PATCH 01/17] broken branch Signed-off-by: lovesh --- README.md | 8 ++--- runtime/src/did.rs | 90 ++++++++++++++++++++++++++++++++++++---------- runtime/src/lib.rs | 2 +- 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index eea040aa4..2d437f9ac 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,10 @@ cargo run -- \ Additional CLI usage options are available and may be shown by running `cargo run -- --help`. +## Polkadot-js UI +To use this chain from [polkadot-js UI](https://polkadot.js.org/apps), some structures need to be created in the `Settings > Developer` section. +The structures can be found in [developer.json file](./developer.json). + ## Advanced: Generate Your Own Substrate Node Template A substrate node template is always based on a certain version of Substrate. You can inspect it by @@ -100,7 +104,3 @@ git checkout Noted though you will likely get faster and more thorough support if you stick with the releases provided in this repository. - -## Polkadot-js UI -To use this chain from [polkadot-js UI](https://polkadot.js.org/apps), some structures need to be created in the `Settings > Developer` section. -The structures can be found in [developer.json file](./developer.json). \ No newline at end of file diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 2e19e8359..1e6a8c58e 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,6 +1,6 @@ use super::{BlockNumber, DID, DID_BYTE_SIZE}; use codec::{Decode, Encode}; -use frame_support::{decl_event, decl_module, decl_storage, dispatch::DispatchResult, traits::Get}; +use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchResult, ensure, traits::Get}; use sp_std::prelude::Vec; /// The module's configuration trait. @@ -10,8 +10,16 @@ pub trait Trait: system::Trait { //type DIDByteSize: Get; } -//pub const DID_BYTE_SIZE: usize = 32; +decl_error! { + /// Error for the token module. + pub enum Error for Module { + /// There is already a DID with same value + DIDAlreadyExists, + } +} +/// Cryptographic algorithm of public key +/// like `Ed25519VerificationKey2018`, `Secp256k1VerificationKey2018` and `Sr25519VerificationKey2018` #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub enum PublicKeyType { Sr25519, @@ -25,6 +33,9 @@ impl Default for PublicKeyType { } } +/// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. +/// `public_key_type` is the type of the key +/// `public_key` is the public key and it is accepted and stored as raw bytes. #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyDetail { controller: DID, @@ -47,6 +58,14 @@ impl Default for KeyDetail { } } +impl KeyDetail { + pub fn new(controller: DID, public_key_type: PublicKeyType, public_key: Vec) -> Self { + KeyDetail { + controller, public_key, public_key_type + } + } +} + /// This struct is passed as an argument while updating the key /// `cmd` is the command, in case of key update it will be 1. /// `did` is the DID whose key is being updated. @@ -81,16 +100,14 @@ decl_event!( where AccountId = ::AccountId, { - DIDAdded(Vec), - DIDAlreadyExists(Vec), + DIDAdded(DID), DummyEvent(AccountId), } ); decl_storage! { - trait Store for Module as DidModule { - Dids get(did): map DID => (KeyDetail, T::BlockNumber); - //Dids: map [u8; 32] => (KeyDetail, T::BlockNumber); + trait Store for Module as DIDModule { + DIDs get(did): map DID => (KeyDetail, T::BlockNumber); } } @@ -101,14 +118,16 @@ decl_module! { //fn new(_origin, did: [u8; DID_BYTE_SIZE], detail: KeyDetail) -> DispatchResult { /// Create a new DID. /// `did` is the new DID to create. The method will throw exception if `did` is already registered. + /// `detail` is the details of the key like its type, controller and value fn new(_origin, did: DID, detail: KeyDetail) -> DispatchResult { - if Dids::::exists(did) { - Self::deposit_event(RawEvent::DIDAlreadyExists(did.to_vec())); - } else { - let current_block_no = >::block_number(); - Dids::::insert(did, (detail, current_block_no)); - Self::deposit_event(RawEvent::DIDAdded(did.to_vec())); - } + ensure!( + !DIDs::::exists(did), + Error::::DIDAlreadyExists + ); + + let current_block_no = >::block_number(); + DIDs::::insert(did, (detail, current_block_no)); + Self::deposit_event(RawEvent::DIDAdded(did)); Ok(()) } @@ -127,6 +146,8 @@ decl_module! { // TODO: Ok(()) } + + // TODO: Add sig verification method that can be used by any other module as well. } } @@ -202,16 +223,49 @@ mod tests { .into() } - type DidModule = super::Module; + type DIDModule = super::Module; + + // TODO: Add test for Event DIDAdded #[test] fn new_did_test_case() { new_test_ext().execute_with(|| { let alice = 10u64; - let bob = 20u64; - let charlie = 30u64; - // TODO: Write test + let did = [1; DID_BYTE_SIZE]; + let pk = vec![0, 1]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + + // Add a DID + assert_ok!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ) + ); + + // Try to add the same DID and same key detail again and fail + assert_err!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ), + Error::::DIDAlreadyExists + ); + + // Try to add the same DID again but with different key detail and fail + let pk = vec![0, 1, 9, 10, 12]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + assert_err!( + DIDModule::new( + Origin::signed(alice), + did, + detail + ), + Error::::DIDAlreadyExists + ); }); } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 754728cdb..e1605774f 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -269,7 +269,7 @@ construct_runtime!( Sudo: sudo, // Used for the module template in `./template.rs` TemplateModule: template::{Module, Call, Storage, Event}, - DidModule: did::{Module, Call, Storage, Event}, + DIDModule: did::{Module, Call, Storage, Event}, RandomnessCollectiveFlip: randomness_collective_flip::{Module, Call, Storage}, } ); From d2e28bfe215c7aa2e4231ab3039ea939071fab7e Mon Sep 17 00:00:00 2001 From: lovesh Date: Fri, 6 Mar 2020 17:29:19 +0530 Subject: [PATCH 02/17] check for max key size Signed-off-by: lovesh --- runtime/src/did.rs | 137 +++++++++++++++++++++++++++++++++++++++++---- runtime/src/lib.rs | 6 +- 2 files changed, 131 insertions(+), 12 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 1e6a8c58e..b2a209416 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,7 +1,8 @@ -use super::{BlockNumber, DID, DID_BYTE_SIZE}; +use super::{BlockNumber, DID, DID_BYTE_SIZE, PK_MAX_BYTE_SIZE}; use codec::{Decode, Encode}; use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchResult, ensure, traits::Get}; use sp_std::prelude::Vec; +use system::ensure_signed; /// The module's configuration trait. pub trait Trait: system::Trait { @@ -13,8 +14,15 @@ pub trait Trait: system::Trait { decl_error! { /// Error for the token module. pub enum Error for Module { + /// Given public key is larger than the maximum supported size + LargePublicKey, /// There is already a DID with same value DIDAlreadyExists, + /// There is no such DID + DIDDoesNotExist, + /// For replay protection, an update to state is required to contain the same block number + /// in which the last update was performed. + DifferentBlockNumber } } @@ -27,6 +35,7 @@ pub enum PublicKeyType { Secp256k1, } +/// Default is chosen since its Parity's default algo and due to Parity's reasoning. impl Default for PublicKeyType { fn default() -> Self { PublicKeyType::Sr25519 @@ -59,23 +68,34 @@ impl Default for KeyDetail { } impl KeyDetail { + /// Create new key detail pub fn new(controller: DID, public_key_type: PublicKeyType, public_key: Vec) -> Self { + // XXX: size of public_key can be checked here as well. But this will require making the return + // type a result and an attacker can craft a struct without using this method anyway. + // This can be addressed later KeyDetail { controller, public_key, public_key_type } } + + /// Check if the public key is not bigger than the maximum allowed size + pub fn is_public_key_size_acceptable(&self) -> bool { + self.public_key.len() <= PK_MAX_BYTE_SIZE + } } /// This struct is passed as an argument while updating the key -/// `cmd` is the command, in case of key update it will be 1. /// `did` is the DID whose key is being updated. /// `public_key_type` is new public key type /// `public_key` the new public key /// `controller` If provided None, the controller is unchanged. While serializing, use literal "None" when controller is None -/// The last_modified_in_block is the block number when this DID was last modified is present to prevent replay attack +/// The last_modified_in_block is the block number when this DID was last modified is present to prevent replay attack. +/// This approach allows only 1 update transaction in a block (don't see it as a problem as key updates are rare). +/// An alternate approach can be to have a nonce associated to each detail which is incremented on each +/// successful extrinsic and the chain requiring the extrinsic's nonce to be higher than current. This is +/// little more involved as it involves a ">" check #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyUpdate { - cmd: u8, //did: [u8; DID_BYTE_SIZE], did: DID, public_key_type: PublicKeyType, @@ -84,13 +104,29 @@ pub struct KeyUpdate { last_modified_in_block: BlockNumber, } +impl KeyUpdate { + /// Create new key update to update key of the `did`. + /// Pass `controller` as None when not wishing to change the existing controller + pub fn new(did: DID, public_key_type: PublicKeyType, public_key: Vec, controller: Option, last_modified_in_block: BlockNumber) -> Self { + // XXX: size of public_key can be checked here as well. But this will require making the return + // type a result and an attacker can craft a struct without using this method anyway. + // This can be addressed later + KeyUpdate { + did, public_key_type, public_key, controller, last_modified_in_block + } + } + + /// Check if the public key is not bigger than the maximum allowed size + pub fn is_public_key_size_acceptable(&self) -> bool { + self.public_key.len() <= PK_MAX_BYTE_SIZE + } +} + /// This struct is passed as an argument while removing the DID -/// `cmd` is the command, in case of DID removal it is 2. /// `did` is the DID which is being removed. /// `last_modified_in_block` is the block number when this DID was last modified. The last modified time is present to prevent replay attack. #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct DIDRemoval { - cmd: u8, did: DID, last_modified_in_block: BlockNumber, } @@ -115,11 +151,22 @@ decl_module! { pub struct Module for enum Call where origin: T::Origin { fn deposit_event() = default; + type Error = Error; + //fn new(_origin, did: [u8; DID_BYTE_SIZE], detail: KeyDetail) -> DispatchResult { /// Create a new DID. /// `did` is the new DID to create. The method will throw exception if `did` is already registered. /// `detail` is the details of the key like its type, controller and value - fn new(_origin, did: DID, detail: KeyDetail) -> DispatchResult { + fn new(origin, did: DID, detail: KeyDetail) -> DispatchResult { + ensure_signed(origin)?; + + // public key is not huge + ensure!( + detail.is_public_key_size_acceptable(), + Error::::LargePublicKey + ); + + // DID is not registered already ensure!( !DIDs::::exists(did), Error::::DIDAlreadyExists @@ -131,18 +178,51 @@ decl_module! { Ok(()) } + /// `key_update` specifies which DID's which key needs to be updated /// `signature` is the signature on the serialized `KeyUpdate`. /// The node while processing this extrinsic, should create the above serialized `KeyUpdate` /// using the stored data and try to verify the given signature with the stored key. - pub fn update_key(_origin, key_update: KeyUpdate, signature: Vec) -> DispatchResult { + pub fn update_key(origin, key_update: KeyUpdate, signature: Vec) -> DispatchResult { + ensure_signed(origin)?; + + // public key is not huge + ensure!( + key_update.is_public_key_size_acceptable(), + Error::::LargePublicKey + ); + + // DID must be registered + ensure!( + DIDs::::exists(key_update.did), + Error::::DIDAlreadyExists + ); + + let (current_key_detail, last_modified_in_block) = DIDs::::get(key_update.did); + + // replay protection: the key update should contain the last block in which the key was modified + ensure!( + last_modified_in_block == T::BlockNumber::from(key_update.last_modified_in_block), + Error::::DifferentBlockNumber + ); + + let serz_key_update: Vec = key_update.encode(); // TODO: Ok(()) } /// `to_remove` contains the DID to be removed /// `signature` is the signature on the serialized `DIDRemoval`. - /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` using the stored data and try to verify the given signature with the stored key. - pub fn remove(_origin, to_remove: DIDRemoval, signature: Vec) -> DispatchResult { + /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` + /// using the stored data and try to verify the given signature with the stored key. + pub fn remove(origin, to_remove: DIDRemoval, signature: Vec) -> DispatchResult { + ensure_signed(origin)?; + + // DID must be registered + ensure!( + DIDs::::exists(to_remove.did), + Error::::DIDAlreadyExists + ); + // TODO: Ok(()) } @@ -226,9 +306,32 @@ mod tests { type DIDModule = super::Module; // TODO: Add test for Event DIDAdded + // TODO: Add test for adding DIDs larger than 32 bytes #[test] - fn new_did_test_case() { + fn public_key_must_have_acceptable_size() { + // Public key must not be very large + + // Smaller public key is fine + let did = [1; DID_BYTE_SIZE]; + let pk = vec![2u8; PK_MAX_BYTE_SIZE-1]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + assert!(detail.is_public_key_size_acceptable()); + + // public key with max size is fine + let pk = vec![2u8; PK_MAX_BYTE_SIZE]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + assert!(detail.is_public_key_size_acceptable()); + + // public key with larger than max size is not fine + let pk = vec![2u8; PK_MAX_BYTE_SIZE+1]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + assert!(!detail.is_public_key_size_acceptable()); + } + + #[test] + fn did_creation_tests() { + // DID must be unique. It must have an acceptable public size new_test_ext().execute_with(|| { let alice = 10u64; @@ -266,6 +369,18 @@ mod tests { ), Error::::DIDAlreadyExists ); + + // public key with larger than max size is not fine + let pk = vec![2u8; PK_MAX_BYTE_SIZE+1]; + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + assert_err!( + DIDModule::new( + Origin::signed(alice), + did, + detail + ), + Error::::LargePublicKey + ); }); } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index e1605774f..0d916a125 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -61,9 +61,13 @@ pub type Hash = sp_core::H256; /// Digest item type. pub type DigestItem = generic::DigestItem; -// explore using `parameter_types!` for DID_BYTE_SIZE. The problem is that its needed for defining DID +// XXX: explore using `parameter_types!` for DID_BYTE_SIZE. The problem is that its needed for defining DID +/// Size of the Dock DID in bytes pub const DID_BYTE_SIZE: usize = 32; +/// The type of the Dock DID pub type DID = [u8; DID_BYTE_SIZE]; +/// Maximum allowed size of a public key in bytes. +pub const PK_MAX_BYTE_SIZE: usize = 1024; /// Used for the module template in `./template.rs` mod template; From df911f9896246be261d165668d744a9a31c3e009 Mon Sep 17 00:00:00 2001 From: lovesh Date: Fri, 6 Mar 2020 23:22:26 +0530 Subject: [PATCH 03/17] add signature verification for sr25519 Signed-off-by: lovesh --- README.md | 4 ++ runtime/src/did.rs | 140 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2d437f9ac..26ac240e7 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,10 @@ Additional CLI usage options are available and may be shown by running `cargo ru To use this chain from [polkadot-js UI](https://polkadot.js.org/apps), some structures need to be created in the `Settings > Developer` section. The structures can be found in [developer.json file](./developer.json). +## Dev tips +1. For faster builds during testing use flag `SKIP_WASM_BUILD=1`. This will not generate WASM but only native code. +1. To use `println!` like Rust in `decl_module`'s functions, run the test or binary with flag `SKIP_WASM_BUILD=1` + ## Advanced: Generate Your Own Substrate Node Template A substrate node template is always based on a certain version of Substrate. You can inspect it by diff --git a/runtime/src/did.rs b/runtime/src/did.rs index b2a209416..57b9ea0f9 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,8 +1,11 @@ use super::{BlockNumber, DID, DID_BYTE_SIZE, PK_MAX_BYTE_SIZE}; use codec::{Decode, Encode}; -use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchResult, ensure, traits::Get}; +use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get}; use sp_std::prelude::Vec; use system::ensure_signed; +use sp_std::convert::TryFrom; +use sp_core::{ed25519, sr25519, ecdsa}; +use sp_runtime::traits::Verify; /// The module's configuration trait. pub trait Trait: system::Trait { @@ -22,7 +25,11 @@ decl_error! { DIDDoesNotExist, /// For replay protection, an update to state is required to contain the same block number /// in which the last update was performed. - DifferentBlockNumber + DifferentBlockNumber, + /// Signature verification failed while key update + InvalidSigForKeyUpdate, + /// Signature verification failed while DID removal + InvalidSigForDIDRemoval } } @@ -32,7 +39,8 @@ decl_error! { pub enum PublicKeyType { Sr25519, Ed25519, - Secp256k1, + // TODO: Uncomment + //Secp256k1, } /// Default is chosen since its Parity's default algo and due to Parity's reasoning. @@ -42,6 +50,9 @@ impl Default for PublicKeyType { } } +// TODO: Consider combining the public key and its type in a enum. That way the enum variant can know the public key +// size as well and i don't have to rely on checking input vector length. + /// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. /// `public_key_type` is the type of the key /// `public_key` is the public key and it is accepted and stored as raw bytes. @@ -137,6 +148,7 @@ decl_event!( AccountId = ::AccountId, { DIDAdded(DID), + KeyUpdated(DID), DummyEvent(AccountId), } ); @@ -191,13 +203,15 @@ decl_module! { Error::::LargePublicKey ); + // Not checking for signature size as its not stored + // DID must be registered ensure!( DIDs::::exists(key_update.did), Error::::DIDAlreadyExists ); - let (current_key_detail, last_modified_in_block) = DIDs::::get(key_update.did); + let (mut current_key_detail, last_modified_in_block) = DIDs::::get(key_update.did); // replay protection: the key update should contain the last block in which the key was modified ensure!( @@ -205,8 +219,26 @@ decl_module! { Error::::DifferentBlockNumber ); - let serz_key_update: Vec = key_update.encode(); - // TODO: + // serialize to bytes + let serz_key_update = key_update.encode(); + + let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key_type, ¤t_key_detail.public_key)?; + + // Signature should be valid + ensure!(sig_ver == true, Error::::InvalidSigForKeyUpdate); + + // Key update is safe to do + let current_block_no = >::block_number(); + current_key_detail.public_key_type = key_update.public_key_type; + current_key_detail.public_key = key_update.public_key; + + // If key update specified a controller, then only update the current controller + if let Some(ctrl) = key_update.controller { + current_key_detail.controller = ctrl; + } + + DIDs::::insert(key_update.did, (current_key_detail, current_block_no)); + Self::deposit_event(RawEvent::KeyUpdated(key_update.did)); Ok(()) } @@ -231,6 +263,37 @@ decl_module! { } } +impl Module { + pub fn verify_sig(signature: &[u8], message: &[u8], public_key_type: &PublicKeyType, public_key: &[u8]) -> Result { + Ok( + match *public_key_type { + // Fixme: Remove unwraps + PublicKeyType::Sr25519 => { + // XXX: `&` does not work for taking reference + //let signature = sr25519::Signature::try_from(&signature).unwrap(); + let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let mut pk_bytes: [u8; 32] = [0; 32]; + // Fixme: Assuming key has same length + pk_bytes.clone_from_slice(public_key); + let pk = sr25519::Public(pk_bytes); + signature.verify(message, &pk) + } + PublicKeyType::Ed25519 => { + let signature = ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let mut pk_bytes: [u8; 32] = [0; 32]; + // Fixme: Assuming key has same length + pk_bytes.clone_from_slice(public_key); + let pk = ed25519::Public(pk_bytes); + signature.verify(message, &pk) + } + /*case PublicKeyType::Secp256k1 => { + + }*/ + } + ) + } +} + #[cfg(test)] mod tests { use super::*; @@ -238,7 +301,7 @@ mod tests { use frame_support::{ assert_err, assert_ok, impl_outer_origin, parameter_types, weights::Weight, }; - use sp_core::H256; + use sp_core::{H256, Pair}; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup, OnFinalize, OnInitialize}, @@ -262,6 +325,7 @@ mod tests { impl system::Trait for Test { type Origin = Origin; type Index = u64; + // XXX: Why is it u64 when in lib.rs its u32 type BlockNumber = u64; type Call = (); type Hash = H256; @@ -306,7 +370,8 @@ mod tests { type DIDModule = super::Module; // TODO: Add test for Event DIDAdded - // TODO: Add test for adding DIDs larger than 32 bytes + // TODO: Add test for Event KeyUpdated + // TODO: Add test for public key len check in KeyUpdate #[test] fn public_key_must_have_acceptable_size() { @@ -330,7 +395,7 @@ mod tests { } #[test] - fn did_creation_tests() { + fn did_creation() { // DID must be unique. It must have an acceptable public size new_test_ext().execute_with(|| { let alice = 10u64; @@ -383,4 +448,61 @@ mod tests { ); }); } + + #[test] + fn did_key_update() { + // DID must be unique. It must have an acceptable public size + new_test_ext().execute_with(|| { + let alice = 100u64; + + let did = [1; DID_BYTE_SIZE]; + let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_1 = pair_1.public().0.to_vec(); + + let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk_1.clone()); + + // Add a DID + assert_ok!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + + // Correctly update DID's key. + // Prepare a key update + let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_2 = pair_2.public().0.to_vec(); + let key_update = KeyUpdate::new(did.clone(), PublicKeyType::Sr25519, pk_2.clone(), None, modified_in_block as u32); + let sig = pair_1.sign(&key_update.encode()); + + // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + sig.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + + // Maliciously update DID's key. + // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) + let key_update = KeyUpdate::new(did.clone(), PublicKeyType::Sr25519, pk_1.clone(), None, modified_in_block as u32); + let sig = pair_1.sign(&key_update.encode()); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + sig.0.to_vec() + ), + Error::::InvalidSigForKeyUpdate + ); + }); + } } From e21916b8cd890431e1fa8ed49ef92207865aacd8 Mon Sep 17 00:00:00 2001 From: lovesh Date: Sat, 7 Mar 2020 03:07:57 +0530 Subject: [PATCH 04/17] Abstracted the public key type and value in an enum. This commit builds but has broken tests. This commit will be squashed Signed-off-by: lovesh --- runtime/src/did.rs | 190 ++++++++++++++++++--------------------------- runtime/src/lib.rs | 2 - 2 files changed, 77 insertions(+), 115 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 57b9ea0f9..29f3f7390 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,4 +1,4 @@ -use super::{BlockNumber, DID, DID_BYTE_SIZE, PK_MAX_BYTE_SIZE}; +use super::{BlockNumber, DID, DID_BYTE_SIZE}; use codec::{Decode, Encode}; use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get}; use sp_std::prelude::Vec; @@ -17,8 +17,8 @@ pub trait Trait: system::Trait { decl_error! { /// Error for the token module. pub enum Error for Module { - /// Given public key is larger than the maximum supported size - LargePublicKey, + /// Given public key is not of the correct size + PublicKeySizeIncorrect, /// There is already a DID with same value DIDAlreadyExists, /// There is no such DID @@ -50,6 +50,54 @@ impl Default for PublicKeyType { } } +/// Size of a Sr25519 public key in bytes. +pub const Sr25519_PK_BYTE_SIZE: usize = 32; +/// Size of a Ed25519 public key in bytes. +pub const Ed25519_PK_BYTE_SIZE: usize = 32; + +// XXX: This could have been a tuple struct. Keeping it a normal struct for Substrate UI +/// A wrapper over 32-byte array +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub struct Bytes32 { + value: [u8; 32] +} + +impl Default for Bytes32 { + fn default() -> Self { + Self {value: [0; 32]} + } +} + +/// An abstraction for a public key. Abstracts the type and value of the public key where the value is a +/// byte array +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub enum PublicKey { + Sr25519(Bytes32), + Ed25519(Bytes32) +} + +impl Default for PublicKey { + fn default() -> Self { + PublicKey::Sr25519(Bytes32::default()) + } +} + +// XXX: Substrate UI can't parse them. Maybe later versions will fix it. +/*#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub enum PublicKey { + Sr25519([u8; 32]), + Ed25519([u8; 32]) +}*/ + +/*#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub enum PublicKey { + Sr25519(Bytes32), + Ed25519(Bytes32) +} + +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub struct Bytes32(pub [u8;32]);*/ + // TODO: Consider combining the public key and its type in a enum. That way the enum variant can know the public key // size as well and i don't have to rely on checking input vector length. @@ -59,10 +107,7 @@ impl Default for PublicKeyType { #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyDetail { controller: DID, - //controller: [u8; DID_BYTE_SIZE], - //controller: [u8; 32], - public_key_type: PublicKeyType, - public_key: Vec, + public_key: PublicKey } // XXX: Map requires having a default value for DIDDetail @@ -70,29 +115,21 @@ impl Default for KeyDetail { fn default() -> Self { KeyDetail { controller: DID::default(), - //controller: [0; 32], - //controller: DID, - public_key_type: PublicKeyType::default(), - public_key: Vec::new(), + public_key: PublicKey::default(), } } } impl KeyDetail { /// Create new key detail - pub fn new(controller: DID, public_key_type: PublicKeyType, public_key: Vec) -> Self { + pub fn new(controller: DID, public_key: PublicKey) -> Self { // XXX: size of public_key can be checked here as well. But this will require making the return // type a result and an attacker can craft a struct without using this method anyway. // This can be addressed later KeyDetail { - controller, public_key, public_key_type + controller, public_key } } - - /// Check if the public key is not bigger than the maximum allowed size - pub fn is_public_key_size_acceptable(&self) -> bool { - self.public_key.len() <= PK_MAX_BYTE_SIZE - } } /// This struct is passed as an argument while updating the key @@ -107,10 +144,8 @@ impl KeyDetail { /// little more involved as it involves a ">" check #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyUpdate { - //did: [u8; DID_BYTE_SIZE], did: DID, - public_key_type: PublicKeyType, - public_key: Vec, + public_key: PublicKey, controller: Option, last_modified_in_block: BlockNumber, } @@ -118,19 +153,15 @@ pub struct KeyUpdate { impl KeyUpdate { /// Create new key update to update key of the `did`. /// Pass `controller` as None when not wishing to change the existing controller - pub fn new(did: DID, public_key_type: PublicKeyType, public_key: Vec, controller: Option, last_modified_in_block: BlockNumber) -> Self { + pub fn new(did: DID, public_key: PublicKey, controller: Option, + last_modified_in_block: BlockNumber) -> Self { // XXX: size of public_key can be checked here as well. But this will require making the return // type a result and an attacker can craft a struct without using this method anyway. // This can be addressed later KeyUpdate { - did, public_key_type, public_key, controller, last_modified_in_block + did, public_key, controller, last_modified_in_block } } - - /// Check if the public key is not bigger than the maximum allowed size - pub fn is_public_key_size_acceptable(&self) -> bool { - self.public_key.len() <= PK_MAX_BYTE_SIZE - } } /// This struct is passed as an argument while removing the DID @@ -165,19 +196,12 @@ decl_module! { type Error = Error; - //fn new(_origin, did: [u8; DID_BYTE_SIZE], detail: KeyDetail) -> DispatchResult { /// Create a new DID. /// `did` is the new DID to create. The method will throw exception if `did` is already registered. /// `detail` is the details of the key like its type, controller and value fn new(origin, did: DID, detail: KeyDetail) -> DispatchResult { ensure_signed(origin)?; - // public key is not huge - ensure!( - detail.is_public_key_size_acceptable(), - Error::::LargePublicKey - ); - // DID is not registered already ensure!( !DIDs::::exists(did), @@ -197,12 +221,6 @@ decl_module! { pub fn update_key(origin, key_update: KeyUpdate, signature: Vec) -> DispatchResult { ensure_signed(origin)?; - // public key is not huge - ensure!( - key_update.is_public_key_size_acceptable(), - Error::::LargePublicKey - ); - // Not checking for signature size as its not stored // DID must be registered @@ -222,14 +240,13 @@ decl_module! { // serialize to bytes let serz_key_update = key_update.encode(); - let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key_type, ¤t_key_detail.public_key)?; + let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key)?; // Signature should be valid ensure!(sig_ver == true, Error::::InvalidSigForKeyUpdate); // Key update is safe to do let current_block_no = >::block_number(); - current_key_detail.public_key_type = key_update.public_key_type; current_key_detail.public_key = key_update.public_key; // If key update specified a controller, then only update the current controller @@ -258,32 +275,25 @@ decl_module! { // TODO: Ok(()) } - - // TODO: Add sig verification method that can be used by any other module as well. } } impl Module { - pub fn verify_sig(signature: &[u8], message: &[u8], public_key_type: &PublicKeyType, public_key: &[u8]) -> Result { + /// Verify given signature on the given message with given public key + pub fn verify_sig(signature: &[u8], message: &[u8], public_key: &PublicKey) -> Result { Ok( - match *public_key_type { + match public_key { // Fixme: Remove unwraps - PublicKeyType::Sr25519 => { + PublicKey::Sr25519(bytes) => { // XXX: `&` does not work for taking reference //let signature = sr25519::Signature::try_from(&signature).unwrap(); let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; - let mut pk_bytes: [u8; 32] = [0; 32]; - // Fixme: Assuming key has same length - pk_bytes.clone_from_slice(public_key); - let pk = sr25519::Public(pk_bytes); + let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } - PublicKeyType::Ed25519 => { + PublicKey::Ed25519(bytes) => { let signature = ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; - let mut pk_bytes: [u8; 32] = [0; 32]; - // Fixme: Assuming key has same length - pk_bytes.clone_from_slice(public_key); - let pk = ed25519::Public(pk_bytes); + let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } /*case PublicKeyType::Secp256k1 => { @@ -342,18 +352,6 @@ mod tests { type ModuleToIndex = (); } - /*impl balances::Trait for Test { - type Balance = u64; - type OnNewAccount = (); - type OnFreeBalanceZero = (); - type Event = (); - type TransferPayment = (); - type DustRemoval = (); - type ExistentialDeposit = ExistentialDeposit; - type TransferFee = TransferFee; - type CreationFee = CreationFee; - }*/ - impl super::Trait for Test { type Event = (); } @@ -371,28 +369,6 @@ mod tests { // TODO: Add test for Event DIDAdded // TODO: Add test for Event KeyUpdated - // TODO: Add test for public key len check in KeyUpdate - - #[test] - fn public_key_must_have_acceptable_size() { - // Public key must not be very large - - // Smaller public key is fine - let did = [1; DID_BYTE_SIZE]; - let pk = vec![2u8; PK_MAX_BYTE_SIZE-1]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); - assert!(detail.is_public_key_size_acceptable()); - - // public key with max size is fine - let pk = vec![2u8; PK_MAX_BYTE_SIZE]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); - assert!(detail.is_public_key_size_acceptable()); - - // public key with larger than max size is not fine - let pk = vec![2u8; PK_MAX_BYTE_SIZE+1]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); - assert!(!detail.is_public_key_size_acceptable()); - } #[test] fn did_creation() { @@ -401,8 +377,8 @@ mod tests { let alice = 10u64; let did = [1; DID_BYTE_SIZE]; - let pk = vec![0, 1]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + let pk = PublicKey::default(); + let detail = KeyDetail::new(did.clone(), pk); // Add a DID assert_ok!( @@ -424,8 +400,8 @@ mod tests { ); // Try to add the same DID again but with different key detail and fail - let pk = vec![0, 1, 9, 10, 12]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); + let pk = PublicKey::Ed25519(Bytes32::default()); + let detail = KeyDetail::new(did.clone(), pk); assert_err!( DIDModule::new( Origin::signed(alice), @@ -434,32 +410,20 @@ mod tests { ), Error::::DIDAlreadyExists ); - - // public key with larger than max size is not fine - let pk = vec![2u8; PK_MAX_BYTE_SIZE+1]; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk); - assert_err!( - DIDModule::new( - Origin::signed(alice), - did, - detail - ), - Error::::LargePublicKey - ); }); } #[test] fn did_key_update() { - // DID must be unique. It must have an acceptable public size + // DID's key must be updatable with the authorized key only new_test_ext().execute_with(|| { let alice = 100u64; let did = [1; DID_BYTE_SIZE]; let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); - let pk_1 = pair_1.public().0.to_vec(); + let pk_1 = pair_1.public().0; - let detail = KeyDetail::new(did.clone(), PublicKeyType::Sr25519, pk_1.clone()); + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1})); // Add a DID assert_ok!( @@ -475,8 +439,8 @@ mod tests { // Correctly update DID's key. // Prepare a key update let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); - let pk_2 = pair_2.public().0.to_vec(); - let key_update = KeyUpdate::new(did.clone(), PublicKeyType::Sr25519, pk_2.clone(), None, modified_in_block as u32); + let pk_2 = pair_2.public().0; + let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_2}), None, modified_in_block as u32); let sig = pair_1.sign(&key_update.encode()); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) @@ -492,7 +456,7 @@ mod tests { // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) - let key_update = KeyUpdate::new(did.clone(), PublicKeyType::Sr25519, pk_1.clone(), None, modified_in_block as u32); + let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1}), None, modified_in_block as u32); let sig = pair_1.sign(&key_update.encode()); assert_err!( diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 0d916a125..002222442 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -66,8 +66,6 @@ pub type DigestItem = generic::DigestItem; pub const DID_BYTE_SIZE: usize = 32; /// The type of the Dock DID pub type DID = [u8; DID_BYTE_SIZE]; -/// Maximum allowed size of a public key in bytes. -pub const PK_MAX_BYTE_SIZE: usize = 1024; /// Used for the module template in `./template.rs` mod template; From c6031b413505c6d170e2587390d8eaafdf272b67 Mon Sep 17 00:00:00 2001 From: lovesh Date: Sat, 7 Mar 2020 16:02:49 +0530 Subject: [PATCH 05/17] Signature verification with Secp256k1 and update test Signed-off-by: lovesh --- runtime/src/did.rs | 242 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 209 insertions(+), 33 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 29f3f7390..0ab8c6c90 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -6,6 +6,7 @@ use system::ensure_signed; use sp_std::convert::TryFrom; use sp_core::{ed25519, sr25519, ecdsa}; use sp_runtime::traits::Verify; +use sp_std::fmt; /// The module's configuration trait. pub trait Trait: system::Trait { @@ -39,8 +40,7 @@ decl_error! { pub enum PublicKeyType { Sr25519, Ed25519, - // TODO: Uncomment - //Secp256k1, + Secp256k1, } /// Default is chosen since its Parity's default algo and due to Parity's reasoning. @@ -68,12 +68,44 @@ impl Default for Bytes32 { } } +// XXX: This could have been a tuple struct. Keeping it a normal struct for Substrate UI +/// A wrapper over 33-byte array +#[derive(Encode, Decode, Clone)] +pub struct Bytes33 { + value: [u8; 33] +} + +impl Default for Bytes33 { + fn default() -> Self { + Self {value: [0; 33]} + } +} + +/// Implementing Debug for Bytes33 as it cannot be automatically derived for arrays of size > 32 +impl fmt::Debug for Bytes33 { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.value[..].fmt(f) + } +} + +/// Implementing PartialEq for Bytes33 as it cannot be automatically derived for arrays of size > 32 +impl PartialEq for Bytes33 { + fn eq(&self, other: &Bytes33) -> bool { + self.value[..] == other.value[..] + } +} +impl Eq for Bytes33 {} + /// An abstraction for a public key. Abstracts the type and value of the public key where the value is a /// byte array #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub enum PublicKey { + /// Public key for Sr25519 is 32 bytes Sr25519(Bytes32), - Ed25519(Bytes32) + /// Public key for Ed25519 is 32 bytes + Ed25519(Bytes32), + /// Compressed public key for Secp256k1 is 33 bytes + Secp256k1(Bytes33) } impl Default for PublicKey { @@ -98,9 +130,6 @@ pub enum PublicKey { #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct Bytes32(pub [u8;32]);*/ -// TODO: Consider combining the public key and its type in a enum. That way the enum variant can know the public key -// size as well and i don't have to rely on checking input vector length. - /// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. /// `public_key_type` is the type of the key /// `public_key` is the public key and it is accepted and stored as raw bytes. @@ -123,9 +152,6 @@ impl Default for KeyDetail { impl KeyDetail { /// Create new key detail pub fn new(controller: DID, public_key: PublicKey) -> Self { - // XXX: size of public_key can be checked here as well. But this will require making the return - // type a result and an attacker can craft a struct without using this method anyway. - // This can be addressed later KeyDetail { controller, public_key } @@ -226,7 +252,7 @@ decl_module! { // DID must be registered ensure!( DIDs::::exists(key_update.did), - Error::::DIDAlreadyExists + Error::::DIDDoesNotExist ); let (mut current_key_detail, last_modified_in_block) = DIDs::::get(key_update.did); @@ -237,15 +263,16 @@ decl_module! { Error::::DifferentBlockNumber ); - // serialize to bytes + // serialize `KeyUpdate` to bytes let serz_key_update = key_update.encode(); + // Verify signature on the serialized `KeyUpdate` with the current public key let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key)?; - // Signature should be valid + // Throw error if signature is invalid ensure!(sig_ver == true, Error::::InvalidSigForKeyUpdate); - // Key update is safe to do + // Key update is safe to do, update the block number as well. let current_block_no = >::block_number(); current_key_detail.public_key = key_update.public_key; @@ -283,10 +310,7 @@ impl Module { pub fn verify_sig(signature: &[u8], message: &[u8], public_key: &PublicKey) -> Result { Ok( match public_key { - // Fixme: Remove unwraps PublicKey::Sr25519(bytes) => { - // XXX: `&` does not work for taking reference - //let signature = sr25519::Signature::try_from(&signature).unwrap(); let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) @@ -296,9 +320,11 @@ impl Module { let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } - /*case PublicKeyType::Secp256k1 => { - - }*/ + PublicKey::Secp256k1(bytes) => { + let signature = ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let pk = ecdsa::Public::Compressed(bytes.value.clone()); + signature.verify(message, &pk) + } } ) } @@ -317,6 +343,7 @@ mod tests { traits::{BlakeTwo256, IdentityLookup, OnFinalize, OnInitialize}, Perbill, }; + use crate::did::PublicKeyType::Sr25519; impl_outer_origin! { pub enum Origin for Test {} @@ -414,16 +441,140 @@ mod tests { } #[test] - fn did_key_update() { - // DID's key must be updatable with the authorized key only + fn did_key_update_for_unregistered_did() { + // Updating a DID that has not been registered yet should fail new_test_ext().execute_with(|| { let alice = 100u64; let did = [1; DID_BYTE_SIZE]; - let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); - let pk_1 = pair_1.public().0; - let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1})); + let (pair, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk = pair.public().0; + let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk }), None, 2u32); + let sig = pair.sign(&key_update.encode()); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + sig.0.to_vec() + ), + Error::::DIDDoesNotExist + ); + }); + } + + #[test] + fn did_key_update_with_sr25519_ed25519_keys() { + // DID's key must be updatable with the authorized key only. Check for sr25519 and ed25519 + new_test_ext().execute_with(|| { + let alice = 100u64; + + /// Macro to check the key update for ed25519 and sr25519 + macro_rules! check_key_update { + ( $did:ident, $module:ident, $pk:expr ) => {{ + let (pair_1, _, _) = $module::Pair::generate_with_phrase(None); + let pk_1 = pair_1.public().0; + + let detail = KeyDetail::new($did.clone(), $pk(Bytes32 {value: pk_1})); + + // Add a DID + assert_ok!( + DIDModule::new( + Origin::signed(alice), + $did.clone(), + detail.clone() + ) + ); + + let (_, modified_in_block) = DIDModule::did($did.clone()); + + // Correctly update DID's key. + // Prepare a key update + let (pair_2, _, _) = $module::Pair::generate_with_phrase(None); + let pk_2 = pair_2.public().0; + let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_2}), None, modified_in_block as u32); + let sig = pair_1.sign(&key_update.encode()); + + // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + sig.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did($did.clone()); + + // Maliciously update DID's key. + // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) + let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let sig = pair_1.sign(&key_update.encode()); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + sig.0.to_vec() + ), + Error::::InvalidSigForKeyUpdate + ); + + // Check key update with signature of incorrect size + // Use the correct key + let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let sig = pair_2.sign(&key_update.encode()); + + // Truncate the signature to be of shorter size + let mut short_sig = sig.0.to_vec(); + short_sig.truncate(10); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + short_sig + ), + Error::::InvalidSigForKeyUpdate + ); + + // Add extra bytes to the signature to be of longer size + let mut long_sig = sig.0.to_vec(); + long_sig.append(&mut vec![0, 1, 2, 0]); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + long_sig + ), + Error::::InvalidSigForKeyUpdate + ); + }}; + } + + let did = [1; DID_BYTE_SIZE]; + check_key_update!(did, sr25519, PublicKey::Sr25519); + + let did = [2; DID_BYTE_SIZE]; + check_key_update!(did, ed25519, PublicKey::Ed25519); + }); + } + + #[test] + fn did_key_update_with_ecdsa_key() { + // DID's key must be updatable with the authorized key only. Check for secp256k1. + // The logic is same as above test but the way to generate keys, sig is little different. By creating abstractions + // just for testing, above and this test can be merged. + new_test_ext().execute_with(|| { + let alice = 100u64; + + let did = [1; DID_BYTE_SIZE]; + + let (pair_1, _, _) = ecdsa::Pair::generate_with_phrase(None); + let pk_1= pair_1.public().as_compressed().unwrap(); + let detail = KeyDetail::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_1})); // Add a DID assert_ok!( @@ -438,17 +589,16 @@ mod tests { // Correctly update DID's key. // Prepare a key update - let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); - let pk_2 = pair_2.public().0; - let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_2}), None, modified_in_block as u32); - let sig = pair_1.sign(&key_update.encode()); - + let (pair_2, _, _) = ecdsa::Pair::generate_with_phrase(None); + let pk_2= pair_2.public().as_compressed().unwrap(); + let key_update = KeyUpdate::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_2}), None, modified_in_block as u32); + let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) assert_ok!( DIDModule::update_key( Origin::signed(alice), key_update.clone(), - sig.0.to_vec() + sig.to_vec() ) ); @@ -456,14 +606,40 @@ mod tests { // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) - let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1}), None, modified_in_block as u32); - let sig = pair_1.sign(&key_update.encode()); + let key_update = KeyUpdate::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_1}), None, modified_in_block as u32); + let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); assert_err!( DIDModule::update_key( Origin::signed(alice), key_update.clone(), - sig.0.to_vec() + sig.to_vec() + ), + Error::::InvalidSigForKeyUpdate + ); + + // Truncate the signature to be of shorter size + let mut short_sig = sig.to_vec(); + short_sig.truncate(10); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + short_sig + ), + Error::::InvalidSigForKeyUpdate + ); + + // Add extra bytes to the signature to be of longer size + let mut long_sig = sig.to_vec(); + long_sig.append(&mut vec![0, 1, 2, 0]); + + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update.clone(), + long_sig ), Error::::InvalidSigForKeyUpdate ); From 0de1f896316e7368f2daa668bc2afaf8bc817262 Mon Sep 17 00:00:00 2001 From: lovesh Date: Sat, 7 Mar 2020 17:12:32 +0530 Subject: [PATCH 06/17] Add DID removal Signed-off-by: lovesh --- runtime/src/did.rs | 303 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 255 insertions(+), 48 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 0ab8c6c90..f8126ef56 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,4 +1,4 @@ -use super::{BlockNumber, DID, DID_BYTE_SIZE}; +use super::{BlockNumber, DID}; use codec::{Decode, Encode}; use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get}; use sp_std::prelude::Vec; @@ -27,33 +27,15 @@ decl_error! { /// For replay protection, an update to state is required to contain the same block number /// in which the last update was performed. DifferentBlockNumber, - /// Signature verification failed while key update - InvalidSigForKeyUpdate, - /// Signature verification failed while DID removal - InvalidSigForDIDRemoval + /// Signature verification failed while key update or did removal + InvalidSig } } -/// Cryptographic algorithm of public key -/// like `Ed25519VerificationKey2018`, `Secp256k1VerificationKey2018` and `Sr25519VerificationKey2018` -#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] -pub enum PublicKeyType { - Sr25519, - Ed25519, - Secp256k1, -} - -/// Default is chosen since its Parity's default algo and due to Parity's reasoning. -impl Default for PublicKeyType { - fn default() -> Self { - PublicKeyType::Sr25519 - } -} - -/// Size of a Sr25519 public key in bytes. +/*/// Size of a Sr25519 public key in bytes. pub const Sr25519_PK_BYTE_SIZE: usize = 32; /// Size of a Ed25519 public key in bytes. -pub const Ed25519_PK_BYTE_SIZE: usize = 32; +pub const Ed25519_PK_BYTE_SIZE: usize = 32;*/ // XXX: This could have been a tuple struct. Keeping it a normal struct for Substrate UI /// A wrapper over 32-byte array @@ -130,6 +112,8 @@ pub enum PublicKey { #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct Bytes32(pub [u8;32]);*/ +// TODO: Update developer.json with the new fields + /// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. /// `public_key_type` is the type of the key /// `public_key` is the public key and it is accepted and stored as raw bytes. @@ -160,7 +144,6 @@ impl KeyDetail { /// This struct is passed as an argument while updating the key /// `did` is the DID whose key is being updated. -/// `public_key_type` is new public key type /// `public_key` the new public key /// `controller` If provided None, the controller is unchanged. While serializing, use literal "None" when controller is None /// The last_modified_in_block is the block number when this DID was last modified is present to prevent replay attack. @@ -199,6 +182,13 @@ pub struct DIDRemoval { last_modified_in_block: BlockNumber, } +impl DIDRemoval { + /// Remove an existing DID `did` + pub fn new(did: DID, last_modified_in_block: BlockNumber) -> Self { + DIDRemoval {did, last_modified_in_block} + } +} + decl_event!( pub enum Event where @@ -206,6 +196,7 @@ decl_event!( { DIDAdded(DID), KeyUpdated(DID), + DIDRemoved(DID), DummyEvent(AccountId), } ); @@ -270,11 +261,12 @@ decl_module! { let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key)?; // Throw error if signature is invalid - ensure!(sig_ver == true, Error::::InvalidSigForKeyUpdate); + ensure!(sig_ver == true, Error::::InvalidSig); // Key update is safe to do, update the block number as well. let current_block_no = >::block_number(); current_key_detail.public_key = key_update.public_key; + println!("update in block no={}", current_block_no); // If key update specified a controller, then only update the current controller if let Some(ctrl) = key_update.controller { @@ -296,10 +288,29 @@ decl_module! { // DID must be registered ensure!( DIDs::::exists(to_remove.did), - Error::::DIDAlreadyExists + Error::::DIDDoesNotExist ); - // TODO: + let (current_key_detail, last_modified_in_block) = DIDs::::get(to_remove.did); + + // replay protection: the removal command should contain the last block in which the key was modified + ensure!( + last_modified_in_block == T::BlockNumber::from(to_remove.last_modified_in_block), + Error::::DifferentBlockNumber + ); + + // serialize `DIDRemoval` to bytes + let serz_rem = to_remove.encode(); + + // Verify signature on the serialized `KeyUpdate` with the current public key + let sig_ver = Self::verify_sig(&signature, &serz_rem, ¤t_key_detail.public_key)?; + + // Throw error if signature is invalid + ensure!(sig_ver == true, Error::::InvalidSig); + + // Remove DID + DIDs::::remove(to_remove.did); + Self::deposit_event(RawEvent::DIDRemoved(to_remove.did)); Ok(()) } } @@ -311,17 +322,17 @@ impl Module { Ok( match public_key { PublicKey::Sr25519(bytes) => { - let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Ed25519(bytes) => { - let signature = ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let signature = ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Secp256k1(bytes) => { - let signature = ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSigForKeyUpdate)?; + let signature = ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; let pk = ecdsa::Public::Compressed(bytes.value.clone()); signature.verify(message, &pk) } @@ -333,6 +344,7 @@ impl Module { #[cfg(test)] mod tests { use super::*; + use super::super::DID_BYTE_SIZE; use frame_support::{ assert_err, assert_ok, impl_outer_origin, parameter_types, weights::Weight, @@ -340,10 +352,9 @@ mod tests { use sp_core::{H256, Pair}; use sp_runtime::{ testing::Header, - traits::{BlakeTwo256, IdentityLookup, OnFinalize, OnInitialize}, + traits::{BlakeTwo256, IdentityLookup}, Perbill, }; - use crate::did::PublicKeyType::Sr25519; impl_outer_origin! { pub enum Origin for Test {} @@ -394,9 +405,6 @@ mod tests { type DIDModule = super::Module; - // TODO: Add test for Event DIDAdded - // TODO: Add test for Event KeyUpdated - #[test] fn did_creation() { // DID must be unique. It must have an acceptable public size @@ -456,7 +464,7 @@ mod tests { assert_err!( DIDModule::update_key( Origin::signed(alice), - key_update.clone(), + key_update, sig.0.to_vec() ), Error::::DIDDoesNotExist @@ -487,7 +495,8 @@ mod tests { ) ); - let (_, modified_in_block) = DIDModule::did($did.clone()); + let (current_detail, modified_in_block) = DIDModule::did($did.clone()); + assert_eq!(current_detail.controller, $did); // Correctly update DID's key. // Prepare a key update @@ -500,12 +509,14 @@ mod tests { assert_ok!( DIDModule::update_key( Origin::signed(alice), - key_update.clone(), + key_update, sig.0.to_vec() ) ); - let (_, modified_in_block) = DIDModule::did($did.clone()); + let (current_detail, modified_in_block) = DIDModule::did($did.clone()); + // Since key update passed None for the controller, it should not change + assert_eq!(current_detail.controller, $did); // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) @@ -515,12 +526,28 @@ mod tests { assert_err!( DIDModule::update_key( Origin::signed(alice), - key_update.clone(), + key_update, sig.0.to_vec() ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig + ); + + // Keep the public key same but update the controller + let new_controller = [9; DID_BYTE_SIZE]; + let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_2}), Some(new_controller), modified_in_block as u32); + let sig = pair_2.sign(&key_update.encode()); + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + ) ); + // Since key update passed a new controller, it should be reflected + let (current_detail, _) = DIDModule::did($did.clone()); + assert_eq!(current_detail.controller, new_controller); + // Check key update with signature of incorrect size // Use the correct key let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_1}), None, modified_in_block as u32); @@ -536,7 +563,7 @@ mod tests { key_update.clone(), short_sig ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig ); // Add extra bytes to the signature to be of longer size @@ -546,10 +573,10 @@ mod tests { assert_err!( DIDModule::update_key( Origin::signed(alice), - key_update.clone(), + key_update, long_sig ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig ); }}; } @@ -597,7 +624,7 @@ mod tests { assert_ok!( DIDModule::update_key( Origin::signed(alice), - key_update.clone(), + key_update, sig.to_vec() ) ); @@ -615,7 +642,7 @@ mod tests { key_update.clone(), sig.to_vec() ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig ); // Truncate the signature to be of shorter size @@ -628,7 +655,7 @@ mod tests { key_update.clone(), short_sig ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig ); // Add extra bytes to the signature to be of longer size @@ -641,8 +668,188 @@ mod tests { key_update.clone(), long_sig ), - Error::::InvalidSigForKeyUpdate + Error::::InvalidSig ); }); } + + #[test] + fn did_key_update_replay_protection() { + // FIXME: Block number does not increase with extrinsics. Make them + // A `KeyUpdate` payload should not be replayable + // Add a DID with `pk_1`. + // `pk_1` changes key to `pk_2` and `pk_2` changes key to `pk_3` and `pk_3` changes key back to `pk_1`. + // It should not be possible to replay `pk_1`'s original message and change key to `pk_2`. + + new_test_ext().execute_with(|| { + let alice = 100u64; + + let did = [1; DID_BYTE_SIZE]; + + let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_1 = pair_1.public().0; + + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1})); + + // Add a DID with key `pk_1` + assert_ok!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + println!("block number1={}", modified_in_block); + + let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_2 = pair_2.public().0; + + // The following key update and signature will be included in a replay attempt to change key to `pk_2` without `pk_1`'s intent + let key_update_to_be_replayed = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_2}), None, modified_in_block as u32); + let sig_to_be_replayed = pair_1.sign(&key_update_to_be_replayed.encode()); + // Update key from `pk_1` to `pk_2` using `pk_1`'s signature + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update_to_be_replayed.clone(), + sig_to_be_replayed.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + println!("block number2={}", modified_in_block); + + let (pair_3, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_3 = pair_3.public().0; + + let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_3}), None, modified_in_block as u32); + let sig = pair_2.sign(&key_update.encode()); + // Update key from `pk_2` to `pk_3` using `pk_2`'s signature + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + println!("block number3={}", modified_in_block); + + let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let sig = pair_3.sign(&key_update.encode()); + // Update key from `pk_3` to `pk_1` using `pk_3`'s signature + assert_ok!( + DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + println!("block number4={}", modified_in_block); + + // Attempt to replay `pk_1`'s older payload for key update to `pk_2` + assert_err!( + DIDModule::update_key( + Origin::signed(alice), + key_update_to_be_replayed, + sig_to_be_replayed.0.to_vec() + ), + Error::::DifferentBlockNumber + ); + }); + } + + #[test] + fn did_remove() { + // Remove DID. Unregistered DIDs cannot be removed. + // Registered DIDs can only be removed by the authorized key + // Removed DIDs can be added again + + new_test_ext().execute_with(|| { + let alice = 100u64; + + let did = [1; DID_BYTE_SIZE]; + + let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_1 = pair_1.public().0; + let to_remove = DIDRemoval::new(did.clone(), 2u32); + let sig = pair_1.sign(&to_remove.encode()); + + // Trying to remove the DID before it was added will fail + assert_err!( + DIDModule::remove( + Origin::signed(alice), + to_remove, + sig.0.to_vec() + ), + Error::::DIDDoesNotExist + ); + + // Add a DID + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_1})); + assert_ok!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + + // The block number will be non zero as write was successful and will be 1 since its the first extrinsic + assert_eq!(modified_in_block, 1); + + // A key not controlling the DID but trying to remove the DID should fail + let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); + let pk_2 = pair_2.public().0; + let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); + let sig = pair_2.sign(&to_remove.encode()); + assert_err!( + DIDModule::remove( + Origin::signed(alice), + to_remove, + sig.0.to_vec() + ), + Error::::InvalidSig + ); + + // The key controlling the DID should be able to remove the DID + let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); + let sig = pair_1.sign(&to_remove.encode()); + assert_ok!( + DIDModule::remove( + Origin::signed(alice), + to_remove, + sig.0.to_vec() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + // The block number will be 0 as the did has been removed + assert_eq!(modified_in_block, 0); + + // A different public key than previous owner of the DID should be able to register the DID + // Add the same DID but with different public key + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_2})); + assert_ok!( + DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + ) + ); + + let (_, modified_in_block) = DIDModule::did(did.clone()); + // The block number will be non zero as the did has been written + assert_ne!(modified_in_block, 0); + }); + } + + // TODO: Add test for events DIDAdded, KeyUpdated, DIDRemoval } From 6576a280263c5f236ea52f5eca7bef8aea312a08 Mon Sep 17 00:00:00 2001 From: lovesh Date: Mon, 9 Mar 2020 18:59:12 +0530 Subject: [PATCH 07/17] Add utility to increase block number for and fix test Signed-off-by: lovesh --- developer.json | 26 ++++++------ runtime/src/did.rs | 98 +++++++++++++++++++++++++++------------------- 2 files changed, 72 insertions(+), 52 deletions(-) diff --git a/developer.json b/developer.json index ce54ca5e1..09863d91f 100644 --- a/developer.json +++ b/developer.json @@ -1,27 +1,29 @@ { "DID": "[u8;32]", - "PublicKeyType": { - "_enum": [ - "Sr25519", - "Ed25519", - "Secp256k1" - ] + "Bytes32": { + "value": "[u8;32]" + }, + "Bytes33": { + "value": "[u8;33]" + }, + "PublicKey": { + "_enum": { + "Sr25519": "Bytes32", + "Ed25519": "Bytes32", + "Secp256k1": "Bytes33" + } }, "KeyDetail": { "controller": "DID", - "public_key_type": "PublicKeyType", - "public_key": "Vec" + "public_key": "PublicKey" }, "KeyUpdate": { - "cmd": "u8", "did": "DID", - "public_key_type": "PublicKeyType", - "public_key": "Vec", + "public_key": "PublicKey", "controller": "Option", "last_modified_in_block": "u64" }, "DIDRemoval": { - "cmd": "u8", "did": "DID", "last_modified_in_block": "u64" } diff --git a/runtime/src/did.rs b/runtime/src/did.rs index f8126ef56..b03a5ac3c 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,6 +1,6 @@ use super::{BlockNumber, DID}; use codec::{Decode, Encode}; -use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get}; +use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get, fail}; use sp_std::prelude::Vec; use system::ensure_signed; use sp_std::convert::TryFrom; @@ -112,8 +112,6 @@ pub enum PublicKey { #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct Bytes32(pub [u8;32]);*/ -// TODO: Update developer.json with the new fields - /// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. /// `public_key_type` is the type of the key /// `public_key` is the public key and it is accepted and stored as raw bytes. @@ -236,23 +234,10 @@ decl_module! { /// The node while processing this extrinsic, should create the above serialized `KeyUpdate` /// using the stored data and try to verify the given signature with the stored key. pub fn update_key(origin, key_update: KeyUpdate, signature: Vec) -> DispatchResult { - ensure_signed(origin)?; - // Not checking for signature size as its not stored - // DID must be registered - ensure!( - DIDs::::exists(key_update.did), - Error::::DIDDoesNotExist - ); - - let (mut current_key_detail, last_modified_in_block) = DIDs::::get(key_update.did); - - // replay protection: the key update should contain the last block in which the key was modified - ensure!( - last_modified_in_block == T::BlockNumber::from(key_update.last_modified_in_block), - Error::::DifferentBlockNumber - ); + // DID is registered and the update is not being replayed + let mut current_key_detail = Self::ensure_registered_and_new(origin, &key_update.did, key_update.last_modified_in_block)?; // serialize `KeyUpdate` to bytes let serz_key_update = key_update.encode(); @@ -266,7 +251,6 @@ decl_module! { // Key update is safe to do, update the block number as well. let current_block_no = >::block_number(); current_key_detail.public_key = key_update.public_key; - println!("update in block no={}", current_block_no); // If key update specified a controller, then only update the current controller if let Some(ctrl) = key_update.controller { @@ -283,21 +267,8 @@ decl_module! { /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` /// using the stored data and try to verify the given signature with the stored key. pub fn remove(origin, to_remove: DIDRemoval, signature: Vec) -> DispatchResult { - ensure_signed(origin)?; - - // DID must be registered - ensure!( - DIDs::::exists(to_remove.did), - Error::::DIDDoesNotExist - ); - - let (current_key_detail, last_modified_in_block) = DIDs::::get(to_remove.did); - - // replay protection: the removal command should contain the last block in which the key was modified - ensure!( - last_modified_in_block == T::BlockNumber::from(to_remove.last_modified_in_block), - Error::::DifferentBlockNumber - ); + // DID is registered and the removal is not being replayed + let current_key_detail = Self::ensure_registered_and_new(origin, &to_remove.did, to_remove.last_modified_in_block)?; // serialize `DIDRemoval` to bytes let serz_rem = to_remove.encode(); @@ -317,6 +288,29 @@ decl_module! { } impl Module { + /// Ensure that the DID is registered and this is not a replayed payload by checking the equality + /// with stored block number when the DID was last modified. + pub fn ensure_registered_and_new(origin: T::Origin, did: &DID, last_modified_in_block: BlockNumber) -> Result { + ensure_signed(origin)?; + + let (current_key_detail, last_modified) = DIDs::::get(did); + + // DID must be registered. If its not the above `get` with return default values making `last_modified` as 0. + // Not doing an `exists` call on the map as that will result in 2 calls (1 for `exists`, 1 for `get`) to storage in + // the case when the DID is registered. + if last_modified == T::BlockNumber::from(0) { + fail!(Error::::DIDDoesNotExist) + } + + // replay protection: the command should contain the last block in which the DID was modified + ensure!( + last_modified == T::BlockNumber::from(last_modified_in_block), + Error::::DifferentBlockNumber + ); + + Ok(current_key_detail) + } + /// Verify given signature on the given message with given public key pub fn verify_sig(signature: &[u8], message: &[u8], public_key: &PublicKey) -> Result { Ok( @@ -352,7 +346,7 @@ mod tests { use sp_core::{H256, Pair}; use sp_runtime::{ testing::Header, - traits::{BlakeTwo256, IdentityLookup}, + traits::{BlakeTwo256, IdentityLookup, OnFinalize, OnInitialize}, Perbill, }; @@ -405,6 +399,19 @@ mod tests { type DIDModule = super::Module; + pub type System = system::Module; + + /// Changes the block number. Calls `on_finalize` and `on_initialize` + pub fn run_to_block(n: u64) { + while System::block_number() < n { + if System::block_number() > 1 { + System::on_finalize(System::block_number()); + } + System::set_block_number(System::block_number() + 1); + System::on_initialize(System::block_number()); + } + } + #[test] fn did_creation() { // DID must be unique. It must have an acceptable public size @@ -675,7 +682,6 @@ mod tests { #[test] fn did_key_update_replay_protection() { - // FIXME: Block number does not increase with extrinsics. Make them // A `KeyUpdate` payload should not be replayable // Add a DID with `pk_1`. // `pk_1` changes key to `pk_2` and `pk_2` changes key to `pk_3` and `pk_3` changes key back to `pk_1`. @@ -700,8 +706,11 @@ mod tests { ) ); + // Block number should increase to 1 as extrinsic is successful + run_to_block(1); + assert_eq!(System::block_number(), 1); + let (_, modified_in_block) = DIDModule::did(did.clone()); - println!("block number1={}", modified_in_block); let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; @@ -718,8 +727,11 @@ mod tests { ) ); + // Block number should increase to 2 as extrinsic is successful + run_to_block(2); + assert_eq!(System::block_number(), 2); + let (_, modified_in_block) = DIDModule::did(did.clone()); - println!("block number2={}", modified_in_block); let (pair_3, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_3 = pair_3.public().0; @@ -735,8 +747,11 @@ mod tests { ) ); + // Block number should increase to 3 as extrinsic is successful + run_to_block(3); + assert_eq!(System::block_number(), 3); + let (_, modified_in_block) = DIDModule::did(did.clone()); - println!("block number3={}", modified_in_block); let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1}), None, modified_in_block as u32); let sig = pair_3.sign(&key_update.encode()); @@ -749,8 +764,11 @@ mod tests { ) ); + // Block number should increase to 4 as extrinsic is successful + run_to_block(4); + assert_eq!(System::block_number(), 4); + let (_, modified_in_block) = DIDModule::did(did.clone()); - println!("block number4={}", modified_in_block); // Attempt to replay `pk_1`'s older payload for key update to `pk_2` assert_err!( From e104138a85013595a20e910378a46cd1e3f478c7 Mon Sep 17 00:00:00 2001 From: lovesh Date: Tue, 10 Mar 2020 16:41:35 +0530 Subject: [PATCH 08/17] Refactoring Signed-off-by: lovesh --- runtime/src/did.rs | 477 ++++++++++++++++++++++++--------------------- 1 file changed, 250 insertions(+), 227 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index b03a5ac3c..f7f1d93a8 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,12 +1,15 @@ use super::{BlockNumber, DID}; use codec::{Decode, Encode}; -use frame_support::{decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, dispatch::DispatchResult, ensure, traits::Get, fail}; -use sp_std::prelude::Vec; -use system::ensure_signed; -use sp_std::convert::TryFrom; -use sp_core::{ed25519, sr25519, ecdsa}; +use frame_support::{ + decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, + dispatch::DispatchResult, ensure, fail, traits::Get, +}; +use sp_core::{ecdsa, ed25519, sr25519}; use sp_runtime::traits::Verify; +use sp_std::convert::TryFrom; use sp_std::fmt; +use sp_std::prelude::Vec; +use system::ensure_signed; /// The module's configuration trait. pub trait Trait: system::Trait { @@ -16,20 +19,20 @@ pub trait Trait: system::Trait { } decl_error! { - /// Error for the token module. - pub enum Error for Module { - /// Given public key is not of the correct size - PublicKeySizeIncorrect, - /// There is already a DID with same value - DIDAlreadyExists, - /// There is no such DID - DIDDoesNotExist, - /// For replay protection, an update to state is required to contain the same block number - /// in which the last update was performed. - DifferentBlockNumber, - /// Signature verification failed while key update or did removal - InvalidSig - } + /// Error for the token module. + pub enum Error for Module { + /// Given public key is not of the correct size + PublicKeySizeIncorrect, + /// There is already a DID with same value + DIDAlreadyExists, + /// There is no such DID + DIDDoesNotExist, + /// For replay protection, an update to state is required to contain the same block number + /// in which the last update was performed. + DifferentBlockNumber, + /// Signature verification failed while key update or did removal + InvalidSig + } } /*/// Size of a Sr25519 public key in bytes. @@ -41,12 +44,12 @@ pub const Ed25519_PK_BYTE_SIZE: usize = 32;*/ /// A wrapper over 32-byte array #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct Bytes32 { - value: [u8; 32] + value: [u8; 32], } impl Default for Bytes32 { fn default() -> Self { - Self {value: [0; 32]} + Self { value: [0; 32] } } } @@ -54,12 +57,12 @@ impl Default for Bytes32 { /// A wrapper over 33-byte array #[derive(Encode, Decode, Clone)] pub struct Bytes33 { - value: [u8; 33] + value: [u8; 33], } impl Default for Bytes33 { fn default() -> Self { - Self {value: [0; 33]} + Self { value: [0; 33] } } } @@ -87,7 +90,7 @@ pub enum PublicKey { /// Public key for Ed25519 is 32 bytes Ed25519(Bytes32), /// Compressed public key for Secp256k1 is 33 bytes - Secp256k1(Bytes33) + Secp256k1(Bytes33), } impl Default for PublicKey { @@ -118,7 +121,7 @@ pub struct Bytes32(pub [u8;32]);*/ #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyDetail { controller: DID, - public_key: PublicKey + public_key: PublicKey, } // XXX: Map requires having a default value for DIDDetail @@ -135,7 +138,8 @@ impl KeyDetail { /// Create new key detail pub fn new(controller: DID, public_key: PublicKey) -> Self { KeyDetail { - controller, public_key + controller, + public_key, } } } @@ -160,13 +164,20 @@ pub struct KeyUpdate { impl KeyUpdate { /// Create new key update to update key of the `did`. /// Pass `controller` as None when not wishing to change the existing controller - pub fn new(did: DID, public_key: PublicKey, controller: Option, - last_modified_in_block: BlockNumber) -> Self { + pub fn new( + did: DID, + public_key: PublicKey, + controller: Option, + last_modified_in_block: BlockNumber, + ) -> Self { // XXX: size of public_key can be checked here as well. But this will require making the return // type a result and an attacker can craft a struct without using this method anyway. // This can be addressed later KeyUpdate { - did, public_key, controller, last_modified_in_block + did, + public_key, + controller, + last_modified_in_block, } } } @@ -183,7 +194,10 @@ pub struct DIDRemoval { impl DIDRemoval { /// Remove an existing DID `did` pub fn new(did: DID, last_modified_in_block: BlockNumber) -> Self { - DIDRemoval {did, last_modified_in_block} + DIDRemoval { + did, + last_modified_in_block, + } } } @@ -243,7 +257,7 @@ decl_module! { let serz_key_update = key_update.encode(); // Verify signature on the serialized `KeyUpdate` with the current public key - let sig_ver = Self::verify_sig(&signature, &serz_key_update, ¤t_key_detail.public_key)?; + let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_key_update, ¤t_key_detail.public_key)?; // Throw error if signature is invalid ensure!(sig_ver == true, Error::::InvalidSig); @@ -274,7 +288,7 @@ decl_module! { let serz_rem = to_remove.encode(); // Verify signature on the serialized `KeyUpdate` with the current public key - let sig_ver = Self::verify_sig(&signature, &serz_rem, ¤t_key_detail.public_key)?; + let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_rem, ¤t_key_detail.public_key)?; // Throw error if signature is invalid ensure!(sig_ver == true, Error::::InvalidSig); @@ -290,17 +304,14 @@ decl_module! { impl Module { /// Ensure that the DID is registered and this is not a replayed payload by checking the equality /// with stored block number when the DID was last modified. - pub fn ensure_registered_and_new(origin: T::Origin, did: &DID, last_modified_in_block: BlockNumber) -> Result { + pub fn ensure_registered_and_new( + origin: T::Origin, + did: &DID, + last_modified_in_block: BlockNumber, + ) -> Result { ensure_signed(origin)?; - let (current_key_detail, last_modified) = DIDs::::get(did); - - // DID must be registered. If its not the above `get` with return default values making `last_modified` as 0. - // Not doing an `exists` call on the map as that will result in 2 calls (1 for `exists`, 1 for `get`) to storage in - // the case when the DID is registered. - if last_modified == T::BlockNumber::from(0) { - fail!(Error::::DIDDoesNotExist) - } + let (current_key_detail, last_modified) = Self::get_key_detail(did)?; // replay protection: the command should contain the last block in which the DID was modified ensure!( @@ -311,39 +322,73 @@ impl Module { Ok(current_key_detail) } + /// Get the key detail and the block number of last modification of the given DID. + /// It assumes that the DID has only 1 public key which is true for now but will change later. + /// This function will then be modified to indicate which key(s) of the DID should be used. + /// If DID is not registered an error is raised. + pub fn get_key_detail(did: &DID) -> Result<(KeyDetail, T::BlockNumber), DispatchError> { + let (current_key_detail, last_modified) = DIDs::::get(did); + // DID must be registered. If its not the above `get` with return default values making `last_modified` as 0. + // Not doing an `exists` call on the map as that will result in 2 calls (1 for `exists`, 1 for `get`) to storage in + // the case when the DID is registered. + if last_modified == T::BlockNumber::from(0) { + fail!(Error::::DIDDoesNotExist) + } + Ok((current_key_detail, last_modified)) + } + + /// Verify given signature on the given message with the given DID's only public key. + /// It assumes that the DID has only 1 public key which is true for now but will change later. + /// This function will then be modified to indicate which key(s) of the DID should be used. + /// If DID is not registered an error is raised. + /// This function is intended to be used by other modules as well to check the signature from a DID. + pub fn verify_sig_from_DID( + signature: &[u8], + message: &[u8], + did: &DID, + ) -> Result { + let (current_key_detail, _) = Self::get_key_detail(did)?; + Self::verify_sig_with_public_key(signature, message, ¤t_key_detail.public_key) + } + /// Verify given signature on the given message with given public key - pub fn verify_sig(signature: &[u8], message: &[u8], public_key: &PublicKey) -> Result { - Ok( - match public_key { - PublicKey::Sr25519(bytes) => { - let signature = sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; - let pk = sr25519::Public(bytes.value.clone()); - signature.verify(message, &pk) - } - PublicKey::Ed25519(bytes) => { - let signature = ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; - let pk = ed25519::Public(bytes.value.clone()); - signature.verify(message, &pk) - } - PublicKey::Secp256k1(bytes) => { - let signature = ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; - let pk = ecdsa::Public::Compressed(bytes.value.clone()); - signature.verify(message, &pk) - } + pub fn verify_sig_with_public_key( + signature: &[u8], + message: &[u8], + public_key: &PublicKey, + ) -> Result { + Ok(match public_key { + PublicKey::Sr25519(bytes) => { + let signature = + sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + let pk = sr25519::Public(bytes.value.clone()); + signature.verify(message, &pk) + } + PublicKey::Ed25519(bytes) => { + let signature = + ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + let pk = ed25519::Public(bytes.value.clone()); + signature.verify(message, &pk) } - ) + PublicKey::Secp256k1(bytes) => { + let signature = + ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + let pk = ecdsa::Public::Compressed(bytes.value.clone()); + signature.verify(message, &pk) + } + }) } } #[cfg(test)] mod tests { - use super::*; use super::super::DID_BYTE_SIZE; + use super::*; use frame_support::{ assert_err, assert_ok, impl_outer_origin, parameter_types, weights::Weight, }; - use sp_core::{H256, Pair}; + use sp_core::{Pair, H256}; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup, OnFinalize, OnInitialize}, @@ -423,21 +468,15 @@ mod tests { let detail = KeyDetail::new(did.clone(), pk); // Add a DID - assert_ok!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ) - ); + assert_ok!(DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + )); // Try to add the same DID and same key detail again and fail assert_err!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ), + DIDModule::new(Origin::signed(alice), did.clone(), detail.clone()), Error::::DIDAlreadyExists ); @@ -445,11 +484,7 @@ mod tests { let pk = PublicKey::Ed25519(Bytes32::default()); let detail = KeyDetail::new(did.clone(), pk); assert_err!( - DIDModule::new( - Origin::signed(alice), - did, - detail - ), + DIDModule::new(Origin::signed(alice), did, detail), Error::::DIDAlreadyExists ); }); @@ -465,15 +500,16 @@ mod tests { let (pair, _, _) = sr25519::Pair::generate_with_phrase(None); let pk = pair.public().0; - let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk }), None, 2u32); + let key_update = KeyUpdate::new( + did.clone(), + PublicKey::Sr25519(Bytes32 { value: pk }), + None, + 2u32, + ); let sig = pair.sign(&key_update.encode()); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ), + DIDModule::update_key(Origin::signed(alice), key_update, sig.0.to_vec()), Error::::DIDDoesNotExist ); }); @@ -491,16 +527,14 @@ mod tests { let (pair_1, _, _) = $module::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; - let detail = KeyDetail::new($did.clone(), $pk(Bytes32 {value: pk_1})); + let detail = KeyDetail::new($did.clone(), $pk(Bytes32 { value: pk_1 })); // Add a DID - assert_ok!( - DIDModule::new( - Origin::signed(alice), - $did.clone(), - detail.clone() - ) - ); + assert_ok!(DIDModule::new( + Origin::signed(alice), + $did.clone(), + detail.clone() + )); let (current_detail, modified_in_block) = DIDModule::did($did.clone()); assert_eq!(current_detail.controller, $did); @@ -509,17 +543,20 @@ mod tests { // Prepare a key update let (pair_2, _, _) = $module::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; - let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_2}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + $did.clone(), + $pk(Bytes32 { value: pk_2 }), + None, + modified_in_block as u32, + ); let sig = pair_1.sign(&key_update.encode()); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ) - ); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + )); let (current_detail, modified_in_block) = DIDModule::did($did.clone()); // Since key update passed None for the controller, it should not change @@ -527,29 +564,33 @@ mod tests { // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) - let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + $did.clone(), + $pk(Bytes32 { value: pk_1 }), + None, + modified_in_block as u32, + ); let sig = pair_1.sign(&key_update.encode()); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ), + DIDModule::update_key(Origin::signed(alice), key_update, sig.0.to_vec()), Error::::InvalidSig ); // Keep the public key same but update the controller let new_controller = [9; DID_BYTE_SIZE]; - let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_2}), Some(new_controller), modified_in_block as u32); - let sig = pair_2.sign(&key_update.encode()); - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ) + let key_update = KeyUpdate::new( + $did.clone(), + $pk(Bytes32 { value: pk_2 }), + Some(new_controller), + modified_in_block as u32, ); + let sig = pair_2.sign(&key_update.encode()); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + )); // Since key update passed a new controller, it should be reflected let (current_detail, _) = DIDModule::did($did.clone()); @@ -557,7 +598,12 @@ mod tests { // Check key update with signature of incorrect size // Use the correct key - let key_update = KeyUpdate::new($did.clone(), $pk(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + $did.clone(), + $pk(Bytes32 { value: pk_1 }), + None, + modified_in_block as u32, + ); let sig = pair_2.sign(&key_update.encode()); // Truncate the signature to be of shorter size @@ -565,11 +611,7 @@ mod tests { short_sig.truncate(10); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update.clone(), - short_sig - ), + DIDModule::update_key(Origin::signed(alice), key_update.clone(), short_sig), Error::::InvalidSig ); @@ -578,11 +620,7 @@ mod tests { long_sig.append(&mut vec![0, 1, 2, 0]); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - long_sig - ), + DIDModule::update_key(Origin::signed(alice), key_update, long_sig), Error::::InvalidSig ); }}; @@ -607,48 +645,50 @@ mod tests { let did = [1; DID_BYTE_SIZE]; let (pair_1, _, _) = ecdsa::Pair::generate_with_phrase(None); - let pk_1= pair_1.public().as_compressed().unwrap(); - let detail = KeyDetail::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_1})); + let pk_1 = pair_1.public().as_compressed().unwrap(); + let detail = KeyDetail::new(did.clone(), PublicKey::Secp256k1(Bytes33 { value: pk_1 })); // Add a DID - assert_ok!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ) - ); + assert_ok!(DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + )); let (_, modified_in_block) = DIDModule::did(did.clone()); // Correctly update DID's key. // Prepare a key update let (pair_2, _, _) = ecdsa::Pair::generate_with_phrase(None); - let pk_2= pair_2.public().as_compressed().unwrap(); - let key_update = KeyUpdate::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_2}), None, modified_in_block as u32); + let pk_2 = pair_2.public().as_compressed().unwrap(); + let key_update = KeyUpdate::new( + did.clone(), + PublicKey::Secp256k1(Bytes33 { value: pk_2 }), + None, + modified_in_block as u32, + ); let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.to_vec() - ) - ); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.to_vec() + )); let (_, modified_in_block) = DIDModule::did(did.clone()); // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) - let key_update = KeyUpdate::new(did.clone(), PublicKey::Secp256k1(Bytes33 {value: pk_1}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + did.clone(), + PublicKey::Secp256k1(Bytes33 { value: pk_1 }), + None, + modified_in_block as u32, + ); let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update.clone(), - sig.to_vec() - ), + DIDModule::update_key(Origin::signed(alice), key_update.clone(), sig.to_vec()), Error::::InvalidSig ); @@ -657,11 +697,7 @@ mod tests { short_sig.truncate(10); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update.clone(), - short_sig - ), + DIDModule::update_key(Origin::signed(alice), key_update.clone(), short_sig), Error::::InvalidSig ); @@ -670,11 +706,7 @@ mod tests { long_sig.append(&mut vec![0, 1, 2, 0]); assert_err!( - DIDModule::update_key( - Origin::signed(alice), - key_update.clone(), - long_sig - ), + DIDModule::update_key(Origin::signed(alice), key_update.clone(), long_sig), Error::::InvalidSig ); }); @@ -695,16 +727,14 @@ mod tests { let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; - let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1})); + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_1 })); // Add a DID with key `pk_1` - assert_ok!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ) - ); + assert_ok!(DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + )); // Block number should increase to 1 as extrinsic is successful run_to_block(1); @@ -716,16 +746,19 @@ mod tests { let pk_2 = pair_2.public().0; // The following key update and signature will be included in a replay attempt to change key to `pk_2` without `pk_1`'s intent - let key_update_to_be_replayed = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_2}), None, modified_in_block as u32); + let key_update_to_be_replayed = KeyUpdate::new( + did.clone(), + PublicKey::Sr25519(Bytes32 { value: pk_2 }), + None, + modified_in_block as u32, + ); let sig_to_be_replayed = pair_1.sign(&key_update_to_be_replayed.encode()); // Update key from `pk_1` to `pk_2` using `pk_1`'s signature - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update_to_be_replayed.clone(), - sig_to_be_replayed.0.to_vec() - ) - ); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update_to_be_replayed.clone(), + sig_to_be_replayed.0.to_vec() + )); // Block number should increase to 2 as extrinsic is successful run_to_block(2); @@ -736,16 +769,19 @@ mod tests { let (pair_3, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_3 = pair_3.public().0; - let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_3}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + did.clone(), + PublicKey::Sr25519(Bytes32 { value: pk_3 }), + None, + modified_in_block as u32, + ); let sig = pair_2.sign(&key_update.encode()); // Update key from `pk_2` to `pk_3` using `pk_2`'s signature - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ) - ); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + )); // Block number should increase to 3 as extrinsic is successful run_to_block(3); @@ -753,23 +789,24 @@ mod tests { let (_, modified_in_block) = DIDModule::did(did.clone()); - let key_update = KeyUpdate::new(did.clone(), PublicKey::Sr25519(Bytes32 {value: pk_1}), None, modified_in_block as u32); + let key_update = KeyUpdate::new( + did.clone(), + PublicKey::Sr25519(Bytes32 { value: pk_1 }), + None, + modified_in_block as u32, + ); let sig = pair_3.sign(&key_update.encode()); // Update key from `pk_3` to `pk_1` using `pk_3`'s signature - assert_ok!( - DIDModule::update_key( - Origin::signed(alice), - key_update, - sig.0.to_vec() - ) - ); + assert_ok!(DIDModule::update_key( + Origin::signed(alice), + key_update, + sig.0.to_vec() + )); // Block number should increase to 4 as extrinsic is successful run_to_block(4); assert_eq!(System::block_number(), 4); - let (_, modified_in_block) = DIDModule::did(did.clone()); - // Attempt to replay `pk_1`'s older payload for key update to `pk_2` assert_err!( DIDModule::update_key( @@ -800,23 +837,17 @@ mod tests { // Trying to remove the DID before it was added will fail assert_err!( - DIDModule::remove( - Origin::signed(alice), - to_remove, - sig.0.to_vec() - ), + DIDModule::remove(Origin::signed(alice), to_remove, sig.0.to_vec()), Error::::DIDDoesNotExist ); // Add a DID - let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_1})); - assert_ok!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ) - ); + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_1 })); + assert_ok!(DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + )); let (_, modified_in_block) = DIDModule::did(did.clone()); @@ -829,24 +860,18 @@ mod tests { let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); let sig = pair_2.sign(&to_remove.encode()); assert_err!( - DIDModule::remove( - Origin::signed(alice), - to_remove, - sig.0.to_vec() - ), + DIDModule::remove(Origin::signed(alice), to_remove, sig.0.to_vec()), Error::::InvalidSig ); // The key controlling the DID should be able to remove the DID let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); let sig = pair_1.sign(&to_remove.encode()); - assert_ok!( - DIDModule::remove( - Origin::signed(alice), - to_remove, - sig.0.to_vec() - ) - ); + assert_ok!(DIDModule::remove( + Origin::signed(alice), + to_remove, + sig.0.to_vec() + )); let (_, modified_in_block) = DIDModule::did(did.clone()); // The block number will be 0 as the did has been removed @@ -854,14 +879,12 @@ mod tests { // A different public key than previous owner of the DID should be able to register the DID // Add the same DID but with different public key - let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_2})); - assert_ok!( - DIDModule::new( - Origin::signed(alice), - did.clone(), - detail.clone() - ) - ); + let detail = KeyDetail::new(did.clone(), PublicKey::Sr25519(Bytes32 { value: pk_2 })); + assert_ok!(DIDModule::new( + Origin::signed(alice), + did.clone(), + detail.clone() + )); let (_, modified_in_block) = DIDModule::did(did.clone()); // The block number will be non zero as the did has been written From e8c24736b6a8224ef4ff556fa8058dd904963c09 Mon Sep 17 00:00:00 2001 From: lovesh Date: Wed, 11 Mar 2020 17:00:40 +0530 Subject: [PATCH 09/17] Misc changes. Introduce a SignableCommand for wrapping any object before signing. Introduce a Signature type and replace Vector. Wrappers over bytearrays of different sizes. Signed-off-by: lovesh --- developer.json | 13 ++ runtime/src/did.rs | 406 +++++++++++++++++++++++++-------------------- runtime/src/lib.rs | 17 +- 3 files changed, 246 insertions(+), 190 deletions(-) diff --git a/developer.json b/developer.json index 09863d91f..73c54982d 100644 --- a/developer.json +++ b/developer.json @@ -6,6 +6,12 @@ "Bytes33": { "value": "[u8;33]" }, + "Bytes64": { + "value": "[u8;64]" + }, + "Bytes65": { + "value": "[u8;65]" + }, "PublicKey": { "_enum": { "Sr25519": "Bytes32", @@ -13,6 +19,13 @@ "Secp256k1": "Bytes33" } }, + "Signature": { + "_enum": { + "Sr25519": "Bytes64", + "Ed25519": "Bytes64", + "Secp256k1": "Bytes65" + } + }, "KeyDetail": { "controller": "DID", "public_key": "PublicKey" diff --git a/runtime/src/did.rs b/runtime/src/did.rs index f7f1d93a8..b1b851ab8 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,4 +1,4 @@ -use super::{BlockNumber, DID}; +use super::{BlockNumber, SignableCommand}; use codec::{Decode, Encode}; use frame_support::{ decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, @@ -8,9 +8,14 @@ use sp_core::{ecdsa, ed25519, sr25519}; use sp_runtime::traits::Verify; use sp_std::convert::TryFrom; use sp_std::fmt; -use sp_std::prelude::Vec; use system::ensure_signed; +// XXX: explore using `parameter_types!` for DID_BYTE_SIZE. The problem is that its needed for defining DID +/// Size of the Dock DID in bytes +pub const DID_BYTE_SIZE: usize = 32; +/// The type of the Dock DID +pub type DID = [u8; DID_BYTE_SIZE]; + /// The module's configuration trait. pub trait Trait: system::Trait { /// The overarching event type. @@ -24,9 +29,9 @@ decl_error! { /// Given public key is not of the correct size PublicKeySizeIncorrect, /// There is already a DID with same value - DIDAlreadyExists, - /// There is no such DID - DIDDoesNotExist, + DidAlreadyExists, + /// There is no such DID registered + DidDoesNotExist, /// For replay protection, an update to state is required to contain the same block number /// in which the last update was performed. DifferentBlockNumber, @@ -35,11 +40,6 @@ decl_error! { } } -/*/// Size of a Sr25519 public key in bytes. -pub const Sr25519_PK_BYTE_SIZE: usize = 32; -/// Size of a Ed25519 public key in bytes. -pub const Ed25519_PK_BYTE_SIZE: usize = 32;*/ - // XXX: This could have been a tuple struct. Keeping it a normal struct for Substrate UI /// A wrapper over 32-byte array #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] @@ -47,39 +47,60 @@ pub struct Bytes32 { value: [u8; 32], } -impl Default for Bytes32 { - fn default() -> Self { - Self { value: [0; 32] } +impl Bytes32 { + pub fn as_bytes(&self) -> &[u8] { + &self.value } } -// XXX: This could have been a tuple struct. Keeping it a normal struct for Substrate UI -/// A wrapper over 33-byte array -#[derive(Encode, Decode, Clone)] -pub struct Bytes33 { - value: [u8; 33], -} +// XXX: These could have been a tuple structs. Keeping them normal struct for Substrate UI +/// Creates a struct named `$name` which contains only 1 element which is a bytearray, useful when +/// wrapping arrays of size > 32. `$size` is the size of the underlying bytearray. Implements the `Default`, +/// `sp_std::fmt::Debug`, `PartialEq` and `Eq` trait as they will not be automatically implemented for arrays of size > 32. +macro_rules! struct_over_byte_array { + ( $name:ident, $size:tt ) => { + /// A wrapper over a byte array + #[derive(Encode, Decode, Clone)] + pub struct $name { + value: [u8; $size], + } -impl Default for Bytes33 { - fn default() -> Self { - Self { value: [0; 33] } - } -} + /// Implementing Default as it cannot be automatically derived for arrays of size > 32 + impl Default for $name { + fn default() -> Self { + Self { value: [0; $size] } + } + } -/// Implementing Debug for Bytes33 as it cannot be automatically derived for arrays of size > 32 -impl fmt::Debug for Bytes33 { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - self.value[..].fmt(f) - } -} + /// Implementing Debug as it cannot be automatically derived for arrays of size > 32 + impl fmt::Debug for $name { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.value[..].fmt(f) + } + } + + /// Implementing PartialEq as it cannot be automatically derived for arrays of size > 32 + impl PartialEq for $name { + fn eq(&self, other: &Self) -> bool { + self.value[..] == other.value[..] + } + } -/// Implementing PartialEq for Bytes33 as it cannot be automatically derived for arrays of size > 32 -impl PartialEq for Bytes33 { - fn eq(&self, other: &Bytes33) -> bool { - self.value[..] == other.value[..] + impl Eq for $name {} + + impl $name { + /// Return a slice to the underlying bytearray + pub fn as_bytes(&self) -> &[u8] { + &self.value + } + } } } -impl Eq for Bytes33 {} + + +struct_over_byte_array!(Bytes33, 33); +struct_over_byte_array!(Bytes64, 64); +struct_over_byte_array!(Bytes65, 65); /// An abstraction for a public key. Abstracts the type and value of the public key where the value is a /// byte array @@ -93,14 +114,51 @@ pub enum PublicKey { Secp256k1(Bytes33), } -impl Default for PublicKey { - fn default() -> Self { - PublicKey::Sr25519(Bytes32::default()) +/// An abstraction for a signature. +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +pub enum Signature { + /// Signature for Sr25519 is 64 bytes + Sr25519(Bytes64), + /// Signature for Ed25519 is 64 bytes + Ed25519(Bytes64), + /// Signature for Secp256k1 is 65 bytes + Secp256k1(Bytes65), +} + +impl Signature { + /// Try to get reference to the bytes if its a Sr25519 signature. Return error if its not. + pub fn as_Sr25519_sig_bytes(&self) -> Result<&[u8], ()> { + match self { + Signature::Sr25519(bytes) => Ok(bytes.as_bytes()), + _ => Err(()) + } + } + + /// Try to get reference to the bytes if its a Ed25519 signature. Return error if its not. + pub fn as_Ed25519_sig_bytes(&self) -> Result<&[u8], ()> { + match self { + Signature::Ed25519(bytes) => Ok(bytes.as_bytes()), + _ => Err(()) + } + } + + /// Try to get reference to the bytes if its a Secp256k1 signature. Return error if its not. + pub fn as_Secp256k1_sig_bytes(&self) -> Result<&[u8], ()> { + match self { + Signature::Secp256k1(bytes) => Ok(bytes.as_bytes()), + _ => Err(()) + } } } // XXX: Substrate UI can't parse them. Maybe later versions will fix it. -/*#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +/* +/// Size of a Sr25519 public key in bytes. +pub const Sr25519_PK_BYTE_SIZE: usize = 32; +/// Size of a Ed25519 public key in bytes. +pub const Ed25519_PK_BYTE_SIZE: usize = 32; + +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub enum PublicKey { Sr25519([u8; 32]), Ed25519([u8; 32]) @@ -115,8 +173,7 @@ pub enum PublicKey { #[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct Bytes32(pub [u8;32]);*/ -/// `controller` is the controller DID and its value might be same as `did`. When that is the case, pass `controller` as None. -/// `public_key_type` is the type of the key +/// `controller` is the controller DID and its value might be same as `did`. /// `public_key` is the public key and it is accepted and stored as raw bytes. #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyDetail { @@ -124,16 +181,6 @@ pub struct KeyDetail { public_key: PublicKey, } -// XXX: Map requires having a default value for DIDDetail -impl Default for KeyDetail { - fn default() -> Self { - KeyDetail { - controller: DID::default(), - public_key: PublicKey::default(), - } - } -} - impl KeyDetail { /// Create new key detail pub fn new(controller: DID, public_key: PublicKey) -> Self { @@ -170,9 +217,6 @@ impl KeyUpdate { controller: Option, last_modified_in_block: BlockNumber, ) -> Self { - // XXX: size of public_key can be checked here as well. But this will require making the return - // type a result and an attacker can craft a struct without using this method anyway. - // This can be addressed later KeyUpdate { did, public_key, @@ -206,16 +250,16 @@ decl_event!( where AccountId = ::AccountId, { - DIDAdded(DID), + DidAdded(DID), KeyUpdated(DID), - DIDRemoved(DID), + DidRemoved(DID), DummyEvent(AccountId), } ); decl_storage! { trait Store for Module as DIDModule { - DIDs get(did): map DID => (KeyDetail, T::BlockNumber); + DIDs get(did): map DID => Option<(KeyDetail, T::BlockNumber)>; } } @@ -226,7 +270,7 @@ decl_module! { type Error = Error; /// Create a new DID. - /// `did` is the new DID to create. The method will throw exception if `did` is already registered. + /// `did` is the new DID to create. The method will fail if `did` is already registered. /// `detail` is the details of the key like its type, controller and value fn new(origin, did: DID, detail: KeyDetail) -> DispatchResult { ensure_signed(origin)?; @@ -234,33 +278,35 @@ decl_module! { // DID is not registered already ensure!( !DIDs::::exists(did), - Error::::DIDAlreadyExists + Error::::DidAlreadyExists ); let current_block_no = >::block_number(); DIDs::::insert(did, (detail, current_block_no)); - Self::deposit_event(RawEvent::DIDAdded(did)); + Self::deposit_event(RawEvent::DidAdded(did)); Ok(()) } - /// `key_update` specifies which DID's which key needs to be updated + /// `key_update` specifies which DID's key needs to be updated /// `signature` is the signature on the serialized `KeyUpdate`. /// The node while processing this extrinsic, should create the above serialized `KeyUpdate` /// using the stored data and try to verify the given signature with the stored key. - pub fn update_key(origin, key_update: KeyUpdate, signature: Vec) -> DispatchResult { + pub fn update_key(origin, key_update: KeyUpdate, signature: Signature) -> DispatchResult { + ensure_signed(origin)?; + // Not checking for signature size as its not stored // DID is registered and the update is not being replayed - let mut current_key_detail = Self::ensure_registered_and_new(origin, &key_update.did, key_update.last_modified_in_block)?; + let mut current_key_detail = Self::ensure_registered_and_new(&key_update.did, key_update.last_modified_in_block)?; // serialize `KeyUpdate` to bytes - let serz_key_update = key_update.encode(); + let serz_key_update = SignableCommand::KeyUpdate(key_update.clone()).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_key_update, ¤t_key_detail.public_key)?; // Throw error if signature is invalid - ensure!(sig_ver == true, Error::::InvalidSig); + ensure!(sig_ver, Error::::InvalidSig); // Key update is safe to do, update the block number as well. let current_block_no = >::block_number(); @@ -280,22 +326,25 @@ decl_module! { /// `signature` is the signature on the serialized `DIDRemoval`. /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` /// using the stored data and try to verify the given signature with the stored key. - pub fn remove(origin, to_remove: DIDRemoval, signature: Vec) -> DispatchResult { + pub fn remove(origin, to_remove: DIDRemoval, signature: Signature) -> DispatchResult { + ensure_signed(origin)?; + // DID is registered and the removal is not being replayed - let current_key_detail = Self::ensure_registered_and_new(origin, &to_remove.did, to_remove.last_modified_in_block)?; + let current_key_detail = Self::ensure_registered_and_new(&to_remove.did, to_remove.last_modified_in_block)?; + let did = to_remove.did; // serialize `DIDRemoval` to bytes - let serz_rem = to_remove.encode(); + let serz_rem = SignableCommand::DIDRemoval(to_remove).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_rem, ¤t_key_detail.public_key)?; // Throw error if signature is invalid - ensure!(sig_ver == true, Error::::InvalidSig); + ensure!(sig_ver, Error::::InvalidSig); // Remove DID - DIDs::::remove(to_remove.did); - Self::deposit_event(RawEvent::DIDRemoved(to_remove.did)); + DIDs::::remove(did); + Self::deposit_event(RawEvent::DidRemoved(did)); Ok(()) } } @@ -305,12 +354,9 @@ impl Module { /// Ensure that the DID is registered and this is not a replayed payload by checking the equality /// with stored block number when the DID was last modified. pub fn ensure_registered_and_new( - origin: T::Origin, did: &DID, last_modified_in_block: BlockNumber, ) -> Result { - ensure_signed(origin)?; - let (current_key_detail, last_modified) = Self::get_key_detail(did)?; // replay protection: the command should contain the last block in which the DID was modified @@ -327,14 +373,12 @@ impl Module { /// This function will then be modified to indicate which key(s) of the DID should be used. /// If DID is not registered an error is raised. pub fn get_key_detail(did: &DID) -> Result<(KeyDetail, T::BlockNumber), DispatchError> { - let (current_key_detail, last_modified) = DIDs::::get(did); - // DID must be registered. If its not the above `get` with return default values making `last_modified` as 0. - // Not doing an `exists` call on the map as that will result in 2 calls (1 for `exists`, 1 for `get`) to storage in - // the case when the DID is registered. - if last_modified == T::BlockNumber::from(0) { - fail!(Error::::DIDDoesNotExist) + // DID must be registered. + if let Some((current_key_detail, last_modified)) = DIDs::::get(did) { + Ok((current_key_detail, last_modified)) + } else { + fail!(Error::::DidDoesNotExist) } - Ok((current_key_detail, last_modified)) } /// Verify given signature on the given message with the given DID's only public key. @@ -343,7 +387,7 @@ impl Module { /// If DID is not registered an error is raised. /// This function is intended to be used by other modules as well to check the signature from a DID. pub fn verify_sig_from_DID( - signature: &[u8], + signature: &Signature, message: &[u8], did: &DID, ) -> Result { @@ -353,26 +397,26 @@ impl Module { /// Verify given signature on the given message with given public key pub fn verify_sig_with_public_key( - signature: &[u8], + signature: &Signature, message: &[u8], public_key: &PublicKey, ) -> Result { Ok(match public_key { PublicKey::Sr25519(bytes) => { let signature = - sr25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + sr25519::Signature::try_from(signature.as_Sr25519_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Ed25519(bytes) => { let signature = - ed25519::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + ed25519::Signature::try_from(signature.as_Ed25519_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Secp256k1(bytes) => { let signature = - ecdsa::Signature::try_from(signature).map_err(|_| Error::::InvalidSig)?; + ecdsa::Signature::try_from(signature.as_Secp256k1_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; let pk = ecdsa::Public::Compressed(bytes.value.clone()); signature.verify(message, &pk) } @@ -382,7 +426,6 @@ impl Module { #[cfg(test)] mod tests { - use super::super::DID_BYTE_SIZE; use super::*; use frame_support::{ @@ -457,6 +500,44 @@ mod tests { } } + #[test] + fn signature_verification() { + // Check that the signature should be wrapped in correct variant of enum `Signature`. + // Trying to wrap a Sr25519 signature in a Signature::Ed25519 should fail. + // Trying to wrap a Ed25519 signature in a Signature::Sr25519 should fail. + // Not checking for Signature::Secp256k1 as it has a different size + // XXX: The following test should not have been wrapped in a Externalities-provided environment but + // ed25519_verify needs it. + new_test_ext().execute_with(|| { + let msg = vec![1, 2, 4, 5, 7]; + + // The macro checks that a signature verification only passes when sig wrapped in `$correct_sig_type` + // but fails when wrapped in `$incorrect_sig_type` + macro_rules! check_sig_verification { + ( $module:ident, $pk_type:expr, $correct_sig_type:expr, $incorrect_sig_type:expr ) => {{ + + let (pair, _, _) = $module::Pair::generate_with_phrase(None); + let pk = $pk_type(Bytes32 { value: pair.public().0 }); + let sig_bytes = pair.sign(&msg).0; + let correct_sig = $correct_sig_type(Bytes64 {value: sig_bytes}); + + // Valid signature wrapped in a correct type works + assert!(DIDModule::verify_sig_with_public_key(&correct_sig, &msg, &pk).unwrap()); + + // Valid signature wrapped in an incorrect type does not work + let incorrect_sig = $incorrect_sig_type(Bytes64 {value: sig_bytes}); + assert_err!( + DIDModule::verify_sig_with_public_key(&incorrect_sig, &msg, &pk), + Error::::InvalidSig + ); + }} + } + + check_sig_verification!(sr25519, PublicKey::Sr25519, Signature::Sr25519, Signature::Ed25519); + check_sig_verification!(ed25519, PublicKey::Ed25519, Signature::Ed25519, Signature::Sr25519); + }); + } + #[test] fn did_creation() { // DID must be unique. It must have an acceptable public size @@ -464,7 +545,7 @@ mod tests { let alice = 10u64; let did = [1; DID_BYTE_SIZE]; - let pk = PublicKey::default(); + let pk = PublicKey::Sr25519(Bytes32 {value: [0;32]}); let detail = KeyDetail::new(did.clone(), pk); // Add a DID @@ -477,15 +558,15 @@ mod tests { // Try to add the same DID and same key detail again and fail assert_err!( DIDModule::new(Origin::signed(alice), did.clone(), detail.clone()), - Error::::DIDAlreadyExists + Error::::DidAlreadyExists ); // Try to add the same DID again but with different key detail and fail - let pk = PublicKey::Ed25519(Bytes32::default()); + let pk = PublicKey::Ed25519(Bytes32 {value: [0;32]}); let detail = KeyDetail::new(did.clone(), pk); assert_err!( DIDModule::new(Origin::signed(alice), did, detail), - Error::::DIDAlreadyExists + Error::::DidAlreadyExists ); }); } @@ -506,11 +587,11 @@ mod tests { None, 2u32, ); - let sig = pair.sign(&key_update.encode()); + let sig = Signature::Sr25519(Bytes64 {value: pair.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update, sig.0.to_vec()), - Error::::DIDDoesNotExist + DIDModule::update_key(Origin::signed(alice), key_update, sig), + Error::::DidDoesNotExist ); }); } @@ -523,7 +604,7 @@ mod tests { /// Macro to check the key update for ed25519 and sr25519 macro_rules! check_key_update { - ( $did:ident, $module:ident, $pk:expr ) => {{ + ( $did:ident, $module:ident, $pk:expr, $sig_type:expr, $sig_bytearray_type:ident ) => {{ let (pair_1, _, _) = $module::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; @@ -536,7 +617,7 @@ mod tests { detail.clone() )); - let (current_detail, modified_in_block) = DIDModule::did($did.clone()); + let (current_detail, modified_in_block) = DIDModule::get_key_detail(&$did).unwrap(); assert_eq!(current_detail.controller, $did); // Correctly update DID's key. @@ -549,16 +630,16 @@ mod tests { None, modified_in_block as u32, ); - let sig = pair_1.sign(&key_update.encode()); + let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, - sig.0.to_vec() + sig )); - let (current_detail, modified_in_block) = DIDModule::did($did.clone()); + let (current_detail, modified_in_block) = DIDModule::get_key_detail(&$did).unwrap(); // Since key update passed None for the controller, it should not change assert_eq!(current_detail.controller, $did); @@ -570,10 +651,10 @@ mod tests { None, modified_in_block as u32, ); - let sig = pair_1.sign(&key_update.encode()); + let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update, sig.0.to_vec()), + DIDModule::update_key(Origin::signed(alice), key_update, sig), Error::::InvalidSig ); @@ -585,52 +666,24 @@ mod tests { Some(new_controller), modified_in_block as u32, ); - let sig = pair_2.sign(&key_update.encode()); + let sig = $sig_type($sig_bytearray_type {value: pair_2.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, - sig.0.to_vec() + sig )); // Since key update passed a new controller, it should be reflected - let (current_detail, _) = DIDModule::did($did.clone()); + let (current_detail, _) = DIDModule::get_key_detail(&$did).unwrap(); assert_eq!(current_detail.controller, new_controller); - - // Check key update with signature of incorrect size - // Use the correct key - let key_update = KeyUpdate::new( - $did.clone(), - $pk(Bytes32 { value: pk_1 }), - None, - modified_in_block as u32, - ); - let sig = pair_2.sign(&key_update.encode()); - - // Truncate the signature to be of shorter size - let mut short_sig = sig.0.to_vec(); - short_sig.truncate(10); - - assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update.clone(), short_sig), - Error::::InvalidSig - ); - - // Add extra bytes to the signature to be of longer size - let mut long_sig = sig.0.to_vec(); - long_sig.append(&mut vec![0, 1, 2, 0]); - - assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update, long_sig), - Error::::InvalidSig - ); }}; } let did = [1; DID_BYTE_SIZE]; - check_key_update!(did, sr25519, PublicKey::Sr25519); + check_key_update!(did, sr25519, PublicKey::Sr25519, Signature::Sr25519, Bytes64); let did = [2; DID_BYTE_SIZE]; - check_key_update!(did, ed25519, PublicKey::Ed25519); + check_key_update!(did, ed25519, PublicKey::Ed25519, Signature::Ed25519, Bytes64); }); } @@ -655,7 +708,7 @@ mod tests { detail.clone() )); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); // Correctly update DID's key. // Prepare a key update @@ -667,15 +720,17 @@ mod tests { None, modified_in_block as u32, ); - let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); + // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) + let value: [u8; 65] = pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).into(); + let sig = Signature::Secp256k1(Bytes65 {value}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, - sig.to_vec() + sig )); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); // Maliciously update DID's key. // Signing with the old key (`pair_1`) to update to the new key (`pair_2`) @@ -685,28 +740,10 @@ mod tests { None, modified_in_block as u32, ); - let sig: [u8; 65] = pair_1.sign(&key_update.encode()).into(); - + let value: [u8; 65] = pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).into(); + let sig = Signature::Secp256k1(Bytes65 {value}); assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update.clone(), sig.to_vec()), - Error::::InvalidSig - ); - - // Truncate the signature to be of shorter size - let mut short_sig = sig.to_vec(); - short_sig.truncate(10); - - assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update.clone(), short_sig), - Error::::InvalidSig - ); - - // Add extra bytes to the signature to be of longer size - let mut long_sig = sig.to_vec(); - long_sig.append(&mut vec![0, 1, 2, 0]); - - assert_err!( - DIDModule::update_key(Origin::signed(alice), key_update.clone(), long_sig), + DIDModule::update_key(Origin::signed(alice), key_update.clone(), sig), Error::::InvalidSig ); }); @@ -740,7 +777,7 @@ mod tests { run_to_block(1); assert_eq!(System::block_number(), 1); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; @@ -752,19 +789,20 @@ mod tests { None, modified_in_block as u32, ); - let sig_to_be_replayed = pair_1.sign(&key_update_to_be_replayed.encode()); + // Update key from `pk_1` to `pk_2` using `pk_1`'s signature + let sig_to_be_replayed = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update_to_be_replayed.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update_to_be_replayed.clone(), - sig_to_be_replayed.0.to_vec() + sig_to_be_replayed.clone() )); // Block number should increase to 2 as extrinsic is successful run_to_block(2); assert_eq!(System::block_number(), 2); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); let (pair_3, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_3 = pair_3.public().0; @@ -775,19 +813,20 @@ mod tests { None, modified_in_block as u32, ); - let sig = pair_2.sign(&key_update.encode()); + // Update key from `pk_2` to `pk_3` using `pk_2`'s signature + let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, - sig.0.to_vec() + sig )); // Block number should increase to 3 as extrinsic is successful run_to_block(3); assert_eq!(System::block_number(), 3); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); let key_update = KeyUpdate::new( did.clone(), @@ -795,12 +834,13 @@ mod tests { None, modified_in_block as u32, ); - let sig = pair_3.sign(&key_update.encode()); + // Update key from `pk_3` to `pk_1` using `pk_3`'s signature + let sig = Signature::Sr25519(Bytes64 {value: pair_3.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, - sig.0.to_vec() + sig )); // Block number should increase to 4 as extrinsic is successful @@ -812,7 +852,7 @@ mod tests { DIDModule::update_key( Origin::signed(alice), key_update_to_be_replayed, - sig_to_be_replayed.0.to_vec() + sig_to_be_replayed ), Error::::DifferentBlockNumber ); @@ -833,12 +873,12 @@ mod tests { let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; let to_remove = DIDRemoval::new(did.clone(), 2u32); - let sig = pair_1.sign(&to_remove.encode()); + let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); // Trying to remove the DID before it was added will fail assert_err!( - DIDModule::remove(Origin::signed(alice), to_remove, sig.0.to_vec()), - Error::::DIDDoesNotExist + DIDModule::remove(Origin::signed(alice), to_remove, sig), + Error::::DidDoesNotExist ); // Add a DID @@ -849,7 +889,7 @@ mod tests { detail.clone() )); - let (_, modified_in_block) = DIDModule::did(did.clone()); + let (_, modified_in_block) = DIDModule::get_key_detail(&did).unwrap(); // The block number will be non zero as write was successful and will be 1 since its the first extrinsic assert_eq!(modified_in_block, 1); @@ -858,24 +898,23 @@ mod tests { let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); - let sig = pair_2.sign(&to_remove.encode()); + let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); assert_err!( - DIDModule::remove(Origin::signed(alice), to_remove, sig.0.to_vec()), + DIDModule::remove(Origin::signed(alice), to_remove, sig), Error::::InvalidSig ); // The key controlling the DID should be able to remove the DID let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); - let sig = pair_1.sign(&to_remove.encode()); + let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); assert_ok!(DIDModule::remove( Origin::signed(alice), to_remove, - sig.0.to_vec() + sig )); - let (_, modified_in_block) = DIDModule::did(did.clone()); - // The block number will be 0 as the did has been removed - assert_eq!(modified_in_block, 0); + // Error as the did has been removed + assert!(DIDModule::get_key_detail(&did).is_err()); // A different public key than previous owner of the DID should be able to register the DID // Add the same DID but with different public key @@ -886,11 +925,10 @@ mod tests { detail.clone() )); - let (_, modified_in_block) = DIDModule::did(did.clone()); - // The block number will be non zero as the did has been written - assert_ne!(modified_in_block, 0); + // Ok as the did has been written + assert!(DIDModule::get_key_detail(&did).is_ok()); }); } - // TODO: Add test for events DIDAdded, KeyUpdated, DIDRemoval + // TODO: Add test for events DidAdded, KeyUpdated, DIDRemoval } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 002222442..40c4c5c6d 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -21,6 +21,8 @@ use sp_runtime::{ ApplyExtrinsicResult, MultiSignature, }; use sp_std::prelude::*; +use codec::{Decode, Encode}; + #[cfg(feature = "std")] use sp_version::NativeVersion; use sp_version::RuntimeVersion; @@ -61,17 +63,20 @@ pub type Hash = sp_core::H256; /// Digest item type. pub type DigestItem = generic::DigestItem; -// XXX: explore using `parameter_types!` for DID_BYTE_SIZE. The problem is that its needed for defining DID -/// Size of the Dock DID in bytes -pub const DID_BYTE_SIZE: usize = 32; -/// The type of the Dock DID -pub type DID = [u8; DID_BYTE_SIZE]; - /// Used for the module template in `./template.rs` mod template; mod did; +/// Any command that needs to be signed is first wrapped in this enum and then its serialized. +/// This is done to prevent make it unambiguous which command was intended as the SCALE codec's +/// not self describing. +#[derive(Encode, Decode)] +pub enum SignableCommand { + KeyUpdate(did::KeyUpdate), + DIDRemoval(did::DIDRemoval) +} + /// Opaque types. These are used by the CLI to instantiate machinery that don't need to know /// the specifics of the runtime. They can then be made to be agnostic over specific formats /// of data like extrinsics, allowing for them to continue syncing the network through upgrades From fc808be6f2968b1ca8c10f7e09546d846e46462d Mon Sep 17 00:00:00 2001 From: lovesh Date: Wed, 11 Mar 2020 19:00:30 +0530 Subject: [PATCH 10/17] Add new error type Signed-off-by: lovesh --- runtime/src/did.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index b1b851ab8..e2eaea542 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -35,6 +35,8 @@ decl_error! { /// For replay protection, an update to state is required to contain the same block number /// in which the last update was performed. DifferentBlockNumber, + /// Signature type does not match public key type + InvalidSigType, /// Signature verification failed while key update or did removal InvalidSig } @@ -404,19 +406,19 @@ impl Module { Ok(match public_key { PublicKey::Sr25519(bytes) => { let signature = - sr25519::Signature::try_from(signature.as_Sr25519_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; + sr25519::Signature::try_from(signature.as_Sr25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Ed25519(bytes) => { let signature = - ed25519::Signature::try_from(signature.as_Ed25519_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; + ed25519::Signature::try_from(signature.as_Ed25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Secp256k1(bytes) => { let signature = - ecdsa::Signature::try_from(signature.as_Secp256k1_sig_bytes().map_err(|_| Error::::InvalidSig)?).map_err(|_| Error::::InvalidSig)?; + ecdsa::Signature::try_from(signature.as_Secp256k1_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = ecdsa::Public::Compressed(bytes.value.clone()); signature.verify(message, &pk) } @@ -528,7 +530,7 @@ mod tests { let incorrect_sig = $incorrect_sig_type(Bytes64 {value: sig_bytes}); assert_err!( DIDModule::verify_sig_with_public_key(&incorrect_sig, &msg, &pk), - Error::::InvalidSig + Error::::InvalidSigType ); }} } From 61999ff6ebd714d7a5f081ab3300ff3a5c8cf4ef Mon Sep 17 00:00:00 2001 From: lovesh Date: Thu, 12 Mar 2020 02:11:23 +0530 Subject: [PATCH 11/17] Renaming a few types Signed-off-by: lovesh --- developer.json | 12 ++--- runtime/src/did.rs | 110 +++++++++++++++++++++++---------------------- runtime/src/lib.rs | 7 +-- 3 files changed, 66 insertions(+), 63 deletions(-) diff --git a/developer.json b/developer.json index 73c54982d..d87f4a165 100644 --- a/developer.json +++ b/developer.json @@ -1,5 +1,5 @@ { - "DID": "[u8;32]", + "Did": "[u8;32]", "Bytes32": { "value": "[u8;32]" }, @@ -27,17 +27,17 @@ } }, "KeyDetail": { - "controller": "DID", + "controller": "Did", "public_key": "PublicKey" }, "KeyUpdate": { - "did": "DID", + "did": "Did", "public_key": "PublicKey", - "controller": "Option", + "controller": "Option", "last_modified_in_block": "u64" }, - "DIDRemoval": { - "did": "DID", + "DidRemoval": { + "did": "Did", "last_modified_in_block": "u64" } } \ No newline at end of file diff --git a/runtime/src/did.rs b/runtime/src/did.rs index e2eaea542..4a2aba27b 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -1,4 +1,4 @@ -use super::{BlockNumber, SignableCommand}; +use super::{BlockNumber, StateChange}; use codec::{Decode, Encode}; use frame_support::{ decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, @@ -14,7 +14,7 @@ use system::ensure_signed; /// Size of the Dock DID in bytes pub const DID_BYTE_SIZE: usize = 32; /// The type of the Dock DID -pub type DID = [u8; DID_BYTE_SIZE]; +pub type Did = [u8; DID_BYTE_SIZE]; /// The module's configuration trait. pub trait Trait: system::Trait { @@ -129,7 +129,7 @@ pub enum Signature { impl Signature { /// Try to get reference to the bytes if its a Sr25519 signature. Return error if its not. - pub fn as_Sr25519_sig_bytes(&self) -> Result<&[u8], ()> { + pub fn as_sr25519_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Sr25519(bytes) => Ok(bytes.as_bytes()), _ => Err(()) @@ -137,7 +137,7 @@ impl Signature { } /// Try to get reference to the bytes if its a Ed25519 signature. Return error if its not. - pub fn as_Ed25519_sig_bytes(&self) -> Result<&[u8], ()> { + pub fn as_ed25519_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Ed25519(bytes) => Ok(bytes.as_bytes()), _ => Err(()) @@ -145,7 +145,7 @@ impl Signature { } /// Try to get reference to the bytes if its a Secp256k1 signature. Return error if its not. - pub fn as_Secp256k1_sig_bytes(&self) -> Result<&[u8], ()> { + pub fn as_secp256k1_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Secp256k1(bytes) => Ok(bytes.as_bytes()), _ => Err(()) @@ -179,13 +179,13 @@ pub struct Bytes32(pub [u8;32]);*/ /// `public_key` is the public key and it is accepted and stored as raw bytes. #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyDetail { - controller: DID, + controller: Did, public_key: PublicKey, } impl KeyDetail { /// Create new key detail - pub fn new(controller: DID, public_key: PublicKey) -> Self { + pub fn new(controller: Did, public_key: PublicKey) -> Self { KeyDetail { controller, public_key, @@ -199,14 +199,16 @@ impl KeyDetail { /// `controller` If provided None, the controller is unchanged. While serializing, use literal "None" when controller is None /// The last_modified_in_block is the block number when this DID was last modified is present to prevent replay attack. /// This approach allows only 1 update transaction in a block (don't see it as a problem as key updates are rare). +/// Its possible to submit one txn per block but the sender has to guess and he might guess it right in high block +/// latency and low traffic network. /// An alternate approach can be to have a nonce associated to each detail which is incremented on each /// successful extrinsic and the chain requiring the extrinsic's nonce to be higher than current. This is /// little more involved as it involves a ">" check #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyUpdate { - did: DID, + did: Did, public_key: PublicKey, - controller: Option, + controller: Option, last_modified_in_block: BlockNumber, } @@ -214,9 +216,9 @@ impl KeyUpdate { /// Create new key update to update key of the `did`. /// Pass `controller` as None when not wishing to change the existing controller pub fn new( - did: DID, + did: Did, public_key: PublicKey, - controller: Option, + controller: Option, last_modified_in_block: BlockNumber, ) -> Self { KeyUpdate { @@ -232,15 +234,15 @@ impl KeyUpdate { /// `did` is the DID which is being removed. /// `last_modified_in_block` is the block number when this DID was last modified. The last modified time is present to prevent replay attack. #[derive(Encode, Decode, Clone, PartialEq, Debug)] -pub struct DIDRemoval { - did: DID, +pub struct DidRemoval { + did: Did, last_modified_in_block: BlockNumber, } -impl DIDRemoval { +impl DidRemoval { /// Remove an existing DID `did` - pub fn new(did: DID, last_modified_in_block: BlockNumber) -> Self { - DIDRemoval { + pub fn new(did: Did, last_modified_in_block: BlockNumber) -> Self { + DidRemoval { did, last_modified_in_block, } @@ -252,16 +254,16 @@ decl_event!( where AccountId = ::AccountId, { - DidAdded(DID), - KeyUpdated(DID), - DidRemoved(DID), + DidAdded(Did), + KeyUpdated(Did), + DidRemoved(Did), DummyEvent(AccountId), } ); decl_storage! { trait Store for Module as DIDModule { - DIDs get(did): map DID => Option<(KeyDetail, T::BlockNumber)>; + Dids get(did): map Did => Option<(KeyDetail, T::BlockNumber)>; } } @@ -274,17 +276,17 @@ decl_module! { /// Create a new DID. /// `did` is the new DID to create. The method will fail if `did` is already registered. /// `detail` is the details of the key like its type, controller and value - fn new(origin, did: DID, detail: KeyDetail) -> DispatchResult { + fn new(origin, did: Did, detail: KeyDetail) -> DispatchResult { ensure_signed(origin)?; // DID is not registered already ensure!( - !DIDs::::exists(did), + !Dids::::exists(did), Error::::DidAlreadyExists ); let current_block_no = >::block_number(); - DIDs::::insert(did, (detail, current_block_no)); + Dids::::insert(did, (detail, current_block_no)); Self::deposit_event(RawEvent::DidAdded(did)); Ok(()) } @@ -302,7 +304,7 @@ decl_module! { let mut current_key_detail = Self::ensure_registered_and_new(&key_update.did, key_update.last_modified_in_block)?; // serialize `KeyUpdate` to bytes - let serz_key_update = SignableCommand::KeyUpdate(key_update.clone()).encode(); + let serz_key_update = StateChange::KeyUpdate(key_update.clone()).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_key_update, ¤t_key_detail.public_key)?; @@ -319,7 +321,7 @@ decl_module! { current_key_detail.controller = ctrl; } - DIDs::::insert(key_update.did, (current_key_detail, current_block_no)); + Dids::::insert(key_update.did, (current_key_detail, current_block_no)); Self::deposit_event(RawEvent::KeyUpdated(key_update.did)); Ok(()) } @@ -328,7 +330,7 @@ decl_module! { /// `signature` is the signature on the serialized `DIDRemoval`. /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` /// using the stored data and try to verify the given signature with the stored key. - pub fn remove(origin, to_remove: DIDRemoval, signature: Signature) -> DispatchResult { + pub fn remove(origin, to_remove: DidRemoval, signature: Signature) -> DispatchResult { ensure_signed(origin)?; // DID is registered and the removal is not being replayed @@ -336,7 +338,7 @@ decl_module! { let did = to_remove.did; // serialize `DIDRemoval` to bytes - let serz_rem = SignableCommand::DIDRemoval(to_remove).encode(); + let serz_rem = StateChange::DIDRemoval(to_remove).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_rem, ¤t_key_detail.public_key)?; @@ -345,7 +347,7 @@ decl_module! { ensure!(sig_ver, Error::::InvalidSig); // Remove DID - DIDs::::remove(did); + Dids::::remove(did); Self::deposit_event(RawEvent::DidRemoved(did)); Ok(()) } @@ -356,7 +358,7 @@ impl Module { /// Ensure that the DID is registered and this is not a replayed payload by checking the equality /// with stored block number when the DID was last modified. pub fn ensure_registered_and_new( - did: &DID, + did: &Did, last_modified_in_block: BlockNumber, ) -> Result { let (current_key_detail, last_modified) = Self::get_key_detail(did)?; @@ -374,9 +376,9 @@ impl Module { /// It assumes that the DID has only 1 public key which is true for now but will change later. /// This function will then be modified to indicate which key(s) of the DID should be used. /// If DID is not registered an error is raised. - pub fn get_key_detail(did: &DID) -> Result<(KeyDetail, T::BlockNumber), DispatchError> { + pub fn get_key_detail(did: &Did) -> Result<(KeyDetail, T::BlockNumber), DispatchError> { // DID must be registered. - if let Some((current_key_detail, last_modified)) = DIDs::::get(did) { + if let Some((current_key_detail, last_modified)) = Dids::::get(did) { Ok((current_key_detail, last_modified)) } else { fail!(Error::::DidDoesNotExist) @@ -388,10 +390,10 @@ impl Module { /// This function will then be modified to indicate which key(s) of the DID should be used. /// If DID is not registered an error is raised. /// This function is intended to be used by other modules as well to check the signature from a DID. - pub fn verify_sig_from_DID( + pub fn verify_sig_from_Did( signature: &Signature, message: &[u8], - did: &DID, + did: &Did, ) -> Result { let (current_key_detail, _) = Self::get_key_detail(did)?; Self::verify_sig_with_public_key(signature, message, ¤t_key_detail.public_key) @@ -406,19 +408,19 @@ impl Module { Ok(match public_key { PublicKey::Sr25519(bytes) => { let signature = - sr25519::Signature::try_from(signature.as_Sr25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + sr25519::Signature::try_from(signature.as_sr25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Ed25519(bytes) => { let signature = - ed25519::Signature::try_from(signature.as_Ed25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + ed25519::Signature::try_from(signature.as_ed25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Secp256k1(bytes) => { let signature = - ecdsa::Signature::try_from(signature.as_Secp256k1_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + ecdsa::Signature::try_from(signature.as_secp256k1_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; let pk = ecdsa::Public::Compressed(bytes.value.clone()); signature.verify(message, &pk) } @@ -589,7 +591,7 @@ mod tests { None, 2u32, ); - let sig = Signature::Sr25519(Bytes64 {value: pair.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 {value: pair.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); assert_err!( DIDModule::update_key(Origin::signed(alice), key_update, sig), @@ -632,7 +634,7 @@ mod tests { None, modified_in_block as u32, ); - let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) assert_ok!(DIDModule::update_key( @@ -653,7 +655,7 @@ mod tests { None, modified_in_block as u32, ); - let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = $sig_type($sig_bytearray_type {value: pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); assert_err!( DIDModule::update_key(Origin::signed(alice), key_update, sig), @@ -668,7 +670,7 @@ mod tests { Some(new_controller), modified_in_block as u32, ); - let sig = $sig_type($sig_bytearray_type {value: pair_2.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = $sig_type($sig_bytearray_type {value: pair_2.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -724,7 +726,7 @@ mod tests { ); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) - let value: [u8; 65] = pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).into(); + let value: [u8; 65] = pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).into(); let sig = Signature::Secp256k1(Bytes65 {value}); assert_ok!(DIDModule::update_key( Origin::signed(alice), @@ -742,7 +744,7 @@ mod tests { None, modified_in_block as u32, ); - let value: [u8; 65] = pair_1.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).into(); + let value: [u8; 65] = pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).into(); let sig = Signature::Secp256k1(Bytes65 {value}); assert_err!( DIDModule::update_key(Origin::signed(alice), key_update.clone(), sig), @@ -793,7 +795,7 @@ mod tests { ); // Update key from `pk_1` to `pk_2` using `pk_1`'s signature - let sig_to_be_replayed = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::KeyUpdate(key_update_to_be_replayed.clone()).encode()).0}); + let sig_to_be_replayed = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::KeyUpdate(key_update_to_be_replayed.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update_to_be_replayed.clone(), @@ -817,7 +819,7 @@ mod tests { ); // Update key from `pk_2` to `pk_3` using `pk_2`'s signature - let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -838,7 +840,7 @@ mod tests { ); // Update key from `pk_3` to `pk_1` using `pk_3`'s signature - let sig = Signature::Sr25519(Bytes64 {value: pair_3.sign(&SignableCommand::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 {value: pair_3.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -863,9 +865,9 @@ mod tests { #[test] fn did_remove() { - // Remove DID. Unregistered DIDs cannot be removed. - // Registered DIDs can only be removed by the authorized key - // Removed DIDs can be added again + // Remove DID. Unregistered Dids cannot be removed. + // Registered Dids can only be removed by the authorized key + // Removed Dids can be added again new_test_ext().execute_with(|| { let alice = 100u64; @@ -874,8 +876,8 @@ mod tests { let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; - let to_remove = DIDRemoval::new(did.clone(), 2u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); + let to_remove = DidRemoval::new(did.clone(), 2u32); + let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); // Trying to remove the DID before it was added will fail assert_err!( @@ -899,16 +901,16 @@ mod tests { // A key not controlling the DID but trying to remove the DID should fail let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; - let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); + let to_remove = DidRemoval::new(did.clone(), modified_in_block as u32); + let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); assert_err!( DIDModule::remove(Origin::signed(alice), to_remove, sig), Error::::InvalidSig ); // The key controlling the DID should be able to remove the DID - let to_remove = DIDRemoval::new(did.clone(), modified_in_block as u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&SignableCommand::DIDRemoval(to_remove.clone()).encode()).0}); + let to_remove = DidRemoval::new(did.clone(), modified_in_block as u32); + let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); assert_ok!(DIDModule::remove( Origin::signed(alice), to_remove, diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 40c4c5c6d..8685fbda5 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -68,13 +68,14 @@ mod template; mod did; -/// Any command that needs to be signed is first wrapped in this enum and then its serialized. +/// Any state change that needs to be signed is first wrapped in this enum and then its serialized. /// This is done to prevent make it unambiguous which command was intended as the SCALE codec's /// not self describing. +/// Never change the order of variants in this enum #[derive(Encode, Decode)] -pub enum SignableCommand { +pub enum StateChange { KeyUpdate(did::KeyUpdate), - DIDRemoval(did::DIDRemoval) + DIDRemoval(did::DidRemoval) } /// Opaque types. These are used by the CLI to instantiate machinery that don't need to know From aaca7fc0b7c609419d959edb5976053e5a3b0d43 Mon Sep 17 00:00:00 2001 From: Andrew Dirksen Date: Thu, 12 Mar 2020 13:47:16 -0700 Subject: [PATCH 12/17] cargo fmt --- runtime/src/did.rs | 97 ++++++++++++++++++++++++++++++++-------------- runtime/src/lib.rs | 4 +- 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 4a2aba27b..53e80c055 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -96,10 +96,9 @@ macro_rules! struct_over_byte_array { &self.value } } - } + }; } - struct_over_byte_array!(Bytes33, 33); struct_over_byte_array!(Bytes64, 64); struct_over_byte_array!(Bytes65, 65); @@ -132,7 +131,7 @@ impl Signature { pub fn as_sr25519_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Sr25519(bytes) => Ok(bytes.as_bytes()), - _ => Err(()) + _ => Err(()), } } @@ -140,7 +139,7 @@ impl Signature { pub fn as_ed25519_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Ed25519(bytes) => Ok(bytes.as_bytes()), - _ => Err(()) + _ => Err(()), } } @@ -148,7 +147,7 @@ impl Signature { pub fn as_secp256k1_sig_bytes(&self) -> Result<&[u8], ()> { match self { Signature::Secp256k1(bytes) => Ok(bytes.as_bytes()), - _ => Err(()) + _ => Err(()), } } } @@ -407,20 +406,32 @@ impl Module { ) -> Result { Ok(match public_key { PublicKey::Sr25519(bytes) => { - let signature = - sr25519::Signature::try_from(signature.as_sr25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + let signature = sr25519::Signature::try_from( + signature + .as_sr25519_sig_bytes() + .map_err(|_| Error::::InvalidSigType)?, + ) + .map_err(|_| Error::::InvalidSig)?; let pk = sr25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Ed25519(bytes) => { - let signature = - ed25519::Signature::try_from(signature.as_ed25519_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + let signature = ed25519::Signature::try_from( + signature + .as_ed25519_sig_bytes() + .map_err(|_| Error::::InvalidSigType)?, + ) + .map_err(|_| Error::::InvalidSig)?; let pk = ed25519::Public(bytes.value.clone()); signature.verify(message, &pk) } PublicKey::Secp256k1(bytes) => { - let signature = - ecdsa::Signature::try_from(signature.as_secp256k1_sig_bytes().map_err(|_| Error::::InvalidSigType)?).map_err(|_| Error::::InvalidSig)?; + let signature = ecdsa::Signature::try_from( + signature + .as_secp256k1_sig_bytes() + .map_err(|_| Error::::InvalidSigType)?, + ) + .map_err(|_| Error::::InvalidSig)?; let pk = ecdsa::Public::Compressed(bytes.value.clone()); signature.verify(message, &pk) } @@ -549,7 +560,7 @@ mod tests { let alice = 10u64; let did = [1; DID_BYTE_SIZE]; - let pk = PublicKey::Sr25519(Bytes32 {value: [0;32]}); + let pk = PublicKey::Sr25519(Bytes32 { value: [0; 32] }); let detail = KeyDetail::new(did.clone(), pk); // Add a DID @@ -566,7 +577,7 @@ mod tests { ); // Try to add the same DID again but with different key detail and fail - let pk = PublicKey::Ed25519(Bytes32 {value: [0;32]}); + let pk = PublicKey::Ed25519(Bytes32 { value: [0; 32] }); let detail = KeyDetail::new(did.clone(), pk); assert_err!( DIDModule::new(Origin::signed(alice), did, detail), @@ -591,7 +602,11 @@ mod tests { None, 2u32, ); - let sig = Signature::Sr25519(Bytes64 {value: pair.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 { + value: pair + .sign(&StateChange::KeyUpdate(key_update.clone()).encode()) + .0, + }); assert_err!( DIDModule::update_key(Origin::signed(alice), key_update, sig), @@ -726,8 +741,10 @@ mod tests { ); // Signing with the current key (`pair_1`) to update to the new key (`pair_2`) - let value: [u8; 65] = pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).into(); - let sig = Signature::Secp256k1(Bytes65 {value}); + let value: [u8; 65] = pair_1 + .sign(&StateChange::KeyUpdate(key_update.clone()).encode()) + .into(); + let sig = Signature::Secp256k1(Bytes65 { value }); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -744,8 +761,10 @@ mod tests { None, modified_in_block as u32, ); - let value: [u8; 65] = pair_1.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).into(); - let sig = Signature::Secp256k1(Bytes65 {value}); + let value: [u8; 65] = pair_1 + .sign(&StateChange::KeyUpdate(key_update.clone()).encode()) + .into(); + let sig = Signature::Secp256k1(Bytes65 { value }); assert_err!( DIDModule::update_key(Origin::signed(alice), key_update.clone(), sig), Error::::InvalidSig @@ -795,7 +814,11 @@ mod tests { ); // Update key from `pk_1` to `pk_2` using `pk_1`'s signature - let sig_to_be_replayed = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::KeyUpdate(key_update_to_be_replayed.clone()).encode()).0}); + let sig_to_be_replayed = Signature::Sr25519(Bytes64 { + value: pair_1 + .sign(&StateChange::KeyUpdate(key_update_to_be_replayed.clone()).encode()) + .0, + }); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update_to_be_replayed.clone(), @@ -819,7 +842,11 @@ mod tests { ); // Update key from `pk_2` to `pk_3` using `pk_2`'s signature - let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 { + value: pair_2 + .sign(&StateChange::KeyUpdate(key_update.clone()).encode()) + .0, + }); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -840,7 +867,11 @@ mod tests { ); // Update key from `pk_3` to `pk_1` using `pk_3`'s signature - let sig = Signature::Sr25519(Bytes64 {value: pair_3.sign(&StateChange::KeyUpdate(key_update.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 { + value: pair_3 + .sign(&StateChange::KeyUpdate(key_update.clone()).encode()) + .0, + }); assert_ok!(DIDModule::update_key( Origin::signed(alice), key_update, @@ -877,7 +908,11 @@ mod tests { let (pair_1, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_1 = pair_1.public().0; let to_remove = DidRemoval::new(did.clone(), 2u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 { + value: pair_1 + .sign(&StateChange::DIDRemoval(to_remove.clone()).encode()) + .0, + }); // Trying to remove the DID before it was added will fail assert_err!( @@ -902,7 +937,11 @@ mod tests { let (pair_2, _, _) = sr25519::Pair::generate_with_phrase(None); let pk_2 = pair_2.public().0; let to_remove = DidRemoval::new(did.clone(), modified_in_block as u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_2.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); + let sig = Signature::Sr25519(Bytes64 { + value: pair_2 + .sign(&StateChange::DIDRemoval(to_remove.clone()).encode()) + .0, + }); assert_err!( DIDModule::remove(Origin::signed(alice), to_remove, sig), Error::::InvalidSig @@ -910,12 +949,12 @@ mod tests { // The key controlling the DID should be able to remove the DID let to_remove = DidRemoval::new(did.clone(), modified_in_block as u32); - let sig = Signature::Sr25519(Bytes64 {value: pair_1.sign(&StateChange::DIDRemoval(to_remove.clone()).encode()).0}); - assert_ok!(DIDModule::remove( - Origin::signed(alice), - to_remove, - sig - )); + let sig = Signature::Sr25519(Bytes64 { + value: pair_1 + .sign(&StateChange::DIDRemoval(to_remove.clone()).encode()) + .0, + }); + assert_ok!(DIDModule::remove(Origin::signed(alice), to_remove, sig)); // Error as the did has been removed assert!(DIDModule::get_key_detail(&did).is_err()); diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 8685fbda5..0db19f42d 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -8,6 +8,7 @@ #[cfg(feature = "std")] include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs")); +use codec::{Decode, Encode}; use grandpa::fg_primitives; use grandpa::AuthorityList as GrandpaAuthorityList; use sp_api::impl_runtime_apis; @@ -21,7 +22,6 @@ use sp_runtime::{ ApplyExtrinsicResult, MultiSignature, }; use sp_std::prelude::*; -use codec::{Decode, Encode}; #[cfg(feature = "std")] use sp_version::NativeVersion; @@ -75,7 +75,7 @@ mod did; #[derive(Encode, Decode)] pub enum StateChange { KeyUpdate(did::KeyUpdate), - DIDRemoval(did::DidRemoval) + DIDRemoval(did::DidRemoval), } /// Opaque types. These are used by the CLI to instantiate machinery that don't need to know From c6e586d02413973d8b69784f2cf236f2c8206b7d Mon Sep 17 00:00:00 2001 From: Andrew Dirksen Date: Thu, 12 Mar 2020 13:50:47 -0700 Subject: [PATCH 13/17] handle warnings from cargo check --- runtime/src/did.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 53e80c055..f0e2303f5 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -2,7 +2,7 @@ use super::{BlockNumber, StateChange}; use codec::{Decode, Encode}; use frame_support::{ decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchError, - dispatch::DispatchResult, ensure, fail, traits::Get, + dispatch::DispatchResult, ensure, fail, }; use sp_core::{ecdsa, ed25519, sr25519}; use sp_runtime::traits::Verify; @@ -389,7 +389,7 @@ impl Module { /// This function will then be modified to indicate which key(s) of the DID should be used. /// If DID is not registered an error is raised. /// This function is intended to be used by other modules as well to check the signature from a DID. - pub fn verify_sig_from_Did( + pub fn verify_sig_from_did( signature: &Signature, message: &[u8], did: &Did, From 2def58a4eaff30480f5b5e3d0124a8e3ea5cbeb0 Mon Sep 17 00:00:00 2001 From: Andrew Dirksen Date: Thu, 12 Mar 2020 14:19:41 -0700 Subject: [PATCH 14/17] Edit KeyUpdate doc comment for clarity. --- runtime/src/did.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index f0e2303f5..2aff9c11a 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -192,17 +192,18 @@ impl KeyDetail { } } -/// This struct is passed as an argument while updating the key +/// This struct is passed as an argument while updating the key for a DID. /// `did` is the DID whose key is being updated. /// `public_key` the new public key -/// `controller` If provided None, the controller is unchanged. While serializing, use literal "None" when controller is None -/// The last_modified_in_block is the block number when this DID was last modified is present to prevent replay attack. -/// This approach allows only 1 update transaction in a block (don't see it as a problem as key updates are rare). -/// Its possible to submit one txn per block but the sender has to guess and he might guess it right in high block -/// latency and low traffic network. +/// `controller` If provided None, the controller is unchanged. While serializing, use literal +/// "None" when controller is None. +/// The last_modified_in_block is the block number when this DID was last modified. It is used to +/// prevent replay attacks. This approach allows easy submission of 1 update transaction in a block. +/// It's theoretically possible to submit more than one txn per block, but the method is +/// non-trivial and potentially unreliable. /// An alternate approach can be to have a nonce associated to each detail which is incremented on each -/// successful extrinsic and the chain requiring the extrinsic's nonce to be higher than current. This is -/// little more involved as it involves a ">" check +/// successful extrinsic and the chain requiring the extrinsic's nonce to be higher than current. +/// This is little more involved as it involves a ">" check #[derive(Encode, Decode, Clone, PartialEq, Debug)] pub struct KeyUpdate { did: Did, From 86f4c2ec2a98179500a768f90aae1d9af32b4209 Mon Sep 17 00:00:00 2001 From: Andrew Dirksen Date: Thu, 12 Mar 2020 14:45:50 -0700 Subject: [PATCH 15/17] rustfmt interior if decl_module in did.rs --- runtime/src/did.rs | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 2aff9c11a..7759bf300 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -280,10 +280,7 @@ decl_module! { ensure_signed(origin)?; // DID is not registered already - ensure!( - !Dids::::exists(did), - Error::::DidAlreadyExists - ); + ensure!(!Dids::::exists(did), Error::::DidAlreadyExists); let current_block_no = >::block_number(); Dids::::insert(did, (detail, current_block_no)); @@ -295,19 +292,30 @@ decl_module! { /// `signature` is the signature on the serialized `KeyUpdate`. /// The node while processing this extrinsic, should create the above serialized `KeyUpdate` /// using the stored data and try to verify the given signature with the stored key. - pub fn update_key(origin, key_update: KeyUpdate, signature: Signature) -> DispatchResult { + pub fn update_key( + origin, + key_update: KeyUpdate, + signature: Signature, + ) -> DispatchResult { ensure_signed(origin)?; // Not checking for signature size as its not stored // DID is registered and the update is not being replayed - let mut current_key_detail = Self::ensure_registered_and_new(&key_update.did, key_update.last_modified_in_block)?; + let mut current_key_detail = Self::ensure_registered_and_new( + &key_update.did, + key_update.last_modified_in_block, + )?; // serialize `KeyUpdate` to bytes let serz_key_update = StateChange::KeyUpdate(key_update.clone()).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key - let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_key_update, ¤t_key_detail.public_key)?; + let sig_ver = Self::verify_sig_with_public_key( + &signature, + &serz_key_update, + ¤t_key_detail.public_key, + )?; // Throw error if signature is invalid ensure!(sig_ver, Error::::InvalidSig); @@ -334,14 +342,19 @@ decl_module! { ensure_signed(origin)?; // DID is registered and the removal is not being replayed - let current_key_detail = Self::ensure_registered_and_new(&to_remove.did, to_remove.last_modified_in_block)?; + let current_key_detail = + Self::ensure_registered_and_new(&to_remove.did, to_remove.last_modified_in_block)?; let did = to_remove.did; // serialize `DIDRemoval` to bytes let serz_rem = StateChange::DIDRemoval(to_remove).encode(); // Verify signature on the serialized `KeyUpdate` with the current public key - let sig_ver = Self::verify_sig_with_public_key(&signature, &serz_rem, ¤t_key_detail.public_key)?; + let sig_ver = Self::verify_sig_with_public_key( + &signature, + &serz_rem, + ¤t_key_detail.public_key, + )?; // Throw error if signature is invalid ensure!(sig_ver, Error::::InvalidSig); @@ -357,7 +370,7 @@ decl_module! { impl Module { /// Ensure that the DID is registered and this is not a replayed payload by checking the equality /// with stored block number when the DID was last modified. - pub fn ensure_registered_and_new( + fn ensure_registered_and_new( did: &Did, last_modified_in_block: BlockNumber, ) -> Result { From 5ab882baa0aa3aa35be95a7e3bdaa8f7c7cde867 Mon Sep 17 00:00:00 2001 From: Andrew Dirksen Date: Thu, 12 Mar 2020 16:40:01 -0700 Subject: [PATCH 16/17] Remove some vestigals. Formatting. Update doc comments. --- runtime/src/did.rs | 41 ++++++++++++++++++++++------------------- runtime/src/lib.rs | 2 +- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 7759bf300..5e0b41b8c 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -10,7 +10,6 @@ use sp_std::convert::TryFrom; use sp_std::fmt; use system::ensure_signed; -// XXX: explore using `parameter_types!` for DID_BYTE_SIZE. The problem is that its needed for defining DID /// Size of the Dock DID in bytes pub const DID_BYTE_SIZE: usize = 32; /// The type of the Dock DID @@ -19,8 +18,7 @@ pub type Did = [u8; DID_BYTE_SIZE]; /// The module's configuration trait. pub trait Trait: system::Trait { /// The overarching event type. - type Event: From> + Into<::Event>; - //type DIDByteSize: Get; + type Event: From + Into<::Event>; } decl_error! { @@ -250,14 +248,10 @@ impl DidRemoval { } decl_event!( - pub enum Event - where - AccountId = ::AccountId, - { + pub enum Event { DidAdded(Did), KeyUpdated(Did), DidRemoved(Did), - DummyEvent(AccountId), } ); @@ -284,14 +278,19 @@ decl_module! { let current_block_no = >::block_number(); Dids::::insert(did, (detail, current_block_no)); - Self::deposit_event(RawEvent::DidAdded(did)); + Self::deposit_event(Event::DidAdded(did)); Ok(()) } + /// Sets the single publicKey (and possibly its controller) stored in this DID. + /// /// `key_update` specifies which DID's key needs to be updated - /// `signature` is the signature on the serialized `KeyUpdate`. - /// The node while processing this extrinsic, should create the above serialized `KeyUpdate` - /// using the stored data and try to verify the given signature with the stored key. + /// `signature` is the signature on a serialized [StateChange][statechange] + /// + /// During execution this function checks for a signature over [StateChange][statechange] + /// and verifies the given signature with the stored key. + /// + /// [statechange]: ../enum.StateChange.html pub fn update_key( origin, key_update: KeyUpdate, @@ -299,8 +298,6 @@ decl_module! { ) -> DispatchResult { ensure_signed(origin)?; - // Not checking for signature size as its not stored - // DID is registered and the update is not being replayed let mut current_key_detail = Self::ensure_registered_and_new( &key_update.did, @@ -330,14 +327,20 @@ decl_module! { } Dids::::insert(key_update.did, (current_key_detail, current_block_no)); - Self::deposit_event(RawEvent::KeyUpdated(key_update.did)); + Self::deposit_event(Event::KeyUpdated(key_update.did)); Ok(()) } + /// Deletes a DID from chain storage. Once the DID is deleted, anyone can call new to claim + /// it for their own. + /// /// `to_remove` contains the DID to be removed - /// `signature` is the signature on the serialized `DIDRemoval`. - /// The node while processing this extrinsic, should create the above serialized `DIDRemoval` - /// using the stored data and try to verify the given signature with the stored key. + /// `signature` is the signature on a serialized [StateChange][statechange] + /// + /// During execution this function checks for a signature over [StateChange][statechange] + /// and verifies the given signature with the stored key. + /// + /// [statechange]: ../enum.StateChange.html pub fn remove(origin, to_remove: DidRemoval, signature: Signature) -> DispatchResult { ensure_signed(origin)?; @@ -361,7 +364,7 @@ decl_module! { // Remove DID Dids::::remove(did); - Self::deposit_event(RawEvent::DidRemoved(did)); + Self::deposit_event(Event::DidRemoved(did)); Ok(()) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 0db19f42d..a02b04860 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -277,7 +277,7 @@ construct_runtime!( Sudo: sudo, // Used for the module template in `./template.rs` TemplateModule: template::{Module, Call, Storage, Event}, - DIDModule: did::{Module, Call, Storage, Event}, + DIDModule: did::{Module, Call, Storage, Event}, RandomnessCollectiveFlip: randomness_collective_flip::{Module, Call, Storage}, } ); From d15396e3cb9e4e72ee495d645f47b544cf7300e1 Mon Sep 17 00:00:00 2001 From: lovesh Date: Fri, 13 Mar 2020 17:04:13 +0530 Subject: [PATCH 17/17] Minor doc edits Signed-off-by: lovesh --- runtime/src/did.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/runtime/src/did.rs b/runtime/src/did.rs index 5e0b41b8c..49286fc4c 100644 --- a/runtime/src/did.rs +++ b/runtime/src/did.rs @@ -285,12 +285,14 @@ decl_module! { /// Sets the single publicKey (and possibly its controller) stored in this DID. /// /// `key_update` specifies which DID's key needs to be updated - /// `signature` is the signature on a serialized [StateChange][statechange] + /// `signature` is the signature on a serialized [StateChange][statechange] that wraps the + /// [KeyUpdate][keyupdate] struct /// /// During execution this function checks for a signature over [StateChange][statechange] /// and verifies the given signature with the stored key. /// /// [statechange]: ../enum.StateChange.html + /// [keyupdate]: ./struct.KeyUpdate.html pub fn update_key( origin, key_update: KeyUpdate, @@ -335,12 +337,14 @@ decl_module! { /// it for their own. /// /// `to_remove` contains the DID to be removed - /// `signature` is the signature on a serialized [StateChange][statechange] + /// `signature` is the signature on a serialized [StateChange][statechange] that wraps the + /// [DidRemoval][didremoval] struct /// /// During execution this function checks for a signature over [StateChange][statechange] /// and verifies the given signature with the stored key. /// /// [statechange]: ../enum.StateChange.html + /// [didremoval]: ./struct.DidRemoval.html pub fn remove(origin, to_remove: DidRemoval, signature: Signature) -> DispatchResult { ensure_signed(origin)?;