Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(identify): implement signedPeerRecord #5785

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+226 −43
Open

feat(identify): implement signedPeerRecord #5785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2a09cdb
implement signedPeerRecord
drHuangMHT Dec 31, 2024
b5d7aec
apply suggestions
drHuangMHT Jan 1, 2025
85d7496
Merge branch 'master' into identify-peer-record
drHuangMHT Jan 3, 2025
4d3d692
add test
drHuangMHT Jan 3, 2025
59f80f2
reduce diff
drHuangMHT Jan 4, 2025
ef89b5b
Merge branch 'master' into identify-peer-record
drHuangMHT Jan 4, 2025
12b9a29
rename symbols
drHuangMHT Jan 10, 2025
d3d8ae6
prefer addresses in signedPeerRecord
drHuangMHT Jan 10, 2025
59151b6
rename symbols
drHuangMHT Jan 11, 2025
9b6a139
lint and fmt
drHuangMHT Jan 11, 2025
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
51 changes: 28 additions & 23 deletions core/src/peer_record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use libp2p_identity::{Keypair, PeerId, SigningError};
use libp2p_identity::{Keypair, PeerId, PublicKey, SigningError};
use quick_protobuf::{BytesReader, Writer};
use web_time::SystemTime;

use crate::{proto, signed_envelope, signed_envelope::SignedEnvelope, DecodeError, Multiaddr};

const PAYLOAD_TYPE: &str = "/libp2p/routing-state-record";
const DOMAIN_SEP: &str = "libp2p-routing-state";
pub const PAYLOAD_TYPE: &str = "/libp2p/routing-state-record";
pub const DOMAIN_SEP: &str = "libp2p-routing-state";

