Merge lp:~zyga/checkbox/fix-1299201 into lp:checkbox

[Zygmunt Krynicki]

[Daniel Manrique (roadmr)]

This looks OK in principle, but rather than just assuming polkit over ssh does not work at all, did we consider fixing the policy file? see comment in the bug, we can make everything auth_admin_keep:

<defaults>
      <allow_any>auth_admin_keep</allow_any>
      <allow_inactive>auth_admin_keep</allow_inactive>
      <allow_active>auth_admin_keep</allow_active>
</defaults>

This may require clearing with the security team, as the secure policy is the one we ship in Ubuntu, but barring their disapproval I think that's also a possible solution.

Still, it's good for the controller to not try doing something it can't, but I wonder if instead of blanket assuming we don't work over SSH, we could use pkcheck to actively verify this.

review: Needs Information

[Zygmunt Krynicki (zyga)]

> Still, it's good for the controller to not try doing something it can't, but I
> wonder if instead of blanket assuming we don't work over SSH, we could use
> pkcheck to actively verify this.

This would equate the insecure policy (which is insecure for a reason) with the secure policy. I don't think that would fly.

One other approach would be to have different (and co-installable) policies that we can detect and, e.g. know that we have the 'remote access policy'.

[Daniel Manrique (roadmr)]

using auth_admin_keep still asks for the password and keeps the authorization for 5 minutes. If over SSH (or not X), it uses a text-mode prompt. That doesn't sound terribly insecure :)

[Zygmunt Krynicki (zyga)]

It also requires the user to be in the admin group which IIRC is not the case with $random_cloud

[Daniel Manrique (roadmr)]

OK, for the use case where this is intended (server runs) it's likely people will be OK using sudo (which also requires being in some admin group but that's usually configured correctly), so in the end I think this is reasonable. I'd still like to check for, and use, pkexec if possible, but that may be too much extra work for too little benefit.

review: Approve

[Daniel Manrique (roadmr)]

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

