The sudo feature

Wed Dec 14 06:52:59 UTC 2005

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Dec 13, 2005, at 3:52 AM, David Miller wrote:

> I discussed this on IRC a bit tonight, but mentioning it here as  
> well for a little wider audience.
>
> The sudo feature is making me damn nervous.  Just the kinds of bugs  
> I'm seeing going by, and the particular places in the code that are  
> getting touched by the patches fixing them.  The authentication  
> code is getting hacked to hell and back, and I'm just scared we're  
> going to wind up introducing more security bugs that we haven't  
> even thought of.

	I have remained pretty quiet throughout this discussion, and most of  
what I would say has been said already.  However, I do want to say a  
few things now.  Although this may turn out to be false, I think that  
much of the suspense over this feature is a bit overdone.  If you  
look at all of the patches related to user impersonation, Bugzilla is  
not touched in as many areas as you would think.  In fact, up to now  
only one area of Bugzilla has needed to be modified to work with this  
feature (user prefs, see bug 313679).  Most of the bugs only touch  
code introduced by bug 204498 (the initial implementation of user  
impersonation), and many of the bugs address issues that (depending  
on your views) are not critical issues.

	Now that I mention it, I am not sure if anyone has actually come up  
with a list of bugs that were filed as a result of the landing of bug  
204498.  Since it might be a good idea to do that, I am now going to  
provide a list of bugs that were filed as a result of the  
introduction of user impersonation, along with a description of what  
(in my view) the bug actually affected.  To come up with the list, I  
searched for Bugzilla bugs that were RESOLVED after 2005-10-13 (when  
bug 204498 landed), where I was the assignee, the reporter, or on the  
CC list.  That gave me a number of bugs, including the following (the  
URLs appear after my signature at the end of the email):

Bug 204498: Add su (setuser/sudo/impersonate) function
	This, of course, is the big one.  However, it does not seem as big  
as expected for the impact it produces, especially when you consider  
that more than a third of the patch is template code (with  
appropriate filters) and documentation.  In fact I do not actually  
touch any of the authentication code at all (that is, I did not  
modify any files in the Bugzilla::Auth space).  The code that does  
the work is in relogin.cgi and Bugzilla.pm (the login subroutine).   
Checksetup was touched to define two new system groups (with  
predefined membership), and Bugzilla::Template was touched to define  
a new template directive function.

	The code in Bugzilla.pm is relatively straightforward.  I have a new  
function to return the identity of the impersonator, and I have a new  
function to convert the current session into an sudo session (used  
when sessions are being started/ended).  The login sub experienced  
the biggest change, as I now begin an impersonation ONLY if a number  
of important tests are passed (which should be pretty clear from the  
code and the comments in the bug).  I do not even attempt to begin/ 
resume a sudo session if the user is not authenticated.  I also knew  
that many people would be interested in this, so I tried to comment  
liberally, and to keep the POD up-to-date.

Bug 312386: Checksetup fails to check for existing group_group_map  
for new admins
Bug 312406: Checksetup fails to check for existing group_group_map  
for Bugzilla <2.17
	These two bugs came about as a result of the landing of bug 204498,  
so I thought I'd mention them here.  It appeared that bug 204498 was  
the first time that someone had created a new system group and  
immediately set up initial group memberships.  This caused checksetup  
to crash in certain situations.  These two bugs fixed that problem  
(and led me to adopt the practice of running the full checksetup  
tinderbox test suite on all patches I am about to check in).

Bug 312437: Do not display the "Impersonate this user" link when the  
user is in the bz_sudo_protect group
	This is a bit of a convenience thing, really.  If you try to  
impersonate a user who is protected from impersonation, an error will  
be thrown (this has always been true).  By removing the "Impersonate  
this user" link from the pages of protected users, the end result is  
the same (you are not able to begin a session), but it is brought on  
a few steps earlier.

Bug 312439: The user being impersonated has "moral" rights to keep  
informed
	This was a simple enough bug to address, but raises moral issues  
that are not so simple.  Maybe it is the moral issues that make this  
bug so problematic?

	There is support for not providing notification:  When you use the  
`su` command in *nix to log in as someone else, should they be  
notified (yes, I know that this is not *nix)?  When you remove a  
Bugzilla user from one or more groups, should they be notified?  I  
decided to err on the side of notification, since it is much easier  
to deactivate the feature (by commenting out some code) than it is to  
properly implement the feature from scratch.  Hopefully the fact that  
this can be easily disabled is enough to mollify the people who would  
prefer to see this bug marked WONTFIX or WORKSFORME.

	But how complex was this patch?  As with bug 204498, a good-sized  
portion (in this case, at least half) of the patch was to  
documentation and templates.  The CGI code in the patch was limited  
to taking the user-provided reason, trimming it down to a reasonable  
length for a URL, running it through a template, and sending it to  
the user.  The templates included a new plain text template for the  
email, a modification of the sudo start page to ask for a reason, and  
the introduction of a new page.cgi "page" explaining what this  
feature is.  You can actually see the "page" in action at <http:// 
landfill.bugzilla.org/bugzilla-tip/page.cgi?id=sudo.html>.

