Determine which IP address to use for sshfs
ClosedPublic

Authored by jeanv on Jul 16 2017, 11:43 AM.

Details

Summary

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.

Test Plan

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

Repository
R224 KDE Connect
Lint
Lint Skipped
Unit
Unit Tests Skipped
jeanv created this revision.Jul 16 2017, 11:43 AM
jeanv edited the summary of this revision. (Show Details)

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.

jeanv added a comment.EditedJul 16 2017, 4:09 PM

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.

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

sredman added a comment.EditedJul 16 2017, 4:49 PM

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.

In D6730#125937, @jeanv wrote:

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

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.

jeanv added a comment.EditedJul 17 2017, 3:46 PM

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?

Just upload two separate diffs, one for each repo

jeanv updated this revision to Diff 16833.Jul 17 2017, 4:22 PM

Transmit all ip addresses to the desktop application so that it can determine which one to use

jeanv updated this revision to Diff 16834.Jul 17 2017, 4:25 PM
jeanv changed the repository for this revision from R225 KDE Connect - Android application to R224 KDE Connect.

Loop on the addresses received in the network package and determine which one is open to KDE Connect

jeanv added a comment.Jul 17 2017, 4:27 PM

Just upload two separate diffs, one for each repo

I tried, I'm not sure I did it right <.<

Create a separate revision for one of the diffs

jeanv retitled this revision from Ignore interface "tun" in getLocalIpAddress to Determine which IP address to use for sshfs.Jul 17 2017, 4:50 PM
jeanv edited the summary of this revision. (Show Details)
apol added a subscriber: apol.Jul 19 2017, 12:52 AM

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("")

jeanv added a comment.Jul 19 2017, 8:25 AM
In D6730#126708, @apol wrote:

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?

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.

jeanv updated this revision to Diff 16903.Jul 19 2017, 8:28 AM

Instead of transmitting all IP addresses for the device, maybe store the correct one in LanDeviceLink

jeanv added a comment.Jul 19 2017, 5:44 PM

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.

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.

jeanv updated this revision to Diff 16937.EditedJul 20 2017, 8:34 AM
jeanv edited the summary of this revision. (Show Details)
jeanv edited the test plan for this revision. (Show Details)
jeanv set the repository for this revision to R224 KDE Connect.

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.

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?

Oh, wait, that's what you just did in the latest version of the patch! Okay, let me review it :P

albertvaka requested changes to this revision.Jul 21 2017, 9:20 AM
albertvaka added inline comments.
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 ↗(On Diff #16937)

The id parameter is redundant: All the device links for this device are gonna match the id.

plugins/sftp/mounter.cpp
117 ↗(On Diff #16937)

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.

This revision now requires changes to proceed.Jul 21 2017, 9:20 AM
jeanv updated this revision to Diff 16962.Jul 21 2017, 11:30 AM
jeanv edited edge metadata.
jeanv marked an inline comment as done.
  • 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
jeanv marked 2 inline comments as done.Jul 21 2017, 11:36 AM
jeanv added inline comments.
plugins/sftp/mounter.cpp
117 ↗(On Diff #16937)

Yes I was just realizing that while rereading the code. There's no reason to expose the link, you're right.

albertvaka accepted this revision.Jul 21 2017, 4:25 PM

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 ↗(On Diff #16962)

I think we can remove the special case for IPv6, and it should just work? Hopefully :D

This revision is now accepted and ready to land.Jul 21 2017, 4:25 PM
jeanv marked an inline comment as done.Jul 21 2017, 4:54 PM

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.

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 ↗(On Diff #16962)

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.
Here's what I found about this: https://bugreports.qt.io/browse/QTBUG-53298

In D6730#127267, @jeanv wrote:

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?

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.

This revision was automatically updated to reflect the committed changes.