Distinguishes between TemplateVM & AppVM in log collection #487
Conversation
conorsch
force-pushed
the
476-simplify-logging-hostname-config
branch
from
ea4aaf7 to
0775f06
Compare
Come to think of it, that deserves a config test. Added a few service-related checks in this PR, but we should also guard against regressions. |
There was a problem hiding this comment.
Thanks @conorsch , left some comments in freedomofpress/securedrop-log#16 (review) . for sd-app and sd-devices (and their templates), everything does seem to work as expected. However, I've observed a few regressions with other VMs:
-
Logging in whonix-based VMs does not go to the correct folder on
sd-logdue toHOSTNAMEbeing set tohost -
There's also a regression in sd-viewer-dvm, sd-viewer and sd-viewer-buster-template where the rsyslog service start but appears to throw an error (see inline). The service is marked as running but it seems like logs are not flowing properly (not sure if this is due to local changes, but i also don't see where you are installing/configuring the
sd-logging-setupfor sd-viewer* in the diff introduced here.
| @@ -38,15 +40,12 @@ base: | |||
| - sd-sys-firewall-files | |||
There was a problem hiding this comment.
Not sure if this is a regression, but getting an issue with any sd-viewer-* vm:
Mar 05 15:46:42 sd-viewer rsyslogd[465]: omprog: program '/usr/sbin/sd-rsyslog' (pid 5737) terminated; will be restarted [v8.1901.0 try https://www.rsyslog.com/e/2119 ]
Mar 05 15:46:42 sd-viewer rsyslogd[465]: action 'action-0-omprog' suspended (module 'omprog'), retry 0. There should be messages before this one giving the reason for suspension. [v8.1901.0 try https://www.rsysl
Mar 05 15:46:43 sd-viewer rsyslogd[465]: action 'action-0-omprog' resumed (module 'omprog') [v8.1901.0 try https://www.rsyslog.com/e/2359 ]
Mar 05 15:46:43 sd-viewer rsyslogd[465]: child process (pid 5738) exited with status 1 [v8.1901.0]
Mar 05 15:46:43 sd-viewer rsyslogd[465]: omprog: program '/usr/sbin/sd-rsyslog' (pid 5738) terminated; will be restarted [v8.1901.0 try https://www.rsyslog.com/e/2119 ]
Mar 05 15:46:43 sd-viewer rsyslogd[465]: action 'action-0-omprog' suspended (module 'omprog'), retry 0. There should be messages before this one giving the reason for suspension. [v8.1901.0 try https://www.rsysl
Mar 05 15:46:44 sd-viewer rsyslogd[465]: action 'action-0-omprog' resumed (module 'omprog') [v8.1901.0 try https://www.rsyslog.com/e/2359 ]
Mar 05 15:46:44 sd-viewer rsyslogd[465]: child process (pid 5745) exited with status 1 [v8.1901.0]
Mar 05 15:46:44 sd-viewer rsyslogd[465]: omprog: program '/usr/sbin/sd-rsyslog' (pid 5745) terminated; will be restarted [v8.1901.0 try https://www.rsyslog.com/e/2119 ]
Mar 05 15:46:44 sd-viewer rsyslogd[465]: action 'action-0-omprog' suspended (module 'omprog'), retry 0. There should be messages before this one giving the reason for suspension. [v8.1901.0 try https://www.rsysl
There was a problem hiding this comment.
Good idea. I've added a config test for this case, but it's failing, so commented it out. We weren't testing for this case before, so we should compare behavior on master and see if the log messages occur there.
On this branch, I see those messages, even though a given VM is shipping logs successfully to sd-log. So it doesn't appear to be a final indicator of failure.
| @@ -0,0 +1,79 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
👍 .sls in dom0/ folder will be automatically picked up by manifest.in and the rpm spec
conorsch
force-pushed
the
476-simplify-logging-hostname-config
branch
from
c057de1 to
548a2ac
Compare
|
I'm running into some issues when running make clean / make clone / make all / make test, are you seeing these? It might be due to changes in whonix-gw-template that were not reverted properly when i ran make clean. Do you think twe should review #488 first, as the perhaps it may have benefits for the issues i've observed |
There was a problem hiding this comment.
With the diff [1] below, tests are passing, and everything looks good. However, I observe errors in sd-gpg due to misconfiguration of the package, as it's installed in the base template. It might make sense to configure logging for sd-gpg, I tried applying the diff below it appears to work [2]. What do you think?
Not sure if this is a regression from the second diff, but sd-whonix doesn't have a netvm when the diff below is applied
[1]
diff --git a/tests/base.py b/tests/base.py
index cc3c89c8a..5bef47494 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -124,14 +124,14 @@ class SD_VM_Local_Test(unittest.TestCase):
self.assertTrue(self._fileExists("/etc/rsyslog.d/sdlog.conf"))
self.assertTrue(self._fileExists("/etc/sd-rsyslog.conf"))
# Then we check the configuration inside of the file.
- # Using .strip() so trailing whitespace doesn't cause failures
- file_content = self._get_file_contents("/etc/sd-rsyslog.conf").strip()
+ file_content = self._get_file_contents("/etc/sd-rsyslog.conf")
static_content = """[sd-rsyslog]
-remotevm = sd-log"""
+remotevm = sd-log
+"""
# A hardcoded vmname should only be present if required,
# since securedrop-log will default to value of `hostname`.
if vmname:
- static_content += "localvm = {}".format(self.vm_name)
+ static_content += "localvm = {}\n".format(self.vm_name)
self.assertEqual(file_content, static_content)
# Check for evidence of misconfigured logging in syslog,
# fail if matching events found
[2]
diff --git a/dom0/sd-logging-setup.sls b/dom0/sd-logging-setup.sls
index ca628492e..29780df1f 100644
--- a/dom0/sd-logging-setup.sls
+++ b/dom0/sd-logging-setup.sls
@@ -43,9 +43,10 @@ sd-log-remove-rsyslog-qubes-plugin:
cmd.run:
- name: /rw/config/rc.local
-{% elif grains['id'] in ["sd-whonix", "sd-proxy", "sd-proxy-buster-template"] %}
+{% elif grains['id'] in ["sd-whonix", "sd-proxy", "sd-proxy-buster-template", "sd-gpg"] %}
# We can not place the file on the template under /etc/rsyslog.d/ because of whonix
-# template. This sdlog.conf file is the same from the securedrop-log package, to
+# template, or because it's the base template.
+# This sdlog.conf file is the same from the securedrop-log package, to
# make sure that rsyslogd use our logging plugin.
sd-rsyslog-sdlog-conf-for-sd-whonix:
file.managed:
diff --git a/dom0/sd-workstation.top b/dom0/sd-workstation.top
index afa613922..611dbe410 100644
--- a/dom0/sd-workstation.top
+++ b/dom0/sd-workstation.top
@@ -25,6 +25,7 @@ base:
sd-devices-buster-template:
- sd-devices-files
sd-gpg:
+ - sd-logging-setup
- sd-gpg-files
sd-log:
- sd-logging-setup
diff --git a/tests/test_gpg.py b/tests/test_gpg.py
index cb8df361d..3348d003d 100644
--- a/tests/test_gpg.py
+++ b/tests/test_gpg.py
@@ -50,6 +50,9 @@ class SD_GPG_Tests(SD_VM_Local_Test):
def test_we_have_the_key(self):
self.assertEqual(get_local_fp(), get_remote_fp())
+ def test_logging_configured(self):
+ self.logging_configured(vmname=True)
+
| @@ -21,13 +21,17 @@ base: | |||
| - sd-remove-unused-templates | |||
|
|
|||
| sd-log-buster-template: | |||
| - sd-log-template-files | |||
| - sd-logging-setup | |||
There was a problem hiding this comment.
This implies that sd-gpg will have this package installed. We might want to configure logging in that VM, since syslog is getting a deluge of errors due to misconfigured sd-log:
Mar 6 15:07:49 localhost rsyslogd: child process (pid 8971) exited with status 1 [v8.1901.0]
Mar 6 15:07:49 localhost rsyslogd: omprog: program '/usr/sbin/sd-rsyslog' (pid 8971) terminated; will be restarted [v8.1901.0 try https://www.rsyslog.com/e/2119 ]
Mar 6 15:07:49 localhost rsyslogd: action 'action-0-omprog' suspended (module 'omprog'), retry 0. There should be messages before this one giving the reason for suspension. [v8.1901.0 try https://www.rsyslog.com/e/2007 ]
Mar 6 15:07:50 localhost rsyslogd: action 'action-0-omprog' resumed (module 'omprog') [v8.1901.0 try https://www.rsyslog.com/e/2359 ]
Mar 6 15:07:50 localhost rsyslogd: child process (pid 8972) exited with status 1 [v8.1901.0]
Mar 6 15:07:50 localhost rsyslogd: omprog: program '/usr/sbin/sd-rsyslog' (pid 8972) terminated; will be restarted [v8.1901.0 try https://www.rsyslog.com/e/2119 ]
Mar 6 15:07:50 localhost rsyslogd: action 'action-0-omprog' suspended (module 'omprog'), retry 0. There should be messages before this one giving the reason for suspension. [v8.1901.0 try https://www.rsyslog.com/e/2007 ]
|
Addressing review comments now, will rebase to include changes from #487, then ping for re-review. |
We were adding multiple tasks, customized per-VM, to several state files. Let's instead intelligently handle all logging-related config in a single state file, and include that in the various VM configurations as necessary. Requires a small change to the "securedrop-log" package (managed separately), to infer the hostname from the logclient, rather than hardcoding it in a config file on each VM. That way, we can reuse the same config file for all machines. There's still an exception
Ensures services are running in 'sd-log', since log aggregation can't work if they're not. These checks are added to make the overall test suite more durable. In addition, updates some of the test logic to stop expecting a hardcoded VM name in the config file. This way, we can reuse the same config logic on all VMs.
We had the same task writing twice in dom0, so removing the duplicate as unnecessary. Also updating the tests to ensure the right policy is configured. In updating the config tests, discovered that we didn't have any checks for the custom (i.e. `securedrop.*`) RPC policies, so added those. Had to adjust the testing path assumptions somewhat in order to accommodate.
The sd-viewer config didn't have the logging setup included, which was an oversight. The Whonix-related VMs, sd-whonix, sd-proxy, and corresponding templates, require special treatment that's now accounted for in the single log-config state file. Significantly expanded test coverage in order to verify the end state.
During the consolidation of the logging config, `sd-gpg` started trying to log again, but it wasn't working correctly. Here we disable the logging config explicitly, to match what's on master, and add a config test to confirm.
The `sd-proxy` machine was previously based on a Whonix Workstation template, which required special handling of the hostname, which is "host" on all Whonix VMs. We've since ported the proxy to the SDW base template, which returns accurate hostnames, so we can drop the custom logic here.
A side-effect of consolidating the log collection logic is that all VMs, even the TemplateVM for sd-log, begins shipping logs immediately upon configuration. That's a problem for sd-log AppVM & TemplateVM because the RPC calls to send logs will cause the AppVM to boot. If the AppVM doesn't have the final config from the TemplateVM, because of an early boot, log aggregation won't work. Let's configure both the TemplateVM & AppVM, then shut down the TemplateVM (automatic as part of salt disp mgmt vm logic), then reboot the AppVM. All tests passing on a clean install.
conorsch
force-pushed
the
476-simplify-logging-hostname-config
branch
from
548a2ac to
c23da19
Compare
|
Ready for re-review. Addressed all comments. Opted to disable logging on |
|
Thanks @conorsch for the changes, and your patience, this looks good to merge from my perspective. Functionally tested as follows:
Confirmed, however there is an sd-gpg folder and syslog that contains logs from provisioning-time. I think this is fine, but we should be aware of this One new change introduced by this PR is that disposable VMs will now have their own folder in QubesIncomingLogs (eg. @conorsch if you want to drop the final commit, I think we can merge this alongside the securedrop-log and packaging changes. I do think that it might be good to capture this diff somewhere (wiki?) as it was extremely useful in the course of testing this pr. |
There was a problem hiding this comment.
- Check out Infers hostname for "localvm" setting securedrop-log#16 and create new tarball (
python3 setup.py sdist) - Check out Updates securedrop-log to 0.1.1 securedrop-builder#150 and build new deb pkg (
PKG_VERSION=0.1.1 make securedrop-log) - Copy new 0.1.1 securedrop-log deb package to
sd-workstation/securedrop-log_0.1.1+buster_all.deb, so you can install it as part of testing without promoting to apt repo -
make clone && make clean && make all && make testpasses without error - Inspect
sd-log:/home/user/QubesIncomingLogs/and ensure that both TemplateVM names and AppVM names are present as subdirs
This is good, thank you @conorsch
conorsch
force-pushed
the
476-simplify-logging-hostname-config
branch
from
c23da19 to
adc6dd4
Compare
|
Thanks @emkll & @kushaldas for the detailed review.
Done: temporary commit referencing a locally built package has been dropped.
Also done, see here: https://github.com/freedomofpress/securedrop-workstation/wiki/Evaluating-new-deb-package-behavior |
There was a problem hiding this comment.
Thanks @conorsch, next steps are as follows:
- Review/merge https://github.com/freedomofpress/securedrop-log/pull/16/files
- Review (but not merge) Updates securedrop-log to 0.1.1 securedrop-builder#150
- Cut 0.1.1 tag
- rerun ci and merge Updates securedrop-log to 0.1.1 securedrop-builder#150
- Merge this PR
|
securedrop-log and packaging changes have been merged per #487 (review), merging this now |
Status
Ready for review.
Description of Changes
Fixes #476
Changes proposed in this pull request:
sd-log:~/QubesIncomingLogs/Testing
These changes require a patch to the
securedrop-logpackage, so in order to test, you'll need to build a custom package. Details below.You'll need to apply the workaround #485 (comment) in order to review effectively.
python3 setup.py sdist)PKG_VERSION=0.1.1 make securedrop-log)sd-workstation/securedrop-log_0.1.1+buster_all.deb, so you can install it as part of testing without promoting to apt repomake clone && make clean && make all && make testpasses without errorsd-log:/home/user/QubesIncomingLogs/and ensure that both TemplateVM names and AppVM names are present as subdirsChecklist
If you have made code changes
make flake8) passes in the development environment (this box maybe left unchecked, as
flake8also runs in CI)If you have made changes to the provisioning logic
All tests (
make test) pass indom0of a Qubes installThis PR adds/removes files, and includes required updates to the packaging
logic in
MANIFEST.inandrpm-build/SPECS/securedrop-workstation-dom0-config.spec^ haven't updated the spec, nor the version; local build passes successfully