Configure apt environment based on bootstrap environment choice by rocodes · Pull Request #1392 · freedomofpress/securedrop-workstation

rocodes · 2025-08-06T16:51:04Z

Ready for a look, depends on #1210 [edit: merged!]

Fixes #1058
Fixes #1232
Refs #1053 (can supersede/close with another approach in a followup PR)

Use bootstrap rpm instead of config.json to decide what environment (dev, staging, prod) we're using.

Build apt .sources file dynamically, from a single j2 template (drop apt_test j2 template).
Always default to prod
Remove pubkey and test key from this repo, and source them from the file managed by the bootstrap rpm. (One fewer place to lose track of pubkey)
(chore) Clean up unused config.json imports in a couple of VM sls files

Test plan

base branch is feat/sdw-keyring-compat
Configure dom0 rpm repo settings via boostrap rpm package #1210 is merged
CI (openqa) passing (clean install)
visual review

Checklist

This change accounts for:

any necessary RPM packaging updates (e.g., added/removed files, see MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec)
any required documentation

rocodes · 2025-08-21T16:04:35Z

(triggered openqa, will rebase once #1210 is merged then mark ready for review)

…ing key and repo urls.

legoktm

Overall it looks good, most of my comments are related to the tests.

legoktm · 2025-08-22T19:17:51Z

+### BEGIN TESTS ###
+
+# Get config, needed for test
+ENV, CONFIG = _get_expected_config()


Let's turn these into pytest fixtures.

legoktm · 2025-08-22T19:19:45Z

+    return key_info
+
+
+def _validate_key(info: dict, expected_fpr: str, expected_uid: str, expected_expiry: str):


instead of checking things and raising exceptions, I think we can just use normal pytest assertions

legoktm · 2025-08-22T19:20:19Z

+def test_key_apt_sources():
+    """Check the key in the apt_sources file used for provisioning Debian-based templates."""
+    app = Qubes()
+    for template in ["small", "large"]:


pytest parameterization?

legoktm · 2025-08-22T19:20:41Z

+                CONFIG["signing_key_uid"],
+                CONFIG["signing_key_exp"],
+            )
+        except ValueError as e:


Above I suggested using assertions in _read_and_validate_key so then we don't need to catch exception and then immediately fail. Plus if we use parameterization then the test name will also have the VM name.

legoktm · 2025-08-22T19:24:13Z


-# Append repo URL with appropriate distribution
-{% set _ = sdvars.update({"distribution": "bookworm"}) %}
+{% set apt_config = environments.get(env) %}


I'm wondering if we just leave this called sdvars even though it's different, just to cut down on the number of salt files modified in this PR that are entirely just s/sdvars/apt_config/.

It's up to you, but I just found sdvars such a confusing/nondescriptive name that it seemed like this would be long-term easier to understand.

legoktm · 2025-08-22T19:26:29Z

      - qvm: dom0-install-debian-minimal-template
+
+# Store signing key in salt cache so that it can later be provisioned to templates in their apt-sources files.
+# Using cp.push or cp.cache_file don't work due to the minion-minion setup.


Sorry, this is going over my head. Why can't the step in fpf-apt-repo.sls just read the original apt_config['keyfile']?

apt_config['keyfile'] is a reference to the signing key in /etc/pki in dom0, and which one it is depends on the keyring package (test key or real release key).

The fpf-apt-repo.sls steps are run on the Debian VMs, but the bootstrap keyring files live in dom0.
The debian vms have access to /srv/salt (as salt://) and to the cache dir but not to dom0 files, so somewhere in a state that's run on dom0 (I just picked this file for simplicity since then we only do it once, at initial provisioning time, and after that the key is managed via the keyring package), we cache the key, and then the VMs can read it.

Gotcha, appreciate the explanation. How about:

# Debian VMs need access to the signing key for initial provisioning, store it in salt cache since they # only have access to the cache and `/srv/salt`, not all of dom0. # Using cp.push or cp.cache_file don't work due to the minion-minion setup.

legoktm · 2025-08-22T19:28:07Z

+        return False
+
+
+# A bit clunky, but we have limited tooling options in dom0.


Cursed but if it works, it works :)

Could you add an explicit TODO: use sequoia here instead - I think we can do that once we're on 4.3.

…y shipped to dom0 in /etc/pki from securedrop-workstation-keyring. Key must be cached by dom0 and retrieved by the vm. Consolidate all apt repo configuration into sd-default-config.sls (delete sd-default-config.yml). Rename sdvars to apt_config for clarity.

…he keyring is used consistently across yum and apt components. Move test_sources functionality out of launcher tests. Deprecate skip_in_dom0 conftest annotation.

…ring. openqa

…bootstrap is complete

…dd it to bootstrap target openqa

rocodes · 2025-08-25T18:52:41Z

The CI failure shows that the installation is looking for the prod key, when it should be looking for the test key. Here's why: sd-default-config.sls file correctly selects the apt configuration we want when it's run on dom0, but even when it's imported with context in the VM files, it still tries to run the same checks on the vm, as opposed to remembering the configuration we selected.

