204498 - Add su (setuser/sudo/impersonate) function

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Joel Peshkin]

For debugging, report generation, and user support, it is useful for a
sufficiently privileged administrator to be able to "become" another user.

This could be accomplished by defining a "sudoers" group who would get a link to
"su" to another user.  When a user "su-does" to another user, they would get an
additional cookie ("su as") in addition to their OWN login tokens.  When a
browser presents the "su as" cookie in addition to an otherwise valid
credential, the authentication mechanisms could validate the login, then confirm
that the account is a "sudoer" and return the user information for the target
account, and log the event.  

The only additional change needed is to set a flag indicating that a setuser
mode is in effect and to change relogin.cgi to just delete the "su as" cookie.

Comment 1

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 2

[A. Karl Kornel]

Attachment: Patch v1

OK, here's some high-level overview.

There are 2 new groups: sudoer and sudo_protect.  People in the sudoer group
can use this feature.  People in the sudo_protect group are protected from
impersonation.	Administrators are included automatically in the sudo group,
and everyone in the sudo group is automatically included in the sudo_protect
group.	Only administrators are explicitly allowed to add/change group
membership for these 2 groups.	The sudo cookie holds the unique ID of the user
being impersonated.

Bugzilla.pm has a couple of important changes.	First of all, $_sudoer is
added, to contain the Bugzilla::User object of the person who initiated the
session.  When a session is in progress, $_login holds the object of the person
being impersonated (so most CGIs don't need to be touched).  There's also an
sudo subroutine to return the value.  A new subroutine, sudo_request, is
created to update $_sudoer and $_user for the times when sessions are being
started or ended.  The login subroutine is updated to check for a valid cookie,
and to modify the intial values of $_user and $_sudoer if needed.

checksetup.pl is modified to add the 2 new groups.  editusers.cgi is updated to
not allow non-admins to change group membership for the sudoer and sudo_protect
groups.  relogin.cgi holds most of the logic for beginning and ending sudo
sessions.  Template.pm is updated to include a new variable, sudoer, that
returns Bugzilla->sudoer (see also the definition of 'user').

The template admin/users/edit.html.tmpl is updated to remove the checkboxes in
the sudoer/sudo_protect groups if the logged-in user is not an admin. 
global/messages.html.tmpl is updated to include 2 new messages, one saying
"session started" and one saying "session ended". 
global/useful-links.html.tmpl is updated to change the Log In line: If a
session is in progress, a notice appears and an "End Session" link is provided.
 If the user is an sudoer and there is no session in progress, a "Begin
session" link is provided.  global/user-error.html.tmpl is updated to include
the different errors that may be raised.  Finally, a new template
(admin/sudo.html.tmpl) is created to display the "begin an sudo session" page. 
I'm especially open to changes (markup or content) for this page!

Comment 3

[Joel Peshkin]

This is pretty neat.  I have just started to play with it.

A few comments. 
1) The format of the footer is a bit verbose both normally and while
impersonating.  How about just "Sudo" instead of "Begin Sudo Session" and some
shorthand for the "Logged in as [e-mail] -- sudo session in progress
(impersonating [e-mail])" such as "[e-mail] (impersonating
[e-mail])"

2) I may have missed it, but is there really any additional logging being done?
 I think we should log the start and stop of the sudo mode.  If I could come up
with a clean way to do it, I would make the activity log distinguish between
sudo actions and non-sudo actions.  So far, I have not thought of a clean way to
do it.

3) I'm not sure that the changes to editgroups really help anything.  Someone
with editusers privileges can always add himself to the admin group.  Anyone
else would have to have the bless privileges for the sudoers and sudo_protect to
make the change.  I think that should suffice.  What else are you trying to make
it do?

Comment 4

[Joel Peshkin]

Also...  make sure you pass runtests.  You have a bugwords violation in the patch.

Comment 5

[Joel Peshkin]

Comment on attachment 199028 [details] [diff] [review]
Patch v1

See preceding comments.  In general, this is a good approach.  I do like the
way you centralized the change so that it does not leave a bunch of potential
security holes.

This does need to be tested with environment variable authentication to make
sure nothing silly happens.  Also, I still need to try it with disabled
accounts.  (I assume we want to be able to sudo to disabled accounts even
though they could not log in themselves, correct?)

