Use qubesdb-read instead of gethostname #18
Conversation
This prevents misidentification of Whonix VMs, which always use 'host' as the hostname.
|
I've tested this extensively and am no longer able to reproduce the |
There was a problem hiding this comment.
Works beautifully. Followed the test plan, and am extremely pleased to report the problem appears to be knocked flat! Left some comments about print-vs-logging (which is a bit confusing because this package handles logging).
Regarding the test plan, the only failure I encountered was this:
For good measure, run make test in your dom0 SecureDrop Workstation checkout directory. The sd-log test should no longer fail, and there should be no new test failures.
But that's because the tests had special handling for the sd-whonix case, which we can finally clean up now. I'll tack that on as a comment to freedomofpress/securedrop-workstation#618
sd-rsyslog
Outdated
| vm_name_output, vm_name_error = get_vm_name_process.communicate() | ||
| if vm_name_error != b"": | ||
| print("Error obtaining VM name via qubesdb-read:") | ||
| print(vm_name_error.decode("utf-8").strip()) |
There was a problem hiding this comment.
The print() statements here should really be logging.error() calls instead, no? Elsewhere in the file there's some inconsistency, but where print() is used it includes file=sys.stderr. In both cases, we should prefer logging.error() AFAICT.
There was a problem hiding this comment.
That makes more sense for sure. Will switch to logging.
There was a problem hiding this comment.
Done in 3afc03f, not tested yet.
| print(vm_name_error.decode("utf-8").strip()) | ||
| sys.exit(1) | ||
| localvmname = vm_name_output.decode("utf-8").strip() | ||
| except FileNotFoundError: # not on Qubes? |
There was a problem hiding this comment.
Confirmed that FileNotFoundError is the proper exception here (same as subprocess.check_output would raise):
>>> from subprocess import Popen
>>> p = Popen(["/bin/echo2", "hello"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.7/subprocess.py", line 775, in __init__
restore_signals, start_new_session)
File "/usr/lib/python3.7/subprocess.py", line 1522, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/bin/echo2': '/bin/echo2'
>>>
| # fails, we exit, to avoid falsely identified logs. | ||
| if localvmname is None: | ||
| try: | ||
| get_vm_name_process = Popen(["/usr/bin/qubesdb-read", "/name"], |
There was a problem hiding this comment.
Rather than Popen, we could use subprocess.check_output() and catch CalledProcessError along with FileNotFoundError below, which would avoid the need to inspect the stderr. Not blocking, though, both are easy enough to read.
sd-rsyslog
Outdated
| @@ -76,7 +76,8 @@ def onInit(): | |||
|
|
|||
| global process | |||
| if not os.path.exists("/etc/sd-rsyslog.conf"): | |||
| print("Please create the configuration file at /etc/sd-rsyslog.conf", file=sys.stderr) | |||
| logging.exception("Please create the configuration file at /etc/sd-rsyslog.conf", | |||
| file=sys.stderr) | |||
There was a problem hiding this comment.
>>> logging.exception("oops")
ERROR:root:oops
NoneType: None
>>> logging.exception("oops", file=sys.stderr)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.7/logging/__init__.py", line 1969, in exception
error(msg, *args, exc_info=exc_info, **kwargs)
File "/usr/lib/python3.7/logging/__init__.py", line 1961, in error
root.error(msg, *args, **kwargs)
File "/usr/lib/python3.7/logging/__init__.py", line 1412, in error
self._log(ERROR, msg, args, **kwargs)
TypeError: _log() got an unexpected keyword argument 'file'
The file=sys.stderr is leftover from the print() invocation, I'll clean up and proceed with re-review.
Holdover from the previous refactor from 'print' to 'logging.exception'.
There was a problem hiding this comment.
Works great. Re-tested after changes from previous review, confirmed the "host" dir is no longer showing up. No further asks, looks great. Will wait for freedomofpress/securedrop-workstation#618 to be marked ready-for-review, then tackle that, then we can work on a version bump and packaging PR.
Description
Switches to the use of
qubesdb-readto identify the source VM. This prevents misidentification of Whonix VMs, which always usehostas the hostname, and should generally be a bit more qubesy.Preserves the
localvmoption mainly for testing outside Qubes; if not set,sd-rsyslogwill fail on non-Qubes systems.Fixes freedomofpress/securedrop-workstation#583
Status
Ready for review.
Test plan
Prerequisites
/etc/rsyslog.d/sdlog.confexists onwhonix-gw-15. If not, abort (we'll have to debug why).Instructions
Build a package from this branch by following the standard
securedrop-debian-packagingprocedure. You don't have to bump version numbers as long as you don't run the updater during this test. In a nutshell (assuming build prereqs are installed in your dev VM), runpython3 setup.py sdistin thesecuredrop-logcheckout and thenPKG_VERSION=0.1.1 PKG_PATH=/path/to/package.tgz make securedrop-login thesecuredrop-debian-packagingcheckout.Install the package in
sd-log-buster-template,sd-app-buster-template, andwhonix-gw-15by copying it there and then runningdpkg -i /path/to/package.deb. I took the intermediate step of copying it from my dev VM todom0to avoid updating RPC policies.Why these three TemplateVMs? Updating the package in
sd-log-buster-templateensures that we've not broken the logging package itself. Updating the package inwhonix-gw-15ensures we're actually fixing sd-log sometimes ingests logs ashostsecuredrop-workstation#583. Updating the package insd-app-buster-templateensures that we've not broken logging from other AppVMs.Boot up
sd-whonixand remove the entire block### BEGIN securedrop-workstation ###...### END securedrop-workstation ###from/rw/config/rc.local.Why this edit? This ensures that the
localvmparameter is no longer set late in the boot process (too late, which causes the sorting of early logs intohost), and we instead rely on autodetection of the VM name usingqubesdb-read, with the version ofsd-rsyslogthat ships in this package. The related Salt change freedomofpress/securedrop-workstation@ce3a921 will do this automatically.Shut down all running SD Workstation VMs except
sd-log(including templates), then remove or archive all log subdirectories in~/QubesIncomingLogsonsd-log. We want a clean slate for verifying log behavior.Reboot the Qubes machine (we want to verify a production-like VM boot sequence). After reboot, do not run the updater to ensure your package change is presreved.
After reboot, examine the contents of
~/QubesIncomingLogsofsd-log.hostdirectory.logger fooinsd-appandsd-whonix.~/QubesIncomingLogs.For good measure, run
make testin yourdom0SecureDrop Workstation checkout directory. Thesd-logtest should no longer fail, and there should be no new test failures.In
dom0, runmake cleanto remove your SecureDrop Workstation setup completely, or runsdw-admin --applyand the updater to restore the previous state.