Code review comment for lp:~raphink/byobu/acl

Hi Dustin,

2010/4/24 Dustin Kirkland <email address hidden>

> Review: Needs Fixing
>

Yep, I totally agree that it needs fixing.

First of all, thanks a bunch for the branch. I think it's a great idea to
> expose and make screen sharing easier for Byobu users.
>
> As for the implementation, I'm not sure putting all of this into
> byobu-config is the best way to go about it. Let's discuss it a little.
> (I'm just now getting Lucid behind me, and opening the byobu tree for
> Maverick).
>
> What do you think about creating two new utilities, /usr/bin/byobu-share
> and /usr/bin/byobu-unshare, that toggle the acl's on and off?
>
>
Well as you probably noticed, I haven't touched this branch for a while. The
reason is that I couldn't properly access the acls that screen knows about
at a given time. This is a known bug/feature request in screen, that has
been in the whishlist since about 10 years (from reading the changelog on
this subject).

As a result, the changes I've made to byobu allow me to set acls in screen
(provided the setuid root bit is set on the binary of course) but not to
modify them or even display the currently set acls. I came up with a model
that stored the acls that byobu created so that I could find them and modify
them, but there are several issues with this way of doing things:
 * if the users use the builtin acl commands in screen, I have no idea what
they changed and I can't display the new acls ;
 * I currently can't properly support multi-session, which means byobu
thinks the set acls for a session applies to all sessions even though
they've never been applied there ;
 * I don't flush the known acls when the session stops, so when a new
session is started, byobu thinks acls are already applied when they are not.

All this to say that really this feature needs a patch in screen itself, to
allow accessing the current acls somewhere (and not in a non-parseable pager
like screen often displays info). I've begun looking at the code for acls,
but my C skills are very poor, and the code is quite complicated I find.

> As an Ubuntu developer, I'm a little concerned that the security team might
> not like such a package in Ubuntu Main (but I'll try to deal with that
> separately from Byobu as an upstream project).
>
>
I agree entirely with this. Perhaps there could be a debconf question in the
screen package to ask users if they want screen set with a setuid root bit.
After all, this feature is not only useful for byobu, and dealing with it in
debconf would allow to do it cleanly with packaging tools, when most users
would override the setuid bit manually.

I'd be happy to discuss this with you some time soon.

Raphael