More comprehensive "make clean" action #351

conorsch · 2019-11-26T17:28:18Z

Expands the "make clean" logic to purge configuration files on dom0 and sys-firewall created during the installation of SDW. Now "make clean" will restore the system state quite nearly to the point prior to installing in the first place. One notable exception is that it won't purge the TemplateVM RPM packages, because re-fetching those every time would be time- and bandwidth-intensive.

Relevant issues:

Closes Make "clean" action more robust #337
Closes make clean does not remove sd-svs-disp and sd-svs-disp template #350
Refs Updates dom0 repo URL #349 (test cleanup)
Refs fedora VMs fail to upgrade: unrecognized arguments: --dist_upgrade #339 (use of update.qubes-vm at install time)

The clean-it-like-you-mean-it approach will be particularly useful as we tinker with strategies for improving the update mechanisms (#341, freedomofpress/securedrop-updater#34).

There are a few changes in here, particularly the top file consolidation, that result in a large diff, but a substantially more readable configuration over time. While not the best metric, the changes reduce both the number of files and the number of LoC to maintain. If you weight the transition from Bash to Python, then the results are even better.

LoC Before

 $ cloc . --force-lang YAML,sls --force-lang YAML,top 
      89 text files.
      89 unique files.                              
      29 files ignored.

github.com/AlDanial/cloc v 1.81  T=0.02 s (3037.3 files/s, 142505.4 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
YAML                             43            145            272            835
Python                           16            179            107            573
Markdown                          2            240              0            562
Bourne Again Shell                9             71             86            209
make                              1             35              9            117
JSON                              1              0              0             10
Dockerfile                        1              2              1              9
Bourne Shell                      1              1              3              6
--------------------------------------------------------------------------------
SUM:                             74            673            478           2321
--------------------------------------------------------------------------------

LoC after

 $ cloc . --force-lang YAML,sls --force-lang YAML,top 
      70 text files.
      70 unique files.                              
      29 files ignored.

github.com/AlDanial/cloc v 1.81  T=0.02 s (2380.2 files/s, 147876.9 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
YAML                             25            131            238            830
Python                           17            193            121            627
Markdown                          2            240              0            562
Bourne Again Shell                7             63             75            160
make                              1             34              9            102
JSON                              1              0              0             10
Dockerfile                        1              2              1              9
Bourne Shell                      1              1              3              6
--------------------------------------------------------------------------------
SUM:                             55            664            447           2306
--------------------------------------------------------------------------------

Testing

make clean # to run the old clean logic from master
qvm-ls --tags sd-workstation # observe sd-svs-disp still hanging around
make clone
make clean # to run the new clean logic from this PR
qvm-ls --tags sd-workstation # observe only RPM-installed template remains
make all
make test

Should have been included as part of #349. That's what I get for omitting `make test` as part of the test plan. =)

There's no need for a 1:1 correspondence between Salt states and top files. A single top-level provides the same capability to map Salt states against target VMs. When running, Salt will merge all separate top files to plan the execution steps anyway. Let's consolidate to make the config flatter and more readable.

Ensures that the various system files configured on dom0 and sys-firewall as part of installation are removed as part of the "make clean" action. The file cleanups are implemented in Salt. The VM destruction has been ported from Bash to Python (via the Qubes Admin API). The removal logic is smarter: it only removes VMs that have the tag "sd-workstation", and removes TemplateVMs last, to avoid ordering problems in the execution of `qvm-remove`.

Rather than duplicating logic to mitigate apt CVEs, let's use the Qubes-maintained Salt calls to manage VM upgrades. The reuse will be particularly helpful as we revisit the unattended-vm-upgrades-via-cron logic.

Removes hardcoding of VM names to leverage bits of the Admin API (via `qvm-ls --tags`) where appropriate. Updated comments throughout. Trying to lean on Salt's parallel execution as much as possible, to keep runtimes low. The step-by-step execution of Salt (sys-firewall, then dom0, then VMs) could be handled by Salt orchestrators, but not implementing such a substantial refactor right now. [0] https://docs.saltstack.com/en/latest/topics/orchestrate/orchestrate_runner.html

emkll

Thanks @conorsch this is a very significant and important improvement.
I've tested make all make test make clean make all and everything works exactly as it should, and I cannot find any workstation-related files after the clean operation.

I have a few comments/questions inline for discussion. I don't think these are significant enough to block merge, but will leave it to your judgement if you would like to append a commit.

In the meantime, I will rebase the changes in #352 on top of these.

emkll · 2019-11-28T21:21:46Z

dom0/sd-clean-all.sls

+sd-cleanup-rpc-mgmt-policy:
+  file.replace:
+    - names:
+      - /etc/qubes-rpc/policy/qubes.VMShell


these policies do not exist on my machine. How/when are these created? qubesctl?

These policies are created automatically during upgrades via a disp-mgmt VM, and also removed automatically. If someone ctrl+c's the make all process, it's possible for them to hang around. Possibly overkill to worry about them here, I'm happy to excise.

emkll · 2019-11-28T21:24:50Z

dom0/sd-clean-all.sls

+    - repl: ''
+    - pattern: '^disp-mgmt-sd-\w+\s+sd-\w+\s+allow,user=root'
+
+{% set sdw_customized_rpc_files = salt['cmd.shell']('grep -rl "BEGIN securedrop-workstation" /etc/qubes-rpc/ | cat').splitlines() %}


Awesome! There's one edge case that we could. When vim or other editor leaves a swap file (like it did on my system), the file is viewed as a binary and the make clean action will fail. This is obviously an edge case (and deleting that file resolved), see output below. Perhaps grepping for strings that don't start with a dot is sufficient here, but IMO not required for merge.

---------- ID: sd-cleanup-rpc-policy-grants Function: file.replace Name: /etc/qubes-rpc/policy/.qubes.Filecopy.swp Result: False Comment: An exception occurred in this state: Traceback (most recent call last): File "/usr/lib/python2.7/site-packages/salt/state.py", line 1837, in call **cdata['kwargs']) File "/usr/lib/python2.7/site-packages/salt/loader.py", line 1794, in wrapper return f(*args, **kwargs) File "/usr/lib/python2.7/site-packages/salt/states/file.py", line 3934, in replace ignore_if_missing=ignore_if_missing) File "/usr/lib/python2.7/site-packages/salt/modules/file.py", line 2035, in replace .format(path) SaltInvocationError: Cannot perform string replacements on a binary file: /etc/qubes-rpc/policy/.qubes.Filecopy.swp Started: 16:16:15.438709 Duration: 5.279 ms Changes: Summary for local ------------- Succeeded: 28 (changed=26) Failed: 1

Hmm, great catch. We can just pass -I to grep so it doesn't report binary files. That should handle the edge case you describe.

emkll · 2019-11-28T21:25:25Z

dom0/sd-workstation.top

@@ -0,0 +1,39 @@
+# -*- coding: utf-8 -*-


This is a massive improvement

emkll · 2019-11-28T21:29:06Z

scripts/destroy-vm

+    if vm.is_running():
+        vm.kill()
+    print("Destroying VM '{}'... ".format(vm.name), end="")
+    subprocess.check_call("qvm-remove -f {}".format(vm.name).split())


nit: why not explicitly write the array here? probably less overall characters and easier to read

Fair, that's a rather inscrutable line as written. 😃 Will update as you suggest!

emkll · 2019-11-28T21:30:23Z

scripts/destroy-vm

+    """
+    # Remove DispVMs first, then AppVMs, then TemplateVMs last.
+    sdw_vms = [vm for vm in q.domains if SDW_DEFAULT_TAG in vm.tags]
+    sdw_template_vms = [vm for vm in sdw_vms


This is very helpful, let's refactor/reuse this in #352

emkll · 2019-11-28T21:34:01Z

dom0/fpf-apt-test-repo.sls

-      - test -f /etc/apt/sources.list
+#
+include:
+  - update.qubes-vm


Where is this salt file located? Is this provided by qubes' build in salt mgmt commands? Might be worth adding a comment here for future maintainers.

+1 for more comments everywhere, will add.

emkll · 2019-11-28T21:34:22Z

dom0/fpf-apt-test-repo.sls

@@ -22,22 +11,14 @@ install-python-apt-for-repo-config:
    - pkgs:
      - python-apt
    - require:
-      - file: remove-jessie-backports-repo
+      # Require that the Qubes update state has run first


same comment as https://github.com/freedomofpress/securedrop-workstation/pull/351/files#diff-f739e903ed54a409c69c5835698a9336R5

emkll · 2019-11-29T14:24:06Z

It just occurred to me, as I am reading Qubes Fedora-29 EOL announcement [1], that even with the changes introduced here, we not remove unused RPM template in dom0: but we would need to check if any AppVMs are based on that template, otherwise the dnf remove operation would fail.

This

[1] https://www.qubes-os.org/news/2019/11/29/fedora-29-eol/

emkll · 2019-12-02T15:04:27Z

dom0/sd-clean-all.sls

+    - flags:
+      - MULTILINE
+      - DOTALL
+    - repl: ''


While running make all/make clean several times, I've observed blank lines in the rpc policy files

I think that adding append_newline to False here could resolve

Interesting, have not seen that, will get a repro and cleanup as you suggest.

conorsch · 2019-12-02T17:55:30Z

@emkll All requested changes have been addressed. Ready for re-review.

we not remove unused RPM template in dom0: but we would need to check if any AppVMs are based on that template, otherwise the dnf remove operation would fail.

Agreed, we shouldn't be purging the old RPMs until we're sure the operation will succeed. As we make heavier use of the Admin API, we can implement "migration" tasks that handle each case intelligently, including references to DispVM settings. Both are relevant for #352, will review with that in mind.

@emkll

Hat tip to @emkll. Specifically: * Don't process binary files with grep in dom0 * Makes destroy-vm subprocess cmd more readable * Adds comments to use of "update.qubes-vm" SLS * Removes extraneous newlines from RPC policy cleanup

emkll

Thanks for the changes @conorsch

top.enable was removed in #351 in favor of sd-workstation.top file. top.enable will now be handled by prep-salt target, invoked by prep-dom0.

Conor Schaefer added 5 commits November 25, 2019 16:09

Updates config tests for dom0 repo URL

f4eac01

Should have been included as part of #349. That's what I get for omitting `make test` as part of the test plan. =)

Includes upstream update.qubes-vm formula

63edaed

Rather than duplicating logic to mitigate apt CVEs, let's use the Qubes-maintained Salt calls to manage VM upgrades. The reuse will be particularly helpful as we revisit the unattended-vm-upgrades-via-cron logic.

conorsch requested review from kushaldas, emkll and rmol November 26, 2019 17:28

conorsch mentioned this pull request Nov 27, 2019

Update base templates to buster #352

Merged

emkll added the small off-sprint PR label Nov 28, 2019

emkll reviewed Nov 28, 2019

View reviewed changes

emkll reviewed Dec 2, 2019

View reviewed changes

Address review comments

1b7a901

Hat tip to @emkll. Specifically: * Don't process binary files with grep in dom0 * Makes destroy-vm subprocess cmd more readable * Adds comments to use of "update.qubes-vm" SLS * Removes extraneous newlines from RPC policy cleanup

conorsch force-pushed the 337-new-and-improved-make-clean branch from 22f4b1d to 1b7a901 Compare December 2, 2019 18:07

emkll approved these changes Dec 2, 2019

View reviewed changes

emkll merged commit ef7f86f into master Dec 2, 2019

emkll deleted the 337-new-and-improved-make-clean branch December 2, 2019 18:55

emkll added a commit that referenced this pull request Jan 6, 2020

fix prep-dom0 target

a706f44

top.enable was removed in #351 in favor of sd-workstation.top file. top.enable will now be handled by prep-salt target, invoked by prep-dom0.

emkll mentioned this pull request Jan 6, 2020

Fix prep-dom0 target #395

Merged

3 tasks

conorsch mentioned this pull request Mar 8, 2021

Test install process, workstation functionality against Qubes 4.0.4 #640

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More comprehensive "make clean" action #351

More comprehensive "make clean" action #351

conorsch commented Nov 26, 2019

emkll left a comment

emkll Nov 28, 2019

conorsch Dec 2, 2019

emkll Nov 28, 2019

conorsch Dec 2, 2019

emkll Nov 28, 2019

emkll Nov 28, 2019

conorsch Dec 2, 2019

emkll Nov 28, 2019

emkll Nov 28, 2019

conorsch Dec 2, 2019

emkll Nov 28, 2019

emkll commented Nov 29, 2019

emkll Dec 2, 2019

conorsch Dec 2, 2019

conorsch commented Dec 2, 2019

emkll left a comment

More comprehensive "make clean" action #351

More comprehensive "make clean" action #351

Conversation

conorsch commented Nov 26, 2019

LoC Before

LoC after

Testing

emkll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkll commented Nov 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Dec 2, 2019

emkll left a comment

Choose a reason for hiding this comment