Skip to content
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

Dialing regards any established connection as successful / does not check device ID #5934

Closed
olifre opened this issue Aug 6, 2019 · 3 comments

Comments

@olifre
Copy link
Contributor

commented Aug 6, 2019

Dialing a host's address may end up connecting (successfully) to a different host (to which a connection may already exist).
Currently, the dialer regards this as success, and the connection is only dropped later when the device ID is checked. This prevents falling back to dialing of other addresses.

Two possible solutions come to mind:

  • Dialer should check node ID after establishing connection before returning success.
  • If a successful connection exists via an address and a different device announces the very same address (including the dialing device's addresses themselves), dialing that and assuming to reach the other device is not logical. This could be suppressed.

Two examples in which this causes issues:

  1. Two hosts behind the same NAT, one directly reachable via port forwarding, both configured to the same (default) listening port. An outside node successfully connects to the port-forwarded node, then tries to connect to the NATed node using the same external IP and port, and establishes a successful connection - but to the wrong node.
  2. Syncthing running on a NATting device itself with other nodes behind the NAT. In this case, the NATting device may be tricked into trying to connect to itself.

Example logs (anonymized) follow.
Case 1:

2019/08/06 19:25:34.081495 structs.go:217: DEBUG: dialing UUID_OF_DEVICE_WE_WANT_TO_REACH tcp://EXTERNAL_IP_OF_NAT:22000 prio 10
2019/08/06 19:25:34.117970 tcp_dial.go:48: DEBUG: Dial (BEP/tcp): setting traffic class: setsockopt: protocol not available
2019/08/06 19:25:34.546076 structs.go:222: DEBUG: dialing UUID_OF_DEVICE_WE_WANT_TO_REACH tcp://EXTERNAL_IP_OF_NAT:22000 success: LOCAL_IP_OF_DIALING_NODE:49308-EXTERNAL_IP_OF_NAT:22000/tcp-client/TLS1.3-TLS_CHACHA20_POLY1305_SHA256
2019/08/06 19:25:34.546099 service.go:872: DEBUG: connected to UUID_OF_DEVICE_WE_WANT_TO_REACH 10 using LOCAL_IP_OF_DIALING_NODE:49308-EXTERNAL_IP_OF_NAT:22000/tcp-client/TLS1.3-TLS_CHACHA20_POLY1305_SHA256 10
...
2019/08/06 19:25:34.546161 service.go:875: DEBUG: discarding 0 connections while connecting to UUID_OF_DEVICE_WE_WANT_TO_REACH 10
...
2019/08/06 19:25:34.546258 service.go:477: DEBUG: sleep until next dial 1m0s
2019/08/06 19:25:34.723211 service.go:282: INFO: Connected to already connected device UUID_OF_OTHER_DEVICE_BEHIND_THE_SAME_NAT (existing: LOCAL_IP_OF_DIALING_NODE:48686-EXTERNAL_IP_OF_NAT:22000/tcp-client/TLS1.3-TLS_CHACHA20_POLY1305_SHA256 new: LOCAL_IP_OF_DIALING_NODE:49308-EXTERNAL_IP_OF_NAT:22000/tcp-client/TLS1.3-TLS_CHACHA20_POLY1305_SHA256)

Case 2:

2019/08/06 19:21:34.136916 structs.go:217: DEBUG: dialing UUID_OF_DEVICE_WE_WANT_TO_REACH tcp://EXTERNAL_IP_OF_NATTING_DEVICE:22000 prio 9
2019/08/06 19:21:34.137011 tcp_listen.go:107: DEBUG: Listen (BEP/tcp): connect from EXTERNAL_IP_OF_NATTING_DEVICE:39702
2019/08/06 19:21:34.186905 structs.go:222: DEBUG: dialing UUID_OF_DEVICE_WE_WANT_TO_REACH tcp://EXTERNAL_IP_OF_NATTING_DEVICE:22000 success: EXTERNAL_IP_OF_NATTING_DEVICE:39702-EXTERNAL_IP_OF_NATTING_DEVICE:22000/tcp-client/TLS1.3-TLS_AES_128_GCM_SHA256
2019/08/06 19:21:34.186926 service.go:872: DEBUG: connected to UUID_OF_DEVICE_WE_WANT_TO_REACH 9 using EXTERNAL_IP_OF_NATTING_DEVICE:39702-EXTERNAL_IP_OF_NATTING_DEVICE:22000/tcp-client/TLS1.3-TLS_AES_128_GCM_SHA256 10
2019/08/06 19:21:34.187022 service.go:229: INFO: Connected to myself (UUID_OF_NATTING_DEVICE) at EXTERNAL_IP_OF_NATTING_DEVICE:39702-EXTERNAL_IP_OF_NATTING_DEVICE:22000/tcp-client/TLS1.3-TLS_AES_128_GCM_SHA256 - should not happen
2019/08/06 19:21:34.194114 service.go:229: INFO: Connected to myself (UUID_OF_NATTING_DEVICE) at EXTERNAL_IP_OF_NATTING_DEVICE:22000-EXTERNAL_IP_OF_NATTING_DEVICE:39702/tcp-server/TLS1.3-TLS_AES_128_GCM_SHA256 - should not happen

Version Information

Syncthing Version: v1.2.1
OS Version: Gentoo Linux x86_64

@olifre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

AudriusButkevicius added a commit to AudriusButkevicius/syncthing that referenced this issue Aug 6, 2019

@olifre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@AudriusButkevicius Wow, that was quick!
I can already confirm that fixes case 2.
I will give the patched version a test for case 1 hopefully tonight (need to reproduce the setup), but I don't see why it should not work 😉
Will be interesting to see if QUIC usage rises with 1.2.2 in September, then 😄.

@olifre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@AudriusButkevicius I can confirm also case 1 is fixed, as expected. I suspect there are also other cases fixed by that, so it will be really interesting to see the relay vs. QUIC usage with the next release 😉.

@calmh calmh added this to the v1.2.2 milestone Aug 12, 2019

@calmh calmh added the bug label Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.