ssbc / ssb-ebt

secure scuttlebutt replication with epidemic-broadcast-trees
MIT License
18 stars 10 forks source link

Only replicate within hops #14

Closed arj03 closed 6 years ago

arj03 commented 6 years ago

The idea with this pull request is that we shouldn't start replicating with any peers outside our hops. As it is now, sbot can connect to anyone but will only sync feeds within our hops.

The biggest reason for this change is that right now anyone can connect to any peer and start requesting all feeds and thus map the whole network and use this for whatever reason they might have.

I have tested this against scuttlebot with ssb-friends 3.1.3 and all tests are passing. Furthermore as far as I can read, invite does not use this code, so is not affected by this change.

This still leaves old-style replication, maybe we can disable it by default? And I wonder if this should be a flag or not.

dominictarr commented 6 years ago

Hmm, I think this should really be fixed in the gossip strategy (scuttlebot/plugins/gossip ... also @pietgeursen has been working on a rewrite of that)

This only changes the decision about wether or not to begin replication - it's initiated by the client, so if you wanted to map the entire network, you could just change this code. If we really want to prevent that, we need servers that rejects incoming connections from outside the hops range... This would also block old-style replication. Someone wanting to map the network can't just change all the servers.

I think that should live in a different place, though, maybe it's own plugin? (all it needs to do is hook auth?)

arj03 commented 6 years ago

Agree that this should be something gossip should be aware of. The reason why it is here is because I started adding it to sbot, hooking up on rpc:connect and killing a rpc connection if it was outside hops. The idea was that this would take care of both ebt & legacy. The problem with that is that invites won't go through. So instead I'm adding it here. I'm thinking that legacy replication will eventually die, and it can be disabled so it shouldn't be that much of a problem.

I have tested the code and my pub and it does stop incoming connections as well :)

Lastly I'm thinking this probably needs a config option, or what do you think @dominictarr?

dominictarr commented 6 years ago

I'm definitely in favor of having this feature, but I think we need to figure out the cleanest way to implement it. My suggestion, is primarily use an auth hook. this method is called when a incomming connection is received. here is the invite auth hook: https://github.com/ssbc/scuttlebot/blob/master/plugins/invite.js#L48-L63

If you have hook that checks wether a peer is within hops, and it's added after the invites plugin, then invites will still work, but someone outside of the hooks won't even know why they are not authorized.

Side note: possibly, instead of hops, it should be the replication set? sometimes peers not yet in hops are replicated - for example, if they are detected on the local network. Or, you might open a feed, and want to see those messages, but havn't actually hit follow yet.

I suggest an auth hook, to prevent incoming connections from remote peers, and something in gossip scheduler to prevent outgoing connections. This could be self contained into a module, too.

arj03 commented 6 years ago

Thanks for the tip @dominictarr. I have created a plugin that does the checking here: https://github.com/arj03/ssb-incoming-guard. I have tested it using my normal account, a new account which is unable to replicate until I accept an invite.

Is it possible you can have a look and see if it makes sense?

dominictarr commented 6 years ago

Yeah, that looks good. but I think it would be lighter to just use sbot.friends.hops({}, cb) rather than the (legacy) stream method.

(side note: I've been meaning to add a few more methods for getting the hop distance between feeds, need that for user-invites to choose pubs that are likely to replicate you)

arj03 commented 6 years ago

Thanks