-
Notifications
You must be signed in to change notification settings - Fork 701
Refactor securedrop-admin to be a Debian package
#7606
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
Conversation
zenmonkeykstop
left a comment
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.
Took an initial code pass (have not yet tried building and installing packages). The main thing that leaps out is the different config paths for Tails and Debian. It should be possible to have both use the same path if it's a dotfile directory in the user home.
|
Looks like having a unified config path across Tails and Qubes is trickier than I thought. The initial idea of using Tails' dotfiles persistence is too unwieldy, as individual files in the dotfiles directory are symlinked into place from /live/persistence at login, so new files just get created in the underlying non-persistent filesystem. So
this would need to happen before |
|
Possibly stupid question (and I haven't read everything yet so might've missed something), but can we just use |
|
It's not stupid, but it would be weird :) - I actually wanna stick it in $XDG_CONFIG_HOME for good citizen reasons. Given that we're changing things up anyway, aiming to have as little potential for user error as possible seems worthwhile. There are some other gotchas in either case - default ansible behaviour is to write to the ansible home dir, so backups are also gonna end up in the hidden directory. It might be nice to write to the user's current working directory by default or optionally be able to specify a path. Same with the pubkeys, would be nice to copy them to the config dir directly once the user gives their initial location in sdconfig |
zenmonkeykstop
left a comment
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.
Couple more notes
| # Custom persistence directory whence site-specific configs will be copied | ||
| # on every login under Tails. | ||
| tails_config_securedrop_dotfiles: "{{ tails_config_amnesia_persistent }}/.securedrop" | ||
| tails_config_securedrop_admin_dotfiles: "{{ tails_config_amnesia_persistent }}/.securedrop-admin" |
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.
This is never used as far as I can tell.
|
|
||
| # Custom persistence directory whence site-specific configs will be copied | ||
| # on every login under Tails. | ||
| tails_config_securedrop_dotfiles: "{{ tails_config_amnesia_persistent }}/.securedrop" |
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.
We can probably either drop files in this directory (desktop icon stuff, no longer used) or move them to the .securedrop-admin config dir.
|
I've added a script to setup persistence in Tails (eventually it will also do the apt setup) and updated the test plan accordingly. |
legoktm
left a comment
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.
Just did a pass on the Debian packaging.
As a general note I do think it'll be worth extracting out some of the broader fixes from this PR and merging them separately and ahead of time, like the UBUNTU_VERSION -> OS_VERSION stuff.
builder/build-debs-admin.sh
Outdated
| source /etc/os-release | ||
|
|
||
| # Install virtualenv in the right place | ||
| mkdir -p /usr/share/securedrop-admin |
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.
This logic should all happen inside the packaging, i.e. debian/rules. There's a sed command in the current packaging that fixes up the paths so the venv will work.
And the other files should be included via install, etc.
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.
virtualenv creation was moved to debian/rules, other files installed via debian/install - though the commands to move them to a working copy still live here.
| import subprocess | ||
| from pathlib import Path | ||
|
|
||
| OS_VERSION = os.environ.get("OS_VERSION", "focal") |
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.
Should we default to bookworm here? I guess it doesn't really matter since it's always set.
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.
trixie now :)
admin/debian/control
Outdated
|
|
||
| Package: securedrop-admin | ||
| Architecture: amd64 | ||
| Depends: python3-virtualenv, python3-yaml, python3-pip, libffi-dev, libssl-dev, libpython3-dev, sq-keyring-linter, netcat-openbsd, tor, rsync |
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.
I don't think we need most of these python- dependencies at runtime, the e.g. virtualenv has already been created, we shouldn't be pip installing anything. Same with the -dev ones, since we're not compiling anything.
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.
removed.
.github/workflows/build.yml
Outdated
| run: | | ||
| find . -name '*.deb' -exec sha256sum {} \; | ||
| # FIXME: securedrop-app-code isn't reproducible | ||
| # FIXME: securedrop-admin isn't reproducible |
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.
IMO non-reproducibility is a blocker to move forward, but it's literally just the bytecode that's unreproducible: https://gist.github.com/legoktm/fc213ffb0dbe20385e50441aaf544a28
If you look in the current Debian packaging we already have an override to strip all the bytecode, we should do the same here.
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.
package should now be reproducible
|
I've hit blocks doing some of the client-side write work this week. So now that removing focal has landed, I'm going to spend some time rebasing this PR. |
|
With the help of Claude Code, I have rebased develop into this branch. I'm also currently working on getting all of the Github Actions checks to pass. Rather than force pushing (in case we want to move forward in a different way), I'm pushing my changes to My plan is, once I succeed in having all of the checks passing, I will do manual testing of admin tools. |
f738b29 to
64acde5
Compare
|
Got wires crossed and did a manual rebase while the Claude one was in progress. Manual looks slightly better so I've force-pushed it. Next up:
|
aec09d2 to
d90e52e
Compare
|
I've been working on #7647, but pushing commits into this PR, so when it's ready it will close that issue as well. I've made a lot of progress. Here are the recent changes. Migration scriptsThe After the migration is complete and the user reboots, there will be no more GUI updater. From that point on, the The migration scripts include:
Changes to ansible playbooksThe GUI installer has logic to ask the user for their sudo password, and then it passes this password into the The way
The GUI installer only provided the sudo password for the first time it was prompted for "SUDO password", and when it was prompted a second time, it would just hang and after 2 minutes the GUI installer would timeout. To fix this, I refactored the ansible playbooks. I made a new So in the end, it does the same thing, but with only a single ansible playbook getting executed instead of two separate ones, so it succeeds with the GUI installer. TestingHere's how to test it:
diff --git a/admin/securedrop_admin/__init__.py b/admin/securedrop_admin/__init__.py
index faac742df..4c9c9ac49 100755
--- a/admin/securedrop_admin/__init__.py
+++ b/admin/securedrop_admin/__init__.py
@@ -762,7 +762,7 @@ def update_check_required(cmd_name: str) -> Callable[[_FuncT], _FuncT]:
@functools.wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
cli_args = args[0]
- if cli_args.force:
+ if True:
sdlog.info("Skipping update check because --force argument was provided.")
return func(*args, **kwargs)
@@ -1028,6 +1028,10 @@ def get_release_key_from_keyserver(
def update(args: argparse.Namespace) -> int:
"""Verify, and apply latest SecureDrop workstation update"""
+ subprocess.call(["git", "checkout", "."], cwd=args.root)
+ subprocess.call(["git", "checkout", "admin-deb"], cwd=args.root)
+ subprocess.call(["git", "pull", "origin", "admin-deb"], cwd=args.root)
+ return 0
sdlog.info("Applying SecureDrop updates...")
update_status, latest_tag = check_for_updates(args)
diff --git a/journalist_gui/journalist_gui/SecureDropUpdater.py b/journalist_gui/journalist_gui/SecureDropUpdater.py
index 75e801803..14882ccfb 100644
--- a/journalist_gui/journalist_gui/SecureDropUpdater.py
+++ b/journalist_gui/journalist_gui/SecureDropUpdater.py
@@ -88,6 +88,12 @@ class UpdateThread(QThread):
self.failure_reason = ""
def run(self):
+ subprocess.call(["git", "checkout", "."], cwd="/home/amnesia/Persistent/securedrop")
+ subprocess.call(["git", "checkout", "admin-deb"], cwd="/home/amnesia/Persistent/securedrop")
+ subprocess.call(["git", "pull", "origin", "admin-deb"], cwd="/home/amnesia/Persistent/securedrop")
+ self.update_success = True
+ self.signal.emit({"status": True, "output": "", "failure_reason": ""})
+ return
sdadmin_path = "/home/amnesia/Persistent/securedrop/securedrop-admin"
update_command = [sdadmin_path, "update"]
try:
When you're ready (your
This will use the patched GUI updater to switch to the It will ask for your password twice, using The second time it's setting up Then it asks for your password again, in a slightly different way, to run the ansible playbook: Then you wait a moment for the Reboot Tails. To test it, you'll also need to manually install sudo dpkg -i ~/Persistent/securedrop-admin.debAfter you connect to Tor, you'll see the new GNOME extension no longer has an option to check for updates: You should be able to ssh into app and mon, connect to the journalist interface, etc. But this time, it's pulling the securedrop config from You should also be able to open the console and run
Next steps: install from apt repoBefore this is done, we'll need to make the migration scripts add the FPF apt repo and install So I think the next step is to create a deb from this branch and publish it to the Then, I can update this branch to install the test repo and then install Once we have confirmed that that works, we can switch the migration script to add the |
…oot.sh) which configures persistence, configures the apt repo, and installs securedrop-admin
…onsole that shows the progress of the update, instead of showing the progress directly in the GUI updater
…onfig` command, so that the GUI updater does not run tailsconfig before setup is complete
…nt it from asking for the password before it's ready
… update text on the intro message about the migration
…r updates at the beginning of ansible playbooks, because the local sudo password does not match the remote sudo password
cb55b7a to
9551535
Compare
|
rebased to clear conflicts - @legoktm |
legoktm
left a comment
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.
Most recent set of changes LGTM, with the note that cffi is also a production dependency and in general we should keep the develop + prod versions in sync, but we can fix that for the next release and not block this on the diff review.
I synced stg-admin-deb with this PR so it'll show up on this PR, once it's green I think we're good to go!





Fixes #7574, #7647.
This PR adds Debian packaging for
securedrop-admin. But in order to do that, I also had to completely refactor the admin code.The way it currently works, files get dropped into
install_files/ansible-base, and also we write settings toinstall_files/ansible-base/group_vars/all/site-specific. With debian packaging, we need to save writable files somewhere else. In Qubes this is~/.securedrop-admin/, and in Tails this is~/Persistent/.securedrop-admin/.This should work in both Qubes and Tails, with installation instructions described below -- we'll need to update the docs. But first, here are some other big changes.
I've made
check_for_updatesno longer rely on git tags and instead use a new ansible playbook and apt to check if thesecuredrop_adminpackage is out-of-date or not. I've also completely deleted the GUI updater, and modified the Tails extension to no longer have a "Check for SecureDrop updates" option in the menu.securedrop-admin verifydoes not work. It requires test dependencies to be installed in the virtualenv, and it runs code in thedevopsfolder in the root of the git repo--and we don't have the git repo anymore. I suggest that we removeverifyand instead find a different way to run testinfra tests.I did spend time getting the admin tests working though. When building the admin test container, it builds the debian package and installs it, and then run tests against that. I also fixed all of the tests so that they work again with all the refactoring, and I deleted a bunch of tests. I removed all of the GUI updater tests, since I deleted the GUI updater, and I also deleted all of the tests related to checking for updates and verifying git tags and stuff, since that has all been ripped out too.
Test plan
Here's the new SecureDrop instructions. There's different instructions when testing from Qubes and from Tails to start, and then you follow the same instructions on both.
From Qubes
Set up servers by following the docs, but save setting up ssh keys to later.
Build the deb:
This will make
build/trixie/securedrop-admin_2.13.0~rc1+trixie_amd64.deb. Copy this to a debian 13-based template (e.g.debian-13-clone) and install it.Create
sd-adminVM based ondebian-13-clone.Open a terminal in
sd-admin.Now set up ssh keys.
Create the
~/.config/securedrop-adminfolder:mkdir -p ~/.config/securedrop-adminFrom Tails
Build the deb in Qubes.
Create a Tails USB with a Persistent volume, with all checkboxes turned on.
Transfer
./tails-bootstrap.shandbuild/trixie/securedrop-admin_2.13.0~rc1+trixie_amd64.debto Tails:In Tails, run
sudo bash tails-bootstrap.shto set up persistent configs, then restart tails to apply the persistence changesAfter restarting, install the debian package:
Continue for both
Copy the following files to
~/.config/securedrop-admin/:Configure:
Install:
Then configure the admin workstation:
Now you should be able to
ssh app,ssh mon, and runsecuredrop-admin --force installagain, and it should all go over onion services.Checklist
This change accounts for:
This PR requires a lot of new documentation, and has major impacts on new SecureDrop installs and upgrades.
securedrop-admindoesn't have an AppArmor policy, but should it?