Implement ReputationManager

[synzhu]

closes #577

[Jorropo]

I don't see what this is trying to solve, is the client code particularly more attackable ?

The issue right here is that I can send you either:

• A really big wantlist updates that will hit the server and ignore this code.
• Garbage protobuf data (protobuf will ignore fields it doesn't know, you aren't checking for this).

I guess your code would only protect if there is some costy operation in the client code, there is also in the server.

[Jorropo]

AFAIT it's just fancy counters, sounds like a thing you could do with locks.
Is one more goroutine really needed ?

[Jorropo]

You already used a goroutine earlier (I don't mind where you start it, just once not twice).

[Jorropo]

I don't like this, if I call this incorrectly I want to know about it so I can fix the bug. (fail fast & fail loudly)
I like when this check is explicit in the consumer.
For example tracing:

go-bitswap/bitswap.go

Lines 175 to 177 in bc4fd4a

	if bs.tracer != nil {
	bs.tracer.MessageReceived(p, incoming)
	}

(I know tracing is virtual so we couldn't do the same thing you did here but anyway)

Applies to this in all other methods.

[synzhu]

I don't see what this is trying to solve, is the client code particularly more attackable ?

The issue right here is that I can send you either:

• A really big wantlist updates that will hit the server and ignore this code.
• Garbage protobuf data (protobuf will ignore fields it doesn't know, you aren't checking for this).

I guess your code would only protect if there is some costy operation in the client code, there is also in the server.

In the server, it's not really possible to tell whether the wantlist updates are legit or not, right? As for the garbage protobuf data, I actually looked into this, but with the current version of Protobuf that is being used it doesn't seem like it's possible to check garbage data: protocolbuffers/protobuf#272. However if you know a way to do it, please let me know and I'm happy to add it.

@Jorropo

[Stebalien]

I think this is a "one step at a time" kind of thing.

• We do have to hash every block received, so it would be nice to be able to drop peers sending us bogus data.
  • Of course, we hash at the transport level for MACs anyways... so this isn't a huge deal.
  • Lots of tiny unwanted blocks would likely cause a lot of contention and work trying to process them.
• We definitely need wantlist size limits, and should consider wantlist ratelimits. But that's a separate problem.
• Bogus protobuf data isn't great, but it's something we can filter out and discard pretty quickly without affecting the rest of the system.

[Jorropo]

@Stebalien IMO whatever this is trying to do, it's not effective, I don't want more complex code that isn't usefull.

One step at a time seems fine, however I havn't seen anyone who claims to be commited to solve the other issues.

[synzhu]

@Jorropo By allowing Score Keeper to be implemented by the application, the application can do things like block / disconnect from peers whose score falls below a certain threshold, based on the amount of unwanted data they have sent us.

Do you have suggestions on how to mitigate this problem in a different way?

[Jorropo]

By allowing Score Keeper to be implemented by the application, the application can do things like block / disconnect from peers whose score falls below a certain threshold, based on the amount of unwanted data they have sent us.

This isn't what it is doing, it's based on the amount of unwanted blocks.
There is a problem: a node can send you lots of unwanted data.
And your solution: blocking nodes that send you too much bad blocks doesn't solve this problem, it solve only one vector there is multiple other vectors.

Your current solution doesn't solve the problem and so is useless, however if you also solve other unwanted data vectors this would be good.
Going by small steps can make sense, but if you don't have a plan to solve the real problem I don't feel like we should merge this (who wants to maintain code that doesn't solve anything ?).

An othre possibility is that maybe sending lots of extra blocks is particularly more effective as an attack vector compared to other vectors, then this could make sense, however you would need to prove this is actually a problem (just write a script that sends a bunch of unwanted block to a node and profile it to see if it's particularly bad).

[synzhu]

@Jorropo added follow up discussion to #577

[Jorropo]

This repository has been moved to https://github.com/ipfs/go-libipfs. There is not an easy way to transfer PRs, so if you would like to continue with this PR then please re-open it in the new repository and link to this PR.