Out-of-band data + SSH Agent forwarding (Take 2) #583

kevinr · 2015-02-07T09:57:43Z

I've cleaned up the compiler warnings in Timo Rinne's [ssh-agent-forwarding branch](https://github.com/rinne/mosh/tree/ssh-agent-forwarding branch) from pull request #423, to address issue #120, and squashed the change down into a single commit. (It looks like you weren't configuring with --enable-compile-warnings=error, Timo, which the README recommends for distributors, and otherwise the Make output suppresses warnings.)

In my limited testing on Ubuntu Trusty, the patch works as advertised.

I'd love to see this merged to master.

luto · 2015-02-07T10:01:45Z

src/agent/agent.h

+    void notify_eof(uint64_t agent_id);
+    AgentConnection *get_session();
+
+    ProxyAgent(const ProxyAgent&);    // unimplemented


are those two still unimplemented?

Yes -- the copy constructor and operator= method aren't used, but the class has pointer members so leaving these methods undefined produces a warning. Defining them as private but not specifying them quiets the warning and results in a link error if someone tries to use them anywhere.

mkay :) running out of C++ knowledge here :D

luto · 2015-02-07T10:07:07Z

Thanks for the work! 👍
Even though I'm neither a C++ programmer nor a mosh maintainer, I've added some comments to help @keithw out a bit :P

kevinr · 2015-02-07T10:28:24Z

Thanks @luto !

I'm sorry @rinne, I didn't manage to tag you when I created this pull request. I cleaned up the compiler warnings from your branch and squashed the commits down.

rinne · 2015-02-10T18:53:18Z

@kevinr No problem. I'm just glad someone is willing to catch that, since I really don't have time for it and it has passed the magical "works for me" -level.

Pluggable out-of-band communication mechanism over Mosh transport layer and SSH agent forwarding support on top of the out-of-band mechanism

Kriechi · 2015-02-17T13:15:36Z

👍
works for me on OS X 10.9 client with Debian 7.8 as server.

Dorthu · 2015-02-19T16:42:15Z

👍 Working on OSX/Debian 7

moltar · 2015-02-26T00:05:58Z

yay.

toresbe · 2015-03-05T15:58:32Z

Awesome! Thanks. This makes my day so much easier when maintaining servers on the other side of the planet. Confirmed working on Debian 8 going to FreeBSD.

jravetch · 2015-03-15T04:23:49Z

👍

nathanlburns · 2015-03-25T15:28:29Z

Is there any update here? Additional testing needed?

Mic92 · 2015-04-29T17:13:10Z

Thanks for this feature. For those on archlinux I have upload the package mosh-sshagent-git in the AUR based on the kevinr:kevinr-ssh-agent-forwarding branch.

PAStheLoD · 2015-05-02T09:27:05Z

Hello,

You might be interested in knowing, that there is a problem with compiling this with g++ 4.9.

g++ -DHAVE_CONFIG_H -I. -I../..  -I./../util -I./../crypto -I../protobufs -pthread   -Wall -Werror -Wextra -pedantic -Wno-long-long -Weffc++ -fno-strict-overflow -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector --param ssp-buffer-size=1 -fPIE -fno-default-inline -pipe -g -O2 -c -o networktransport.o networktransport.cc
In file included from transportsender.h:45:0,
                 from networktransport.h:43,
                 from networktransport.cc:35:
outofband.h:118:9: error: 'class Network::OutOfBandPlugin' has virtual functions and accessible non-virtual destructor [-Werror=non-virtual-dtor]
   class OutOfBandPlugin
         ^
cc1plus: all warnings being treated as errors

Interestingly g++ 4.8 works fine.

glensc · 2015-05-02T13:16:45Z

the warning can be made non-fatal for now: -Wno-error=non-virtual-dtor

mobile-shell/mosh#583 mobile-shell/mosh#423 mobile-shell/mosh#120

MacGyverNL · 2015-06-02T13:30:21Z

I've started using gpg-agent-forwarding through the SSH-mechanism for arbitrary Unix Domain socket forwarding (ssh -R /home/remoteuser/.gnupg/S.gpg-agent /home/localuser/.gnupg/S.forwarded-agent, recent addition to OpenSSH). I have not looked at the code in OpenSSH used to do this, but looking at the patches for SSH-agent-forwarding it seems to me that support for forwarding arbitrary Unix Domain sockets should not be too different from what's here now. Would this be an interesting thing to add?

rinne · 2015-06-03T06:17:22Z

The current implementation of agent forwarding actually parses the content of the stream so that it parses and sanity checks protocol packet length and separates the packets from each other (does not care about packet contents). This is done in order to keep the flow control issues in minimum. Adding specific gpg agent forwarding would probably be quite easy and it could borrow most of the ssh agent forwarding code and run over the out-of-band transport like it. But adding a generic forwarding with full socket flow control would be much bigger effort.

glensc · 2015-06-09T07:24:48Z