Bug 312441: relogin.cgi allows you to impersonate user accounts you  
are not allowed to see when 'usevisibilitygroups' is on
	This bug did two things:  It streamlined the initial access checking  
when sessions are first started, and it includes a visibility check  
to make sure that the impersonator can see the user being  
impersonated.  The streamlining looks pretty messy in patch form, but  
looks nice when applied.  The visibility check is, in my mind, not  
really needed, as it should not even need to use visibility  
restrictions if you are trusted enough to be given access to this  
feature.  However, as has been noted in the bug, this is needed when  
you have many disparate groups of users on the same installation.   
Again, I decided to go with the bug, knowing that the visibility  
check could be turned off in code.

Bug 313677: Sudoers without editusers/bless access see no link to  
sudo page
	This was a templates-only change.  Normally sudo sessions would be  
started by a link ("Impersonate this user") that appears when you  
view the information for a particular user in editusers.cgi.   
However, you need appropriate access to get to that page, and it's  
possible to not have appropriate access but still have sudo access,  
so I needed to provide a link somewhere else.

	Is this UI creep?  Maybe.  I agree with Myk and Dave (in comments 3  
and 4 of the bug) in that it would be nicer to modify editusers to  
provide a "low-power" version for people who can use the sudo feature  
but who do not have editusers membership.  However, given the current  
mood surrounding this feature, I am too scared to even file a bug on  
this, let alone submit a patch.

Bug 313679: Changing email address in sudo mode logs user in as  
impersonated user
	This is another simple change to counteract a seemingly-big  
problem.  In fact, my original solution was big, before Marc found a  
very elegant solution to the problem.  It may seem that making  
Bugzilla ignore the login/password provided by CGI is not a good  
idea, but the parameters are restored after login, and editusers code  
makes sure that the password entered in the HTML form is checked  
against the encrypted password in the database.

> At this point, I'm kind of regretting ever approving that feature.   
> But it's a bit late for that now I guess.  Just everyone keep a  
> vigilant eye out on that code...  pound the heck out of it, and  
> let's make sure it really is safe.

	As has been said already, it's not too late to back it out now, and  
it could be done (though it would take more than executing a few CVS  
commands).  However, I do not think it should be backed out.   
Hopefully my comments above gave you a better idea of the scope of  
the sudo-related bugs that have appeared since bug 204498 landed.

	This new feature will be going through the normal QA testing process  
for 2.22, and will also be made available to everyone who decides to  
try out a 2.22 Release Candidate, not to mention all of the people  
who may be using it now.  Will there be bugs with this feature?   
Yes.  Will there be security bugs because of this?  I certainly hope  
not, as I have been keeping security in my mind while working on  
this, but I stand ready to address questions as they come up (and as  
I have been doing on IRC), and I stand ready to work on all bugs  
found with user impersonation (be they security bugs or not).

	Later!

===================
|  A. Karl Kornel
|
| karl at kornel.name
===================

Bug 204498: https://bugzilla.mozilla.org/show_bug.cgi?id=204498
Bug 312386: https://bugzilla.mozilla.org/show_bug.cgi?id=312386
Bug 312406: https://bugzilla.mozilla.org/show_bug.cgi?id=312406
Bug 312437: https://bugzilla.mozilla.org/show_bug.cgi?id=312437
Bug 312439: https://bugzilla.mozilla.org/show_bug.cgi?id=312439
Bug 312441: https://bugzilla.mozilla.org/show_bug.cgi?id=312441
Bug 313677: https://bugzilla.mozilla.org/show_bug.cgi?id=313677
Bug 313679: https://bugzilla.mozilla.org/show_bug.cgi?id=313679
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Darwin)

iQEVAwUBQ5/BTjjU38yKmHhCAQL4mQf/VAlsgDHWOT+/zWhlnhGRC2uKLoiMCn1t
Y89+bBOIpcpnfECKnH9YzxwdZTeFnOTfLGzEWViYKKzrD/NxVUgcZyJ1+F8w0XCc
vaIg/6oztLDBzya9wvTnUCRwmj+9S+7J1rEv0ct3otRlYrbf8zFpamW+fA/sQi17
wE1mB6KOYsdAjkOqmwqrYbc/u2teZVh8TaIWK79hUl2YfrYTTNLV8o/iJmIgxcJw
vQWNonNYYcHyUkBUEEC+SzorqxCcZyalZBAuk6jGORrrguM7dOTmxGs6h6miGYoY
5qBMMLFNwxS7OLKGIND5+Cfdimudl3ZwwGl5U4+kibyKxCNfDjgSXQ==
=7JXI
-----END PGP SIGNATURE-----