Remove special casing for sd-whonix #618

eloquence · 2020-10-02T01:20:20Z

Part of #583

Description

Removes Salt logic for custom handling of whonix-gw-15 integration with sd-log and instead treats it like any other TemplateVM.

Removes the vmname testing from dom0 tests. The functionality remains in securedrop-log, but is only likely to be used during development.

Status

Ready for review.

Test plan

Prerequisites

The changes in Use qubesdb-read instead of gethostname securedrop-log#18 have been merged and have landed in the latest nightly on https://apt-test.freedom.press/pool/main/s/securedrop-log/
You have a working staging environment built from latest main of securedrop-workstation.

Instructions

Remove ~/QubesIncomingLogs/host directory on sd-log if it exists from previous runs
make clone from this branch.
make staging with a valid config. This will apply the Salt states from this PR.
Ensure the sd-whonix VM, the sd-app VM, and the sd-log VM are all booted.

On sd-whonix: Observe that /rw/config/rc.local no longer contains any customizations
On sd-whonix: Observe that /rw/config/sdlog.conf no longer exists.
On sd-log: Observe that no files exist under host in ~/QubesIncomingLogs
On sd-log: Observe that logs are correctly ingested under sd-whonix.
On sd-log: Observe that logs are correctly ingested under sd-app.

make clean in dom0.

On whonix-gw-15: Observe that /etc/rsyslog.d/sdlog.conf has been removed

eloquence · 2020-10-06T03:09:36Z

dom0/sd-logging-setup.sls

-    - source: "salt://sdlog.conf"
-
-# Because whonix-gw-15 template is not allowing to create the config file on
-# package install time, we do it via rc.local call.


For the record, this was added in 232c56f. @kushaldas, can you elaborate on what you meant at the time with "whonix-gw-15 template is not allowing to create the config file on package install time"? As far as I can tell, the config file /etc/rsyslog.d/sdlog.conf is installed just fine from the securedrop-log package, and I've not observed any behavior specific to whonix-gw-15 that would prevent this from being the case.

#447 this is the PR and as I can see in my logs, the file was still missing even after the package was installed.

Here is my note from that work:

* https://github.com/freedomofpress/securedrop-workstation/pull/447 howt to remove the files? - how to remove gpg key? sudo apt-key del KEY_ID - `make clean` - remove works properly - `make all` agagin. - most of the vms decided not to reply to Qubes calls :( - cleaned, and `make all` again - Someday Qubes and Salt will play nicely together. Question is in which year? - this try, the package is there, but, no logs. - because the sdlog.conf file is missing, WHY WHY WHY - I this before, i commented on the issue, dpkg did not create the sdlog.conf file at all on the system. - one idea, if the file is missing, just copy it over via rc.local file - testing again - testing again, adding sdlog.conf only for sd-whonix if missing. - testing once more, things look better now - meeting with Mickael - need to test some more changes - make clean and make all again at super late night :)

Hi @kushaldas, I experienced this once during testing with dpkg, and in my experience it happens when you've previously removed the file by hand and uninstalled (rather than purged) the package - regardless of the order of the two operations. It's not specific to Whonix, and is just how Debian handles configuration files. Because we purge the file correctly with our uninstall logic, I don't believe it should be an issue on production systems.

eloquence · 2020-10-06T03:30:31Z

dom0/sd-clean-whonix.sls

@@ -14,7 +14,6 @@ remove-securedrop-log-package-from-whonix:
 sd-cleanup-whonix-gw-15:
  cmd.run:
    - names:
-      - sudo rm -f /etc/rsyslog.d/sdlog.conf


Note that we use pkg.purged to remove the securedrop-log package, which removes config files:

Verify that a package is not installed, calling pkg.purge if necessary to purge the package. All configuration files are also removed.

Removing a configuration file on a Debian system by other means can have unpredictable consequences: the package manager will not reinstate it , because it will assume that the user removed it intentionally. For this reason, I think it's best to rely on pkg.purged alone here.

eloquence · 2020-10-06T04:00:56Z

I've added a test plan. I would recommend the following order of operations:

Initial visual review by @kushaldas of the logic changes here and in Use qubesdb-read instead of gethostname securedrop-log#18. Specifically, if there' s anything I'm overlooking that makes the use of /rw/config necessary for sd-whonix, that would be good to know (see comment here: Remove special casing for sd-whonix #618 (comment))
Review and merge of Use qubesdb-read instead of gethostname securedrop-log#18
Review and merge of this PR.

conorsch · 2020-10-08T23:31:46Z

See comment in freedomofpress/securedrop-log#18 (review): we must also update the tests/test_sd_whonix.py checks here in order to get all tests passing!

eloquence · 2020-10-09T23:32:41Z

we must also update the tests/test_sd_whonix.py checks here in order to get all tests passing!

Roger that, will update the tests on Tuesday and then promote to ready. :)

eloquence · 2020-10-13T22:18:59Z

This is ready for review, but to take it for a spin with the associated securedrop-log changes, it would IMO be best to kick off a nightly build of that package; nightlies seem to have gotten stuck around October 5.

conorsch · 2020-10-14T00:48:47Z

tests/test_sd_whonix.py

@@ -54,9 +54,6 @@ def test_sd_whonix_repo_enabled(self):
        """
        assert self._fileExists(self.whonix_apt_list)

-    def test_logging_configured(self):
-        self.logging_configured(vmname=True)
-


The test_logging_configured shouldn't be removed, it should just have its call updated to pass no args (matching the modified test logic).

Fixed (I misread the inheritance logic).

conorsch · 2020-10-14T00:50:42Z

nightlies seem to have gotten stuck around October 5.

You're right! Resolved out of band. Latest version of securedrop-log is now served on apt-test, so testers should not see a sd-log:~/QubesIncomingLogs/host/ dir created any more.

Started, but did not finish review. Left one comment about the test logic—to any future reviewers, e.g. @zenmonkeykstop, please tack on a test if you agree. Also make sure to run make test as part of review, since we should see 100% of tests passing now.

We now handle this in the logger by avoiding reliance on gethostname(). Enforce removal of sd-whonix configuration in /rw/config/rc.local and ensure that /rw/config/sdlog.conf is absent. Don't rm a file that's already being purged as part of package removal.

conorsch · 2020-10-14T21:35:32Z

Reviewing now!

rmol

👍 Ran through the test plan, and other than sd-app not being started by make staging, the changes specific to this PR look good and worked.

Remove ~/QubesIncomingLogs/host directory on sd-log if it exists from previous runs
make clone from this branch.
make staging with a valid config. This will apply the Salt states from this PR.
Ensure the sd-whonix VM, the sd-app VM, and the sd-log VM are all booted.

⚠️ sd-app was not booted after make staging completed.

On sd-whonix: Observe that /rw/config/rc.local no longer contains any customizations
On sd-whonix: Observe that /rw/config/sdlog.conf no longer exists.
On sd-log: Observe that no files exist under host in ~/QubesIncomingLogs
On sd-log: Observe that logs are correctly ingested under sd-whonix.
On sd-log: Observe that logs are correctly ingested under sd-app.

make clean in dom0.

On whonix-gw-15: Observe that /etc/rsyslog.d/sdlog.conf has been removed

conorsch · 2020-10-14T22:37:25Z

sd-app was not booted after make staging completed.

That makes sense: we don't actually enforce that state (although we should). So, my read of the test plan is "manually start those VMs before you proceed to the next step."

eloquence added the blocked label Oct 2, 2020

eloquence mentioned this pull request Oct 5, 2020

Use qubesdb-read instead of gethostname freedomofpress/securedrop-log#18

Merged

3 tasks

eloquence force-pushed the 583-host-to-ghost branch from 4c22e73 to 07c877f Compare October 6, 2020 00:51

eloquence commented Oct 6, 2020

View reviewed changes

eloquence removed the blocked label Oct 13, 2020

eloquence marked this pull request as ready for review October 13, 2020 22:17

conorsch reviewed Oct 14, 2020

View reviewed changes

eloquence force-pushed the 583-host-to-ghost branch from 8bd155e to c81c29b Compare October 14, 2020 03:19

rmol self-assigned this Oct 14, 2020

rmol approved these changes Oct 14, 2020

View reviewed changes

rmol merged commit 396ec0c into main Oct 14, 2020

rmol deleted the 583-host-to-ghost branch October 14, 2020 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove special casing for sd-whonix #618

Remove special casing for sd-whonix #618

eloquence commented Oct 2, 2020 •

edited

Loading

eloquence Oct 6, 2020 •

edited

Loading

kushaldas Oct 8, 2020

eloquence Oct 8, 2020

eloquence Oct 6, 2020

eloquence commented Oct 6, 2020

conorsch commented Oct 8, 2020

eloquence commented Oct 9, 2020 •

edited

Loading

eloquence commented Oct 13, 2020

conorsch Oct 14, 2020

eloquence Oct 14, 2020

conorsch commented Oct 14, 2020

conorsch commented Oct 14, 2020

rmol left a comment

conorsch commented Oct 14, 2020

Remove special casing for sd-whonix #618

Remove special casing for sd-whonix #618

Conversation

eloquence commented Oct 2, 2020 • edited Loading

Description

Status

Test plan

Prerequisites

Instructions

eloquence Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

kushaldas Oct 8, 2020

Choose a reason for hiding this comment

eloquence Oct 8, 2020

Choose a reason for hiding this comment

eloquence Oct 6, 2020

Choose a reason for hiding this comment

eloquence commented Oct 6, 2020

conorsch commented Oct 8, 2020

eloquence commented Oct 9, 2020 • edited Loading

eloquence commented Oct 13, 2020

conorsch Oct 14, 2020

Choose a reason for hiding this comment

eloquence Oct 14, 2020

Choose a reason for hiding this comment

conorsch commented Oct 14, 2020

conorsch commented Oct 14, 2020

rmol left a comment

Choose a reason for hiding this comment

conorsch commented Oct 14, 2020

eloquence commented Oct 2, 2020 •

edited

Loading

eloquence Oct 6, 2020 •

edited

Loading

eloquence commented Oct 9, 2020 •

edited

Loading