feat(identify): implement signedPeerRecord #5785
Conversation
|
Oh CI will compile the protobuf code, didn't see that. |
|
you might rebase to main branch, as zlib license has added to ci， #5769 |
I think you might write some changelog text into this file, just like, the pr might be okay now. |
Thanks for the review! Working on tests right now. |
|
version |
CHANGELOG will be fixed with #5803, you can just add the new entry to |
There was a problem hiding this comment.
Thank you @drHuangMHT! Looks like a great start.
I found a pending libp2p spec for signed peer records: libp2p/specs#630. Most of it matches this PR, just one thing is missing:
If the
signedPeerRecordis present the implementation MUST use the data contained within it and ignore duplicated fields present in the main identify message
IMO that makes sense and should be added to this PR. So instead of copying the received record as it is in handle_incoming_info, I think we should instead (when an signed envelope is present):
- Verify that the signature matches the remote's public key
- Deserialize the record, store the resulting addresses as
listen_addrs, and ignore the originallisten_addrs.
Wdyt?
protocols/identify/src/protocol.rs
Outdated
| // 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()) | ||
| .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))); |
There was a problem hiding this comment.
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()
);There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Do we really need to
deserializethe record again? Can't we just read outPeerRecord::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.
There was a problem hiding this comment.
Ahh we also need to check that the envelope key matches, but that can be done as well by reading the
PeerRecord::peer_idand 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.
There was a problem hiding this comment.
Sorry for the late reply!
recordhas typeSignedEnvelope, whose payload is in the form ofVec<u8>(bytes) that doesn't haveaddressesfield unless we deserialize the payload intoPeerRecord.
Ah I see. The name signed_peer_record had me confused. How about calling it signed_envelope instead, so it matches the type (also in Info)?
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 extractedPeerRecord::try_deserialize_signed_envelopeto cut down allocation.
Good point. Still, I would expect that the compiler optimizes away the unnecessary allocation. IMO the current code is a bit difficult to understand, and for the sake of simplicity it would be worth it to do the conversion to PeerRecord so that we can then simply clone PeerRecord::addresses.
However, if you have a strong preference for the current implementation I am okay with it. But then I think PeerRecord::try_deserialize_signed_envelope logic better fits as SignedEnvelope::try_deserialize(&self) -> ... (same logic, just move to SignedEnvelope).
PeerIdis 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.
We rely on the collision resistance of PeerId throughout all of rust-libp2p, and go-libp2p also only compares IDs here. I'm not against comparing public keys, but given that PeerRecord::try_deserialize_signed_envelope already compares the PeerIds I don't think we need the double check here.
There was a problem hiding this comment.
However, if you have a strong preference for the current implementation I am okay with it. But then I think
PeerRecord::try_deserialize_signed_envelopelogic better fits asSignedEnvelope::try_deserialize(&self) -> ...(same logic, just move toSignedEnvelope).
I don't think it is a good idea to implement deserialization on SignedEnvelope because it can contain arbitrary payload, including but not limited to PeerRecord. We need specific "headers" to properly deserialize the envelope, according to the spec.
We rely on the collision resistance of
PeerIdthroughout all of rust-libp2p, and go-libp2p also only compares IDs here.
That's fair. Good to know.
I'm not against comparing public keys, but given that
PeerRecord::try_deserialize_signed_envelopealready compares thePeerIds I don't think we need the double check here.
The two checks are different. In try_deserialize_signed_envelope it checks whether the signer of the record matches the signer of the envelope. While in Info::try_from we essentially check the signer of the envelope(therefore signer of the address record) against the sender of the identify message. If we skip the check in try_from, the address can come from peers other than the message sender.
Description
May close #4017.
Notes & open questions
pb-rsnow usesCow<'_,T>for the compiled Rust structs. But with borrowed type in the struct,FramedReadcan no longer process frames correctly(trait bound not statisfied).Change checklist