Skip to content
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

Reorganise repository, Salt states and RPM package #837

Open
eaon opened this issue Oct 12, 2022 · 7 comments
Open

Reorganise repository, Salt states and RPM package #837

eaon opened this issue Oct 12, 2022 · 7 comments

Comments

@eaon
Copy link
Contributor

eaon commented Oct 12, 2022

The securedrop-workstation repository grew organically over time - the dom0/ folder for example was added with the first commit. Since then the project has become far more complex and navigating the codebase has become surprisingly difficult even if one is familiar with the project already. As I looked into what changes would make it a bit more intuitive, I researched how Salt states are usually organised in Qubes OS settings. As a result it became apparent that reconsidering where RPM packages deploy files would make sense to tackle at the same time.

For that reason, I propose to revamp the repo layout to make the what/where/how a bit clearer (and consistent with securedrop-client, the approaches of which I'd consider "canon"), as well as adjust where/what gets pulled from or dropped.

Repository

New folders:

  • files/: assorted files (including binaries) we need to deploy to dom0 as part of the RPM package
  • scripts/: scripts only useful in a dev environment, they won't get shipped with the RPM package
  • securedrop_salt/: a new folder in /srv/, containing SDW Salt states, jinja and top files, as well as files to be deployed to VMs (an example of jinja living next to sls)
  • requirements/: like securedrop-client, keep requirements in a separate folder

Folders to be removed:

  • sd-proxy/, sd-whonix/, sd-app/, usb-autoattach/: only contains (jinja) files which are moving to securedrop_salt/ where they live next to the .sls file that references them
  • sys-firewall/: as far as I can tell it only contains one script that isn't being used anymore
  • sd-workstation/: contents move to files/
  • rpm-repo/: as far as I can tell, not used anymore

Salt states

Currently, we drop everything into the Qubes OS' "system Salt states" location in /srv/salt - this has a couple of drawbacks when the rest of the system is used like a normal Qubes OS setup. If a Salt based customisation that was enabled via qubesctl top.enable and was attempted to be applied with state.highstate, our own setup would be dragged into this operation and take forever to be run due to our complex deployment strategy. Worse, if such user enabled Salt states aren't intended to be run with sudo, our setup will fail during the attempt to apply state.highstate to dom0 (the second operation in provision-all).

Instead, I believe it'd be better if we maintained our own "Salt environment" in /srv/securedrop_salt (named to stay consistent with Qubes own /srv/user_salt). This allows us to avoid issues like the ones above (which would become more ubiquitous as we're looking into making the workstation and client extensible). All that's necessary for qubesctl to be aware is a .conf file in /etc/salt/minion.d/

To distinguish between what happens where, I'd propose to create sub-folders along the lines of:

RPM package content locations

At the moment, some of our file locations are a bit unusual, and the way we copy most files stems from a time in which dev-deployment happened entirely with Salt with no package manager involvement. So there's an opportunity to cut down on some 🧂 and let RPM handle it. In addition, instead of reorganising most package contents on the fly, doing that just with files/ would be preferable to me. I think this would be nice:

  • /srv/
    • securedrop_salt/ entirely as is
  • /usr/bin/
    • User-facing executables from files/ (already happens for sdw-admin.py, but do not change the file extension during packaging, remove the file extension in the repo instead)
  • /usr/libexec/securedrop/
    • Non-user-facing executables from files/
  • /usr/share/securedrop/:
    • config.json.example
    • sd-journalist.sec.example
  • Do not ship anything in scripts/

Note on timing / other reorg PRs

This would probably be best to be dealt with after #831 was merged. Both this and the #831 would require dealing with migrations.

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Oct 12, 2022

I realize that we've talked about most of this ideas @eaon 🙂

In reading order for everything I find straightforward:

  1. As an adept of /srv/user_salt I fully second the idea of defining /srv/securedrop_salt. Thanks you for explaining the main advantages, and how such a separate domain environment is defined. 🙌
  2. The scripts/ and files/ directories:
    • are pretty standard as far as I can tell in our repos ✅
    • I find personally very convenient to know where to look for development scripts
    • I also find neat the distinction between what is deployed (files/) and what is never deployed (scripts/)
  3. I find equally neat the distinction between /usr/bin, /usr/lib/securedrop and /usr/share/securedrop
  4. Last but not least, shipping the securedrop_salt directory without modification seems like a nice feature to me, especially from he perspective of making contributions more accessible.