Anyway, please fix the items I noted and I think we are then landing this will
be just a matter  of some more testing.

Comment 6

[A. Karl Kornel]

(In reply to comment #3)
> This is pretty neat.  I have just started to play with it.
> 
> A few comments. 
> 1) The format of the footer is a bit verbose both normally and while
> impersonating.  How about just "Sudo" instead of "Begin Sudo Session" and some
> shorthand for the "Logged in as [e-mail] -- sudo session in progress
> (impersonating [e-mail])" such as "[e-mail] (impersonating
> [e-mail])"

OK, I'll try this (text appears after the Log Out link):
 * For a user in the sudoer group, no session in progress: [e-mail] (sudo)
 * Session in progress: [e-mail] (<b>impersonating [e-mail]</b>, end session)

> 2) I may have missed it, but is there really any additional logging being done?
>  I think we should log the start and stop of the sudo mode.  If I could come up
> with a clean way to do it, I would make the activity log distinguish between
> sudo actions and non-sudo actions.  So far, I have not thought of a clean way to
> do it.

I had thought of this while I was writing the patch.  There are three places where logging could be 
done.  Two are in relogin.cgi (at the end of the 'end-sudo' and 'begin-sudo' actions) where you know a 
session is either starting or ending.  The other is in the login subroutine in Bugzilla.pm, where you 
know that a person has just accessed a page in sudo mode.  You could also hook in logging in the 
sudo_request method of Bugzilla.pm.  I left 3 comments in place (two in Bugzilla.pm, one in 
relogin.cgi/'end-sudo' action) to mark them.  In the next patch I will have a comment in the proper 
place for the 'begin-sudo' action.

If bug 299311 ever lands, that would be one option for logging.  The log messages could easily be 
prefixed by the string "SUDO: " (or something else) to make them easy to find.  The reason I'm simply 
making comments, instead of waiting for bug 299311 to land, is because it has been waiting on a 
review for a while now, and anyway there is no guarantee that this would be best.

> 3) I'm not sure that the changes to editgroups really help anything.  Someone
> with editusers privileges can always add himself to the admin group.  Anyone
> else would have to have the bless privileges for the sudoers and sudo_protect to
> make the change.  I think that should suffice.  What else are you trying to make
> it do?

I'm mainly trying to make this as hard as possible to make the change.  Also, with the code I have in 
place it should not be too hard to remove this little loophole, probably by adding the following code 
into can_change_access in editusers.cgi:

> # For group 'admin':
> # * Only administrators may change group membership
> if ($group_name eq 'admin' && !Bugzilla->user->in_group('admin')) {
>     return ({ group => 'admin',
>               action => 'modify',
>               object => 'group_access' }
>     );
> };

Comment 7

[Joel Peshkin]

(In reply to comment #6)

> 
> > 3) I'm not sure that the changes to editgroups really help anything.  Someone
> > with editusers privileges can always add himself to the admin group.  Anyone
> > else would have to have the bless privileges for the sudoers and sudo_protect to
> > make the change.  I think that should suffice.  What else are you trying to make
> > it do?
> 
> I'm mainly trying to make this as hard as possible to make the change.  Also,
with the code I have in 
> place it should not be too hard to remove this little loophole, probably by
adding the following code 
> into can_change_access in editusers.cgi:
> 
> > # For group 'admin':
> > # * Only administrators may change group membership
> > if ($group_name eq 'admin' && !Bugzilla->user->in_group('admin')) {
> >     return ({ group => 'admin',
> >               action => 'modify',
> >               object => 'group_access' }
> >     );
> > };


Don't do that.  Only administrators wil have editusers.  You must rely on the
fact that anyone who has global permissions to bless all groups is supposed to
be able to do so.

Comment 8

[A. Karl Kornel]

Attachment: Patch v1.2

Modification of attachment 199028 [details] [diff] [review] with respect to comment 3, comment 4, and
comment 5:

The footer text has been modified (see comment 6, specifically my reply to
comment 3 point 1).

(In reply to comment #4)
> Also...  make sure you pass runtests.  You have a bugwords violation in the
patch.

One bareword (Bugzilla) converted to template term.

(In reply to comment #5)
> This does need to be tested with environment variable authentication to make
> sure nothing silly happens.  Also, I still need to try it with disabled
> accounts.  (I assume we want to be able to sudo to disabled accounts even
> though they could not log in themselves, correct?)

I haven't tried this with environment variable authentication, but I think it
might just mean that the forced login would not take place.  As for disabled
accounts, you can impersonate disabled users.

Comment 9

[A. Karl Kornel]

Attachment: Patch v1.4

Modification of attachment 199035 [details] [diff] [review]:

All attempts to keep editusers members but not admins from changing their
sudoer membership have been removed.

Bugzilla::Error now includes the sudoer login next to the user login, if a
session is in progress and something is logged.

Docs have now been added.  Once I get a review+ from joel I'll ask for review
from documentation@.

Comment 10

[Marc Schumann [:Wurblzap]]

From the new whining groups, I was under the impression that new system groups
now get a bz_ prefix. Am I wrong?

Comment 11

[Joel Peshkin]

Yeah.  It is not official, but probably a good idea.  Karl - on the next rev,
let's change the group names to bz_sudoers and bz_sudo_protect or some such bz_
prefixed names.

Comment 12

[A. Karl Kornel]

Attachment: Patch v1.6

Modification of attachment 199040 [details] [diff] [review] with respect to comment 5 and comment 11:

(In reply to comment #11)
> Yeah.  It is not official, but probably a good idea.	Karl - on the next rev,

> let's change the group names to bz_sudoers and bz_sudo_protect or some such
bz_
> prefixed names.

Updated.

(In reply to comment #5)
> This does need to be tested with environment variable authentication to make
> sure nothing silly happens.  Also, I still need to try it with disabled
> accounts.  (I assume we want to be able to sudo to disabled accounts even
> though they could not log in themselves, correct?)

I now query the authentication modules.  If I am told that the user can logout,
the note is displayed.	If the user cannot log out, the note is hidden.

Again, requesting review from joel, with review from documentation@ after that.

Comment 13

[Dave Miller [:justdave]]

I don't like the idea of making this something blatently available (and visible
on screenshots that administrators might make when making internal
documentation, etc).  Can we get the link out of the footer, and instead add an
"Impersonate this user" link to editusers when viewing that user?  It seems to
me that would be the logical place for someone to go who was looking for this
kind of thing anyway...

Comment 14

[Dave Miller [:justdave]]

btw, I have no objection to indicating in the footer when sudo is actually being
used, I just want the link to activate it a little more buried. :)

Comment 15

[A. Karl Kornel]

Attachment: Patch v1.8

Modification of attachment 199177 [details] [diff] [review] with respect to comment 13:

> I don't like the idea of making this something blatently available (and
visible
> on screenshots that administrators might make when making internal
> documentation, etc).	Can we get the link out of the footer, and instead add
an
> "Impersonate this user" link to editusers when viewing that user?  It seems
to
> me that would be the logical place for someone to go who was looking for this

> kind of thing anyway...

Updated.

Requesting review from joel.

Comment 16

[A. Karl Kornel]

Attachment: Patch v1.9

Modification of attachment 199388 [details] [diff] [review]:

Logging out did not deactivate an sudo session if one was in progress.	My
fault, sorry 8-/

Requesting review from joel.

Comment 17

[Joel Peshkin]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.22; previous revision: 1.21
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.441; previous revision: 1.440
done
Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.15; previous revision: 1.14
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.35; previous revision: 1.34
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <-- 
administration.xml
new revision: 1.52; previous revision: 1.51
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v
done
Checking in template/en/default/admin/sudo.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v 
<--  sudo.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/admin/users/userdata.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v
 <--  userdata.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v
 <--  messages.html.tmpl
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.42; previous revision: 1.41
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.132; previous revision: 1.131
done

Comment 18

[A. Karl Kornel]

Some release note notes: bug 312439 and bug 312441 mean that (bug 312439) the
impersonated user will know that a session has started, and can be displayed a
message from the impersonater explaining why the session was started; also (bug
312441) visibility groups will be honored if they are being used.

Comment 19

[Dave Miller [:justdave]]

adding common synonyms to the summary to make this easier to find

Comment 20

[Kevin Benton]

Before I get into my comments regarding the patch, I want you to know that I'm clearly on the "in-favor-of" side of the intent of this patch.  Having said that, I think this patch should be repealed.  Here's why:

I believe we need to make the activity log updates show updates were done by so and so on behalf of someone else.  That way, the admin doesn't truly impersonate, but can do things "on behalf of" the user.  I see this as a security risk because it prevents Bugzilla from being able to differentiate between users updating things for themselves versus admins being able to do things as if the user had done them.  If the admin is in a bad mood or ..., this just makes it that much easier to make someone look bad.  We can't stop admins from hacking the DB manually, but we sure can do a lot to make it harder for them to hack with mallicious intent from the UI.

At this point, due to the method of implementation, I suggest we seriously reconsider the release approval for this patch.

Comment 21

[Vlad Dascalu]

"We" can't reconsider the release approval for this patch.

Comment 22

[Joel Peshkin]

Let's keep this simple.

1) File a bug requesting that administators not be placed in the group by default and that there be a serious warning on the page that it should not be enabled casually.  If you really insist, you can add a param.

