Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Expand remote keystore interface to allow for hybrid mode #7628

Merged
merged 23 commits into from
Dec 9, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
66fb709
update to latest master
gnunicorn Oct 20, 2020
ea6abb0
updates on docs, license, meta
gnunicorn Oct 21, 2020
790f00e
hide ssrs behind feature flag
gnunicorn Oct 27, 2020
29a68ee
Merge remote-tracking branch 'origin/master' into ben-remote-signer
gnunicorn Nov 3, 2020
1f292b5
implement remaining functions on the server
gnunicorn Nov 3, 2020
7ad8521
sign server line length fix
gnunicorn Nov 3, 2020
d310105
fix tests
gnunicorn Nov 3, 2020
2ec7541
fixup in-memory-keystore
gnunicorn Nov 4, 2020
fc9adfe
adding failsafe
gnunicorn Nov 4, 2020
ac8fb13
skipping ecdsa test for now
gnunicorn Nov 4, 2020
95fb534
Merge remote-tracking branch 'origin/master' into ben-remote-signer
gnunicorn Nov 4, 2020
a9377a3
Merge remote-tracking branch 'origin/master' into ben-remote-signer
gnunicorn Nov 25, 2020
610dce8
remote keystore param
gnunicorn Nov 25, 2020
0d58447
remote sign urls made available
gnunicorn Nov 26, 2020
a8ca9ba
integrating keystore remotes features
gnunicorn Nov 26, 2020
81f9356
don't forget the dependency
gnunicorn Nov 27, 2020
418d893
remove old cruft
gnunicorn Nov 27, 2020
24eede6
Merge remote-tracking branch 'origin/master' into ben-remote-signer
gnunicorn Nov 30, 2020
df50b0d
reset local keystore
gnunicorn Nov 30, 2020
4651095
applying suggestions
gnunicorn Nov 30, 2020
369a592
Switch to single remote, minor grumbles
gnunicorn Dec 7, 2020
8361c7d
Merge remote-tracking branch 'origin/master' into ben-remote-keystore…
gnunicorn Dec 7, 2020
be2fbc6
minor grumbles, docs
gnunicorn Dec 7, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bin/node-template/node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ sc-cli = { version = "0.8.0", path = "../../../client/cli", features = ["wasmtim
sp-core = { version = "2.0.0", path = "../../../primitives/core" }
sc-executor = { version = "0.8.0", path = "../../../client/executor", features = ["wasmtime"] }
sc-service = { version = "0.8.0", path = "../../../client/service", features = ["wasmtime"] }
sc-keystore = { version = "2.0.0", path = "../../../client/keystore" }
sp-inherents = { version = "2.0.0", path = "../../../primitives/inherents" }
sc-transaction-pool = { version = "2.0.0", path = "../../../client/transaction-pool" }
sp-transaction-pool = { version = "2.0.0", path = "../../../primitives/transaction-pool" }
Expand Down
23 changes: 22 additions & 1 deletion bin/node-template/node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use sc_executor::native_executor_instance;
pub use sc_executor::NativeExecutor;
use sp_consensus_aura::sr25519::{AuthorityPair as AuraPair};
use sc_finality_grandpa::SharedVoterState;
use sc_keystore::LocalKeystore;

// Our native executor instance.
native_executor_instance!(
Expand Down Expand Up @@ -37,6 +38,10 @@ pub fn new_partial(config: &Configuration) -> Result<sc_service::PartialComponen
sc_finality_grandpa::LinkHalf<Block, FullClient, FullSelectChain>
)
>, ServiceError> {
if config.keystore_remote.is_some() {
return Err(ServiceError::Other(
format!("Remote Keystores are not supported.")))
}
let inherent_data_providers = sp_inherents::InherentDataProviders::new();

let (client, backend, keystore_container, task_manager) =
Expand Down Expand Up @@ -78,14 +83,30 @@ pub fn new_partial(config: &Configuration) -> Result<sc_service::PartialComponen
})
}

