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

Authentication of PeerIds in authority discovery records #10317

Merged
merged 17 commits into from
Dec 5, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 2 additions & 0 deletions bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ pub fn new_full_base(
let name = config.network.node_name.clone();
let enable_grandpa = !config.disable_grandpa;
let prometheus_registry = config.prometheus_registry().cloned();
let strict_record_validation = config.strict_authority_records;

let rpc_handlers = sc_service::spawn_tasks(sc_service::SpawnTasksParams {
config,
Expand Down Expand Up @@ -455,6 +456,7 @@ pub fn new_full_base(
sc_authority_discovery::new_worker_and_service_with_config(
sc_authority_discovery::WorkerConfig {
publish_non_global_ips: auth_disc_publish_non_global_ips,
strict_record_validation,
..Default::default()
},
client.clone(),
Expand Down
4 changes: 2 additions & 2 deletions client/authority-discovery/README.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
Substrate authority discovery.
# Substrate authority discovery

This crate enables Substrate authorities to discover and directly connect to
other authorities. It is split into two components the [`Worker`] and the
[`Service`].

See [`Worker`] and [`Service`] for more documentation.

License: GPL-3.0-or-later WITH Classpath-exception-2.0
License: GPL-3.0-or-later WITH Classpath-exception-2.0
8 changes: 8 additions & 0 deletions client/authority-discovery/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,18 @@ pub enum Error {
EncodingDecodingScale(codec::Error),
/// Failed to parse a libp2p multi address.
ParsingMultiaddress(libp2p::core::multiaddr::Error),
/// Failed to parse a libp2p key.
ParsingLibp2pIdentity(libp2p::identity::error::DecodingError),
/// Failed to sign using a specific public key.
MissingSignature(CryptoTypePublicPair),
/// Failed to sign using all public keys.
Signing,
/// Failed to register Prometheus metric.
Prometheus(prometheus_endpoint::PrometheusError),
/// Received authority record that contains addresses with multiple peer ids
ReceivingDhtValueFoundEventWithDifferentPeerIds,
/// Received authority record without any addresses having a peer id
ReceivingDhtValueFoundEventWithNoPeerIds,
/// Received authority record without a valid signature for the remote peer id.
MissingPeerIdSignature,
}
6 changes: 6 additions & 0 deletions client/authority-discovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ pub struct WorkerConfig {
///
/// Defaults to `true` to avoid the surprise factor.
pub publish_non_global_ips: bool,

/// Reject authority discovery records that are not signed by their network identity (PeerId)
///
/// Defaults to `false` to provide compatibility with old versions
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should default to true to have new networks enabled this by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would cause silent breaking for existing networks, which use Default::default() in their sc-authority-discovery::Worker instantiation. I would be more conservative here and reduce gotchas in the ecosystem.

Copy link
Member

Choose a reason for hiding this comment

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

I think no one uses this, but fine.

Then we should document this in the docs of the crate?

pub strict_record_validation: bool,
}

impl Default for WorkerConfig {
Expand All @@ -98,6 +103,7 @@ impl Default for WorkerConfig {
// `authority_discovery_dht_event_received`.
max_query_interval: Duration::from_secs(10 * 60),
publish_non_global_ips: true,
strict_record_validation: false,
}
}
}
Expand Down
29 changes: 29 additions & 0 deletions client/authority-discovery/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,32 @@ fn get_addresses_and_authority_id() {
);
});
}

#[test]
fn cryptos_are_compatible() {
use sp_core::crypto::Pair;

let libp2p_secret = libp2p::identity::Keypair::generate_ed25519();
let libp2p_public = libp2p_secret.public();

let sp_core_secret = {
let libp2p_ed_secret = match libp2p_secret.clone() {
libp2p::identity::Keypair::Ed25519(x) => x,
_ => panic!("generate_ed25519 should have generated an Ed25519 key ¯\\_(ツ)_/¯"),
};
sp_core::ed25519::Pair::from_seed_slice(&libp2p_ed_secret.secret().as_ref()).unwrap()
};
let sp_core_public = sp_core_secret.public();

let message = b"we are more powerful than not to be better";

let libp2p_signature = libp2p_secret.sign(message).unwrap();
let sp_core_signature = sp_core_secret.sign(message); // no error expected...

assert!(sp_core::ed25519::Pair::verify(
&sp_core::ed25519::Signature::from_slice(&libp2p_signature),
message,
&sp_core_public
));
assert!(libp2p_public.verify(message, sp_core_signature.as_ref()));
}
Loading