-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
lib/connections: Announce LAN addresses by default #6896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/connections: Announce LAN addresses by default #6896
Conversation
IMHO this is not something we can enable by default without getting lynched. We can maybe provide the option, or better perhaps provide it as an encrypted blob for known peers only |
You think so? I think nobody actually cares about this. I think having public ips in the open is a lot more scary I think as it's an actual attack vector. Knowing local address ips does leak some information, but it doesn't really open any attack vectors. |
I could be wrong and it might not be rational (knowing someone is on 192.168.1.42 is clearly less personal than their public IP), but I think many enterprises consider what happens behind their NAT gateway to be proprietary. Then again those aren't our primary users. I don't remember offhand how our public/private criteria works. Does this catch only RFC1918 space or will it also announce public IP space that would otherwise be invisible because I'm behind NAT? The latter might be worse than if it's just 1918 |
This will construct an address for every adapter that is marked as unicast. If you are on the internet, you'd end up with and address with your public ip address, yes. I guess we could do an RFC1918 filter across the addresses, to remove any actually public ips. |
I think this is amazing, perhaps add RFC1918 filtering and let's go? |
Yeah |
Link local v6 will tend to be based on MAC which is a personal identifier. It allows correlating a device ID to a given piece of hardware and potentially a person so we should probably filter those too. Maybe skip all of v6 to begin with. |
I think link local v6 is a whole /64 or something? The last 48 bits usually have the mac address, so I guess we could just advertise the whole /64? |
So it seems all OS'es have a setting to enable ipv6 privacy that generates random addresses. That seems to be on by default on Windows. I guess I need to check linux/osx. |
I think there are really no circumstances where connections over link local v6 works but local discovery doesn't, and non-link-local v6 might well be considered "secret" so I think we should not announce any local v6 found on the host. |
So I agree about not announcing not link local ipv6, but I disagree with hiding it if it is link local. Discovery might not work because the router blocks multicast traffic, which is actually fairly common. |
So I followed that CL in Go that was abandoned which follows RFC1918, but Python seems to include more networks: https://github.com/python/cpython/blob/master/Lib/ipaddress.py#L2253 |
Bunch of obsolete and weird networks there I think. For me I think the safe approach here is to specifically allow just the normal, well known and safe private IPv4: 192.168/24, 10/8, 172.16/12. Things like 100/8 which is CGNAT and never going to be running on a home/corp network we can ignore. Certainly there are lots of networks with broken multicast but then neighbor discovery and autoconf won't work properly either so IPv6 will be something we don't need to worry about at all... Potentially we could allow fe80::/10 excepting for the bits which indicate a mac based address but that seems complicated for little gain. |
I don't think autoconf needs multicast. Autoconf "Just Works™️" ? Everyone has a predefined static way of working out their address. Anyways, latest commit includes link local unicast (fe80/169.254) or ipv4 local |
Yeah the IPv6 thing is a side track, autoconf does indeed require multicast but that's irrelevant because you don't need autoconf to have a link local (or other) address anyway, sorry for the rabbit hole. I think this looks reasonable. |
So in testing this I realize that we should in fact drop all link local v6, because we currently anyway don't have the capability to connect them. Since the addresses are link local they are always link scoped and need an interface identifier (the percent thing) in order to specify which link. We don't have that, so we don't know which interface to use, so we can't connect to one of these link locals even if they were theoretically reachable. (When getting these via local discovery we have our interface identifier and all is good...) |
I'm sorry to reopen such an old thread, but I am currently evaluating the usage of ipv6 link local addressing in various applications. I connected 2 Windows VMs without any uplink or internet connection, and they were able to sync using ipv4 with their 169.254.. addresses. I expected it to work without ipv4 - but that was not the case. When I disabled ipv4 (or set non-matching ip addresses) syncthing was unable to discover the other machine. I started digging through the code and the commits to finally end up in this thread. I understand the reasoning behind your decisions, but I would love to see an option to allow ipv6 link local discovery. I could imagine hosts with ipv4 completely disabled in the near future, and it would be nice if two machines could sync by just connecting them with a LAN cable. Regarding your reasoning about the interface identifier being necessary to send data to ipv6 link local addresses: yes, it might require some additional work, but it is definitely possible. I did some research on linux and windows and all OSes have ways to detect the interface identifier for incoming packets; for sending it is usually trivial (just append %xxx to address in string format). |
This change has likely nothing to do with your problem. Please open a topic on the forum instead. |
Well, actually 115a4c5 seems to have removed exactly what is necessary for ipv6 local sync, isn't it? It removes the fe80 addresses from the list of useable ips. |
It does not, because that code has nothing to do with local discovery, which is where v6 discovery happens. So I'll repeat, you're barking up the wrong tree. We'll be happy to help you out, over at the right tree. |
I suspect this might end up being a controversial PR, but given the recent influx of people on the forum talking about things failing to connect locally, and ufw by default blocking local discovery, I think this makes a good case to have this.
Also, drive-by logging fixes in random places when I was manually testing and reading the debug output, getting confused why announcements were sent twice.