Conversation
emkll
left a comment
emkll
left a comment
There was a problem hiding this comment.
Have done a quick visual (not functional) review of this PR. It looks good to me, though the ordering here may be an issue, especially with the in-place scenario. I think more explicit ordering of tasks will be helpful here. Some comments inline.
If the goal is to bootstrap logging as early as possible in the provisioning phase (to get some provisioning logs in sd-logs as well), it might make sense to apply the state to dom0 and then to sd-log early in the process, as part of a separate qubesctl call, similar to what was done in
, where you could end with the reboot task there.| @@ -0,0 +1,3 @@ | |||
| [sd-rsyslog] | |||
There was a problem hiding this comment.
Templating definitely good. We might be able to use grains["localhost"] to get the VM name, rather than passing in context every time. (See output of e.g. sudo qubesctl --skip-dom0 --show-output --target sys-firewall grains.items.) If that works, we could stick the tasks for 1) install pkg; 2) configure rsyslog in a single state file and include it, reducing a lot of duplication.
Okay, trying to get this in. |
|
We need to get this merged freedomofpress/securedrop-log#12 |
kushaldas
force-pushed
the
configure_logging_world
branch
from
134f890 to
d082fc0
Compare
kushaldas
force-pushed
the
configure_logging_world
branch
from
d082fc0 to
fb88d05
Compare
scripts/provision-all
Outdated
| sudo qubesctl --show-output state.highstate | ||
|
|
||
| echo "Setup sd-log vm first" | ||
| sudo qubesctl --show-output --skip-dom0 --targets sd-log state.sls sd-log-templates-files |
There was a problem hiding this comment.
should be sd-log-template-files here
There was a problem hiding this comment.
I think the issue here is more than a typo (this is untested, merely suggesting):
sudo qubesctl --show-output --skip-dom0 --targets sd-log-template, sd-log state.highstate
I am getting an error after make clean that sd-log can't download the securedrop-log package (which makes sense because sd-log doesn't have internet connectivity, we need to apply state to sd-log-buster-template
emkll
left a comment
emkll
left a comment
There was a problem hiding this comment.
Thanks @kushaldas for the changes. I got (most) of the logging to work (and confirmed logs were flowing ) in an upgrade-type scenario with some packages left over from my previous testing (except for sd-whonix)
However, I ran into some issues after running make clean && make all. See inline for comments / suggestions.
scripts/provision-all
Outdated
| sudo qubesctl --show-output state.highstate | ||
|
|
||
| echo "Setup sd-log vm first" | ||
| sudo qubesctl --show-output --skip-dom0 --targets sd-log state.sls sd-log-templates-files |
There was a problem hiding this comment.
I think the issue here is more than a typo (this is untested, merely suggesting):
sudo qubesctl --show-output --skip-dom0 --targets sd-log-template, sd-log state.highstate
I am getting an error after make clean that sd-log can't download the securedrop-log package (which makes sense because sd-log doesn't have internet connectivity, we need to apply state to sd-log-buster-template
tests/base.py
Outdated
|
|
||
| return True | ||
|
|
||
| def logging_configured(self, vmname = ""): |
There was a problem hiding this comment.
👍 nice tests, confirm these are being run as part of make test. Note that they do introduce linting failures: https://circleci.com/gh/freedomofpress/securedrop-workstation/1815
| - sd-whonix | ||
| - sd-remove-unused-templates | ||
| - sd-log | ||
|
|
dom0/sd-workstation.top
Outdated
|
|
||
| sd-log-buster-template: | ||
| - sd-log-template-files | ||
| sd-log: |
There was a problem hiding this comment.
I don't think this is required since you are shutting down this machine after provisioning the template
dom0/sd-log-reboot.sls
Outdated
| @@ -0,0 +1,10 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I don't think this is required anymore, given the qvm-shutdown sd-log call you make in ./scripts/provision-all. Am I missing something?
There was a problem hiding this comment.
We could also run qvm-shutdown && qvm start if we need it to be up, instead of having to maintain this complex logic
|
While testing locally, Same for |
kushaldas
force-pushed
the
configure_logging_world
branch
from
fb88d05 to
9433daf
Compare
emkll
left a comment
emkll
left a comment
There was a problem hiding this comment.
Thanks @kushaldas this is looking quite good, most logs are now flowing properly. A couple of remaining issues, specifically with the changes to make clean and pertaining to sd-whonix
make cleanfails, see comment inline for the possible cause of the failure- Also related to
make clean, there's an order of precedence issue: When runningmake clean, a popup indicates that the Securedrop.Log RPC policy is not there. I think we should slightly refactor the clean logic to remove the policies until the very end (see image below in [1]). Did you experience that issue yourself? What do you think? sd-whonixlogs still don't flow tosd-logafter make all, and the tests fail. This is because the as part ofmake all, which invokesprovision-allscript. Becausewhonix-gw-15does not have the sd-workstation tag, we must explicitly apply the state to that vm. A local diff appears to resolve for me (see [2] below), but there may be room to optimize.- Could you please rebase on latest
master? Some tests are failing due to kernel versions
[1]
[2]
diff --git a/scripts/provision-all b/scripts/provision-all
index 72b7a73ca..5f10df63d 100755
--- a/scripts/provision-all
+++ b/scripts/provision-all
@@ -22,6 +22,9 @@ sudo qubesctl --show-output --skip-dom0 --targets sd-log-buster-template state.s
# Make sure that the VM is in shutdown state
qvm-shutdown sd-log
+# Provision whonix-gw-15 with log additions because it isn't tagged with sd-workstation (we don't want it removed after a make clean)
+sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.highstate
+
# Format list of all VMs comma-separated, for use as qubesctl target
# We run this after dom0's highstate, so that the VMs are available for listing by tag.
all_sdw_vms_target="$(qvm-ls --tags sd-workstation --raw-list | perl -npE 's/\n/,/g' | perl -npE 's/,$//' )"
dom0/sd-clean-all.sls
Outdated
| sd-cleanup-whonix-gw-15: | ||
| cmd.run: | ||
| - names: | ||
| - qvm-run whonix-gw-15 'sudo apt remove securedrop-log' |
There was a problem hiding this comment.
you probably want to do use apt remove -y here, and make clean fails with the following output at this step:
----------
ID: sd-cleanup-whonix-gw-15
Function: cmd.run
Name: qvm-run whonix-gw-15 'sudo apt remove securedrop-log'
Result: False
Comment: Command "qvm-run whonix-gw-15 'sudo apt remove securedrop-log'" run
Started: 11:28:08.536188
Duration: 8211.254 ms
Changes:
----------
pid:
10820
retcode:
1
stderr:
Running 'sudo apt remove securedrop-log' on whonix-gw-15
whonix-gw-15: command failed with code: 1
stdout:
scripts/provision-all
Outdated
| echo "Setup sd-log-buster-template vm first" | ||
| sudo qubesctl --show-output --skip-dom0 --targets sd-log-buster-template state.sls sd-log-template-files | ||
| # Make sure that the VM is in shutdown state | ||
| qvm-shutdown sd-log |
There was a problem hiding this comment.
Are you sure this is still required? When running make all, I see in standard out:
sd-log: Shutdown error: Domain is powered off: 'sd-log'
9433daf to
e33fe7e
Compare
|
@emkll pushed again. |
kushaldas
force-pushed
the
configure_logging_world
branch
from
e33fe7e to
a40b46c
Compare
|
Thanks for the quick changes @kushaldas . I did another visual pass on this. I expect the order of operations for the RPC grants when running Perhaps requiring Once the make clean issues been addressed, I propose the following test plan. Would you mind walking through the test plan on your workstation as well so we have good coverage here?
|
eloquence
changed the title
|
@kushaldas Small note, can you put "Fixes #xxx" type descriptions in the body of a PR, instead of the title? This makes titles a bit easier to read, and ensures that GitHub picks up the PR as a linked pull request for the issue it references. I tweaked accordingly for this PR. Thanks! |
If that is required I will modify my commit messages accordingly. As those PR titles generally come from the commit messages itself, and in most projects the commit message contains which issue it is related to. https://github.blog/2013-01-22-closing-issues-via-commit-messages/ talks about how commit message in master branch actually closes the issues. |
|
Yes, it closes the issue, but it won't show up as a linked PR on the issue, nor will the issue show up as a linked issue on the PR. You can test that easily by removing the "Resolves" line from the PR body. So I'd recommend having it in the body to enable that cross-linking:
See here for further docs, specifically:
|
Understood, as I said I will modify my description text to make sure that it has the issue number for cross linking from now on. |
kushaldas
force-pushed
the
configure_logging_world
branch
from
a40b46c to
dfa5a03
Compare
|
I have updated the PR with all of the related But, I am stuck with a different problem now, Then, if I manually install the package, the configuration file I added an auditd rule to trace what is going on, I can see The other problem I found (I am guessing this is upstream issue) is that when we remove a package (created via I guess we will have to add somewhere in the debian packaging for some steps after uninstallation. |
|
Thanks for the changes, see below for findings that should unblock you (1) as well as a regression in the
one last request: let's reserve squashing prior to final merge, to make debugging/review easier moving forward. |
|
@emkll the branch is ready to review again, the missing part is for |
There was a problem hiding this comment.
Thanks for the changes @kushaldas , still some sd-whonix (cleanup and log configuration). I've also opened https://github.com/freedomofpress/securedrop-debian-packaging/issues/145 as a follow-up to your finding.
See inline for salt optimization (it seems like we are applying the same state twice). Could you please try running the test plan and sharing your results? Are they different from the ones I've observed below?
make cleanmake allon this branch- QubesIncomingLogs in
sd-logcontainssd-applogs - QubesIncomingLogs in
sd-logcontainssd-proxylogs - ❌ QubesIncomingLogs in
sd-logcontainssd-whonixlogs: logs are not flowing properly with sd-whonix, it seems like/etc/rsyslog.d/sdlog.confis not populated. - QubesIncomingLogs in
sd-logcontainssd-deviceslogs (after exporting a file to drive or print) - QubesIncomingLogs in
sd-logcontainssd-viewerlogs (after opening a submission in a dispvm) -
make cleanworks as expected - There are no RPC popups after make clean
- ❌ whonix-gw-15 no longer contains the syslog customizations: apt sources for apt-test.freedom.press are still present (this is consistent with your report)
scripts/provision-all
Outdated
| sudo qubesctl --show-output --skip-dom0 --targets sd-log-buster-template state.highstate | ||
| # Provision whonix-gw-15 with log additions because it isn't tagged with sd-workstation (we don't want it removed after a make clean) | ||
| sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.highstate | ||
| sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.sls sd-whonix-template-files |
There was a problem hiding this comment.
This line is not necessary: sd-whonix-template-files is specified in sd-worksation.top so will be applied as part of the qubesctl invocation of the line directly above.
There was a problem hiding this comment.
Sadly it did not for me when I tried. I had to add that line to get it working :(
There was a problem hiding this comment.
Can you try again? Based on my local testing, removing L24 works. If the issue is the rebooting of whonix-gw-15, it may be best to qvm-shutdown --wait sd-whonix here intead of re-applying a salt file that has already been applied, as it will be more reliable
There was a problem hiding this comment.
sd-whonix or whonix-gw-15 ?
There was a problem hiding this comment.
woops sorry, whonix-gw-15, if it is required
|
Now new error for me in |
|
Found the error. The network. |
|
|
@emkll Pushed again.
|
emkll
left a comment
emkll
left a comment
There was a problem hiding this comment.
Thanks @kushaldas this looks great 🎉 , working well on my end
make cleanmake allon this branch- QubesIncomingLogs in
sd-logcontainssd-applogs - QubesIncomingLogs in
sd-logcontainssd-proxylogs - QubesIncomingLogs in
sd-logcontainssd-whonixlogs - QubesIncomingLogs in
sd-logcontainssd-deviceslogs (after exporting a file to drive or print) - QubesIncomingLogs in
sd-logcontainssd-viewerlogs (after opening a submission in a dispvm) -
make cleanworks as expected - There are no RPC popups after make clean
- whonix-gw-15 no longer contains the syslog customizations
Three comments inline to help with maintainability, otherwise good to merge from my perspective
| - context: | ||
| vmname: sd-whonix | ||
|
|
||
| sd-rsyslog-sdlog-conf-for-sd-whonix: |
There was a problem hiding this comment.
Can we add a comment here, for future maintainers, as to why this is required here and not in other VMs, where this file is handled by the sd-log package: https://github.com/freedomofpress/securedrop-log/blob/d1799b2477f4250eae43b472472df19a49f5cb75/sdlog.conf
scripts/provision-all
Outdated
| sudo qubesctl --show-output --skip-dom0 --targets sd-log-buster-template state.highstate | ||
| # Provision whonix-gw-15 with log additions because it isn't tagged with sd-workstation (we don't want it removed after a make clean) | ||
| sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.highstate | ||
| sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.sls sd-whonix-template-files |
There was a problem hiding this comment.
Can you try again? Based on my local testing, removing L24 works. If the issue is the rebooting of whonix-gw-15, it may be best to qvm-shutdown --wait sd-whonix here intead of re-applying a salt file that has already been applied, as it will be more reliable
dom0/sd-clean-all.sls
Outdated
| - qvm-run whonix-gw-15 'sudo rm -f /etc/apt/sources.list.d/securedrop_workstation.list' | ||
| - qvm-run whonix-gw-15 'sudo apt remove -y securedrop-log' | ||
| - qvm-run whonix-gw-15 'sudo systemctl restart rsyslog' | ||
| - qvm-run whonix-gw-15 'sudo apt-key del 4ED79CC3362D7D12837046024A3BE4A92211B03C' |
There was a problem hiding this comment.
We should also remove the prod key here: "22245C81E3BAEB4138B36061310F561200F4AD77" (the command will always return zero so we can run both even in dev, and not worry about split logic in the cleanup action.
There was a problem hiding this comment.
Okay, i will add that too.
There was a problem hiding this comment.
The apt remove -y securedrop-log line is not idempotent. So far, we've been careful to ensure that make clean exits zero even if run multiple times.
conorsch
left a comment
conorsch
left a comment
There was a problem hiding this comment.
Left a few thoughts in-line based on visual review. To be clear, I've not performed a functional review of the changes.
dom0/sd-clean-all.sls
Outdated
| - qvm-run whonix-gw-15 'sudo rm -f /etc/apt/sources.list.d/securedrop_workstation.list' | ||
| - qvm-run whonix-gw-15 'sudo apt remove -y securedrop-log' | ||
| - qvm-run whonix-gw-15 'sudo systemctl restart rsyslog' | ||
| - qvm-run whonix-gw-15 'sudo apt-key del 4ED79CC3362D7D12837046024A3BE4A92211B03C' |
There was a problem hiding this comment.
The apt remove -y securedrop-log line is not idempotent. So far, we've been careful to ensure that make clean exits zero even if run multiple times.
| @@ -0,0 +1,3 @@ | |||
| [sd-rsyslog] | |||
There was a problem hiding this comment.
Templating definitely good. We might be able to use grains["localhost"] to get the VM name, rather than passing in context every time. (See output of e.g. sudo qubesctl --skip-dom0 --show-output --target sys-firewall grains.items.) If that works, we could stick the tasks for 1) install pkg; 2) configure rsyslog in a single state file and include it, reducing a lot of duplication.
The whonix vms are showing the right vmname for |
dom0/sd-clean-all.sls
Outdated
| @@ -15,9 +15,9 @@ sd-cleanup-whonix-gw-15: | |||
| - names: | |||
There was a problem hiding this comment.
Is there a reason this cmd.run block was not moved to sd-clean-whonix.sls` ? it might make sense to centralize whonix-ws-15-specific cleanup tasks there
|
|
||
| # FPF repo is setup in "securedrop-workstation" template | ||
| install-securedrop-client-package: | ||
| install-securedrop-client-and-securedrop-log-package: |
Also add related tests.
`make clean` now cleans whonix-gw-15 with proper removal of `securedrop-log` package using Salt states.
kushaldas
force-pushed
the
configure_logging_world
branch
from
c963623 to
3b186c1
Compare
emkll
left a comment
emkll
left a comment
There was a problem hiding this comment.
Thanks @kushaldas for your patience and for your excellent work. Tested your latest change as follows:
make cleanmake allon this branch- QubesIncomingLogs in
sd-logcontainssd-applogs - QubesIncomingLogs in
sd-logcontainssd-proxylogs - QubesIncomingLogs in
sd-logcontainssd-whonixlogs - QubesIncomingLogs in
sd-logcontainssd-deviceslogs (after exporting a file to drive or print) - QubesIncomingLogs in
sd-logcontainssd-viewerlogs (after opening a submission in a dispvm) -
make cleanworks as expected - There are no RPC popups after make clean
- whonix-gw-15 no longer contains the syslog customizations
A couple of follow-ups have been identified in the reviews of this PR:
- Using
grainsas identified by @conorsch #447 (comment) I propose we merge as-is and address post-beta as part of a broader provisioning logic refactor. - Opened #463 to track the aggregation of
dom0logs since it was out of scope of this PR
I'll leave this PR open for comments from the rest of the team, barring any objections will merge this tomorrow morning.
| sudo qubesctl --show-output --skip-dom0 --targets sd-log-buster-template state.highstate | ||
| # Provision whonix-gw-15 with log additions because it isn't tagged with sd-workstation (we don't want it removed after a make clean) | ||
| sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.highstate | ||
| #sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.sls sd-whonix-template-files |
There was a problem hiding this comment.
Nit: commented out instead of deleted, might lead to confusion
|
Looks great. Given @emkll's thorough testing and formal approval, fine for merge. The few comments I'd given earlier are also addressed, plus we have some follow-up tickets to track (post-pilot). |
4 participants
Status
Ready for review
Description of Changes
Resolves #440
Sets up
securedrop-logservice in related VMs. All logs should be flowing intosd-logvm.Changes proposed in this pull request:
Testing
make allshould have logging ready./home/user/QubesIncomingLogs/directory insd-logsvmChecklist
If you have made changes to the provisioning logic
Linting (
make flake8) and tests (make test) pass in dom0 of a Qubes installI have added/removed files, and have updated packaging logic in MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec