Merge lp:~sylvain-pineau/checkbox/fix-1361071 into lp:checkbox

[Sylvain Pineau]

Fixes https://bugs.launchpad.net/plainbox/+bug/1361071 by adding support for template jobs requiring root privileges.

The trusted launcher now understands jobs generated from local AND resources jobs.

[Zygmunt Krynicki (zyga)]

Not sure if needs fixing or not. Have a look at the comments below please

[Zygmunt Krynicki (zyga)]

Let's work on landing via-changes and rebase this one on top

review: Needs Fixing

[Sylvain Pineau (sylvain-pineau)]

rebased on trunk with via changes. also fixed plainbox/plainbox/impl/ctrl.py to get the source job not from the origin object but the new job_state.via_job

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

Looks good apart from the _name that you are already fixing thing :)

The comment below about how jobs are instantiated. Do you think we should copy the checks from the session controller?

[Sylvain Pineau (sylvain-pineau)]

fixed udevadm parser to create a dedicated property for "name"

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

+1

16:05 <@spineau> zyga: when you're referring to the check in session controller code (https://code.launchpad.net/~sylvain-pineau/checkbox/fix-1361071/+merge/245551/comments/607377)
16:05 <@zyga> yes>
16:05 <@zyga> ?
16:06 <@spineau> zyga: are you suggesting to call new_unit.validate() ?
16:06 <@zyga> IIRC not just that
16:06 <@zyga> let me see
16:07 <@zyga> http://bazaar.launchpad.net/~checkbox-dev/checkbox/trunk/view/head:/plainbox/plainbox/impl/ctrl.py#L245
16:07 <@zyga> if you trace how that call unfolds
16:07 <@zyga> you'll see validity checks and duplicate checks
16:07 <@zyga> I think it's okay as is (as it it will work in the good case)
16:08 <@zyga> my worry is just about the bad case and what happens then
16:10 <@spineau> zyga: but the check should have been done by the session controller already, why duplicating this logic a second time in the secure launcher?*
16:10 <@zyga> the outcome may be different
16:10 <@zyga> and the original may have discarded something (separate concern)
16:10 <@zyga> that we're not discarding
16:11 <@spineau> zyga: we had this problem before then
16:13 <@spineau> zyga: as the check for duplicated jobs is only performed by the session controller
16:17 <@zyga> spineau: yes, though that doesn't mean the problem goes away
16:18 <@spineau> zyga: almost EOD for me, could you please explain in the merge request your concern about validity checks and duplicate checks missing from the trusted launcher?
16:18 <@spineau> zyga: are you seeing that as a blocker?
16:18 <@zyga> spineau: well, I think this can be merged
16:18 <@zyga> spineau: but I would still love to explore how the trusted launcher can be broken today
16:18 <@spineau> zyga: in that case we should follow up your remarks in a bug report I think

review: Approve

[Daniel Manrique (roadmr)]

The attempt to merge lp:~sylvain-pineau/checkbox/fix-1361071 into lp:checkbox failed. Below is the output from the failed tests.

