SSH agent forwarding patches re-merged as one patch. #696
Conversation
|
Hi I wonder what causes the autobuild failure. It seems to be that one protocol buffers generated header fails to include. Compiles fine for me in OpenSUSE Leap and OS-X. Must be something trivial. Maybe someone else can give it a quick look. (probably Makefile.am, src/agent/Makefile.am, or src/network/Makefile.am is to blame). |
|
It fails for me on OS X with |
Signed-off-by: Timo J. Rinne <[email protected]>
rinne
force-pushed
the
agent-forwarding-merge-20151128
branch
from
90cf1e5 to
dfa183d
Compare
|
Fixed! |
|
Perhaps should not have squashed my trivial Makefile.am fix directly to previous commit, but that's exactly what I did :/. Anyways, now it works and it the stuff is still in one commit. The original arrangement was to have OOB protocol first and agent stuff on top of that in another commit, but I suppose no-one is about to use OOB to anything else besides agent, so I can't imagine OOB being merged but agent forwarding omitted. |
| } | ||
| string path(dir + "/"); | ||
| for ( i = 0; i < 12; i++ ) { | ||
| path += voc.substr( prng.uint32() % voc.length(), 1 ); |
There was a problem hiding this comment.
This is really not even remotely secure. It would be better to use http://pubs.opengroup.org/onlinepubs/9699919799/functions/mkdtemp.html instead.
There was a problem hiding this comment.
It would be nice to have a bit more elaborate reference to some insecure bit. Claiming "not even remotely safe" needs a bit more than just a statement of "fact".
What it tries to do, is to fail in case the directory can't be safely created, which makes a single agent forwarding fail in extremely unlikely case. While using mkdtemp(3) would probably be ok, the problem is that it doesn't get mode as argument and changing the mode afterwards would easily create a race condition and changing mosh server umask doesn't feel very appealing. Historically unix domain socket endpoints have not been particularly safe on all platforms (they should be safe in 2010's systems though) and that's why they are in security apps wrapped into directories.
That much said, if someone likes to modify the patch to use mkdtemp, I don't object.
There was a problem hiding this comment.
Sorry, what is the race condition created by mkdtemp?
There was a problem hiding this comment.
Just for completeness: According to the the man page, mkdtemp always creates the directory with permissions 0700.
There was a problem hiding this comment.
Quickly glancing through current glibc version of mkdtemp(3) and it seems fine. As said earlier, I don't mind if someone wants to convert (and converts) the patch to use mkdtemp. It's just that I won't be using my time doing that.
There was a problem hiding this comment.
Actually remembering the past and the ssh server implementation, the most tricky thing in creating user's agent proxy socket was due to two things, neither of which is present in mosh.
-
The socket was created while the server was still running with root privileges.
-
All sockets for a single user were generated into a same directory (like /tmp/ssh-keithw, later in openssh those paths are "anonymized" so another user can't as easily create a directory to tmp blocking other user).
Point 1 is obvious in mosh, because mosh server is executed wth user credentials.
Point 2 is a double bladed sword. It surely makes the code more complex and in some cases may even allow other local users to block agent forwarding from the others. On the other hand, each new connection doesn't create another directory to tmp that needs to be cleaned up later.
In mosh patch, the simplest approach is taken and new directory is created for each proxy socket and path name is generated with enough random so that collisions are extremely improbable (and while they may occur in principle, it would only mean that agent forwarding for that particular connection would not work).
There was a problem hiding this comment.
I do not understand this comment as a justification for rolling your own mkdtemp. It seems to me that cleaning up directories created by mkdtemp and friends is not necessary, since they will eventually be cleaned up automatically.
There was a problem hiding this comment.
I think you misunderstood. It was not justification of anything. Just a brief history. And should you be up to it, you are welcome to submit a patch that makes the code use mkdtemp(3). It's done now like it is, just because of a faint recollection that some implementations of mktmp were broken sometime around the dawn of time and instead of looking into it further I just wrote it like I did. Should there be a bug, please point it out. Should you want to get rid of it in favour of mkdtemp(3), write and submit a patch, and stop whining.
There was a problem hiding this comment.
And absolutely, it's not ok, to create a new directory to /tmp with every login and leave it there "because it's going to be eventually cleaned up". That's absolutely idiotic, no matter how the directory gets created. Mkdtemp "and friends" are not like tmpfile(3) which returns a file descriptor and the actual file gets automatically cleaned up when the file descriptor is closed or process terminated.
|
Merged current upstream master branch and resolved a couple of trivial conflicts. |
|
just wanna shout my <3 for this PR. |
|
This makes me so happy. Thank you very much! |
|
Hmm, tried this but getting: ~ ❯ mosh markl@xxxxxx --forward-agent I'm pretty sure I installed it right, but it doesn't seem to work :( |
|
@magicmark Yes. Also server must be compiled from agent-forwarding-merge-20151128 branch using |
I encountered this during dpkg-buildpackage on Debian Jessie: outofband.h:118:9: error: ‘class Network::OutOfBandPlugin’ has virtual functions and accessible non-virtual destructor [-Werror=non-virtual-dtor]
|
Would really love to see this merged, is there anything blocking it that we could help? |
…nd solved a trivial conflict. Signed-off-by: Timo J. Rinne <[email protected]>
…uild_fix build fix for the agent forwarding branch
|
I'm curious about what others are doing to work around the lack of agent forwarding in Mosh. Personally, aside from waiting for this to merge, I'm using a parallel ssh connection when I need my agent to be running on the remote; predictable SSH_AUTH_SOCK location helps with sharing it with Mosh when I need to resume my work flow in my Mosh session. |
|
@yannk i run screen on machine i mosh to, and so the agent runs there, i also use keychain to keep |
|
Is there a reason that this isn't merged yet? Anything we can do? |
|
I would like to use this patch as well. Is it not merged yet due to security implications? |
|
@rinne looks like there are merge conflicts again. :-( |
…nd resolved conflicts.
|
Every time I come to check the status of this I get sad. This is the single thing that stops me recommending mosh to my entire team, and spreading it through organizations I work with. Almost every one of them uses a bastion host. |
|
Could the maintainer of mosh seriously consider merging this, based on the feedback of users? I've been subscribed to this thread and #120 for a few years now, and I have not heard many complaints as to why this shouldn't be in. It's been an overwhelming plea to get this merged and released so we can use mosh more broadly. |
|
I've started a bounty for this bug, because money talks, and put in $50 to get it rolling, I hope. https://www.bountysource.com/issues/1587475-ssh-agent-forwarding @cgull: Have a cookie! 🍪 |
|
I wonder if this is the record for longest open merge-able PR (especially considering this is the second attempt). |
|
IMO the best remaining solution is a rename of @rinne's branch. Perhaps |
|
Hi all, I just found this thread and tried compiled source on server and client, changed my .ssh/config to: and mosh to server with agent-forward option, but I didn't see any process running on server listen to the 9997 port how can I fix the issue? |
|
Can someone take the initiative to describe the current state of affairs to a newbie like me? Will this allow me to use sshfs over mosh? If yes, why hasn't this been merged yet? |
|
@nikhilweee No. This patch is about SSH agent forwarding and is not relevant to any other kind of forwarding (port, filesystem, X11, or otherwise). We have other open feature requests for those kinds of ideas. An sshfs analogue has been requested as #280, but as far as I know, nobody has tried to make it into anything more than an idea. If you don’t have a specific requirement for SSH agent forwarding (you would know it by that specific name if you do), this PR is not what you’re looking for. |
|
Is anyone publishing standalone patches for released mosh versions with changes from this branch/pull req? |
|
I have rebased the pull request on top of current master: https://github.com/Mic92/mosh/commits/agent-forwarding |
|
Why isn't it merged yet? |
|
After reading most of the discussions (#120) and reading about Guardian Agent (https://github.com/StanfordSNR/guardian-agent), I think you should re-consider merging this PR, as we live in different times now. For me and many others, my local "ssh-agent" is 1password or another password/secret manager, which asks every time my ssh key will be used if I want to allow that. It is an improved Guardian Agent. Besides that, if people want to spread their insecure ssh-agents, it's up to them, there may be good reasons for them not to care, but I don't think the software should enforce something on them. The lack of agent forwarding in mosh forces me to now, after years of enjoying mosh, use ssh since I started using a password/secret manager instead of local ssh keys and copying them around computers and internet-exposed contexts (servers). |
|
Also need this patch, without is mosh for us useless. :/ |
Hi
SSH agent forwarding patches re-merged as one patch.
A couple of superficial conflicts resolved. Tested and working.
Merged as requested in #583 but since #583 was made by someone else, I decided just to make another PR.
Signed-off-by: Timo J. Rinne [email protected]