Regarding the internal organization of /srv/securedrop_salt:

  • I've thought about that when setting up my own /srv/user_salt and didn't find any solution that I found fully satisfactory.
  • I thought of mainly two approaches:
    • Grouping Salt files by feature (e.g. /srv/user_salt/split-ssh, /srv/user_salt/split-gpg) which then internally contain states to be applied to dom0 and other qubes. (example)
      • This has the advantage of working well with packaging, as each of those sub-directories can be self-contained.
      • The cost of being self-contained is some duplication, typically of states that ensure that common packages are available (example).
    • Grouping Salt files by target (e.g. dom0, sd-devices) which is the current proposal.
      • I think this helps thinking of the system as a whole, as it allows to get an idea of what each qube looks like at a given point in time (release number). Ultimately, the breakdown of features carries a very clear "maintainer bias". It may help someone who knows the project history to think about the code, but at the end of the day, dom0 or sd-devices are in a single state at any given time, that's the result of the combination of all the features. Having all those interacting states in a single place can greatly help understand how and why they interact. (By contrast, it can be very difficult to understand the origin of a given state if you're completely unaware that feature X exists.)
      • I think that it is a less obviously modular approach than the feature-based one. I'd think that any given update is likely to touch files across the entire /srv/securedrop_salt directory more often. That's not necessarily a problem? (I'd suggest it's probably enough to keep an eye on it to prevent that from making maintenance excessively error-prone.)

I'm pretty sure there are other possible approaches, but I didn't find any obviously popular patterns when I looked for them, and this is what I've experimented with, so this is my contribution. 🙂


Edit: One aspect that makes intuitive sense to me is to keep the /srv/securedrop_salt/migrations separate and clearly identified. Upon reflection, I believe it's because I think of migration as highly context-dependent and self-contained in their own way. So I'd exclude those from any feature-based thinking... even though that clearly blurs the lines! (And lines are nice, if not always practical 🦓)

@rocodes
Copy link
Contributor

rocodes commented Oct 12, 2022

Hi @eaon, thanks for filing this. I'm very in favour of improving organization (clear agreement on moving away from dumping everything in /srv/salt/ and on creating a migration directory). I need to think on it before I have strong opinions on the RPM stuff but broadly, reorganizing makes sense to me.

As for the discussion @gonzalo-bulnes raises about how to organize our salt states, and the approach in his split-ssh/split-gpg vs the structure proposed here: I think DRY wins the day and it's better to avoid duplication, especially since that can lead to confusion/maintenance errors, so I'm inclined towards the structure you are proposing here.

@eaon
Copy link
Contributor Author

eaon commented Oct 18, 2022

Thanks for your feedback @rocodes and @gonzalo-bulnes!

Re: Salt state organisation, so far, I've thought of securedrop_salt/ as distinct from user_salt/ insofar that we haven't had to think of what could be thought of as distinct "packages" in that directory… Something along the lines of Gonzalo's packages user_salt packages) I mean.

But I think that may actually be desirable. Thinking about dangerzone integration being a separate RPM package that drops its bits in the securedrop_salt environment, and gets set up with the initial install. Going to think about that a bit more.

@gonzalo-bulnes
Copy link
Contributor

Note: Let's not forget updating https://github.com/freedomofpress/securedrop-client/wiki/Project-Patterns as we adopt new / spread existing patterns!

@eaon
Copy link
Contributor Author

eaon commented Jan 27, 2023

Some of this is already being addressed in #851

@eaon
Copy link
Contributor Author

eaon commented Jan 27, 2023

Regarding salt 🧂 organisation, I did think about this more but never got around to replying: I believe we should keep integrations like Dangerzone separate from SecureDrop Workstation salt files. Dangerzone might want to have it's own dangerzone_salt/ directory instead, and integrate into the workstation "platform" via configuration rather than entangling salt states/top files.

I also removed references to migrations with salt as the pattern we decided on for migrations does not involve salt after all.

@rocodes
Copy link
Contributor

rocodes commented May 11, 2023

Bumping this to reiterate support for at mininum moving salt files to their own sub-directory, both for our own selfish purposes and also to move towards a better setup for users who aren't using their Qubes laptops as a single-purpose SDW access machine.

The current RPM post install hook finds and enables all .top files in /srv/salt/ (non-recursively, just in that directory), which is not friendly to users that have other salt files sitting in /srv/salt, and which could have unintended effects for everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants