Restrict and test and qubes-rpc policies for dom0 #187

emkll · 2018-11-02T18:01:07Z

Status

Ready for review.

Fixes #175, #147

Add tag for workstation provisioned templates and AppVM (sd-workstation)
Create tests to ensure workstation-provisioned templates and AppVMs have label properly applied
Consolidated qubes-rpc policies in dom0/sd-dom0-qvm-rpc.sls, to make it easier to manage and slightly more idempotent.
Create tests to ensure workstation provisioned templates and AppVMs have deny-by-default and least privilege qubes-rpc policies, by way of sd-workstation label.

Test plan

clean && make all should succeed with no error
make test should pass
Editing the top of the rpc policies should cause tests to fail
Ensure qubes-rpc policies in dom0-sdkfjdsf are sane
And, as always:
Download, decrypt and view a plaintext submission in disposable VM
Download, decrypt and view a submission (e.g., image) in disposable VM

conorsch · 2018-11-06T22:18:27Z

Tested and uncovered one error. Output from test failure:

python -m unittest discover tests
............Policy for OpenInVM is:
sd-journalist sd-decrypt allow
sd-journalist sd-svs allow
$tag:sd-svs-disp-vm sd-svs allow
sd-svs $dispvm:sd-svs-disp allow
$anyvm $tag:sd-workstation deny
sd-svs $dispvm:sd-svs-disp allow
## Note that policy parsing stops at the first match,
## so adding anything below "$anyvm $anyvm action" line will have no effect

## Please use a single # to start your custom comments

$anyvm	$dispvm	allow
$anyvm	$anyvm	ask

Policy for OpenInVM should be:
sd-journalist sd-svs allow
$tag:sd-svs-disp-vm sd-svs allow
sd-svs $dispvm:sd-svs-disp allow
$anyvm $tag:sd-workstation deny
F
.
.
.
.
....
.
.
.
.
..
======================================================================
FAIL: test_Policies (test_qubes_rpc.SD_Qubes_Rpc_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/securedrop-workstation/tests/test_qubes_rpc.py", line 25, in test_Policies
    self.assertFalse(fail)
AssertionError: True is not false

----------------------------------------------------------------------
Ran 27 tests in 209.334s

FAILED (failures=1)

The Qubes workstation I'm using was installed fresh about a week ago, so looks like the RPC policy files have had additional comments added. I've also been reviewing #174, so will re-run the test plan here to ensure I'm testing in a rather clean environment.

conorsch · 2018-11-07T01:34:34Z

Was able to reproduce the error above, even after cleaning up my dom0 env of leftovers from reviewing #174. Ported the OpenInVM logic to use Salt's blockreplace module. With that change in place, the RPC policy was fixed on disk in dom0, and the tests began passing. I was able to add junk lines to the policy and re-provision, causing the tests to fail; reverting the junk lines caused the salt logic to "fix" the files again, with tests passing.

We should update all the policy adds to use similar logic, so we can confidently manage changes over time. As long as we keep the "markers" stable, we can insert or remove customizations at will. Some items of note for making the requested changes here:

Use |, not |-, for YAML scalar syntax; we want the trailing newline, so lines aren't concatenated
Make sure the markers are identical throughout all config tasks
Consider updating the RPC config tests (added in this PR) to hardcode the BEGIN/END markers; that should catch any typos in the future

conorsch

Tests are an excellent addition. A few changes requested; see comments in-line; summarized:

convert file.prepend calls -> file.blockreplace (note implementations in Restrict and test and qubes-rpc policies for dom0 #187 (comment))
convert cmd.run qvm-tags calls -> qvm.tags.add blocks on VM creation
show-output on makefile target, so it's easier to debug how files change

conorsch · 2018-11-07T01:36:29Z

Makefile

 	update-whonix-templates prep-whonix prep-dom0 sd-workstation-template \
 	sd-whonix sd-svs sd-gpg	\
 	sd-journalist sd-svs-disp

 clone: assert-dom0 ## Pulls the latest repo from work VM to dom0
 	@./scripts/clone-to-dom0

+qubes-rpc: prep-salt ## Places default deny qubes-rpc policies for sd-svs and sd-gpg
+	sudo qubesctl top.enable sd-dom0-qvm-rpc
+	sudo qubesctl --targets sd-dom0-qvm-rpc state.highstate


Change

sudo qubesctl --targets sd-dom0-qvm-rpc state.highstate

to

sudo qubesctl --show-output --targets sd-dom0-qvm-rpc state.highstate

to match changes in #190.

conorsch · 2018-11-07T01:37:02Z

dom0/sd-dom0-qvm-rpc.sls

+##
+
+dom0-rpc-qubes.ClipboardPaste:
+  file.prepend:


Change file.prepend to file.blockreplace throughout; see the OpenInVM config task for an example.

conorsch · 2018-11-07T01:41:31Z

dom0/sd-gpg.sls

+# Add tag for default qubes-rpc deny catch-all rule
+# (see dom0/sd-dom0-qvm-rpc.sls)
+qvm-tags sd-gpg add sd-workstation:
+  cmd.run


Convert the cmd.run calls to qvm-tags to a - tags: block on the VM spec; see the - prefs: block as an example. Docs here: https://github.com/QubesOS/qubes-mgmt-salt-dom0-qvm/blob/master/README.rst#qvm-vm Note that the syntax is slightly different when using the qvm wrapper lib; e.g.

- tags: - add: - sd-workstation

conorsch · 2018-11-07T01:42:30Z

tests/test_vms_exist.py

@@ -41,6 +41,7 @@ def test_sd_whonix_config(self):
        self.assertTrue(vm.template == "sd-whonix-template")
        self.assertTrue(vm.provides_network)
        self.assertFalse(vm.template_for_dispvms)
+        self.assertTrue('sd-workstation' in vm.tags)


👌 Great addition; keeping these tests will help in confirming that the cmd.run -> qvm.tags conversion suggested above works well.

conorsch · 2018-11-07T01:46:45Z

tests/test_qubes_rpc.py

+            else:
+                print("Policy for {} is:\n{}".format(filename,
+                                                     actualPolicy))
+                print("Policy for {} should be:\n{}".format(filename,


Nit: more aggressive output would be helpful here for when the policies fail. Consider adding more newlines and header blocks like:

print("\n\n#### BEGIN RPC policy error report ####\n\n") print("Policy for {} is:\n{}".format(filename, actualPolicy)) print("Policy for {} should be:\n{}".format(filename, expectedPolicy)) print("\n\n#### END RPC policy error report ####\n\n")

emkll · 2018-11-08T19:52:18Z

@conorsch thanks for all the great feedback. I've incorporated it and cleaned up the git history, this should now be ready for re-review!

conorsch · 2018-11-08T19:54:17Z

Roger that, re-reviewing!

This will ensure we guard against regressions, configuration drift, and enforce least-privilege on qubes-rpc policies for sd-workstation App and Template VMs.

This will ensure we will be able to apply preferences (e.g. qubes-rpc policies) accross all VMs managed in the Qubes Workstation. Added configuration tests for templates VMs that were created as part of #160

@conorsch

Both templates and AppVMs provisioned by the SecureDrop workstation now have the `sd-workstation` tag. This tag will be used to prepend a default deny for all Qubes-RPC operations at the start of the install process. The SecureDrop installer will prepend to this list any other grant necessary. sd-dom0-qvm-rpc will add default deny rule for all policies at the end of the install to ensure that it overrides any policies that may have been added by error as part of the other tasks. @conorsch recommended use blockreplace for the following reasons: "We want to ensure that the newest config (which will change over time) is added to the top of the file. It's OK to add additional grants elsewhere (mostly for developers), but the Workstation-specific grants should be enforced first. Saltstack's "blockreplace" module allows this, and convenient supports "markers" to ensure updates over time are inserted in the correct location. Better yet, it's now obvious where the grants are coming from: the securedrop-workstation config."

This is already handled by the securedrop-workstation-config package. Furthermore, the package won't persist in sd-svs because it is an AppVM.

conorsch · 2018-11-08T21:42:13Z

Works like a charm. After full provision with passing tests, I edited one of the Salt files to write a bad config, and the tests started failing. Reverted the fix, reapplied, and tests are passing—so the new logic is correctly updating the files in-place, as we want.

emkll requested review from joshuathayer and conorsch November 2, 2018 18:01

conorsch suggested changes Nov 7, 2018

View reviewed changes

emkll force-pushed the 175-qubes-rpc branch 6 times, most recently from 60f4a52 to 1aa5fe9 Compare November 8, 2018 19:25

emkll added 4 commits November 8, 2018 14:57

Add tests for qubes-rpc grants

a7fd99e

This will ensure we guard against regressions, configuration drift, and enforce least-privilege on qubes-rpc policies for sd-workstation App and Template VMs.

Add sd-workstation tag to SecureDrop Workstation App and Template VMs

7b7b1a1

This will ensure we will be able to apply preferences (e.g. qubes-rpc policies) accross all VMs managed in the Qubes Workstation. Added configuration tests for templates VMs that were created as part of #160

Do not install nautilus in sd-svs

0e9e71f

This is already handled by the securedrop-workstation-config package. Furthermore, the package won't persist in sd-svs because it is an AppVM.

emkll force-pushed the 175-qubes-rpc branch from 1aa5fe9 to 0e9e71f Compare November 8, 2018 19:58

conorsch approved these changes Nov 8, 2018

View reviewed changes

conorsch merged commit 4091999 into master Nov 8, 2018

conorsch mentioned this pull request Nov 12, 2018

updates sd-gpg autoaccept timeout to 8 hours #207

Merged

redshiftzero mentioned this pull request Nov 12, 2018

Add tests for dom0 configuration #147

Closed

emkll mentioned this pull request Nov 13, 2018

Further restrict qubes-rpc policies #212

Closed

emkll deleted the 175-qubes-rpc branch November 13, 2018 22:18

conorsch mentioned this pull request Aug 22, 2019

Update qrexec policy keywords #310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict and test and qubes-rpc policies for dom0 #187

Restrict and test and qubes-rpc policies for dom0 #187

emkll commented Nov 2, 2018

conorsch commented Nov 6, 2018

conorsch commented Nov 7, 2018

conorsch left a comment

conorsch Nov 7, 2018

conorsch Nov 7, 2018

conorsch Nov 7, 2018

conorsch Nov 7, 2018

conorsch Nov 7, 2018

emkll commented Nov 8, 2018

conorsch commented Nov 8, 2018

conorsch commented Nov 8, 2018

Restrict and test and qubes-rpc policies for dom0 #187

Restrict and test and qubes-rpc policies for dom0 #187

Conversation

emkll commented Nov 2, 2018

Status

Test plan

conorsch commented Nov 6, 2018

conorsch commented Nov 7, 2018

conorsch left a comment

Choose a reason for hiding this comment

conorsch Nov 7, 2018

Choose a reason for hiding this comment

conorsch Nov 7, 2018

Choose a reason for hiding this comment

conorsch Nov 7, 2018

Choose a reason for hiding this comment

conorsch Nov 7, 2018

Choose a reason for hiding this comment

conorsch Nov 7, 2018

Choose a reason for hiding this comment

emkll commented Nov 8, 2018

conorsch commented Nov 8, 2018

conorsch commented Nov 8, 2018