Now that we have the excellent new session multiplexing facility, I have various users (including myself) who'd like to use it to make their use of ssh more efficient in an environment where we have to ssh ProxyCommand through a bastion host in order to get to other systems in a datacentre. The obvious implementation of this is to set 'ControlMaster auto' on the configuration stanza for the bastion host, which works well up to a point. However, each sshd only supports 10 sessions (MAX_SESSIONS), which makes this rather inconvenient. I realise that the functions in session.c do linear searches through the sessions array etc., but 10 seems a bit small nowadays with good session multiplexing support. Could this be raised (64 or so?), and perhaps made into a configuration parameter as well?
Created attachment 963 [details] proposed patch to add MaxSessions config parameter This patch makes MaxSessions a config parameter, default of 64, and grows the table one entry at a time, to minimize memory usage. If MaxSessions is set to 0, then there is no limit. It does not appear (at first glance) that any of the session_* functions are performance criticial, so I didn't work on removing the linear search.
Comment on attachment 963 [details] proposed patch to add MaxSessions config parameter Personally, I have no objection to this in principle. Some comments on the patch: >+ num_sessions=1; I'd be tempted to allocate them in blocks (eg of 8) to avoid excessive reallocs but that's not critical. >+ sessions=calloc(num_sessions,sizeof(sessions[0])); Since sessions is initialized as NULL, could you use xrealloc (which is guaranteed to be happy with xrealloc(NULL, size)) rather than calloc/realloc to simplify this? >+ Session *n=realloc(sessions,++num_sessions*sizeof(Session)); >+ if (!n) >+ return NULL; If the realloc fails you will have already incremented num_sessions, so the next new channel will overflow the array bounds. Along similar lines: it's unlikely but what's to prevent num_sessions exceeding INT_MAX and wrapping when MaxSessions=0 and you have gobs of memory? >+ sessions=n; >+ s=sessions+num_sessions-1; The rest of the function uses array syntax, it's probably easier to follow if you stick to that. "s = sessions[i]" would be right, no? >+.It Cm MaxSession Should be "MaxSessions"?. There were a few style nits too but they're not a big deal.
(In reply to comment #2) > Since sessions is initialized as NULL, could you use xrealloc (which is > guaranteed to be happy with xrealloc(NULL, size)) rather than calloc/realloc > to simplify this? OK, that's a bad idea. xrealloc will fatal() if the allocation fails, realloc allows graceful failure by refusing to allocate the new session.
Comment on attachment 963 [details] proposed patch to add MaxSessions config parameter >+ num_sessions=1; >+ sessions=calloc(num_sessions,sizeof(sessions[0])); Also: it doesn't check if calloc failed.
Created attachment 979 [details] rework patch based on comments #2 and #3. One other thing: MaxSessions=0 could have a legitimate use (eg to permit port forwarding only and no shells) so that might not be the best choice to mean "unlimited".
Any thoughts on incorporating the patch (id=979)to the next OpenSSH release?
Created attachment 1473 [details] adds MaxSessions knob to daemon configuration
Ideally we should permit a "MaxSessions unlimited" that allows creation of as many sessions as there are available file descriptors to serve them. This would require dynamic allocation of the channels list (perhaps with some protection against fragmentation as channels close), similar changes to the channels list and an audit of both the session and channels code to ensure that they fail gracefully when they run out of file descriptors.
(In reply to comment #8) > Ideally we should permit a "MaxSessions unlimited" that allows creation > of as many sessions as there are available file descriptors to serve > them. This would require dynamic allocation of the channels list Did you mean session list here? The channel list is already dynamic (however it uses xrealloc so an allocation failure will kill the connection). > (perhaps with some protection against fragmentation as channels close), > similar changes to the channels list and an audit of both the session > and channels code to ensure that they fail gracefully when they run out > of file descriptors.
Yes, s/channels list/sessions list/
Put this on the 5.1 list
Created attachment 1483 [details] Make sessions array dynamic, audit server for leaks and fatal errors on fd table full conditions This patch makes the sessions array dynamic, and fixes several fd leaks and fatal() errors that occur when out-of-fd conditions are hit in the server. It also adds a channel request confirmation handler to the client, so it can do something intelligent when a request fails (e.g. because the server is out of fds).
a modified version of this patch has been committed, and will be in OpenSSH-5.1 > CVSROOT: /cvs > Module name: src > Changes by: djm@cvs.openbsd.org 2008/05/08 06:21:16 > > Modified files: > usr.bin/ssh : Makefile.inc monitor.c monitor_wrap.c > servconf.c servconf.h session.c session.h > sshd_config sshd_config.5 > > Log message: > Make the maximum number of sessions run-time controllable via > a sshd_config MaxSessions knob. This is useful for disabling > login/shell/subsystem access while leaving port-forwarding working > (MaxSessions 0), disabling connection multiplexing (MaxSessions 1) or > simply increasing the number of allows multiplexed sessions. > > Because some bozos are sure to configure MaxSessions in excess of the > number of available file descriptors in sshd (which, at peak, might be > as many as 9*MaxSessions), audit sshd to ensure that it doesn't leak fds > on error paths, and make it fail gracefully on out-of-fd conditions - > sending channel errors instead of than exiting with fatal(). > > bz#1090; MaxSessions config bits and manpage from junyer AT gmail.com > > ok markus@
Mass update RESOLVED->CLOSED after release of openssh-5.1