-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add securedrop keyring package and update Release Key in sys-firewall #563
Conversation
9f799ec
to
48f8fe6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the strategy is sound. There's a bit of variance in the keyring tests, worth a closer look on why tildes are appended to the keyring files. Once that's resolved, I suggest moving to ready-for-review state.
tests/test_vms_platform.py
Outdated
keyring_path = "/etc/apt/trusted.gpg.d/securedrop-keyring.gpg" | ||
# in whonix-gw-15, the keyring name gets appended with ~ on install | ||
if is_whonix: | ||
keyring_path += "~" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on my system:
user@host:~$ qubesdb-read /name
sd-whonix
user@host:~$ find /etc/apt/trusted.gpg.d/| grep -i securedrop
/etc/apt/trusted.gpg.d/securedrop-keyring.gpg
After patching the test, I was able to get it to pass.
tests/test_vms_platform.py
Outdated
if is_whonix: | ||
fpf_gpg_pub_key_info.append("----------------------------------------------") | ||
else: | ||
fpf_gpg_pub_key_info.append("---------------------------------------------") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different number of hyphens? That's a head-scratcher. I suspect that number is determined by the length of filepath:
>>> hyphens = "----------------------------------------------"
>>> len(hyphens)
46
>>> keyring_path = "/etc/apt/trusted.gpg.d/securedrop-keyring.gpg"
>>> len(keyring_path)
45
>>>
Indeed, matches test output:
FAIL: test_debian_keyring_config (test_vms_platform.SD_VM_Platform_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/user/securedrop-workstation/tests/test_vms_platform.py", line 248, in test_debian_keyring_config
self._ensure_keyring_package_exists_and_has_correct_key(vm, is_whonix=True)
File "/home/user/securedrop-workstation/tests/test_vms_platform.py", line 138, in _ensure_keyring_package_exists_and_has_correct_key
self.assertEqual(fpf_gpg_pub_key_info, results.split('\n'))
AssertionError: Lists differ: ['/et[86 chars]------', 'pub rsa4096 2016-10-20 [SC] [expir[219 chars]ss>'] != ['/et[86 chars]-----', 'pub rsa4096 2016-10-20 [SC] [expire[218 chars]ss>']
First differing element 1:
'----------------------------------------------'
'---------------------------------------------'
['/etc/apt/trusted.gpg.d/securedrop-keyring.gpg',
- '----------------------------------------------',
? -
+ '---------------------------------------------',
'pub rsa4096 2016-10-20 [SC] [expires: 2021-06-30]',
' 22245C81E3BAEB4138B36061310F561200F4AD77',
'uid [ unknown] SecureDrop Release Signing Key',
'uid [ unknown] SecureDrop Release Signing Key '
'<[email protected]>']
----------------------------------------------------------------------
Ran 70 tests in 256.289s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was also some variance to what appears to be a default .gnupg.conf in the Whonix template
Now expires on 20200630
Ensures the securedrop-provisioned apt key resides in a separate keyring file from the default trusted.gpg
Given the limited pilot size and the fact that this enables automatic keyring updates in future, during sprint planning today, we agreed that it's reasonable to rely on a one-time The clearer we can make the failure case for the user, the better, so we don't have to go through pages of scrollback to find out what went wrong. |
48f8fe6
to
2e20446
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this pr and associated packaging PR based on the feedback above, this is now ready for re-review. The next steps should be as follows:
- Use this PR to test securedrop-keyring package (Add securedrop-keyring package securedrop-builder#171)
- Merge Add securedrop-keyring package securedrop-builder#171
- Upload securedrop-keyring to apt-test (since we will not be building nightlies of that package in CI): Adds securedrop-keyring package to apt-test securedrop-apt-test#45
- Drop 3beffa2
- Final review on this PR, once Adds securedrop-keyring package to apt-test securedrop-apt-test#45 is merged
Environment:
Test purpose:PR review Test steps:Followed PR fresh install test plan as far as Test results:
The |
@zenmonkeykstop The
So double-check that you've got that deb file in |
Sorry, thought I'd added another comment - I'd gotten past that after looking at the salt config (also added the necessary setup instructions in the test plan for other potential reviewers). Seeing the same test failure as you due to the ~ in the keyring name not being present, but otherwise all good. |
This ensures all debian-based VMs will contain the latest version of the Release Key, whenever state is applied to a given VM, in a dedicated keyring file in `/etc/apt/trusted.gpg.d/securedrop_keyring.gpg`, see https://github.com/freedomofpress/securedrop-debian-packaging.
* Purge securedrop-keyring package on whonix template cleanup: * Purge is required to completely remove the keyring file from /etc/ and avoid issues with tempfiles * We no longer need to remove the Release Key since postinst will remove the Release Key from `/etc/apt/trusted.gpg` and the package will remove the SecureDrop-specific keyring in `/etc/apt/trusted.gpg.d/` folder * Finally, specify homedir to not use whonix-specific gnupg.conf
2e20446
to
3beffa2
Compare
Thanks @zenmonkeykstop for the review and for updating the test plan. I think i have figured out the source of the confusion (which was entirely on my end): The issue was with the cleanup of whonix-gw-15: because the keyring is in I have updated tests, as well as the clean logic for whonix-gw. I have confirmed this is working locally, the next steps should still be in #563 (review) . This is now ready for re-review |
|
3beffa2
to
cfc60fd
Compare
Dropped the last commit, ready for final review |
Stepped through fresh install scenario again, one new failure on
Confirmed that there is a hosts directory in |
Am able to reproduce the same isolated test failure that @zenmonkeykstop reports. Haven't investigated yet, but let's take another look. |
Looks like the Verified by:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the logging test failure (as documented in comments) is unrelated, this PR LGTM!
Status
Ready for review. Still marked as draft as to drop the final commit on this branch once this is approved and the securedrop-keyring package is uploaded to apt-test for final testing
Description of Changes
Fixes #438
Adds securedrop-keyring package (see freedomofpress/securedrop-builder#171)
This will require all end users to run
securedrop-admin --apply
prior to the key expiry date, once the following packages are uploaded to the package repos:Some challenges with handing sys-firewall state, which will cause a significant (~300 lines, including tests) change to the Preflight Updater:
Testing
Prerequisites
(Steps below not required if
securedrop-keyring
package has been uploaded toapt-test.freedom.press
)Buildsecuredrop-keyring
0.1.4 package as per Add securedrop-keyring package securedrop-builder#171copy package to dom0:/srv/salt/sd/sd-workstation/Clean Install
make clean
to remove all existing SDW VMsmake clone
on this branchmake all
completes without errorsmake test
complete without errorsUpgrade install
make clone
on this branchmake all
completes without errorsmake test
complete without errorsChecklist
If you have made code changes
make flake8
) passes in the development environment (this box maybe left unchecked, as
flake8
also runs in CI)If you have made changes to the provisioning logic
make test
) pass indom0
of a Qubes install