fn remote_keystore(_url: &String) -> Result<Arc<LocalKeystore>, &'static str> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is a remote store going to return a concrete LocalKeystore? Shouldn't this be Arc<dyn CryptoStore>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We face the same CryptoStore + SyncCryptoStore interface problem here, so I can't quite make this a dyn unless I provide a similar wrapper. Which is quite the overkill as in reality this will always return a concrete type and thus the interface is unnecessary. In this example I just used the local-store as the concrete type (that, as you can see, is never returned)

// FIXME: here would the concrete keystore be built,
// must return a concrete type (NOT `LocalKeystore`) that
// implements `CryptoStore` and `SyncCryptoStore`
Err("Remote Keystore not supported.")
}

/// Builds a new service for a full client.
pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError> {
let sc_service::PartialComponents {
client, backend, mut task_manager, import_queue, keystore_container,
client, backend, mut task_manager, import_queue, mut keystore_container,
select_chain, transaction_pool, inherent_data_providers,
other: (block_import, grandpa_link),
} = new_partial(&config)?;

if let Some(url) = &config.keystore_remote {
match remote_keystore(url) {
Ok(k) => keystore_container.set_remote_keystore(k),
Err(e) => {
return Err(ServiceError::Other(
format!("Error hooking up remote keystore for {}: {}", url, e)))
}
};
}
config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into());

