Do not use third party services for IP detection. #5161
Conversation
|
I've tested this (and variations of it) for a while and I'm happy with it. I think its reasonably simple now and avoids moving much of anything around. The testing I performed was two fold, I ran an instrumented copy that logged its announcements and saw it making both types of announcements. I also ran a node behind nat (with a port forward) which hasn't had bitcoin running on it recently without UPNP or other ways of discovering an IP and confirmed that it eventually gained inbound connections. |
|
Can we change the help text on -discover to say something like "Do not rumor your address except as provided by -localip"? (and, more broadly, do we really want a flag for this? It is some strange depend-on-my-peer-not-rumoring-my-address benchmark, when we really should just use fListen?) |
src/main.cpp
Outdated
There was a problem hiding this comment.
Because of this we only clear setAddrKnown when we're listening, which I dont think was intended?
There was a problem hiding this comment.
gah. good catch. I added that at the last minute "oh why bother taking the vnodes lock when we're not going to do anything with it."
Consider, when you're listening to torHS you want listen=1 but you certantly don't want discovery of any type. (capturing that, one of the extra changes I made here was making proxy also imply no discovery but it still should be settable). Beyond hoping that your peers won't disrespect your wishes, it can avoid rumoring incorrect data when your outbound connections don't match your inbound. (p2pool has no way to control what addresses get rumored and this has broken things for at least a couple people). |
There's a number of clients on the network that do absolutely stupid things with the IP address in the handshake and addr messages. One piece of software returns It's likely not a problem for this patch, just be aware that even the big players are writing horribly broken code that doesn't even attempt to conform to the p2p network the core client does. |
|
Yea, this patch doesn't assume the peers are non-evil, much less assuming they aren't conventionally broken. |
|
@TheBlueMatt see updates |
There was a problem hiding this comment.
Does AdvertizeLocal still qualify for the previous comment ("used when scores of local addresses may have changed")?
There was a problem hiding this comment.
Only indirectly. Previously it invoked readvertisement more frequently, potentially triggered by external events. With this change we only advertise on connect and on a timer... if the scores change it'll influence what we advertise, but no longer directly triggers it.
There was a problem hiding this comment.
Is advertise intentionally misspelled?
There was a problem hiding this comment.
It's not misspelled. advertize is EN_US and that was the name previously.
There was a problem hiding this comment.
Weird, I've never seen it spelled with a 'z' even in EN_US, but it appears Google agrees.
There was a problem hiding this comment.
After this change do we ever advertize our Tor hidden service address? I've added logging to my test node that listens both on Tor and plainnet, both external addresses appear as SeenLocal
2014-11-08 12:56:48 SeenLocal: xx.xx.xx.xx:8333
2014-11-08 12:57:05 SeenLocal: xxx.onion:8333
However, it always advertizes the IPv4 address
2014-11-08 09:15:21 AdvertizeLocal: advertizing address xx.xx.xx.xx:8333
(maybe this can be avoided using discover=0, and providing externalips, but I explicitly disabled those to test this code)
There was a problem hiding this comment.
The resason you're seen-localing it is becasue peers who already know it are connecting inbound to you. Since outbound HS peers can never seen your HS address it can't be discovered, and must be configured with externalips.
There was a problem hiding this comment.
OK - so when providing externalips, it will advertise the HS address?
There was a problem hiding this comment.
Yes; for two reasons: one is because externalips disables discover, the other is because if you enable discover it still mosty uses the traditional address source (what it would have used if discover were distabled) if it has one that it thinks is routable.
|
Does this pull request also resolve issue #48 ? |
There was a problem hiding this comment.
I don't think this rationale fits. Proxies aren't always for privacy. More importantly: is there a reason not to default discover to false always?
There was a problem hiding this comment.
If proxy is set complex things are happening with your networking and discovery is much more likely not useful.
Defaulting to discover false would break listening for every host behind NAT by default, precisely those users who are least likely to set things right.
Avoiding it when proxy is set helps minimize unwanted surprises without gratitious breakage (afer all, we already disabling listening in that case).
|
@luke-jr updated |
|
|
|
Does anyone feel this is waiting on anything else? |
src/net.cpp
Outdated
There was a problem hiding this comment.
Why would the address a peer tells us be not good if we're running on a non-standard port?
There was a problem hiding this comment.
The peer can't tell us anything about the port we're listning on, (E.g. you connect out to some peer and they see the source port as some random number).... we're already behind address translation. Seemed more conservative to skip in this case.
|
utACK, with a few nits. |
This is a simplified re-do of closed pull #3088. This patch eliminates the privacy and reliability problematic use of centralized web services for discovering the node's addresses for advertisement. The Bitcoin protocol already allows your peers to tell you what IP they think you have, but this data isn't trustworthy since they could lie. So the challenge is using it without creating a DOS vector. To accomplish this we adopt an approach similar to the one used by P2Pool: If we're announcing and don't have a better address discovered (e.g. via UPNP) or configured we just announce to each peer the address that peer told us. Since peers could already replace, forge, or drop our address messages this cannot create a new vulnerability... but if even one of our peers is giving us a good address we'll eventually make a useful advertisement. We also may randomly use the peer-provided address for the daily rebroadcast even if we otherwise have a seemingly routable address, just in case we've been misconfigured (e.g. by UPNP). To avoid privacy problems, we only do these things if discovery is enabled.
|
@sipa adjusted except for the nLastRebroadcast. I think you were misreading it there. Can you confirm? On the disabling it when using a non-default port, any of the other ACKers have an opinion? I think it could go either way. Avoiding running it seemed more conservative to me, but there are cases where if we require the default port it would fail to discover an address that would be useful... when you're behind nat on a non-standard port and managed to map the ports 1:1, and where the old discovery code would have worked. So I've changed it. |
|
@gmaxwell I'd say it's still useful to be able to discover external IP changes even if not using the default port. Throughout the code, bitcoind assumes that it knows the port where it's listening on. The edge case here would be a) behind NAT b) manually forwarded port c) external port is different from internal port. But in that case |
|
@laanwj Okay, great. (I did change it so that it doesn't require the default port anymore). |
845c86d Do not use third party services for IP detection. (Gregory Maxwell)
ghost
mentioned this pull request
It was removed in bitcoin/bitcoin#5161 Conflicts: doc/coding.md
It was removed in bitcoin/bitcoin#5161
It was removed in bitcoin/bitcoin#5161
It was removed in bitcoin/bitcoin#5161
It was removed in bitcoin#5161 (cherry picked from commit be4ac91)
7 participants
This is a simplified re-do of closed pull #3088.
This patch eliminates the privacy and reliability problematic use
of centralized web services for discovering the node's addresses
for advertisement.
The Bitcoin protocol already allows your peers to tell you what
IP they think you have, but this data isn't trustworthy since
they could lie. So the challenge is using it without creating a
DOS vector.
To accomplish this we adopt an approach similar to the one used
by P2Pool: If we're announcing and don't have a better address
discovered (e.g. via UPNP) or configured we just announce to
each peer the address that peer told us. Since peers could
already replace, forge, or drop our address messages this cannot
create a new vulnerability... but if even one of our peers is
giving us a good address we'll eventually make a useful
advertisement.
We also may randomly use the peer-provided address for the
daily rebroadcast even if we otherwise have a seemingly routable
address, just in case we've been misconfigured (e.g. by UPNP).
To avoid privacy problems, we only do these things if discovery
is enabled.