[precise] starting container
[precise] (timing) 0.04user 0.03system 0:04.19elapsed 1%CPU (0avgtext+0avgdata 7864maxresident)k
[precise] (timing) 0inputs+32outputs (0major+5757minor)pagefaults 0swaps
[precise] provisioning container
[precise] (timing) 39.36user 11.37system 1:19.11elapsed 64%CPU (0avgtext+0avgdata 51604maxresident)k
[precise] (timing) 0inputs+16424outputs (0major+4483301minor)pagefaults 0swaps
[precise-testing] Starting tests...
Found a test script: ./checkbox-gui/requirements/container-tests-checkbox-gui-build
[precise-testing] container-tests-checkbox-gui-build: PASS
[precise-testing] (timing) 32.33user 2.33system 0:35.15elapsed 98%CPU (0avgtext+0avgdata 116152maxresident)k
[precise-testing] (timing) 0inputs+4216outputs (0major+477937minor)pagefaults 0swaps
Found a test script: ./checkbox-ng/requirements/container-tests-checkbox-ng-unit
[precise-testing] container-tests-checkbox-ng-unit: PASS
[precise-testing] (timing) 0.63user 0.12system 0:00.77elapsed 97%CPU (0avgtext+0avgdata 39872maxresident)k
[precise-testing] (timing) 0inputs+3456outputs (0major+20631minor)pagefaults 0swaps
Found a test script: ./checkbox-support/requirements/container-tests-checkbox-support
[precise-testing] container-tests-checkbox-support: PASS
[precise-testing] (timing) 16.67user 0.12system 0:16.88elapsed 99%CPU (0avgtext+0avgdata 83084maxresident)k
[precise-testing] (timing) 0inputs+1032outputs (0major+31508minor)pagefaults 0swaps
Found a test script: ./checkbox-touch/requirements/container-tests-touch-unit-tests
[precise-testing] container-tests-touch-unit-tests: PASS
[precise-testing] (timing) 0.00user 0.00system 0:00.02elapsed 40%CPU (0avgtext+0avgdata 2020maxresident)k
[precise-testing] (timing) 0inputs+8outputs (0major+2345minor)pagefaults 0swaps
Found a test script: ./plainbox/plainbox/impl/providers/categories/requirements/container-tests-provider-categories
[precise-testing] container-tests-provider-categories: PASS
[precise-testing] (timing) 0.16user 0.04system 0:00.21elapsed 94%CPU (0avgtext+0avgdata 13496maxresident)k
[precise-testing] (timing) 0inputs+176outputs (0major+5874minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/001-container-tests-plainbox-egg-info
[precise-testing] 001-container-tests-plainbox-egg-info: PASS
[precise-testing] (timing) 0.13user 0.02system 0:00.17elapsed 92%CPU (0avgtext+0avgdata 10500maxresident)k
[precise-testing] (timing) 0inputs+88outputs (0major+4985minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox
[precise-testing] container-tests-plainbox: FAIL
[precise-testing] stdout: http://paste.ubuntu.com/9699339/
[precise-testing] stderr: http://paste.ubuntu.com/9699340/
[precise-testing] (timing) Command exited with non-zero status 1
[precise-testing] (timing) 5.12user 0.36system 0:05.53elapsed 99%CPU (0avgtext+0avgdata 63804maxresident)k
[precise-testing] (timing) 0inputs+2624outputs (0major+52524minor)pagefaults 0swaps
Found...

[lp:~sylvain-pineau/checkbox/fix-1361071]

3527. By Sylvain Pineau on 2015-01-09

checkbox-support:parsers:udevadm.py: Keep the device name in the slots

Most of local/template jobs are using the device name /dev/<XXXX> to create
child jobs.
To avoid recreating the full device path in the job definition, the udev parser
now returns - if present - the device name, the one stating with "N: <XXXX>" in
the "udevadm info --export-db" output.

3528. By Sylvain Pineau on 2015-01-09

providers:resource-generic:udev_resource: return the device name attribute

The device name attribute is now part of the output and can be used by local or
template jobs.

3529. By Sylvain Pineau on 2015-01-09

plainbox:ctrl: Change how we get the source job to create parent_env

For template jobs, job.origin.source.job fails withthe following error:

AttributeError: 'FileTextSource' object has no attribute 'job'

job.origin.source.job cannot be used for all types of generated jobs.
A generic way to get the source job is to use now job_state.via_job.

3530. By Sylvain Pineau on 2015-01-09

plainbox:impl:secure:launcher1.py: Add support for template jobs

When the generator job is a resource job, the trusted launcher will now recreate
all the possible template jobs that use it.
That way generated jobs requiring root privileges can be created from templates
and still use the plainbox trusted launcher.
The command line options are untouched.

3531. By Sylvain Pineau on 2015-01-09

providers:checkbox:disks.txt.in: staging/disk now use the device resource

This new template job now use the udevadm parser to solve two problems:
- all the device parsing logic is delegated to the resource, not the template
  filter (i.e. removal of ram and loop devices)
- the udev resource is not the default udevadm parser and thus not part of
  default whitelists, causing https://bugs.launchpad.net/plainbox/+bug/1361071

Fixes https://bugs.launchpad.net/plainbox/+bug/1361071

[Sylvain Pineau (sylvain-pineau)]

Fixed ctrl and launcher unit tests

review: Approve

=== modified file 'checkbox-support/checkbox_support/parsers/udevadm.py'
--- checkbox-support/checkbox_support/parsers/udevadm.py	2014-11-19 12:19:27 +0000
+++ checkbox-support/checkbox_support/parsers/udevadm.py	2015-01-09 21:49:01 +0000
@@ -90,6 +90,7 @@
 class UdevadmDevice(object):
     __slots__ = (
         "_environment",
+        "_name",
         "_bits",
         "_stack",
         "_bus",
@@ -99,8 +100,9 @@
         "_vendor",
         "_vendor_id",)
 
-    def __init__(self, environment, bits=None, stack=[]):
+    def __init__(self, environment, name, bits=None, stack=[]):
         self._environment = environment
+        self._name = name
         self._bits = bits
         self._stack = stack
         self._bus = None
@@ -118,6 +120,11 @@
             self.product))
 
     @property
+    def name(self):
+        if self._name is not None:
+            return self._name
+
+    @property
     def bus(self):
         if self._bus is not None:
             return self._bus
@@ -639,7 +646,7 @@
     def as_json(self):
         attributes = ("path", "bus", "category", "driver", "product_id",
                       "vendor_id", "subproduct_id", "subvendor_id", "product",
-                      "vendor", "interface",)
+                      "vendor", "interface", "name")
 
         return {a: getattr(self, a) for a in attributes if getattr(self, a)}
 
@@ -715,8 +722,9 @@
             if not record:
                 continue
 
-            # Determine path and environment
+            # Determine path, name and environment
             path = None
+            name = None
             element = None
             environment = {}
             for line in record.splitlines():
@@ -732,6 +740,8 @@
 
                 if key == "P":
                     path = value
+                elif key == "N":
+                    name = value
                 elif key == "E":
                     key_match = multi_pattern.match(value)
                     if not key_match:
@@ -749,7 +759,8 @@
             # Set default DEVPATH
             environment.setdefault("DEVPATH", path)
 
-            device = self.device_factory(environment, self.bits, list(stack))
+            device = self.device_factory(
+                environment, name, self.bits, list(stack))
             if not self._ignoreDevice(device):
                 if device._raw_path in self.devices:
                     if self.devices[device._raw_path].category == 'CARDREADER':

=== modified file 'plainbox/plainbox/impl/ctrl.py'
--- plainbox/plainbox/impl/ctrl.py	2015-01-07 14:55:36 +0000
+++ plainbox/plainbox/impl/ctrl.py	2015-01-09 21:49:01 +0000
@@ -954,7 +954,7 @@
             cmd += ['--generator', job_state.via_job.checksum]
             parent_env = self.get_differential_execution_environment(
                 # FIXME: job_state is from an unrelated job :/
-                job.origin.source.job, job_state, config, session_dir,
+                job_state.via_job, job_state, config, session_dir,
                 nest_dir)
             for key, value in sorted(parent_env.items()):
                 cmd += ['-G', '{}={}'.format(key, value)]

=== modified file 'plainbox/plainbox/impl/secure/launcher1.py'
--- plainbox/plainbox/impl/secure/launcher1.py	2014-07-22 13:58:51 +0000
+++ plainbox/plainbox/impl/secure/launcher1.py	2015-01-09 21:49:01 +0000
@@ -30,6 +30,8 @@
 
 from plainbox.i18n import gettext as _
 from plainbox.impl.job import JobDefinition
