Skip to content

Conversation

AudriusButkevicius
Copy link
Member

@AudriusButkevicius AudriusButkevicius commented Aug 15, 2020

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.

@calmh
Copy link
Member

calmh commented Aug 17, 2020

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

@AudriusButkevicius
Copy link
Member Author

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.

@calmh
Copy link
Member

calmh commented Aug 17, 2020

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

@AudriusButkevicius
Copy link
Member Author

AudriusButkevicius commented Aug 17, 2020

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.
Do you think that would be more suitable?

@AudriusButkevicius
Copy link
Member Author

I think this is amazing, perhaps add RFC1918 filtering and let's go?

@calmh
Copy link
Member

calmh commented Aug 23, 2020

Yeah

@calmh
Copy link
Member

calmh commented Aug 23, 2020

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.

@AudriusButkevicius
Copy link
Member Author

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?

@AudriusButkevicius
Copy link
Member Author

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.

@calmh
Copy link
Member

calmh commented Aug 23, 2020

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.

@AudriusButkevicius
Copy link
Member Author

So I agree about not announcing not link local ipv6, but I disagree with hiding it if it is link local.
We should hide it if it matches the mac address I guess for the fear of being able to indentify a device, but if it doesn't, I don't see in harm announcing it.

Discovery might not work because the router blocks multicast traffic, which is actually fairly common.
On windows it's enough to mis-click and select the network as public network when first connecting to it, which will break local discovery.

@AudriusButkevicius
Copy link
Member Author

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
https://github.com/python/cpython/blob/master/Lib/ipaddress.py#L1538

@calmh
Copy link
Member

calmh commented Aug 23, 2020

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.

@AudriusButkevicius
Copy link
Member Author

AudriusButkevicius commented Aug 23, 2020

I don't think autoconf needs multicast. Autoconf "Just Works™️" ? Everyone has a predefined static way of working out their address.
Sure, I guess the router needs to accept that and route it. I can do that for link local stuff, yet still prevent sending stuff to multicast addresses.

Anyways, latest commit includes link local unicast (fe80/169.254) or ipv4 local

@calmh
Copy link
Member

calmh commented Aug 24, 2020

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.

@calmh
Copy link
Member

calmh commented Aug 25, 2020

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...)

@calmh calmh merged commit b19b5c9 into syncthing:main Aug 25, 2020
@AudriusButkevicius AudriusButkevicius deleted the announce-local-addresses branch August 25, 2020 15:42
@calmh calmh added this to the v1.10.0 milestone Aug 27, 2020
@tobox
Copy link

tobox commented Mar 2, 2021

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).

@calmh
Copy link
Member

calmh commented Mar 2, 2021

This change has likely nothing to do with your problem. Please open a topic on the forum instead.

@tobox
Copy link

tobox commented Mar 2, 2021

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.

@calmh
Copy link
Member

calmh commented Mar 2, 2021

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.

@bt90 bt90 mentioned this pull request Mar 11, 2021
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Aug 30, 2021
@syncthing syncthing locked and limited conversation to collaborators Aug 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants