Add root access check to cns server.

Add root access check to cns server.

We need to check that we can call traffic control commands with sudo -n
commands.

I added a check to quit cns gracefully if no root access is available.

BUG=109498
TEST=unit tests (traffic_control, cns).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117548

[shadi, 2012-01-07 04:32:36]

Dale, can you please review?

[DaleCurtis, 2012-01-09 19:08:01]

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:42: """Checks if
requirements are available to run traffic control commands.
s/requirements/permissions ?

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:48: if os.geteuid() ==
0:
Not quite correct. You want os.geteuid() == 0 _OR_ sudo -n <blah> passes.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:50: command = ['sudo',
'-n', 'iptables', '-L']
For short commands like these you might consider inlining them instead of
declaring a variable first. Up to you.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:53: command = ['sudo',
'-n', 'tc', 'qdisc', 'add']
For both of these it's enough to just run "--help" (or "-help" for tc). All we
need to know is that sudo will allow these commands to execute.

[shadi, 2012-01-09 23:47:44]

updated comments.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:42: """Checks if
requirements are available to run traffic control commands.
On 2012/01/09 19:08:02, DaleCurtis wrote:
> s/requirements/permissions ?

Done.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:48: if os.geteuid() ==
0:
Shouldn't the sudo -n <blah> pass if os.geteuid() passes? If it doesn't, don't
we want sudo -n to pass either way? 
On 2012/01/09 19:08:02, DaleCurtis wrote:
> Not quite correct. You want os.geteuid() == 0 _OR_ sudo -n <blah> passes.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:50: command = ['sudo',
'-n', 'iptables', '-L']
On 2012/01/09 19:08:02, DaleCurtis wrote:
> For short commands like these you might consider inlining them instead of
> declaring a variable first. Up to you.

Done.

http://codereview.chromium.org/9125022/diff/1/media/tools/constrained_network...
media/tools/constrained_network_server/traffic_control.py:53: command = ['sudo',
'-n', 'tc', 'qdisc', 'add']
I thought we want to try to run a command that *does* require root access. I
didn't know sudo -n checks for root password before trying to run a command :) 
On 2012/01/09 19:08:02, DaleCurtis wrote:
> For both of these it's enough to just run "--help" (or "-help" for tc). All we
> need to know is that sudo will allow these commands to execute.

[DaleCurtis, 2012-01-10 00:17:59]

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control.py:48: if not
os.geteuid() == 0:
Use != 0.

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control.py:49: _Exec(['sudo',
'-n', 'tc'], msg=('Cannot run traffic control commands. '
Add -help just to be sure.

Where is the iptables check? Should have a ['sudo', '-n', 'iptables', '--help'].
You need both tc and iptables.

[shadi, 2012-01-10 00:36:39]

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control.py:48: if not
os.geteuid() == 0:
I thought (from previous CL) 'not' is preferred over '!='
On 2012/01/10 00:17:59, DaleCurtis wrote:
> Use != 0.

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control.py:49: _Exec(['sudo',
'-n', 'tc'], msg=('Cannot run traffic control commands. '
My bad. I thought the purpose was to check if sudo -n works. I forgot this
checks if the commands exists as well :)
On 2012/01/10 00:17:59, DaleCurtis wrote:
> Add -help just to be sure.
> 
> Where is the iptables check? Should have a ['sudo', '-n', 'iptables',
'--help'].
> You need both tc and iptables.

[DaleCurtis, 2012-01-10 00:40:01]

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/4001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control.py:48: if not
os.geteuid() == 0:
On 2012/01/10 00:36:39, shadi wrote:
> I thought (from previous CL) 'not' is preferred over '!='
> On 2012/01/10 00:17:59, DaleCurtis wrote:
> > Use != 0.
> 

For objects and boolean statements, but usually not for a primitive type
comparison.

[shadi, 2012-01-10 21:31:02]

last review? :)

[DaleCurtis, 2012-01-10 21:35:07]

http://codereview.chromium.org/9125022/diff/10001/media/tools/constrained_net...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/10001/media/tools/constrained_net...
media/tools/constrained_network_server/traffic_control.py:50: msg=('Cannot run
traffic control \'tc\' command. '
Better written as "Cannot run 'tc' command. Traffic Control must be run as root
or have password-less sudo access to this command."

[DaleCurtis, 2012-01-10 22:00:51]

LGTM, +fischman for final LGTM.

[Ami GONE FROM CHROMIUM, 2012-01-10 22:30:55]

http://codereview.chromium.org/9125022/diff/14001/media/tools/constrained_net...
File media/tools/constrained_network_server/traffic_control.py (right):

http://codereview.chromium.org/9125022/diff/14001/media/tools/constrained_net...
media/tools/constrained_network_server/traffic_control.py:49: _Exec(['sudo',
'-n', 'tc', '-help'],
Note that this isn't fool-proof; sudo uses an expiring (per-tty?) cookie to
prevent asking for a PW too frequently, so a sudo -n might succeed at the
beginning of a CNS run but later fail.

Still, probably good enough for this use.

[Ami GONE FROM CHROMIUM, 2012-01-10 22:31:09]

Oops, forgot to LGTM.

[commit-bot: I haz the power, 2012-01-12 21:39:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/9125022/14001

[commit-bot: I haz the power, 2012-01-13 00:03:04]

Change committed as 117548