+from plainbox.impl.resource import Resource
+from plainbox.impl.unit.template import TemplateUnit
 from plainbox.impl.secure.origin import JobOutputTextSource
 from plainbox.impl.secure.providers.v1 import all_providers
 from plainbox.impl.secure.rfc822 import load_rfc822_records, RFC822SyntaxError
@@ -92,16 +94,16 @@
         cmd = [job.shell, '-c', job.command]
         return subprocess.call(cmd, env=self.modify_execution_environment(env))
 
-    def run_local_job(self, checksum, env):
+    def run_generator_job(self, checksum, env):
         """
-        Run a job with and interpret the stdout as a job definition.
+        Run a job with and process the stdout to get a job definition.
 
         :param checksum:
             The checksum of the job to execute
         :param env:
             Environment to execute the job in.
         :returns:
-            A list of job definitions that were parsed out of the output.
+            A list of job definitions that were processed from the output.
         :raises LookupError:
             If the checksum does not match any known job
         """
@@ -116,11 +118,26 @@
             record_list = load_rfc822_records(output, source=source)
         except RFC822SyntaxError as exc:
             logging.error(
-                _("Syntax error in job generated from %s: %s"), job, exc)
+                _("Syntax error in record generated from %s: %s"), job, exc)
         else:
-            for record in record_list:
-                job = JobDefinition.from_rfc822_record(record)
-                job_list.append(job)
+            if job.plugin == 'local':
+                for record in record_list:
+                    job = JobDefinition.from_rfc822_record(record)
+                    job_list.append(job)
+            elif job.plugin == 'resource':
+                resource_list = []
+                for record in record_list:
+                    resource = Resource(record.data)
+                    resource_list.append(resource)
+                for plugin in all_providers.get_all_plugins():
+                    for u in plugin.plugin_object.get_units()[0]:
+                        if (
+                            isinstance(u, TemplateUnit) and
+                            u.resource_id == job.id
+                        ):
+                            logging.info(_("Instantiating unit: %s"), u)
+                            for new_unit in u.instantiate_all(resource_list):
+                                job_list.append(new_unit)
         return job_list
 
 
@@ -252,7 +269,7 @@
     # Run the local job and feed the result back to the launcher
     if ns.generator:
         try:
