Sorry for re-opening a 2+ year old thread ( https://lists.mindrot.org/pipermail/openssh-unix-dev/2016-June/035125.html) but the patch I submitted for the X11MaxDisplays feature/fix (h ttps://bugzilla.mindrot.org/show_bug.cgi?id=2580 <https://bugzilla.mindrot.org/show_bug.cgi?id=2580>) was near set for the release into 7.3 (or 7.4?) but did not make it in-time as it required a longer review, per a note off-list from Damien, which was understandable. However, the simpler one I submitted within the same timeframe, wildcard host support for PermitOpen ( https://bugzilla.mindrot.org/show_bug.cgi?id=2582) did make it upstream. I'm now working in another environment where the X11MaxDisplays feature is not as much of an issue but it seems it would be silly to let a feature that is useful for multi-factor X11 forward gateways drop to the floor, especially since a few of us spent time reviewing the use-case and cleaning up the final bits of the patch to the code and the man page. I think this *may* have just fallen off the radar after the release including the PermitOpen patch. I didn't notice until now because RHEL did in fact backport it into their OpenSSH package around that time, but it never went upstream. Is this something that can be looked at and merged for the next upstream release? The bugzilla bug is still open as referenced above and contains a patch that both Jakub and I QA'd. It includes a man page entry and all the usual things one would expect (white space and all) and though I haven't checked carefully in the source, I don't think there will be any merge conflicts (at least not any major ones, specifically due to the new features having been added in the past 1-2 years. Very easy to clean up, and may even apply. While I do not see any users clamoring for this feature now on the list, I would hate to see the effort go to waste, and seeing as there seemed to be a consensus that it was a reasonable feature- or rather, it was somewhat unreasonable not having it, can we follow through and get it upstream? I should note there is a bit of a downside to the gap between the RHEL adoption and the lack of adoption into upstream- many online man pages seems to be mirroring RHEL/CentOS man pages, which could be a bit confusing to Debian/Ubuntu users. Though I doubt it's a big problem, it is a quirky side-effect :) I'm happy to go back and fix any merge conflicts and update the bugzilla if it can still be merged, if not I will close the issue. Thanks for the help in getting the PermitOpen patch committed, hopefully we can put this one to rest one way or another (one way preferred over the other of course) Thanks, AG On Sat, Sep 24, 2016 at 5:04 PM AG <openssh@xxxxxxxxxxxx> wrote: > Hello, > > Please accept this as my quarterly nag regarding the possibility of > merging the patch submitted back in June to the mindrot Bugzilla @ > https://bugzilla.mindrot.org/show_bug.cgi?id=2580 upstream. It has > already been reviewed and cleaned up slightly by Jakub Jelen, and > documentation has been added. > > To summarize, this removes the arbitrary limit in channels.c: > > channels.c:155:#define MAX_DISPLAYS 1000 > > It instead makes it an option in sshd_config, X11MaxDisplays, which > keeps the defaults at 1000, in order to avoid any impact for users not > needing to tune it. This feature allows a sysadmin to make an educated > decision on what is required for their particular system, whether it > is higher or lower than 1000. Additionally, style-wise, it takes a > completely arbitrary number out of the source code. Admittedly, 1000 > is plenty for the majority of cases- but it is still completely > arbitrary. It might be too high for some cases (I hadn't thought about > that) but it is certainly too low for my use case. > > The use case for the patch: systems that are using OpenSSH as a > mulit-factor authenticated X11 gateway into a trusted network. > Currently, the hardcoded value causes there to be an effective maximum > of (1000-X11DisplayOffset) displays forwarded, assuming no other > applications are binding the loopback TCP ports between > 6000+X11DisplayOffset and 6000+MAX_DISPLAYS on the system. We aren't > talking about a box that provides interactive shells here- this > infrastructure is purely an X11 gateway providing a strong PAM stack- > so it can handle well beyond 1000 displays (I hand compile it today > with this patch) and I support well over 5000 users, who utilize > multiple forwards simultaneously- so it's a must for my environment. > > The patch also "corrects" the logic a bit by changing the logic to try > bind() across the range from (6000+X11DisplayOffset) through > (6000+X11DisplayOffset+MAX_DISPLAYS[1]) as well as changing the > integer 6000 (which is the minimum X11 display offset port, well known > to technical users) from a magic number in the loop to a #define that > is a bit more descriptive than the integer value alone in the source > code, for readability. > > In any event, I'd appreciate an eyeball on this before the next > OpenSSH release if someone has the time. The patch is not large, but > it is not tiny. It is however VERY boilerplate. > > It seems to be one of the few _arbitrary_ magic numbers in the OpenSSH > code and seems like it should be configurable- and for my own selfish > reasons (use in my own infrastructure) I would love to see this merged > sooner rather than later. It's been in use in a production environment > for 3+ years, and could be a quick review and merge for someone will > to take the time. > > Thanks folks, sorry for the nudge, I know everyone's busy. > > AG > > (No patch attached, please see patch @ > https://bugzilla.mindrot.org/show_bug.cgi?id=2580) > _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
