Introduce whitelisted peers. #4378
Conversation
|
Appears functional with a quick test. Was able to bind two different ports and have clients connected to each, an intentionally misbehaving node on the whitelisted interface was not banned. Would it be sensible to show this as a line in |
|
Comments:
It seems easier for sysadmins, etc. to make a bad mistake with -whitebind, even though it might seem more convenient at first. |
|
@jgarzik |
|
Added 'whitelisted' to |
|
@dsnark Thanks for testing! |
|
Agree that is an issue, but this is still adding a Big Ole Security Exemption. I would suggest inverting the problem, to achieve a more secure result:
I think it would be useful to distinguish between localhost and Tor in any case. Another useful side effect of "inverting the problem in a more secure way." |
|
Interesting idea, but I'm not sure I like the double exception behavior of "normally peers are subjected to DoS banning, UNLESS they are whitelisted, which happens when they're in a whitelisted range, UNLESS they are connecting to shadybind." |
|
Fundamentally, it is still adding a "default: insecure" feature where the only justification for such insecurity is not a use case, a user request, but an implementation artifact ("that's how our code works"). Based on the problem description, it sounds like the needs are
We should not add -whitebind insecurity just because our DoS banning code is too stupid to figure out how to ban a single Tor node. |
If the peers are connected to a Hidden Service they can't be identified due to the design of the network, they'll always be localhost. I don't see a way around that outside of just not allowing HS peers. |
|
@dsnark You must consider the design of Tor + bitcoin, not just Tor alone. |
|
@dsnark You're absolutely right... I didn't think about that. Still, we want to at least be able to disconnect a misbehaving Tor peer, and not exclude them from DoS banning just because they seem to be connecting from localhost. However, it's not as simple as this patch, as we need also a way to not ban 127.0.0.1. |
|
@jgarzik Not disagreeing with you, but I'm not sure that potential security exception weighs up against the more complex semantics. I'll wait for some more opinions. |
|
@jgarzik In a lot of places the network security policy is implemented elsewhere, blocking that is annoying. The harm of being whitelisted inappropriately is also small— though I'm a little worried that the unconditional relaying is a little bit too powerful... since it does make you into a blind attack multiplier. This is also a little coarse in that immunity from ban is not the same as immunity from disconnection. I think localhost should still remain immune from banning, without being immune from disconnection. (disconnection suffers no overkill) |
|
So, two solutions:
Also new suggested names: |
|
@gmaxwell There is still the setInventoryKnown caching. One single inv will never be announced twice to the same peer (unless the setInventoryKnown cache overflows and is pruned). The largest advantage to rebroadcasts is that it does relay to new peers since the previous broadcast. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4378_95b5f1961edb7a8b393d1cb298de7756621da20d for binaries and test log. |
|
A third thought: extend the whitelist interface so that an ACL entry can specify the port that it was connected to, then instead of trusted bind you'd be able to just bind multiple ports and write whitelist entries like 0/0:1234. I still suspect that special casing localhost to prevent banning is something we want to do, even for onion peers. Banning all onion peers because one behaves is the wrong behavior (and creates a vulnerability where we currently have none: a single trouble maker could turn off bitcoin on tor cheaply, in order to be able to deanonymize users when they drop off tor to get things working). |
|
Extending the ACLs to include what port/interface it is connected to sounds like feature creep to me. Too much work to maintain and test all that. In cases such advanced configurability is needed I suggest using the host firewall combined with |
|
For as long as anti-DoS policy involves the notion of banning peers, it won't be completely compatible with Tor either via hidden services or exit nodes. We can still use Tor when it works and hope that some attacker that is willing to very noisily interfere with the entire network to force people back onto the clearnet doesn't come along. Eventually perhaps we'll have an alternative strategy that doesn't rely on bans, which anyway aren't really effective with large a enough botnet. Until then I don't think we should hold up this kind of nice improvement on the behalf of Tor. |
|
Rebased. |
This adds a -whitelist option to specify subnet ranges from which peers that connect are whitelisted. In addition, there is a -whitebind option which works like -bind, except peers connecting to it are also whitelisted (allowing a separate listen port for trusted connections). Being whitelisted has two effects (for now): * They are immune to DoS disconnection/banning. * Transactions they broadcast (which are valid) are always relayed, even if they were already in the mempool. This means that a node can function as a gateway for a local network, and that rebroadcasts from the local network will work as expected. Whitelisting replaces the magic exemption localhost had for DoS disconnection (local addresses are still never banned, though), which implied hidden service connects (from a localhost Tor node) were incorrectly immune to DoS disconnection as well. This old behaviour is removed for that reason, but can be restored using -whitelist=127.0.0.1 or -whitelist=::1 can be specified. -whitebind is safer to use in case non-trusted localhost connections are expected (like hidden services).
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4378_dc942e6f276b9fabc21f06d11cd16871d4054f82/ for binaries and test log. |
|
ACK |
dc942e6 Introduce whitelisted peers. (Pieter Wuille)
There was a problem hiding this comment.
Why? This will cause any misbehaviour that is equal to or greater than the threshold set to cause the node to never be disconnected!
There was a problem hiding this comment.
As I pointed out in #4378 this isn't the case. (though I'm glad you're reading the code!)
This adds a
-whitelistoption to specify subnet ranges from which peers that connect are whitelisted. In addition, there is a-whitebindoption which works like-bind, except peers connecting to it are also whitelisted (allowing a separate listen port for trusted connections).Being whitelisted has two effects (for now):
Whitelisting replaces the magic exemption localhost had for DoS banning, which implies hidden service connects (from a localhost Tor node) were incorrectly immune to DoS banning as well. This old behaviour is removed for that reason, but can be restored using
-whitelist=127.0.0.1or-whitelist=::1.-whitebindis safer to use in case non-trusted localhost connections are expected (like hidden services).This is a partial replacement for #3403 (but does not add RPC commands to make whitelisting dynamic). Also, hhitelisting becomes a boolean property of a peer here (set at connect time), rather than defined by a set of netmasks. This means we don't need to match the address on every invocation of a relay.