Gossipsub message signing #1583

AgeManning · 2020-05-19T07:42:04Z

Description

This adds optional message signing to the gossipsub protocol as per the libp2p specifications.

This also removes the LruCache received cache and simply uses the memcache in it's place.

burdges · 2020-05-19T08:50:04Z

As an aside, we've gossip messages that benefit from exotic signatures, like VRFs for approval assignments (A&V) or maybe even aggregation tricks in GRANDPA.

It's normally fastest if gossip messages run over authenticated channels that we drop on harmless but incorrect and spammy messages, but then do their own authentication, probably polite, possibly weird, before being sent back out. It's also bad for layering, auditing, etc. to do this. shrug

mxinden

I did a first pass. Thanks for the work!

protocols/gossipsub/src/behaviour.rs

mxinden · 2020-05-22T07:32:36Z

protocols/gossipsub/src/config.rs

+    /// When set to `true` all published messages are signed by the libp2p key (default is `true`).
+    pub sign_messages: bool,
+
+    /// Determines whether unsigned messages will be accepted. If set to false, unsigned messages


Suggested change

/// Determines whether unsigned messages will be accepted. If set to false, unsigned messages

/// Determines whether incoming unsigned messages will be accepted. If set to false, unsigned messages

This is about incoming messages only, right? Would help me a bit to better comprehend.

Yes. There is a config in go, around whether to use message signing or not.

I put this in, to mimic the current implementation which completely ignores the signature field. I was concerned with interoperability with other implementations and wanted this maximally configurable such that it could accept messages with other implementations that were not signing messages, even if we were.

However, now that you have pointed this out, perhaps this will cause more trouble than it saves in practice and we should just have a boolean that dictates we either sign/verify or don't sign/verify.

What do you think?

mxinden · 2020-05-22T07:36:30Z

protocols/gossipsub/src/config.rs

