[PATCH] X11MaxDisplays for multi-factor X11 gateways

2018-09-26 04:51:29 UTC

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

Post by AG
Hello,
Please accept this as my quarterly nag regarding the possibility of
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.
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
https://bugzilla.mindrot.org/show_bug.cgi?id=2580)