let (network, network_status_sinks, system_rpc_tx, network_starter) =
Expand Down
2 changes: 1 addition & 1 deletion client/cli/src/commands/insert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl InsertCmd {
.ok_or_else(|| Error::MissingBasePath)?;

let (keystore, public) = match self.keystore_params.keystore_config(base_path)? {
KeystoreConfig::Path { path, password } => {
(_, KeystoreConfig::Path { path, password }) => {
let public = with_crypto_scheme!(
self.crypto_scheme.scheme,
to_vec(&suri, password.clone())
Expand Down
8 changes: 5 additions & 3 deletions client/cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,10 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
///
/// Bu default this is retrieved from `KeystoreParams` if it is available. Otherwise it uses
/// `KeystoreConfig::InMemory`.
fn keystore_config(&self, base_path: &PathBuf) -> Result<KeystoreConfig> {
fn keystore_config(&self, base_path: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
self.keystore_params()
.map(|x| x.keystore_config(base_path))
.unwrap_or(Ok(KeystoreConfig::InMemory))
.unwrap_or_else(|| Ok((None, KeystoreConfig::InMemory)))
}

/// Get the database cache size.
Expand Down Expand Up @@ -471,6 +471,7 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
let role = self.role(is_dev)?;
let max_runtime_instances = self.max_runtime_instances()?.unwrap_or(8);
let is_validator = role.is_network_authority();
let (keystore_remote, keystore) = self.keystore_config(&config_dir)?;

let unsafe_pruning = self
.import_params()
Expand All @@ -491,7 +492,8 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
node_key,
DCV::p2p_listen_port(),
)?,
keystore: self.keystore_config(&config_dir)?,
keystore_remote,
keystore,
database: self.database_config(&config_dir, database_cache_size, database)?,
state_cache_size: self.state_cache_size()?,
state_cache_child_ratio: self.state_cache_child_ratio()?,
Expand Down
9 changes: 7 additions & 2 deletions client/cli/src/params/keystore_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ const DEFAULT_KEYSTORE_CONFIG_PATH: &'static str = "keystore";
/// Parameters of the keystore
#[derive(Debug, StructOpt)]
pub struct KeystoreParams {
/// Specify custom URIs to connect to for keystore-services
#[structopt(long = "keystore-uri")]
pub keystore_uri: Option<String>,
/// Specify custom keystore path.
#[structopt(long = "keystore-path", value_name = "PATH", parse(from_os_str))]
pub keystore_path: Option<PathBuf>,
Expand Down Expand Up @@ -67,7 +70,9 @@ pub fn secret_string_from_str(s: &str) -> std::result::Result<SecretString, Stri

impl KeystoreParams {
/// Get the keystore configuration for the parameters
pub fn keystore_config(&self, base_path: &PathBuf) -> Result<KeystoreConfig> {
/// returns a vector of remote-urls and the local Keystore configuration
pub fn keystore_config(&self, base_path: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {

let password = if self.password_interactive {
#[cfg(not(target_os = "unknown"))]
{
Expand All @@ -89,7 +94,7 @@ impl KeystoreParams {
.clone()
.unwrap_or_else(|| base_path.join(DEFAULT_KEYSTORE_CONFIG_PATH));

Ok(KeystoreConfig::Path { path, password })
Ok((self.keystore_uri.clone(), KeystoreConfig::Path { path, password }))
}

/// helper method to fetch password from `KeyParams` or read from stdin
Expand Down
50 changes: 38 additions & 12 deletions client/service/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use sp_core::traits::{
CodeExecutor,
SpawnNamed,
};
use sp_keystore::{CryptoStore, SyncCryptoStorePtr};
use sp_keystore::{CryptoStore, SyncCryptoStore, SyncCryptoStorePtr};
use sp_runtime::BuildStorage;
use sc_client_api::{
BlockBackend, BlockchainEvents,
Expand Down Expand Up @@ -205,12 +205,25 @@ pub type TLightClientWithBackend<TBl, TRtApi, TExecDisp, TBackend> = Client<
TRtApi,
>;

enum KeystoreContainerInner {
Local(Arc<LocalKeystore>)
trait AsCryptoStoreRef {
fn keystore_ref(&self) -> Arc<dyn CryptoStore>;
fn sync_keystore_ref(&self) -> Arc<dyn SyncCryptoStore>;
}

impl<T> AsCryptoStoreRef for Arc<T> where T: CryptoStore + SyncCryptoStore + 'static {
fn keystore_ref(&self) -> Arc<dyn CryptoStore> {
self.clone()
}
fn sync_keystore_ref(&self) -> Arc<dyn SyncCryptoStore> {
self.clone()
}
}

/// Construct and hold different layers of Keystore wrappers
pub struct KeystoreContainer(KeystoreContainerInner);
pub struct KeystoreContainer {
remote: Option<Box<dyn AsCryptoStoreRef>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep pondering if there is any way to make this nicer. We know this will be an Arc, but here I still have to wrap it into a Box or would have to declare <T> on the Keystore as a whole... neither is nice.

local: Arc<LocalKeystore>,
}

impl KeystoreContainer {
/// Construct KeystoreContainer
Expand All @@ -223,20 +236,35 @@ impl KeystoreContainer {
KeystoreConfig::InMemory => LocalKeystore::in_memory(),
});

Ok(Self(KeystoreContainerInner::Local(keystore)))
Ok(Self{remote: Default::default(), local: keystore})
}

/// Set the remote keystore.
/// Should be called right away at startup and not at runtime:
/// even though this overrides any previously set remote store, it
/// does not reset any references previously handed out - they will
/// stick araound.
pub fn set_remote_keystore<T>(&mut self, remote: Arc<T>)
where T: CryptoStore + SyncCryptoStore + 'static
{
self.remote = Some(Box::new(remote))
}

/// Returns an adapter to the asynchronous keystore that implements `CryptoStore`
pub fn keystore(&self) -> Arc<dyn CryptoStore> {
match self.0 {
KeystoreContainerInner::Local(ref keystore) => keystore.clone(),
if let Some(c) = self.remote.as_ref() {
c.keystore_ref()
} else {
self.local.clone()
}
}

/// Returns the synchrnous keystore wrapper
pub fn sync_keystore(&self) -> SyncCryptoStorePtr {
match self.0 {
KeystoreContainerInner::Local(ref keystore) => keystore.clone() as SyncCryptoStorePtr,
if let Some(c) = self.remote.as_ref() {
c.sync_keystore_ref()
} else {
self.local.clone() as SyncCryptoStorePtr
}
}

Expand All @@ -249,9 +277,7 @@ impl KeystoreContainer {
/// Using the [`LocalKeystore`] will result in loosing the ability to use any other keystore implementation, like
/// a remote keystore for example. Only use this if you a certain that you require it!
pub fn local_keystore(&self) -> Option<Arc<LocalKeystore>> {
match self.0 {
KeystoreContainerInner::Local(ref keystore) => Some(keystore.clone()),
}
Some(self.local.clone())
}
}

Expand Down
2 changes: 2 additions & 0 deletions client/service/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ pub struct Configuration {
pub network: NetworkConfiguration,
/// Configuration for the keystore.
pub keystore: KeystoreConfig,
/// Remote URIs to connect to for async keystore support
pub keystore_remote: Option<String>,
/// Configuration for the database.
pub database: DatabaseConfig,
/// Size of internal state cache in Bytes
Expand Down
1 change: 1 addition & 0 deletions client/service/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ fn node_config<G: RuntimeGenesis + 'static, E: ChainSpecExtension + Clone + 'sta
task_executor,
transaction_pool: Default::default(),
network: network_config,
keystore_remote: Default::default(),
keystore: KeystoreConfig::Path {
path: root.join("key"),
password: None
Expand Down
2 changes: 1 addition & 1 deletion primitives/application-crypto/test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
sp-core = { version = "2.0.0", default-features = false, path = "../../core" }
sp-keystore = { version = "0.8.0", path = "../../keystore" }
sp-keystore = { version = "0.8.0", path = "../../keystore", default-features = false }
substrate-test-runtime-client = { version = "2.0.0", path = "../../../test-utils/runtime/client" }
sp-runtime = { version = "2.0.0", path = "../../runtime" }
sp-api = { version = "2.0.0", path = "../../api" }
Expand Down
2 changes: 1 addition & 1 deletion primitives/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ std = [
"base58",
"substrate-bip39",
"tiny-bip39",
"serde",
"byteorder/std",
"rand",
"sha2/std",
"schnorrkel/std",
"schnorrkel/serde",
"regex",
"num-traits/std",
"tiny-keccak",
Expand Down
3 changes: 3 additions & 0 deletions primitives/core/src/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,7 @@ pub trait CryptoType {
Copy, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Encode, Decode, PassByInner,
crate::RuntimeDebug
)]
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct KeyTypeId(pub [u8; 4]);

impl From<u32> for KeyTypeId {
Expand Down Expand Up @@ -1058,10 +1059,12 @@ impl<'a> TryFrom<&'a str> for KeyTypeId {

/// An identifier for a specific cryptographic algorithm used by a key pair
#[derive(Debug, Copy, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Encode, Decode)]
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct CryptoTypeId(pub [u8; 4]);

/// A type alias of CryptoTypeId & a public key
#[derive(Debug, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Encode, Decode)]
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct CryptoTypePublicPair(pub CryptoTypeId, pub Vec<u8>);

#[cfg(feature = "std")]
Expand Down
11 changes: 10 additions & 1 deletion primitives/keystore/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,19 @@ futures = { version = "0.3.1" }
schnorrkel = { version = "0.9.1", features = ["preaudit_deprecated", "u64_backend"], default-features = false }
merlin = { version = "2.0", default-features = false }
parking_lot = { version = "0.10.0", default-features = false }

serde = { version = "1.0", optional = true}
sp-core = { version = "2.0.0", path = "../core" }
sp-externalities = { version = "0.8.0", path = "../externalities", default-features = false }

[dev-dependencies]
rand = "0.7.2"
rand_chacha = "0.2.2"


[features]
default = ["std"]
std = [
"serde",
"schnorrkel/std",
"schnorrkel/serde",
gnunicorn marked this conversation as resolved.
Show resolved Hide resolved
]
3 changes: 3 additions & 0 deletions primitives/keystore/src/vrf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
use codec::Encode;
use merlin::Transcript;
use schnorrkel::vrf::{VRFOutput, VRFProof};

/// An enum whose variants represent possible
/// accepted values to construct the VRF transcript
#[derive(Clone, Encode)]
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub enum VRFTranscriptValue {
/// Value is an array of bytes
Bytes(Vec<u8>),
Expand All @@ -38,6 +40,7 @@ pub struct VRFTranscriptData {
pub items: Vec<(&'static str, VRFTranscriptValue)>,
}
/// VRF signature data
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct VRFSignature {
/// The VRFOutput serialized
pub output: VRFOutput,
Expand Down
1 change: 1 addition & 0 deletions utils/browser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ where

DatabaseConfig::Custom(sp_database::as_database(db))
},
keystore_remote: Default::default(),
keystore: KeystoreConfig::InMemory,
default_heap_pages: Default::default(),
dev_key_seed: Default::default(),
Expand Down