@kevinr would you mind rebasing it to current master? so could build this patch against 1.2.4.95rc1 pre-release

cgull · 2015-06-28T22:34:51Z

src/agent/agent.cc

+    return;
+  }
+  if (server) {
+    PRNG prng;


This really is close to being a bikeshed...but I have to agree that mkdtemp() is a better thing to do here. Your code looks OK, but the people who write libc code have thought longer and harder on this topic than you or I have.

This is pretty much a non-issue, but I have a recollection that there really is a valid reason not to use library function here. Also this is really simple stuff.

cgull · 2015-06-28T23:32:45Z

I finally looked at this code a couple of days ago. The larger question of whether the Mosh project wants to add this sort of functionality is still there, but I thought I should at least review the code and see how it looks.

@kevinr, @rinne: I reviewed this pull request because it was conveniently lumped together in one diff. But the Mosh project prefers a clean Git history: we want to see changes better broken down into more discrete, incremental pieces of work, commits that reflect the initial development process and wouldn't be relevant once brought into the Mosh repo (bug fixes, implementation changes, etc) should be squashed, and in general the commits should tell a clear, readable, reviewable story of how you got from there to here. I didn't look quite closely enough, but I think having everything squashed down into one commit is a better place to start than @rinne's original commits, which have one large commit containing most of the agent-forwarding work (and the MOSH_ESCAPE_KEY feature too) and then a series of bug fixes following from there. It's not immediately clear to me how you'd break this into incremental pieces of work (which does say something about how tied the various parts are to each other).

But it mostly looks pretty good to me. The biggest issue that I see is that it integrates awkwardly with Mosh's (imperfect) event handling, and has a couple of problems with blocking I/O. Everything I noticed is quite fixable, though.

The network stack here is fairly simple (which I think is appropriate). It has what's needed for agent forwarding. But its reliable stream implementation is very simple, it's synchronous and unbuffered. Those of you hoping that this code could bring port forwarding, X11 forwarding, and other higher-performance things like that...this isn't ready for that.

rinne · 2015-07-02T07:24:21Z

@cgull Thanks for going through the stuff.

I merged the forwarding branch with original commit history to current upstream master. The resulting branch is ssh-agent-forwarding-ng in https://github.com/rinne/mosh/

The agent work has two main commits. One being the oob layer and another being the agent forwarding on top of that. Probably with some of the history might benefit from rebasing, but I don't think I have time nor energy for that. If someone can spare the time, please do.

What you said about OOB not being ready for generic stream forwarding, I fully agree. Generic streaming actually sets quite a few requirements to intra-protocol flow control and congestion detection (flow control must be fully implemented for each forwarded stream, not just the protocol connection itself). Also for it to have a decent performance, it needs to have some kind of self adjusting windowing etc also in protocol level. I remember all this being such a pain for ssh development, that I feel pretty strongly that implementing generic stream forwarding to mosh would be much more work than it's worth. And even with someone willing to invest the effort, it would have stability issues and even possible security implications that would take a huge effort to solve. I just don't think it's worth it.

However, it was practically free to implement OOB so that it has separate modes (unreliable datagram mode, reliable datagram mode, reliable stream mode) at least in API level. It should more or less work in either mode, but agent connection is forwarded using reliable datagram mode (one agent protocol packet per datagram) simply in order to avoid inherent problems of generic streaming. It does however mean, that agent protocol packets have to be parsed in mosh-server just enough to read their length and the corresponding payload.

As I said earlier, I feel quite comfortable with current "works for me" -level of mosh agent forwarding implementation. It's not just hacked together up to the point it's somewhat usable, but as far as I can see, it doesn't have any obvious problems and also the way it's hooked into mosh has been at least somewhat designed and not just somehow scotchtaped together. I'm sure that could have been done also in another way, but at least I'm not going to re-implement that. I think it's useful functionality and use it myself every single day, so in one form or another, I hope it makes its way to mosh upstream.

Kriechi · 2015-10-28T16:56:07Z

@kevinr if this PR is still considered active, would you mind rebasing and including recent changes from master? Thanks.

rinne · 2015-11-28T15:44:13Z

I merged OOB + agent forwarding on top of current master and did another PR #696.

cgull · 2016-05-25T15:33:38Z

I'd like to consider this for Mosh 1.3, after we release 1.2.6. I've started a discussion on the mosh-devel mailing list; if you're interested please comment there.

OneOfOne · 2016-06-02T17:31:15Z

This is very important for us with crappy connections that need to transfer files over ssh.

andersk · 2016-06-03T02:30:44Z

@OneOfOne: File transfer is outside the scope of this patch (and probably of Mosh, for the foreseeable future).

kevinr · 2016-06-03T04:28:31Z

@cgull I'd love to get it merged in 1.3. I don't see your thread on the mosh-devel list, though.

andersk · 2016-06-03T04:38:27Z

@kevinr http://mailman.mit.edu/pipermail/mosh-devel/2016-May/001297.html

cgull · 2016-06-04T03:59:44Z

@kevinr: I got a bounce from Gmail when I sent to mosh-devel, which I think is related to recent DMARC changes they've made, which I believe will cause some mailing list mail (including from my domain) to bounce. If you're using Gmail that's probably why you didn't see it.

ErwanAliasr1 · 2017-01-13T11:07:47Z

Any chance to get that feature in ?

rinne · 2017-01-13T11:19:59Z

@ErwanAliasr1 See #696 instead. This one should be closed anyways.

jspraul · 2022-02-11T14:19:30Z

@ErwanAliasr1 See #696 instead. This one should be closed anyways.

To be clear to passers-by like me who might otherwise get excited: @andersk explained #696 only covers only SSH agent forwarding (and has been merged 🎉), not all out-of-band data (port forwarding).

It's unfortunate that half of this was dropped, it's probably a lot of work now to fit the pieces into the latest code. Is it possible that's what's happening on this fork by @rinne?

Mic92 · 2022-02-11T15:04:19Z

I also have some goodies merged in here. https://github.com/Mic92/mosh/commits/agent-forwarding
I rebase this fork every once in a while

rinne · 2022-02-11T16:46:27Z

@ErwanAliasr1 See #696 instead. This one should be closed anyways.

To be clear to passers-by like me who might otherwise get excited: @andersk explained #696 only covers only SSH agent forwarding (and has been merged 🎉), not all out-of-band data (port forwarding).

It's unfortunate that half of this was dropped, it's probably a lot of work now to fit the pieces into the latest code. Is it possible that's what's happening on this fork by @rinne?

Hi. Actually none of that has been merged to upstream. #696 is just another thread of the same thing because of something I don't remember any longer. Anyways, there OOB data is not port forwarding as such, but it's just a protocol level extension that could be used in port forwarding like it's now used with agent forwarding. Anyways, none of these have made itself into upstream. I merge the upstream master to agent enabled branch (see #696) usually when I have to compile Mosh for myself, which happens maybe every few months or so.

rinne · 2022-02-11T16:49:06Z

And OOB stuff as it is now isn't really suitable for generic port forwarding. It lacks flow control and should there be a connection in which the reader side is blocking and producer side keeps writing, it simply closes the connection.

luto reviewed Feb 7, 2015
View reviewed changes

out-of-band communication and ssh-agent forwarding

18af9d4

Pluggable out-of-band communication mechanism over Mosh transport layer and SSH agent forwarding support on top of the out-of-band mechanism

glensc added a commit to pld-linux/mosh that referenced this pull request May 2, 2015

build with ssh-agent forwarding

501df8d

mobile-shell/mosh#583 mobile-shell/mosh#423 mobile-shell/mosh#120

cgull reviewed Jun 28, 2015
View reviewed changes

patcon mentioned this pull request Sep 10, 2015

Merge in source install method jtimberman/mosh-cookbook#20

Closed

rinne mentioned this pull request Nov 28, 2015

SSH agent forwarding patches re-merged as one patch. #696

Open

kevinr closed this Feb 27, 2017

Out-of-band data + SSH Agent forwarding (Take 2) #583

Out-of-band data + SSH Agent forwarding (Take 2) #583

Conversation

kevinr commented Feb 7, 2015

luto Feb 7, 2015

Choose a reason for hiding this comment

kevinr Feb 7, 2015

Choose a reason for hiding this comment

luto Feb 7, 2015

Choose a reason for hiding this comment

luto commented Feb 7, 2015

kevinr commented Feb 7, 2015

rinne commented Feb 10, 2015

Kriechi commented Feb 17, 2015

Dorthu commented Feb 19, 2015

moltar commented Feb 26, 2015

toresbe commented Mar 5, 2015

jravetch commented Mar 15, 2015

nathanlburns commented Mar 25, 2015

Mic92 commented Apr 29, 2015

PAStheLoD commented May 2, 2015

glensc commented May 2, 2015

MacGyverNL commented Jun 2, 2015

rinne commented Jun 3, 2015

glensc commented Jun 9, 2015

cgull Jun 28, 2015

Choose a reason for hiding this comment

rinne Jul 2, 2015

Choose a reason for hiding this comment

cgull commented Jun 28, 2015

rinne commented Jul 2, 2015

Kriechi commented Oct 28, 2015

rinne commented Nov 28, 2015

cgull commented May 25, 2016

OneOfOne commented Jun 2, 2016

andersk commented Jun 3, 2016

kevinr commented Jun 3, 2016

andersk commented Jun 3, 2016

cgull commented Jun 4, 2016

ErwanAliasr1 commented Jan 13, 2017

rinne commented Jan 13, 2017

jspraul commented Feb 11, 2022 • edited Loading

Mic92 commented Feb 11, 2022

rinne commented Feb 11, 2022

rinne commented Feb 11, 2022

jspraul commented Feb 11, 2022 •

edited

Loading