2) I agree with the point on the activity logging.  We should add a mechanism (new column probably) to indicate who ACTUALLY did a function.  That would be easy.

3) Don't enable this on BMO or on any other system where you don't trust the same people who have the database password not to falsify the record.

4) Please consider filing a security bug because a user who has access to the database password could revise the bug activity log. ;-)

Comment 23

[Kevin Benton]

(In reply to comment #22)
> Let's keep this simple.

Yes, I agree.

> 1) File a bug requesting that administators not be placed in the group by
> default and that there be a serious warning on the page that it should not be
> enabled casually.  If you really insist, you can add a param.

Frankly, I think that bug should block this one.  I just filed it.

> 2) I agree with the point on the activity logging.  We should add a mechanism
> (new column probably) to indicate who ACTUALLY did a function.  That would be
> easy.

Cool :)  That's the main reason I asked for this.

> 3) Don't enable this on BMO or on any other system where you don't trust the
> same people who have the database password not to falsify the record.

Based on #1 above, it seems this has already been enabled if the site is running TIP.

> 4) Please consider filing a security bug because a user who has access to the
> database password could revise the bug activity log. ;-)

LOL :)  I think you got my point ... :)

Comment 24

[A. Karl Kornel]

(In reply to comment #23)
> 
> Frankly, I think that bug should block this one.  I just filed it.
> 

I'm not sure that it should be a blocker, simply on procedural grounds.  This bug has already been "resolved", so any bug this depends on should also be appropriately closed.  However, if the decision is made to back this out, and others feel that the dependency is valid, then the dependency should be returned.

(for future quick reference, the "just filed" bug is bug 320177)

> > 2) I agree with the point on the activity logging.  We should add a mechanism
> > (new column probably) to indicate who ACTUALLY did a function.  That would be
> > easy.
> 
> Cool :)  That's the main reason I asked for this.

I have no problem with logging, I just could not think of a good place to log sudo-related stuff.  Do I want to augment the bug activity page?  Do I want to make up a completely new combination of CGI, DB tables, and template code to track and display these actions?  Or do I want to do something similar to the logging of profile activity, where I log stuff but do not actually provide a way to view it (outside of direct database queries)?  Or maybe I should log it to a file?

In my personal opinion Bugzilla would do well with a more unified logging system, and using it to log actions taken in an sudo session would be a good place to start.

> > 3) Don't enable this on BMO or on any other system where you don't trust the
> > same people who have the database password not to falsify the record.
> 
> Based on #1 above, it seems this has already been enabled if the site is
> running TIP.

Correct, but remember that administrators, AND ONLY administrators, are initially granted sudo access.  In addition, administrators are (by default) protected from impersonation, and you can easily augment this list so that the number of protected users is great while the number of sudoers is small.  Of course anyone with editgroups and/or editusers will be able to modify this on a person-to-person or system-wide level (as is true for any special access right that is assigned using group membership).  So I'd augment #3 by also saying "Don't give editgroups or editusers memberships to anyone that you do not ultimately trust".

Comment 25

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960.