[precise] Bringing VM 'up'
[precise] (timing) 8.00user 3.50system 4:10.30elapsed 4%CPU (0avgtext+0avgdata 21792maxresident)k
[precise] (timing) 0inputs+208outputs (0major+185787minor)pagefaults 0swaps
[precise] Starting tests...
Found a test script: ./checkbox-gui/requirements/container-tests-checkbox-gui-build
[precise] container-tests-checkbox-gui-build: PASS
[precise] (timing) 0.84user 0.38system 1:03.29elapsed 1%CPU (0avgtext+0avgdata 20620maxresident)k
[precise] (timing) 0inputs+64outputs (0major+62561minor)pagefaults 0swaps
Found a test script: ./checkbox-ng/requirements/container-tests-checkbox-ng-unit
[precise] container-tests-checkbox-ng-unit: PASS
[precise] (timing) 0.86user 0.39system 0:08.08elapsed 15%CPU (0avgtext+0avgdata 20324maxresident)k
[precise] (timing) 0inputs+16outputs (0major+61509minor)pagefaults 0swaps
Found a test script: ./checkbox-support/requirements/container-tests-checkbox-support
[precise] container-tests-checkbox-support: PASS
[precise] (timing) 0.86user 0.30system 0:26.51elapsed 4%CPU (0avgtext+0avgdata 20016maxresident)k
[precise] (timing) 0inputs+24outputs (0major+61425minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/001-container-tests-plainbox-egg-info
[precise] 001-container-tests-plainbox-egg-info: PASS
[precise] (timing) 0.84user 0.34system 0:05.95elapsed 19%CPU (0avgtext+0avgdata 20120maxresident)k
[precise] (timing) 0inputs+8outputs (0major+61636minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox
[precise] container-tests-plainbox: FAIL
[precise] stdout: http://paste.ubuntu.com/7457886/
[precise] stderr: http://paste.ubuntu.com/7457887/
[precise] (timing) Command exited with non-zero status 1
[precise] (timing) 1.04user 0.37system 0:19.30elapsed 7%CPU (0avgtext+0avgdata 20292maxresident)k
[precise] (timing) 0inputs+248outputs (0major+61918minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox-documentation
[precise] container-tests-plainbox-documentation: PASS
[precise] (timing) 0.89user 0.32system 0:35.90elapsed 3%CPU (0avgtext+0avgdata 20248maxresident)k
[precise] (timing) 0inputs+32outputs (0major+61792minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox-integration
[precise] container-tests-plainbox-integration: PASS
[precise] (timing) 0.75user 0.39system 0:08.19elapsed 14%CPU (0avgtext+0avgdata 19988maxresident)k
[precise] (timing) 0inputs+8outputs (0major+61149minor)pagefaults 0swaps
Found a test script: ./providers/plainbox-provider-certification-client/requirements/container-tests-provider-certification-client
[precise] container-tests-provider-certification-client: PASS
[precise] (timing) 0.87user 0.32system 0:05.80elapsed 20%CPU (0avgtext+0avgdata 20696maxresident)k
[precise] (timing) 0inputs+8outputs (0major+61440minor)pagefaults 0swaps
Found a test script: ./providers/plainbox-provider-certification-server-soc/requir...

[Zygmunt Krynicki (zyga)]

Oh right. The tests failed as now the *other* tests are not mocking SSH_CONNECTION and are thus affected by how vagrant operates. Let me fix that

[lp:~zyga/checkbox/fix-1299201]

2988. By Zygmunt Krynicki on 2014-05-13

plainbox:ctrl: disable plainbox-trusted-launcher-1 over ssh

This patch detects SSH_CONNECTION and marks the
plainbox-trusted-launcher-1 execution controller as unsuitable. This
effectively forces all jobs that need to run as root to use sudo.

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

Signed-off-by: Zygmunt Krynicki <email address hidden>

[Zygmunt Krynicki (zyga)]

Fixed, approving again.

review: Approve

=== modified file 'plainbox/plainbox/impl/ctrl.py'
--- plainbox/plainbox/impl/ctrl.py	2014-05-13 15:25:13 +0000
+++ plainbox/plainbox/impl/ctrl.py	2014-05-13 15:26:47 +0000
@@ -754,6 +754,9 @@
         # Only works with jobs loaded from the secure PROVIDERPATH
         if not job.provider.secure:
             return -1
+        # Doesn't work when connected over SSH (LP: #1299201)
+        if os.environ.get("SSH_CONNECTION"):
+            return -1
         # Only makes sense with jobs that need to run as another user
         # Promote this controller only if the trusted launcher is authorized to
         # run jobs as another user

=== modified file 'plainbox/plainbox/impl/test_ctrl.py'
--- plainbox/plainbox/impl/test_ctrl.py	2014-05-13 07:58:46 +0000
+++ plainbox/plainbox/impl/test_ctrl.py	2014-05-13 15:26:47 +0000
@@ -814,6 +814,7 @@
         # Ensure that we get a negative score of minus one
         self.assertEqual(self.ctrl.get_checkbox_score(self.job), -1)
 
+    @mock.patch.dict('plainbox.impl.ctrl.os.environ', clear=True)
     def test_get_checkbox_score_for_secure_provider_and_user_job(self):
         # Assume that the job is coming from Provider1 provider
         # and the provider is secure
@@ -823,6 +824,7 @@
         # Ensure that we get a neutral score of zero
         self.assertEqual(self.ctrl.get_checkbox_score(self.job), 0)
 
+    @mock.patch.dict('plainbox.impl.ctrl.os.environ', clear=True)
     @mock.patch('plainbox.impl.ctrl.check_output')
     def test_get_checkbox_score_for_secure_provider_root_job_with_policy(
             self, mock_check_output):
@@ -838,6 +840,7 @@
         ctrl = self.CLS(self.SESSION_DIR, self.PROVIDER_LIST)
         self.assertEqual(ctrl.get_checkbox_score(self.job), 3)
 
+    @mock.patch.dict('plainbox.impl.ctrl.os.environ', clear=True)
     @mock.patch('plainbox.impl.ctrl.check_output')
     def test_get_checkbox_score_for_secure_provider_root_job_with_policy_2(
             self, mock_check_output):
@@ -855,6 +858,27 @@
         ctrl = self.CLS(self.SESSION_DIR, self.PROVIDER_LIST)
         self.assertEqual(ctrl.get_checkbox_score(self.job), 3)
 
+    @mock.patch.dict('plainbox.impl.ctrl.os.environ', values={
+        'SSH_CONNECTION': '1.2.3.4 123 1.2.3.5 22'
+    })
+    @mock.patch('plainbox.impl.ctrl.check_output')
+    def test_get_checkbox_score_for_normally_supported_job_over_ssh(
+            self, mock_check_output):
+        # Assume that the job is coming from Provider1 provider
+        # and the provider is secure
+        self.job.provider = mock.Mock(spec=Provider1, secure=True)
+        # Assume that the job runs as root
+        self.job.user = 'root'
+        # Assume we get the right action id from pkaction(1) even with
+        # polikt version < 0.110 (pkaction always exists with status 1), see:
+        # https://bugs.freedesktop.org/show_bug.cgi?id=29936#attach_78263
+        mock_check_output.side_effect = CalledProcessError(
+            1, '', b"org.freedesktop.policykit.pkexec.run-plainbox-job\n")
+        # Ensure that we get a positive score of three
+        ctrl = self.CLS(self.SESSION_DIR, self.PROVIDER_LIST)
+        self.assertEqual(ctrl.get_checkbox_score(self.job), -1)
+
+    @mock.patch.dict('plainbox.impl.ctrl.os.environ', clear=True)
     @mock.patch('plainbox.impl.ctrl.check_output')
     def test_get_checkbox_score_for_secure_provider_root_job_no_policy(
             self, mock_check_output):