rocodes · 2025-08-26T20:36:58Z

The right solution is to use pillars #1053, which is how Saltstack handles configurable data, and what the qubes team uses. It's possible to have dev- or staging- only pillar files that don't ship with the prod rpm (for example, that are installed into place when a developer runs a make target), and that can optionally override prod defaults.

It's possible to use pillars conservatively/only for the dev and staging setups, leaving all prod stuff defined in the sls files in /srv/salt. Then, for example, if a pillar file is available (because devs installed it - it wouldn't be shipped with the prod rpm), the prod repo data could be overridden with apt-test, or as we switch to new Debian versions, the apt-sources file could contain an new testing repo with another distribution, etc. (This is what whonix does with their testing pillar).

It's also possible to move more of our configuration to pillars, eg and ship a file in /etc/salt/minions.d/ configuring our own pillar with default (or empty) values, as the qubes team does.

I'm saying all this because someone else will probably have to pick this up if it's a priority due to the upcoming changes in my schedule. I'm going to reluctantly close this ticket; I think it would take a pretty short amount of time if we were just converting sd-default-config to prod by default plus an optional dev configuration on top, and I'm not thrilled about leaving things in a halfway state, but it's not something I can finish off in the next day or so.

rocodes added the keyring label Aug 6, 2025

rocodes mentioned this pull request Aug 6, 2025

Remove "environment" from config.json #1393

Closed

6 tasks

rocodes force-pushed the configure-apt-environment-via-bootstrap branch 4 times, most recently from aac4d00 to e468b35 Compare August 20, 2025 18:06

rocodes force-pushed the configure-apt-environment-via-bootstrap branch 4 times, most recently from dde0a4c to 4c2c82d Compare August 21, 2025 15:57

rocodes added this to SecureDrop Aug 21, 2025

rocodes moved this to In Progress in SecureDrop Aug 21, 2025

rocodes requested a review from legoktm August 21, 2025 16:05

rocodes force-pushed the configure-apt-environment-via-bootstrap branch 7 times, most recently from 597582e to 03dff50 Compare August 22, 2025 17:06

rocodes marked this pull request as ready for review August 22, 2025 17:08

rocodes requested a review from a team as a code owner August 22, 2025 17:08

rocodes moved this from In Progress to Ready For Review in SecureDrop Aug 22, 2025

rocodes moved this from Ready For Review to In Progress in SecureDrop Aug 22, 2025

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from 03dff50 to 7e70c5c Compare August 22, 2025 18:19

Combine apt_freedom_press.sources files, use template fields for sign…

a171ae0

…ing key and repo urls.

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from 7e70c5c to 3556876 Compare August 22, 2025 18:56

legoktm requested changes Aug 22, 2025

View reviewed changes

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from 3556876 to bd921d6 Compare August 22, 2025 19:59

legoktm moved this from In Progress to Under Review in SecureDrop Aug 22, 2025

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from bd921d6 to daf10a2 Compare August 22, 2025 22:39

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from daf10a2 to c8a9ccf Compare August 23, 2025 16:09

rocodes requested a review from legoktm August 25, 2025 13:05

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from 5c0d7b8 to f44bbc3 Compare August 25, 2025 13:17

rocodes added 4 commits August 25, 2025 10:11

Refactor test_dom0_rpm_repo so that it runs in dom0 and checks that t…

ef9e303

…he keyring is used consistently across yum and apt components. Move test_sources functionality out of launcher tests. Deprecate skip_in_dom0 conftest annotation.

Delete signing pubkey files from securedrop_salt, now provided by key…

3228ce6

…ring. openqa

Ship the tmp keyring bootstrap .repo file enabled and remove it when …

92da658

…bootstrap is complete

Move keyring check to begining of dev and staging make targets, and a…

ecef9c2

…dd it to bootstrap target openqa

rocodes force-pushed the configure-apt-environment-via-bootstrap branch from 510a4bb to ecef9c2 Compare August 25, 2025 14:12

rocodes moved this from Under Review to In Progress in SecureDrop Aug 25, 2025

rocodes closed this Aug 26, 2025

github-project-automation bot moved this from In Progress to Done in SecureDrop Aug 26, 2025

rocodes mentioned this pull request Aug 28, 2025

remaining issues before integration freedomofpress/securedrop-workstation-keyring#13

Open

10 tasks

nathandyer removed this from SecureDrop Sep 2, 2025

		return key_info


		def _validate_key(info: dict, expected_fpr: str, expected_uid: str, expected_expiry: str):

		return False


		# A bit clunky, but we have limited tooling options in dom0.

Conversation

rocodes commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Checklist

Uh oh!

rocodes commented Aug 21, 2025

Uh oh!

legoktm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocodes Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocodes commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rocodes commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rocodes commented Aug 6, 2025 •

edited

Loading

rocodes Aug 22, 2025 •

edited

Loading

rocodes commented Aug 25, 2025 •

edited

Loading