-    pub fn hash_topics(&mut self) -> &mut Self {
-        self.config.hash_topics = true;
+    /// Flag determining if gossipsub topics are hashed or sent as plain strings (default is false).
+    pub fn hash_topics(&mut self, value: bool) -> &mut Self {


Not particularly important: Why change the signatures here? Why would someone call hash_topics(false)? If we keep it as is I would prefer set_hash_topics as one can set the value to both states. I prefer the previous implementation.

Same for the other boolean configuration options below.

I agree. I preferred the original version also, but had been playing with other builder implementations and somehow convinced myself that the boolean signature was a more canonical pattern in the rust community (for some reason I thought tokio used this pattern, on inspection some other packages do, like hyper for example: https://docs.rs/hyper/0.13.5/hyper/server/struct.Builder.html).

I also prefer the previous implementation, so its good to know someone else thinks so.
Will change back.

protocols/gossipsub/src/config.rs

protocols/gossipsub/src/mcache.rs

protocols/gossipsub/src/protocol.rs

mxinden · 2020-05-22T08:32:09Z

protocols/gossipsub/src/protocol.rs

+    use super::*;
+
+    #[test]
+    fn sign_and_verify_message_peer_inline() {


I think this could greatly benefit of random test input generation e.g. via quickcheck. I am happy to provide a commit in case you agree.

Yeah I agree, I'm about to go off and build out 1.1. If you have the time to add this in, it would be great

@AgeManning see sigp#31.

protocols/gossipsub/src/rpc.proto

AgeManning · 2020-05-25T05:21:38Z

Thanks for the quick review @mxinden.

On closer inspection of your comments, I've realised I've made a few pretty significant errors.

Firstly, I was signing all messages and didn't consider forwarded messages, which obviously we shouldn't sign. There are two ways to address this imo:

Perform the signing at the behaviour level. This is nice because we could sign messages in the publish_many() function and prevent the cloning of keypair in the encoding as well as a few other variables. The downside of this is that the signing requires the protobuf encoding of the messages which is mainly dealt with in protocol.rs. For this reason, I went with (2) (also because it required less code changes).
Check outgoing messages. If the source_id is us, then we should be signing it (provided no signature already exists).

This logic, also brings to light a vulnerability in using allow_unsigned_messages. If a peer sent us a message using us as the source id, and we ignored the signature, then signed the message, a malicious peer could get us to sign arbitrary messages. Therefore, I've removed that option, and signing and verification is binary, we either sign and verify, or we don't.

If we are not signing, we also don't bother checking the signatures (even if one exists). I prefer this functionality, somewhat selfishly because in my use, I'm not signing and don't care about signatures and don't want to waste any computation checking signatures that I don't care about, regardless if they are valid or not.

mxinden

As far as I can tell a GossipsubMessage instantiated in publish_many is
cloned twice, once for the mesh peers and once for the subscribed peers. Adding
the signature later on implies that we sign the same message at least twice,
correct?

Doing the signing in publish_many as you suggest in (1) would solve the above,
correct?

Small caveat on sigining in NetworkBehaviour is the dealy this introduces to
the main thread with this somewhat heavy computation. But without a benchmark
I doubt we should take this into consideration.

This logic, also brings to light a vulnerability in using
allow_unsigned_messages. If a peer sent us a message using us as the source
id, and we ignored the signature, then signed the message, a malicious peer
could get us to sign arbitrary messages. Therefore, I've removed that option,
and signing and verification is binary, we either sign and verify, or we
don't.

Doing the signing in publish_many would prevent this, right?

Overall I would suggest moving the process of signing close to
GossipsubMessage instantiation for the above reasons. Given that this is
purely protocols/gossipsub internal it can still be changed later on without a
breaking change. What do you think?

If we are not signing, we also don't bother checking the signatures (even if
one exists). I prefer this functionality, somewhat selfishly because in my
use, I'm not signing and don't care about signatures and don't want to waste
any computation checking signatures that I don't care about, regardless if
they are valid or not.

That sounds reasonable to me.

mxinden · 2020-05-28T09:56:49Z

protocols/gossipsub/src/behaviour.rs

+            );
+            return;
+        }
+
        debug!(
            "Publishing message: {:?}",
            (self.config.message_id_fn)(&message)


Suggested change

(self.config.message_id_fn)(&message)

msg_id,

See let msg_id = (self.config.message_id_fn)(&message); above.

mxinden · 2020-05-28T10:17:31Z

Do I understand correctly that currently if a node is signing messages (disable_message_signing == false) there is no way to allow unsigned messages from other nodes?

Just checking for now, haven't made up my mind whether that is good or not.

AgeManning · 2020-06-01T05:36:21Z

Do I understand correctly that currently if a node is signing messages (disable_message_signing == false) there is no way to allow unsigned messages from other nodes?

I changed it to this, yes. I originally allowed the other behaviour for debugging. After a bit of thought, I decided this current logic was best. Signing is either enabled or not. The negative case can accept and read signed messages and will propagate them but the affirmative case rejects all unsigned/invalid messages.

The reason was to try and avoid networks of mixed cases. If a node wants to enable signing, I think it wouldn't be great that it propagates unsigned messages also.

I haven't completed the scoring in 1.1 in relation to how a peer would score another peer that sent unsigned messages. But if in a network where peers expect signed messages, and one peer was not signing and another peer was forwarding unsigned messages (i.e we enabled reading unsigned messages) I think both peers would be scored negatively (need to confirm this).

We could add flags that allow reading of unsigned messages without propagation and then with propagation, but the complexity grows and I'm not entirely sure we gain much from this added complexity of allowing mixed networks.

For these reasons I decided to leave it binary. I guess the correct logic would be to follow other implementations. If other clients support it, we should do. From memory the go-client has it binary, but perhaps there is a flag that allows it, will need to check.

I don't use the signing part of gossipsub, so if others have some use for mixed networks, I'm happy to allow it add the flags in, but we might end up with more issues.

mxinden

@AgeManning I am sorry for the delay. Thanks for moving the message signing.

I changed it to this, yes. I originally allowed the other behaviour for debugging. After a bit of thought, I decided this current logic was best. Signing is either enabled or not. The negative case can accept and read signed messages and will propagate them but the affirmative case rejects all unsigned/invalid messages.

This sounds reasonable to me. In case anyone has a use-case for mixed networks we can still adjust the code.

protocols/gossipsub/src/behaviour.rs

protocols/gossipsub/src/config.rs

protocols/gossipsub/src/behaviour.rs

mxinden · 2020-06-16T08:37:52Z

protocols/gossipsub/src/behaviour.rs

+        let message = rpc_proto::Message {
+            from: Some(self.message_source_id.clone().into_bytes()),
+            data: Some(data),
+            seqno: Some(sequence_number.to_be_bytes().to_vec()),
+            topic_ids: topics.clone().into_iter().map(|t| t.into()).collect(),
+            signature: None,
+            key: None,
+        };
+
+        // If a signature is required, generate it
+        let signature = if let Some(keypair) = self.keypair.as_ref() {


message is only needed when Some(keypair) == self.keypair.as_ref() right?

Suggested change

let message = rpc_proto::Message {

from: Some(self.message_source_id.clone().into_bytes()),

data: Some(data),

seqno: Some(sequence_number.to_be_bytes().to_vec()),

topic_ids: topics.clone().into_iter().map(|t| t.into()).collect(),

signature: None,

key: None,

};

// If a signature is required, generate it

let signature = if let Some(keypair) = self.keypair.as_ref() {

// If a signature is required, generate it

let signature = if let Some(keypair) = self.keypair.as_ref() {

let message = rpc_proto::Message {

from: Some(self.message_source_id.clone().into_bytes()),

data: Some(data),

seqno: Some(sequence_number.to_be_bytes().to_vec()),

topic_ids: topics.clone().into_iter().map(|t| t.into()).collect(),

signature: None,

key: None,

};

This is true, but if i recall correctly, I was trying to avoid the clone on data.
I'll change it, but it now clones the data.

I see. I guess as this is only scoped to the function itself we can delay the decision. In case it shows up in benchmarks we can easily change it. Rustc or LLVM might as well just optimize it away, not sure.

protocols/gossipsub/src/mcache.rs

AgeManning · 2020-07-01T06:54:42Z

I finally got back around to this.

I think I've addressed all your comments, I've left comments on ones that I think require them.

We've since noticed a bug in gossipsub where subscriptions were not being sent to all peers. I've added that correction in too, whilst I'm here.

mxinden

I think I've addressed all your comments, I've left comments on ones that I think require them.

Thanks. Overall this looks good, just 2 small things that need fixing before merging.

We've since noticed a bug in gossipsub where subscriptions were not being sent to all peers. I've added that correction in too, whilst I'm here.

Thanks!

mxinden · 2020-07-08T08:28:53Z

core/src/identity.rs

@@ -29,7 +29,7 @@ pub mod secp256k1;
 pub mod error;

 use self::error::*;
-use crate::{PeerId, keys_proto};


I am guessing that the changes in identity.rs and core/src/identity/error.rs were done by rustfmt. Would you mind reverting them in this pull-request as they seem unrelated? If you feel like the suggestions would improve code readability please open a separate pull request.

Yep, editor has it in by default. I usually catch these, seems I missed one.

protocols/gossipsub/src/error.rs

mxinden · 2020-07-08T08:42:06Z

protocols/gossipsub/src/behaviour.rs

+        let message = rpc_proto::Message {
+            from: Some(self.message_source_id.clone().into_bytes()),
+            data: Some(data),
+            seqno: Some(sequence_number.to_be_bytes().to_vec()),
+            topic_ids: topics.clone().into_iter().map(|t| t.into()).collect(),
+            signature: None,
+            key: None,
+        };
+
+        // If a signature is required, generate it
+        let signature = if let Some(keypair) = self.keypair.as_ref() {


I see. I guess as this is only scoped to the function itself we can delay the decision. In case it shows up in benchmarks we can easily change it. Rustc or LLVM might as well just optimize it away, not sure.

mxinden · 2020-07-14T13:20:30Z

core/src/identity/error.rs

@@ -27,16 +27,22 @@ use std::fmt;
 #[derive(Debug)]
 pub struct DecodingError {
    msg: String,
-    source: Option<Box<dyn Error + Send + Sync>>
+    source: Option<Box<dyn Error + Send + Sync>>,


@AgeManning can you revert these formatting changes as well?

* Make the GossipsubRpc debug instance a bit nicer Basically just don't show empty components to reduce visual noise * Introduce send_message helper function to avoid repetition of boilerplate This also serves to have a central place to log or otherwise intercept all outgoing messages to other peers of the behaviour. * Simplify the arc removal in poll

And do the corresponding changes to make this work.

AgeManning · 2020-07-27T06:18:43Z

Sorry, took a bit of a detour on this one.

The extra changes since last review are:

Added extensible configuration options to provide a range of privacy and validation modes. This is also to accommodate: Signing policy + optional Signature, From and Seqno go-libp2p-pubsub#359. A user can choose the level of message validation and author authenticity/ privacy. Invalid mixing of these configurations result in a panic on initialisation.
Fixes that resolve Gossipsub mesh contains same peer id multiple times #1674 and Gossipsub interop issue with go-ipfs #1671
Adds back a separate duplicates cache. The idea is that this can be much larger than the memcache and only holds message_ids rather than the entire message.
When doing message validation, messages that have not been validated are excluded from gossip and hence propagation.

I'll be doing some tests on the current version, but I think this is ready for another review now.

protocols/gossipsub/src/behaviour.rs

AgeManning · 2020-07-29T02:24:09Z

Thanks for the review. Have updated based on comments.

mxinden

This looks good to me. In case there are no objections and @AgeManning you merge current master I am happy to merge this.

Thanks for upstreaming all the work!

romanb · 2020-08-03T09:18:54Z

Are these changes backward-compatible? Could someone update the changelog?

mxinden · 2020-08-06T06:43:22Z

@AgeManning do you have time to open up a pull request with a changelog entry for these changes? Otherwise I will prepare one in the next couple of days.

AgeManning · 2020-08-10T06:45:04Z

Hey yep, can do.

I've also discovered that the lru_time_cache used for duplicates here does an O(N) lookup for each message. For a network with a lot of gossipsub messages this isn't great. I've got corrections for this on my own branch. I can make a PR for them, but I'm tempted to wait for the 1.1 PR which we'll make within a week and should contain this modifications also.

AgeManning added 2 commits May 19, 2020 17:39

Gossipsub message signing

aeba523

Update ipfs-private example

573e4b6

Add optimisation to prevent key calculation each message

d2babd4

mxinden reviewed May 22, 2020

View reviewed changes

Handle forwarding of signed messages and reviewers comments

ac3c977

AgeManning force-pushed the gossipsub-signing branch from fb51926 to ac3c977 Compare May 25, 2020 06:10

Merge branch 'master' into gossipsub-signing

e9afd95

mxinden reviewed May 28, 2020

View reviewed changes

mxinden mentioned this pull request May 28, 2020

protocols/gossipsub/src/protocol: Use quickcheck to test signing sigp/rust-libp2p#31

Merged

protocols/gossipsub/src/protocol: Use quickcheck to test signing (#31)

3d1a025

Shift signing into behaviour

31b6849

mxinden reviewed Jun 16, 2020

View reviewed changes

AgeManning added 3 commits July 1, 2020 14:47

Merge latest master

a8cec0b

Address reviewers suggestions

2b38112

Send subscriptions to all peers

7d678f5

AgeManning added 2 commits July 1, 2020 17:16

Update examples

dc050dd

Shift signing option into the behaviour

5bf1cfc

mxinden reviewed Jul 8, 2020

View reviewed changes

AgeManning added 3 commits July 9, 2020 11:37

Revert changes to core/

4ffe728

Reviewers suggestion

24b738c

Merge remote-tracking branch 'origin/master' into gossipsub-signing

a2e4bf0

mxinden reviewed Jul 14, 2020

View reviewed changes

protolambda mentioned this pull request Jul 22, 2020

Signing policy + optional Signature, From and Seqno libp2p/go-libp2p-pubsub#359

Merged

AgeManning added 2 commits July 23, 2020 15:00

Merge latest master

2f20571

Revert changes to core/

2c50ba2

AgeManning and others added 4 commits July 24, 2020 18:58

Add privacy and validation options in the config

1ea2dd9

Improve signing validation logic

7b76c8f

Change the gossipsub rpc protocol to use bytes for message ids (#34)

f5d32b2

And do the corresponding changes to make this work.

rklaehn mentioned this pull request Jul 25, 2020

Change the gossipsub rpc protocol to use bytes for message ids #1676

Closed

AgeManning added 4 commits July 26, 2020 19:13

Add optional privacy and validation settings

c3a7757

Correct doc link

0c43ed4

Prevent invalid messages from being gossiped

efa40e3

Remove unvalidated messages from gossip. Reintroduce duplicate cache

a514c60

AgeManning added 2 commits July 28, 2020 15:04

Send grafts when subscriptions are added to the mesh

b257025

Merge latest master

b20771b

mxinden reviewed Jul 28, 2020

View reviewed changes

AgeManning added 4 commits July 28, 2020 22:28

Only add messages to memcache if not duplicates

2f4a50a

Add mesh maintenance tests and remove excess peers from mesh

be7c9e2

Apply reviewers suggestions

e049ff7

Merge latest master

74d2779

Wrap comments

6bac865

mxinden approved these changes Jul 31, 2020

View reviewed changes

AgeManning added 3 commits August 2, 2020 15:35

Ensure sequence number is sent

94469cf

Merge latest master

60f8a1e

Maintain the debug trait

fef66dd

mxinden merged commit d8ad7bd into libp2p:master Aug 3, 2020

rklaehn mentioned this pull request Aug 4, 2020

Public gossipsub api #1677

Merged

AgeManning mentioned this pull request Aug 10, 2020

Update the gossipsub changelog #1695

Merged

AgeManning deleted the gossipsub-signing branch June 15, 2021 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gossipsub message signing #1583

Gossipsub message signing #1583

AgeManning commented May 19, 2020

burdges commented May 19, 2020 •

edited

Loading

mxinden left a comment

mxinden May 22, 2020

AgeManning May 25, 2020 •

edited

Loading

mxinden May 22, 2020

AgeManning May 25, 2020

mxinden May 22, 2020

AgeManning May 25, 2020

mxinden May 28, 2020

AgeManning commented May 25, 2020

mxinden left a comment

mxinden May 28, 2020

mxinden commented May 28, 2020 •

edited

Loading

AgeManning commented Jun 1, 2020

mxinden left a comment

mxinden Jun 16, 2020

AgeManning Jul 1, 2020

mxinden Jul 8, 2020

AgeManning commented Jul 1, 2020

mxinden left a comment

mxinden Jul 8, 2020

AgeManning Jul 9, 2020

mxinden Jul 8, 2020

mxinden Jul 14, 2020

AgeManning commented Jul 27, 2020

AgeManning commented Jul 29, 2020

mxinden left a comment

romanb commented Aug 3, 2020

mxinden commented Aug 6, 2020

AgeManning commented Aug 10, 2020

	/// Determines whether unsigned messages will be accepted. If set to false, unsigned messages
	/// Determines whether incoming unsigned messages will be accepted. If set to false, unsigned messages

Gossipsub message signing #1583

Gossipsub message signing #1583

Conversation

AgeManning commented May 19, 2020

Description

burdges commented May 19, 2020 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning May 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning commented May 25, 2020

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented May 28, 2020 • edited Loading

AgeManning commented Jun 1, 2020

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning commented Jul 1, 2020

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning commented Jul 27, 2020

AgeManning commented Jul 29, 2020

mxinden left a comment

Choose a reason for hiding this comment

romanb commented Aug 3, 2020

mxinden commented Aug 6, 2020

AgeManning commented Aug 10, 2020

burdges commented May 19, 2020 •

edited

Loading

AgeManning May 25, 2020 •

edited

Loading

mxinden commented May 28, 2020 •

edited

Loading