/// Represents a peer routing record.
///
Expand All @@ -30,26 +30,7 @@ impl PeerRecord {
/// If this function succeeds, the [`SignedEnvelope`] contained a peer record with a valid
/// signature and can hence be considered authenticated.
pub fn from_signed_envelope(envelope: SignedEnvelope) -> Result<Self, FromEnvelopeError> {
use quick_protobuf::MessageRead;

let (payload, signing_key) =
envelope.payload_and_signing_key(String::from(DOMAIN_SEP), PAYLOAD_TYPE.as_bytes())?;
let mut reader = BytesReader::from_bytes(payload);
let record = proto::PeerRecord::from_reader(&mut reader, payload).map_err(DecodeError)?;

let peer_id = PeerId::from_bytes(&record.peer_id)?;

if peer_id != signing_key.to_peer_id() {
return Err(FromEnvelopeError::MismatchedSignature);
}

let seq = record.seq;
let addresses = record
.addresses
.into_iter()
.map(|a| a.multiaddr.to_vec().try_into())
.collect::<Result<Vec<_>, _>>()?;

let (_, peer_id, seq, addresses) = Self::try_deserialize_signed_envelope(&envelope)?;
Ok(Self {
peer_id,
seq,
Expand Down Expand Up @@ -126,6 +107,30 @@ impl PeerRecord {
pub fn addresses(&self) -> &[Multiaddr] {
self.addresses.as_slice()
}

pub fn try_deserialize_signed_envelope(
envelope: &SignedEnvelope,
) -> Result<(&PublicKey, PeerId, u64, Vec<Multiaddr>), FromEnvelopeError> {
use quick_protobuf::MessageRead;

let (payload, signing_key) =
envelope.payload_and_signing_key(String::from(DOMAIN_SEP), PAYLOAD_TYPE.as_bytes())?;
let mut reader = BytesReader::from_bytes(payload);
let record = proto::PeerRecord::from_reader(&mut reader, payload).map_err(DecodeError)?;

let peer_id = PeerId::from_bytes(&record.peer_id)?;

if peer_id != signing_key.to_peer_id() {
return Err(FromEnvelopeError::MismatchedSignature);
}

let addresses = record
.addresses
.into_iter()
.map(|a| a.multiaddr.to_vec().try_into())
.collect::<Result<Vec<_>, _>>()?;
Ok((signing_key, peer_id, record.seq, addresses))
}
}

#[derive(thiserror::Error, Debug)]
Expand Down
23 changes: 18 additions & 5 deletions protocols/identify/src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

use asynchronous_codec::{FramedRead, FramedWrite};
use futures::prelude::*;
use libp2p_core::{multiaddr, Multiaddr, SignedEnvelope};
use libp2p_core::{multiaddr, Multiaddr, PeerRecord, SignedEnvelope};
use libp2p_identity as identity;
use libp2p_identity::PublicKey;
use libp2p_swarm::StreamProtocol;
Expand Down Expand Up @@ -226,16 +226,29 @@
}
};

let signed_peer_record = msg
.signedPeerRecord
.and_then(|b| SignedEnvelope::from_protobuf_encoding(b.as_ref()).ok());

// When signedPeerRecord contains valid addresses, ignore addresses in listenAddrs.
// When signedPeerRecord is invalid or signed by others, ignore the signedPeerRecord(set to `None`).
let (signed_peer_record, listen_addrs) = signed_peer_record
.as_ref()
.and_then(|envelope| PeerRecord::try_deserialize_signed_envelope(&envelope).ok())

Check failure on line 237 in protocols/identify/src/protocol.rs

View workflow job for this annotation

GitHub Actions / clippy (1.83.0) 

this expression creates a reference which is immediately dereferenced by the compiler

Check failure on line 237 in protocols/identify/src/protocol.rs

View workflow job for this annotation

GitHub Actions / clippy (beta) 

this expression creates a reference which is immediately dereferenced by the compiler
.and_then(|(envelope_public_key, _, _, addresses)| {
(*envelope_public_key == public_key).then_some(addresses)
})
.map(|addrs| (signed_peer_record, addrs))
.unwrap_or_else(|| (None, parse_listen_addrs(msg.listenAddrs)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to deserialize the record again? Can't we just read out PeerRecord::adddresses:

let listen_addrs = signed_peer_record.map_or_else(
     || parse_listen_addrs(msg.listenAddrs), 
     |record| record.addresses.to_vec()
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh we also need to check that the envelope key matches, but that can be done as well by reading the PeerRecord::peer_id and comparing it with the remote's id, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we really need to deserialize the record again? Can't we just read out PeerRecord::adddresses:

let listen_addrs = signed_peer_record.map_or_else(
     || parse_listen_addrs(msg.listenAddrs), 
     |record| record.addresses.to_vec()
);

record has type SignedEnvelope, whose payload is in the form of Vec<u8>(bytes) that doesn't have addresses field unless we deserialize the payload into PeerRecord. In order to own the addresses the only way is to call record.addresses().to_vec() which in this case allocates twice, which is unnecessary. So I extracted PeerRecord::try_deserialize_signed_envelope to cut down allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh we also need to check that the envelope key matches, but that can be done as well by reading the PeerRecord::peer_id and comparing it with the remote's id, right?

PeerId is derived from a public key, which indirectly proves its identity while the key itself does so directly. ID can collide, but the key is less likely, considering we support multiple key types. I believe it is safer to compare the keys directly when they are already present.


let info = Info {
public_key,
protocol_version: msg.protocolVersion.unwrap_or_default(),
agent_version: msg.agentVersion.unwrap_or_default(),
listen_addrs: parse_listen_addrs(msg.listenAddrs),
listen_addrs,
protocols: parse_protocols(msg.protocols),
observed_addr: parse_observed_addr(msg.observedAddr).unwrap_or(Multiaddr::empty()),
signed_peer_record: msg
.signedPeerRecord
.and_then(|b| SignedEnvelope::from_protobuf_encoding(b.as_ref()).ok()),
signed_peer_record,
};

Ok(info)
Expand Down
Loading