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

On client, connect() to server. #1013

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cgull
Copy link
Member

@cgull cgull commented Oct 19, 2018

This was provoked by @eminence commenting on IRC that he was getting 'packet failed integrity check' messages on his client.

This should be a pretty reasonable add, but I worry about weird scenarios where clients are somehow actually receiving packets on 4-tuples that are different from the ones they're sending packets on.

One effect I've also noticed in testing is that with this change, Mosh is slightly slower to recover from clients hopping to new networks. Comparing old behavior to new:

Old:

  • mosh-client is happily sending on a given Connection with a given 4-tuple including the auto-selected source IP address, translated to a different 4-tuple by NAT
  • client moves to a new network (say, from Wifi to mobile)
  • mosh-client continues sending on that Connection, but with a new 4-tuple because of the new source address. The new NAT on the new network of course translates to some entirely new 4-tuple
  • mosh-server immediately responds to the new 4-tuple as a new connection. The client NAT translates this back into mosh-client's 4-tuple
  • mosh-client continues on with the existing Connection, blissfully unaware that mosh-server sees this as a new connection

New:

  • When mosh-client creates a new Connection, it calls connect(), thus binding the connection to the source IP address
  • mosh-client is happily sending on a given Connection, translated to a different 4-tuple by NAT
  • client moves to a new network (say, from Wifi to mobile), and acquires a new IP address
  • mosh-client tries to continue sending on that Connection, but encounters errors, because the connected socket is on the old IP address, which no longer has a route. No packets are transmitted.
  • After the normal 15-second timeout, mosh-client creates a new Connection, which is connect()ed on the new IP address, and the session resumes.

In my testing, that's not a big issue, because of the human and computer time already taken by manually switching to a new network, acquiring new link/IP address/whatever. But does anybody see this as a problem? Are there any Mosh users who are rapidly hopping between different IP addresses on their clients?

This keeps mosh-client from receiving wayward UDP packets from other
sources.
@keithw
Copy link
Member

keithw commented Oct 19, 2018

The original reason we didn't connect() was that the port-hopping behavior didn't come until much later, so with a connect() it just wasn't possible for the client to switch NICs at all. With the port-hopping, you're right that it will recover anyway after 15 seconds -- but this does seem like something of a regression from the current almost-instant roaming. :-/ I admit I've always had a sort of romantic attachment to the idea that the client can successfully roam without even knowing that it's roamed. And does this mean it would now take 15 seconds to reconnect in the event of, e.g., changing Wi-Fi networks?

In any event, if we do do this I'd rather wait until after the next release since it does seem like something of a major change.

@eminence
Copy link
Member

In all my years of moshing, this is the first time I've seen regular "packet failed integrity check" messages. But my work laptop changes networks about a dozen times per day, and an additional 15-second delay during each change would be pretty noticeable and annoying, I think.

What about quelling the "packet failed integrity check" message in certain cases (like the packet comes from a machine that doesn't look like the the mosh-server machine)?

This approximately restores Mosh's fast recovery on roaming.

retry once on send() error
@cgull
Copy link
Member Author

cgull commented Oct 19, 2018

This new commit should approximately restore the old roaming behavior. But I agree that this idea isn't a good candidate for the next release. I think the connect() has some modest security value, though, by limiting the receipt of packets from other sources.

If we do anything for the next release, it should be quieting the error reporting for short/crypto-bad packets, that seems less controversial.

@cgull
Copy link
Member Author

cgull commented Oct 19, 2018

@eminence, is it possible your machine is getting port-scanned, or otherwise getting actively probed? (Doesn't have to be malign, your IT people could be doing it...)

@eminence
Copy link
Member

Not likely, since this was while I was on my internal home network (and I wasn't doing any scanning). This network has several different devices on it (windows, linux, mobile devices, smart devices), and while I can't take a guess at what might have been sending those packets, I wouldn't be surprised if mosh-client hopped, by sheer coincidence, onto some port that gets periodically probed by something on my network.

@cgull
Copy link
Member Author

cgull commented Oct 20, 2018

Hmm, that might be broadcast/multicast from mDNS (port 5353) or UPNP (port 1900) or something like that. That's another thing Mosh could check, it could drop non-unicast packets, or packets not to a local address, or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants