Merge lp:~brendan-donegan/checkbox/bug1066094 into lp:checkbox

[Brendan Donegan]

Fix bug 1066094 by dealing with each case where superuser is needed in network_restart individually

As pointed out in the linked bug, the network_restart script took a very brute-force approach to informing the user of the script that superuser permissions may be required for specific operations - i.e. it assumes that they are and doesn't allow the script to be run without them. This has been changed so that for each command that may require superuser permissions, if an error occurs then a check is done to see if the script is not being run as the superuser and the user of the script is informed if that's the case.

[lp:~brendan-donegan/checkbox/bug1066094]

1778. By Brendan Donegan on 2012-10-16

Tweaks to the code.

[Marc Tardif (cr3)]

> - output = check_output(['/sbin/ifconfig', '-s', '-a'])
> + output = check_output(['ifconfig', '-s', '-a'])

Thank you for changing the path to ifconfig and the other commands from
absolute to just their names!

> - for interface in self.interfaces:
> + for iface in self.interfaces:

What was wrong with the name "interface"?

> try:
> - check_output(['/sbin/ifconfig', interface, 'up'])
> + check_output(['ifconfig', iface, 'up'])
> except CalledProcessError:

This will still output the stderr of ifconfig to the terminal. The
exception raised by the check_output function only has one output
attribute, so you need to pipe stderr into stdout:

    check_output(
        ['/sbin/ifconfig', interface, 'up'],
        stderr=subprocess.STDOUT)

> - logging.error('Unable to bring up interface {0!r}'
> - .format(interface))
> + message = 'Unable to bring up interface {0!r}'.format(iface)
> +
> + if os.getuid():
> + message += self.SUPERUSER_MSG
> +
> + message += '.'
> +
> + print(message, file=sys.stderr)
> + logging.error(message)
> raise

How do you know that just because check_output raised a CalledProcessError
that it was because of missing superuser privileges? Here's another error
that has nothing to do with privileges:

    $ ifconfig eth8 up
    eth8: ERROR while getting interface flags: No such device

So, once you piped stderr into stdout, you should use the output variable
in the exception to provide a useful and accurate error message to the
user:

    message = "Unable to bring up interface {0!r}:\n".format(interface)
    message += exception.output.decode("utf-8")
    print(message, file=sys.stderr)
    logging.error(message)
    raise

the same applies for the other calls that assumer that any error is because
of missing superuser privileges.

> - default='/var/log',
> + default='.',
> help='The path to the log directory. \
> Default is /var/log')

The problem here is that the default kwarg was changed but the help wasn't
changed accordingly. The solution would be to use %(default)s in the help
instead of hard coding a duplicate value.

[Marc Tardif (cr3)]

See above comment, I wonder how I can set the review to "needs fixing" by email...

review: Needs Fixing

[Brendan Donegan (brendan-donegan)]

On 16/10/12 14:52, Marc Tardif wrote:
>> - output = check_output(['/sbin/ifconfig', '-s', '-a'])
>> + output = check_output(['ifconfig', '-s', '-a'])
> Thank you for changing the path to ifconfig and the other commands from
> absolute to just their names!
You're welcome :)
>
>> - for interface in self.interfaces:
>> + for iface in self.interfaces:
> What was wrong with the name "interface"?
Not a lot, just that it avoids having to split certain statements across
multiple lines. I reckoned iface is a fairly common contraction of
interface so it shouldn't be confusing
>
>> try:
>> - check_output(['/sbin/ifconfig', interface, 'up'])
>> + check_output(['ifconfig', iface, 'up'])
>> except CalledProcessError:
> This will still output the stderr of ifconfig to the terminal. The
> exception raised by the check_output function only has one output
> attribute, so you need to pipe stderr into stdout:
This is an existing problem with the script as far as I know, but it
would be ok to fix it now.
>
> check_output(
> ['/sbin/ifconfig', interface, 'up'],
> stderr=subprocess.STDOUT)
>
>> - logging.error('Unable to bring up interface {0!r}'
>> - .format(interface))
>> + message = 'Unable to bring up interface {0!r}'.format(iface)
>> +
>> + if os.getuid():
>> + message += self.SUPERUSER_MSG
>> +
>> + message += '.'
>> +
>> + print(message, file=sys.stderr)
>> + logging.error(message)
>> raise
> How do you know that just because check_output raised a CalledProcessError
> that it was because of missing superuser privileges? Here's another error
> that has nothing to do with privileges:
>
> $ ifconfig eth8 up
> eth8: ERROR while getting interface flags: No such device
>
> So, once you piped stderr into stdout, you should use the output variable
> in the exception to provide a useful and accurate error message to the
> user:
>
> message = "Unable to bring up interface {0!r}:\n".format(interface)
> message += exception.output.decode("utf-8")
> print(message, file=sys.stderr)
> logging.error(message)
> raise
Sounds like a fine method, I'll give it a try
>
> the same applies for the other calls that assumer that any error is because
> of missing superuser privileges.
>
>> - default='/var/log',
>> + default='.',
>> help='The path to the log directory. \
>> Default is /var/log')
> The problem here is that the default kwarg was changed but the help wasn't
> changed accordingly. The solution would be to use %(default)s in the help
> instead of hard coding a duplicate value.
I fixed this specific issue but in principle where defaults are
mentioned in help text that convention should be used, so I'll change it.
>

[Brendan Donegan (brendan-donegan)]

Rather disappointingly, the contents of exception.output in the case of 'start network-manager' failing are:

'Unable to stop network-manager, stop: Rejected send message, 1 matched rules; type="method_call", sender=":1.159" (uid=1000 pid=20099 comm="stop network-manager ") interface="com.ubuntu.Upstart0_6.Job" member="Stop" error name="(unset)" requested_reply="0" destination="com.ubuntu.Upstart" (uid=0 pid=1 comm="/sbin/init")\n'

Not exactly helpful. I appreciate the observation that we shouldn't assume that lack of superuser privileges caused a command to fail, but if this is the alternative then it's not really helpful.

[Marc Tardif (cr3)]

As mentionned in #ubuntu-server, I would suggest we special case upstart errors:

  when the output string contains "Rejected send message", change the message to invalid permission
  otherwise, use the original message

[Brendan Donegan (brendan-donegan)]

I don't have time to work on this right now and it's a very low priority fix

Unmerged revisions

1778. By Brendan Donegan on 2012-10-16

Tweaks to the code.

1777. By Brendan Donegan on 2012-10-16