-            generated_job_list = launcher.run_local_job(
+            generated_job_list = launcher.run_generator_job(
                 ns.generator, ns.generator_env)
             launcher.add_job_list(generated_job_list)
         except LookupError as exc:

=== modified file 'plainbox/plainbox/impl/secure/test_launcher1.py'
--- plainbox/plainbox/impl/secure/test_launcher1.py	2014-09-13 11:37:17 +0000
+++ plainbox/plainbox/impl/secure/test_launcher1.py	2015-01-09 21:49:01 +0000
@@ -117,7 +117,7 @@
     def test_run_local_job(self, mock_check_output, mock_load_rfc822_records,
                            mock_from_rfc822_record):
         # Create a mock job and add it to the launcher
-        job = mock.Mock(spec=JobDefinition, name='job')
+        job = mock.Mock(spec=JobDefinition, name='job', plugin='local')
         self.launcher.add_job_list([job])
         # Create two mock rfc822 records
         record1 = mock.Mock(spec=RFC822Record, name='record')
@@ -125,7 +125,7 @@
         # Ensure that load_rfc822_records() returns some mocked records
         mock_load_rfc822_records.return_value = [record1, record2]
         # Run the tested method
-        job_list = self.launcher.run_local_job(job.checksum, None)
+        job_list = self.launcher.run_generator_job(job.checksum, None)
         # Ensure that we run the job command via job.shell
         mock_check_output.assert_called_with(
             [job.shell, '-c', job.command], env={}, universal_newlines=True)
@@ -271,7 +271,7 @@
         with TestIO(combined=True) as io:
             retval = main(['--target=1234', '--generator=5678'])
         # Ensure that the local job command was invoked
-        mock_launcher().run_local_job.assert_called_with(local_job.checksum, None)
+        mock_launcher().run_generator_job.assert_called_with(local_job.checksum, None)
         # Ensure that the target job command was invoked
         mock_launcher().run_shell_from_job.assert_called_with(
             target_job.checksum, None)

=== modified file 'plainbox/plainbox/impl/test_ctrl.py'
--- plainbox/plainbox/impl/test_ctrl.py	2015-01-07 14:55:36 +0000
+++ plainbox/plainbox/impl/test_ctrl.py	2015-01-09 21:49:01 +0000
@@ -792,7 +792,7 @@
         verify that we run plainbox-trusted-launcher-1 as the desired user
         """
         self.job.get_environ_settings.return_value = []
-        self.job.origin.source.job = mock.Mock(
+        self.job_state.via_job = mock.Mock(
             name='generator_job',
             spec=JobDefinition,
             provider=mock.Mock(
@@ -803,7 +803,7 @@
                 units_dir="units_dir-generator",
                 CHECKBOX_SHARE='CHECKBOX_SHARE-generator'))
         # Mock the default flags (empty set)
-        self.job.origin.source.job.get_flag_set.return_value = frozenset()
+        self.job_state.via_job.get_flag_set.return_value = frozenset()
         PATH = os.pathsep.join([self.NEST_DIR, 'vanilla-path'])
         expected = [
             'pkexec', '--user', self.job.user,

=== modified file 'providers/plainbox-provider-checkbox/jobs/disk.txt.in'
--- providers/plainbox-provider-checkbox/jobs/disk.txt.in	2014-09-09 16:02:53 +0000
+++ providers/plainbox-provider-checkbox/jobs/disk.txt.in	2015-01-09 21:49:01 +0000
@@ -6,16 +6,16 @@
 
 unit: template
 template-unit: job
-template-imports: from 2013.com.canonical.certification import staging/udev as udev
-template-resource: udev
-template-filter: udev.attr_DEVTYPE == 'disk' and 'ram' not in udev.name and 'loop' not in udev.name
+template-imports: from 2013.com.canonical.certification import device
+template-resource: device
+template-filter: device.category == 'DISK'
 plugin: shell
 id: staging/disk/stats_{name}
 requires:
  block_device.{name}_state != 'removable'
 user: root
 command: disk_stats_test {name}
-_summary: Disk statistics for {attr_DEVNAME}
+_summary: Disk statistics for /dev/{name}
 _description:
  This test checks disk stats, generates some activity and rechecks stats to
  verify they've changed. It also verifies that disks appear in the various
@@ -23,11 +23,9 @@
  .
  This test will inspect the following disk:
  .
-     product vendor: {attr_ID_VENDOR}
-     product name: {attr_ID_MODEL}
-     serial number: {attr_ID_SERIAL_SHORT}
+     product name: {product}
      sysfs path: {path}
-     device node path: {attr_DEVNAME}
+     device node path: /dev/{name}
 
 plugin: local
 _summary: Check stats changes for each disk

=== modified file 'providers/plainbox-provider-resource-generic/bin/udev_resource'
--- providers/plainbox-provider-resource-generic/bin/udev_resource	2014-08-08 16:35:49 +0000
+++ providers/plainbox-provider-resource-generic/bin/udev_resource	2015-01-09 21:49:01 +0000
@@ -27,7 +27,7 @@
 
 class UdevResult:
 
-    attributes = ("path", "bus", "category", "driver", "product_id",
+    attributes = ("path", "name", "bus", "category", "driver", "product_id",
                   "vendor_id", "subproduct_id", "subvendor_id", "product",
                   "vendor", "interface",)