There is currently an issue where the device doesn't send the correct IP address for sshfs when a VPN is present.
Instead of asking the device to find and send its address, we can store it from when the device link is created then reuse it.
Details
Details
- Reviewers
albertvaka - Group Reviewers
KDE Connect - Commits
- R224:25b86c6c7f9f: Determine which IP address to use for sshfs
All unit tests pass.
In these different situations:
- without a VPN
- with a VPN running
- with a VPN started then stopped (the tun interface might still be there)
Try to remotely browse the device. It should work in all cases.
Diff Detail
Diff Detail
- Repository
- R224 KDE Connect
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
I wonder if there is a different way to fix this bug? I actually (successfully, including sshfs) use OpenVPN to get around poorly-managed networks where KDE Connect would otherwise not work very well, so making this change would break that for me :)
Maybe rather than blacklisting tun connections, it would make sense to make the sshfs command more robust? I haven't looked into this, so I don't know what that might involve.
Comment Actions
I see. I am not particularly good with networks, how did you make it work with a VPN? The only idea I had was using iptables to redirect traffic from tun0 to wlan0, but I've never used iptables and didn't manage to do that...
Edit:
Concerning your proposition to make the sshfs command more robust: from what I can see, all the desktop application has to work with is the network package it receives (see onPakcageReceived() in mounter.cpp), which a single IP address. This is why my first solution was on the device side, making sure the address sent is not from tun. But it could be problematic as you said.
Is there a way to know which one should be sent?
Or maybe we could send both addresses and use one as fallback after a certain timeout... ? Although this feels a bit hacky
Comment Actions
I didn't do anything particularly clever -- On the Android side I had to use the app 'OpenVPN for Android' because the official OpenVPN Connect app didn't work, then I used the KDE Connect app to manually add a device by IP and used the VPN IP address of my laptop.
Every time I do anything with a network, I always walk away thinking "My god, that was a gigantic hack" :)
My initial thought was to do some kind of ping / port scan to see if the passed IP address has an SSH server running on the specified port, then failover to other IP addresses. We should see if anyone else has better ideas.
Comment Actions
Okay, I did it differently (close to what you said):
The android application sends all IPs it can find in the device interfaces, then the desktop application loops on those IPs and tries to open a TCP connection on the current IP on the requested port: if it succeeds before the timeout, that's the IP that will be used to mount with sshfs.
I made changes in both repository though: so how should I upload the diff in that case?
Comment Actions
Transmit all ip addresses to the desktop application so that it can determine which one to use
Comment Actions
Loop on the addresses received in the network package and determine which one is open to KDE Connect
Comment Actions
In general, this looks like an overkill to me.
Maybe it would make sense to allow the DeviceLink to tell which IP it's connected to?
| plugins/sftp/mounter.cpp | ||
|---|---|---|
| 105 | No need to initialize with "". Just leave QString ipAddress;. | |
| 110 | We should at least parallelize the connectToHost. i.e. connect them all and then wait them all. | |
| 118 | if (ipAddress.isEmpty()) { Says more things and doesn't need to construct a QString("") | |
Comment Actions
I agree, that was my first idea, but I couldn't find where to get the IP address...
Now I found I can get it from newUdpConnection in LanLinkProvider. From there, I added a mHostAddress field to LanDeviceLink so I can store the address. However, I can't access it in onPakcageReceived from Mounter because m_deviceLinks is private in Device.
I'll update the revision to show the new address field.
Comment Actions
Instead of transmitting all IP addresses for the device, maybe store the correct one in LanDeviceLink
Comment Actions
I realized this is just bad.
I'm working on something that works... I'll update when it's presentable, right now it works, but it's not great.
Comment Actions
Thank you very much for digging into this! Feel free to email the mailing list if you want help or a second opinion on something.
Comment Actions
This seems to work for me. The correct IP address is stored and reused.
When a connection is started with a device, LanDeviceLink::reset is called. In it we can get the IP address from the SslSocket and store it as a member in the parent class DeviceLink. From there, the sftp plugin can get the device link from its deviceId member then the address from this link.
Two things to note, and can perhaps be improved:
- mHostAddress is stored in DeviceLink even though it should be a member of LanDeviceLink. Unfortunately, the sftp plugin only has access to DeviceLink, not its children. (The alternative would be to give access to the plugin to LanDeviceLink, either directly or through DeviceLink but I don't know what solution is better.)
- I had to create a public accessor to get a DeviceLink from its id.
Comment Actions
Thanks for looking into this long standing issue. The main problem here is that the getLocalIpAddress() function on the Android app is really unreliable, and my ideal solution would be to get rid of it completely, but I haven't found how.
My last thoughts are that we could take advantage from that fact that we are already connected to the device on a valid IP (via the LanLinkProvider). And even though the design behind KDE Connect is about decoupling the plugins from the transport (the "links"), in this case we are already assuming that the transport is gonna be a TCP connection over network... so I guess it's okay to hack a way to get the IP from the LanLink to the Sftp plugin. It will be definitely more reliable and should solve this problem.
Do you think you can try this approach? Maybe you have a better solution that I haven't though of?
Comment Actions
Oh, wait, that's what you just did in the latest version of the patch! Okay, let me review it :P
| core/backends/devicelink.h | ||
|---|---|---|
| 74 ↗ | (On Diff #16937) | I don't think is necessary to move this to the DeviceLink class: you can do a qobject_cast or dynamic_cast to LanDeviceLink on all the DeviceLinks of a Device until you find the one that is actually a LanDeviceLink (which, since we only have one type of link as of now, will always be the case), and then get the IP from that one. |
| core/device.cpp | ||
| 383 | The id parameter is redundant: All the device links for this device are gonna match the id. | |
| plugins/sftp/mounter.cpp | ||
| 117 | Maybe you can add a function to device that returns the IP directly? m_sftp->device()->getLocalIpAddress() That function would be responsible of finding a LanDeviceLink for the device, and if it found one get the IP from it. If the device doesn't have a LanDeviceLink, this function could return an empty QHostAddress object, so the plugin prints an error and doesn't try to connect. | |
Comment Actions
- mHostAddress is now in LanDeviceLink instead of DeviceLink
- Device::deviceLink changed to Device::getLocalIpAddress and only returns a QHostAddress instead of the whole link
- the block in LanDeviceLink::reset that gets and stores the IP address is cleaner now
| plugins/sftp/mounter.cpp | ||
|---|---|---|
| 117 | Yes I was just realizing that while rereading the code. There's no reason to expose the link, you're right. | |
Comment Actions
Looks good to me. I will merge this in as soon as I have a moment. Can you check my comment about IPv6? I think v6 should just work.
| core/backends/lan/landevicelink.cpp | ||
|---|---|---|
| 60 | I think we can remove the special case for IPv6, and it should just work? Hopefully :D | |
Comment Actions
Great, thanks :). Yep I answered.
If this is accepted, then this revision is no longer needed; I could update it to remove getLocalIpAddress() from the android side (if it is not going to be used). Is it OK?
| core/backends/lan/landevicelink.cpp | ||
|---|---|---|
| 60 | Just copying the address is what I had tried first, but it doesn't work, you have to construct a new QHostAddress with toIPv4Address() since Qt 5.6. Without that, toString() returns something like ::ffff:192.168.0.1 which sshfs doesn't like. | |
Comment Actions
We can't kill getLocalIpAddress() yet, because users update their Android apps way more frequently than they update their desktops apps. That means: it's very likely that they are gonna have an old KDE Connect on their desktop that still needs the IP to be sent from Android.