scripts/network_restart - be specific about where superuser permissions
are required (LP: #1066094)

=== modified file 'debian/changelog'
--- debian/changelog	2012-10-15 20:03:32 +0000
+++ debian/changelog	2012-10-16 13:20:27 +0000
@@ -7,6 +7,8 @@
     don't get tampered with (LP: #1065932)
   * plugins/launchpad_exchange.py - Remove call to string_to_type on
     self.timeout, which is an int (LP: #1066967)
+  * scripts/network_restart - be specific about where superuser permissions
+    are required (LP: #1066094)
 
   [Daniel Manrique]
   * [FEATURE] checkbox/job.py: Fixed intltool warning about unnamed

=== modified file 'scripts/network_restart'
--- scripts/network_restart	2012-10-12 20:16:20 +0000
+++ scripts/network_restart	2012-10-16 13:20:27 +0000
@@ -21,6 +21,7 @@
 except (ImportError, RuntimeError):
     gtk_found = False
 
+SUPERUSER_MSG = ", superuser permissions required"
 
 class PingError(Exception):
     def __init__(self, address, reason):
@@ -31,12 +32,6 @@
 def main():
     args = parse_args()
 
-    # Verify that script is run as root
-    if os.getuid():
-        sys.stderr.write(
-            'This script needs superuser permissions to run correctly\n')
-        return 1
-
     configure_logging(args.log_level, args.output)
 
     # Select interface based on graphich capabilities available
@@ -196,6 +191,8 @@
     """
     Networking abstraction to start/stop all interfaces
     """
+
+
     def __init__(self, address):
         self.address = address
         self.interfaces = self._get_interfaces()
@@ -204,7 +201,7 @@
         """
         Get all network interfaces
         """
-        output = check_output(['/sbin/ifconfig', '-s', '-a'])
+        output = check_output(['ifconfig', '-s', '-a'])
         lines = output.splitlines()[1:]
         interfaces = ([interface for interface in
             [line.split()[0] for line in lines] if interface != 'lo'])
@@ -222,19 +219,34 @@
         Start networking
         """
         logging.info('Bringing all interfaces up...')
-        for interface in self.interfaces:
+        for iface in self.interfaces:
             try:
-                check_output(['/sbin/ifconfig', interface, 'up'])
+                check_output(['ifconfig', iface, 'up'])
             except CalledProcessError:
-                logging.error('Unable to bring up interface {0!r}'
-                              .format(interface))
+                message = 'Unable to bring up interface {0!r}'.format(iface)
+
+                if os.getuid():
+                    message += SUPERUSER_MSG
+
+                message += '.'
+
+                print(message, file=sys.stderr)
+                logging.error(message)
                 raise
 
         logging.info('Starting network manager...')
         try:
-            check_output(['/sbin/start', 'network-manager'])
+            check_output(['start', 'network-manager'])
         except CalledProcessError:
-            logging.error('Unable to start network manager')
+            message = 'Unable to stop network-manager'
+
+            if os.getuid():
+                message += SUPERUSER_MSG
+
+            message += "."
+
+            print(message)
+            logging.error(message)
             raise
 
         # Verify that network interface is up
@@ -245,8 +257,7 @@
             if success:
                 break
         else:
-            raise PingError(self.address,
-                            'Some interface is still down')
+            raise PingError(self.address, 'Some interface is still down')
 
     def _stop(self):
         """
@@ -254,18 +265,33 @@
         """
         logging.info('Stopping network manager...')
         try:
-            check_output(['/sbin/stop', 'network-manager'])
+            check_output(['stop', 'network-manager'])
         except CalledProcessError:
-            logging.error('Unable to stop network manager')
+            message = 'Unable to stop network-manager'
+
+            if os.getuid():
+                message += SUPERUSER_MSG
+
+            message += "."
+
+            print(message, file=sys.stderr)
+            logging.error(message)
             raise
 
         logging.info('Bringing all interfaces down...')
-        for interface in self.interfaces:
+        for iface in self.interfaces:
             try:
-                check_output(['/sbin/ifconfig', interface, 'down'])
+                check_output(['ifconfig', iface, 'down'])
             except CalledProcessError:
-                logging.error('Unable to bring down interface {0!r}'
-                              .format(interface))
+                message = "Unable to bring down interface {0!r}".format(iface)
+
+                if os.getuid():
+                    message += SUPERUSER_MSG
+
+                message += "."
+
+                print(message, file=sys.stderr)
+                logging.error(message)
                 raise
 
         # Verify that network interface is down
@@ -327,8 +353,22 @@
     log_filename = os.path.join(output,
                                 '{0}.log'.format(os.path.basename(__file__)))
     rollover = os.path.exists(log_filename)
-    log_handler = logging.handlers.RotatingFileHandler(log_filename, mode='a+',
-                                                       backupCount=3)
+    
+    try:
+        log_handler = logging.handlers.RotatingFileHandler(log_filename,
+                                                           mode='a+',
+                                                           backupCount=3)
+    except IOError:
+        message = 'Unable to log to {0}'.format(log_filename)
+
+        if os.getuid():
+            message += SUPERUSER_MSG
+
+        message += '.'
+        
+        print(message)
+        return
+
     formatter = logging.Formatter('%(asctime)s %(levelname)-8s %(message)s')
     log_handler.setFormatter(formatter)
     log_handler.setLevel(logging.DEBUG)