Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

Commit

Permalink
fix: check trust with all known keys, not just the src matching ones
Browse files Browse the repository at this point in the history
  • Loading branch information
madadam committed Mar 3, 2021
1 parent 76f33ab commit 2c9a1b2
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 46 deletions.
44 changes: 9 additions & 35 deletions src/messages/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub(crate) use self::{
variant::{JoinRequest, ResourceProofResponse, Variant},
};
use crate::{
crypto::{self, name, Verifier},
crypto::{self, Verifier},
error::{Error, Result},
node::Node,
section::{SectionChain, SectionChainError, SectionKeyShare},
Expand All @@ -28,7 +28,7 @@ use serde::{Deserialize, Serialize};
use sn_messaging::{Aggregation, DstLocation};
use std::fmt::{self, Debug, Formatter};
use thiserror::Error;
use xor_name::{Prefix, XorName};
use xor_name::XorName;

/// Message sent over the network.
#[derive(Clone, Eq, Serialize, Deserialize)]
Expand Down Expand Up @@ -218,9 +218,9 @@ impl Message {
}

/// Verify this message is properly signed and trusted.
pub(crate) fn verify<'a, I>(&'a self, trusted_keys: I) -> Result<VerifyStatus>
pub(crate) fn verify<'a, I>(&self, trusted_keys: I) -> Result<VerifyStatus>
where
I: IntoIterator<Item = (&'a Prefix, &'a bls::PublicKey)>,
I: IntoIterator<Item = &'a bls::PublicKey>,
{
let bytes = bincode::serialize(&SignableView {
dst: &self.dst,
Expand All @@ -239,17 +239,9 @@ impl Message {
}

// Variant-specific verification.
let trusted_keys = trusted_keys
.into_iter()
.filter(|(known_prefix, _)| known_prefix.matches(&name(public_key)))
.map(|(_, key)| key);
self.variant.verify(self.proof_chain.as_ref(), trusted_keys)
}
SrcAuthority::BlsShare {
proof_share,
public_key,
..
} => {
SrcAuthority::BlsShare { proof_share, .. } => {
// Proof chain is required for accumulation at destination.
let proof_chain = if let Some(proof_chain) = self.proof_chain.as_ref() {
proof_chain
Expand All @@ -260,18 +252,14 @@ impl Message {
if !proof_share.verify(&bytes) {
return Err(Error::FailedSignature);
}
let trusted_keys = trusted_keys
.into_iter()
.filter(|(known_prefix, _)| known_prefix.matches(&name(public_key)))
.map(|(_, key)| key);

if proof_chain.check_trust(trusted_keys) {
Ok(VerifyStatus::Full)
} else {
Ok(VerifyStatus::Unknown)
}
}
SrcAuthority::Section { prefix, signature } => {
SrcAuthority::Section { signature, .. } => {
// Proof chain is required for section-src messages.
let proof_chain = if let Some(proof_chain) = self.proof_chain.as_ref() {
proof_chain
Expand All @@ -283,11 +271,6 @@ impl Message {
return Err(Error::FailedSignature);
}

let trusted_keys = trusted_keys
.into_iter()
.filter(|(known_prefix, _)| prefix.is_compatible(known_prefix))
.map(|(_, key)| key);

if proof_chain.check_trust(trusted_keys) {
Ok(VerifyStatus::Full)
} else {
Expand Down Expand Up @@ -495,21 +478,12 @@ mod tests {
Some(pk1),
)?;

assert_eq!(
message.verify(iter::once((&Prefix::default(), &pk1)))?,
VerifyStatus::Full
);
assert_eq!(
message.verify(iter::once((&Prefix::default(), &pk0)))?,
VerifyStatus::Unknown
);
assert_eq!(message.verify(iter::once(&pk1))?, VerifyStatus::Full);
assert_eq!(message.verify(iter::once(&pk0))?, VerifyStatus::Unknown);

let message = message.extend_proof_chain(&pk0, &full_proof_chain)?;

assert_eq!(
message.verify(iter::once((&Prefix::default(), &pk0)))?,
VerifyStatus::Full
);
assert_eq!(message.verify(iter::once(&pk0))?, VerifyStatus::Full);

Ok(())
}
Expand Down
3 changes: 1 addition & 2 deletions src/routing/approved.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,8 +798,7 @@ impl Approved {
.section
.chain()
.keys()
.map(move |key| (self.section.prefix(), key))
.chain(self.network.keys());
.chain(self.network.keys().map(|(_, key)| key));

match msg.verify(known_keys) {
Ok(VerifyStatus::Full) => Ok(true),
Expand Down
6 changes: 1 addition & 5 deletions src/routing/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,11 +483,7 @@ impl<'a> State<'a> {
}

fn verify_message(&self, message: &Message, trusted_key: Option<&bls::PublicKey>) -> bool {
// The message verification will use only those trusted keys whose prefix is compatible with
// the message source. By using empty prefix, we make sure `trusted_key` is always used.
let prefix = Prefix::default();

match message.verify(trusted_key.map(|key| (&prefix, key))) {
match message.verify(trusted_key) {
Ok(VerifyStatus::Full) => true,
Ok(VerifyStatus::Unknown) if trusted_key.is_none() => true,
Ok(VerifyStatus::Unknown) => {
Expand Down
5 changes: 1 addition & 4 deletions src/routing/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1508,10 +1508,7 @@ async fn handle_elders_update() -> Result<()> {
assert_eq!(section.chain().last_key(), &pk1);

// The message is trusted even by peers who don't yet know the new section key.
assert_matches!(
message.verify(iter::once((&Prefix::default(), &pk0))),
Ok(VerifyStatus::Full)
);
assert_matches!(message.verify(iter::once(&pk0)), Ok(VerifyStatus::Full));

// Merging the section contained in the message with the original section succeeds.
assert_matches!(section0.clone().merge(section.clone()), Ok(()));
Expand Down

0 comments on commit 2c9a1b2

Please sign in to comment.