[PATCH] X11MaxDisplays for multi-factor X11 gateways

[AG]

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 at mzpqnxow.com> 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)
>