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

Begins dynamic dev/prod config logic #432

Merged
merged 22 commits into from Feb 5, 2020
Merged

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Jan 25, 2020

Status

Ready for review

Description of Changes

Refs #406

Changes proposed in this pull request:

  • Adds new config.json value "environment", accepts "dev" or "prod"
  • Updates Salt logic to dynamically use dev/prod vars, based on config.json "environment"
  • Converts URLs https://apt-test-qubes.freedom.press -> https://apt-test.freedom.press

There are a few caveats with the current implementation:

  • config.json is modified on-the-fly as part of e.g. make prod / make dev. Is that what we want?
  • The prod URLs don't actually have stable, signed packages yet—so full testing isn't possible

Testing

dev target

make clone
make clean
make all
make test

staging target

Only run on test hardware. We don't want to install CI-built artifacts in dom0.

make clone
make clean
make staging # confirm prompt
make test

Checklist

If you have made changes to the provisioning logic

  • Linting (make flake8) and tests (make test) pass in dom0 of a Qubes install

  • I have added/removed files, and have updated packaging logic in MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

@conorsch conorsch mentioned this pull request Jan 25, 2020
8 tasks
@emkll
Copy link
Contributor

emkll commented Jan 28, 2020

  • Added securedrop-admin logic to staging and prod scenarios (the /usr/bin/ file is brought in exclusively by the RPM), reusing existing logic from the scripts/ directory.
  • Added conditional logic to install securedrop-workstation-dom0-config for staging and prod environments
  • Added a test-only staging scenario, which should mirror prod, but use test-specific URLs.
  • Fixed the tests

Note that the staging environment should be used on test hardware only, and provisions the workstation VMs and their environment based on the config that's in the RPM itself (initial dom0 logic is ensured by the Makefile target). Dom0 is configured by installing the package, and the package will replace the files in /srv/salt in place, during the run.

To prevent a race condition, once an RPM is is uploaded with changes from this branch (with the securedrop-admin wrapper and script it place, we should apply the following diff, for staging and prod targets):

diff --git a/Makefile b/Makefile
index 9e83f0611..585e783f7 100644
--- a/Makefile
+++ b/Makefile
@@ -27,7 +27,8 @@ staging: ## Configures a STAGING install. To be used on test hardware ONLY
        ./scripts/configure-environment --env staging
        $(MAKE) validate
        $(MAKE) prep-salt
-       ./scripts/provision-all
+       $(MAKE) prep-dom0
+       /usr/bin/securedrop-admin --apply
 
 dom0-rpm: ## Builds rpm package to be installed on dom0
        @./scripts/build-dom0-rpm

@emkll emkll added this to Ready for Review in SecureDrop Team Board Jan 30, 2020
Copy link
Contributor Author

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the collab, @emkll, these changes are looking solid. Pushed up some more cleanup (the most likely breaking change is config.json "target" -> "environment"; that was to resolve some lingering conflicts between our branches, sorry for the override), and left a few questions throughout. If you can comment back on the in-line, I'm confident we can get this into a mergable shape early next week.

set -u
set -o pipefail

python3 /usr/share/securedrop-workstation-dom0-config/scripts/securedrop-admin.py "$@"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the indirection of this wrapper script? Seems simpler to me to copy scripts/securedrop-admin directly to /usr/bin/seucredrop-admin in the RPM spec, no...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, indirection is not required here: the first prep-dom0 pass will install the packages (and drop the securedrop-admin script in place). I will update this

install -m 644 dom0/securedrop-update %{buildroot}/srv/salt/
install -m 644 dom0/securedrop-login %{buildroot}/srv/salt/
install -m 644 dom0/securedrop-launcher.desktop %{buildroot}/srv/salt/
install -m 655 dom0/securedrop-handle-upgrade %{buildroot}/srv/salt/
# The next file should get installed via RPM not via salt
install -m 755 dom0/securedrop-update %{buildroot}/srv/salt/securedrop-update
install -m 755 dom0/securedrop-admin %{buildroot}/%{_bindir}/securedrop-admin
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of:

install -m 755 dom0/securedrop-admin %{buildroot}/%{_bindir}/securedrop-admin

why not

install -m 755 scripts/securedrop-admin.py %{buildroot}/%{_bindir}/securedrop-admin

? N.B. Haven't tested, mostly questioning the need for the wrapper script indirection, as above.

)
except subprocess.CalledProcessError as e:
print(str(e))
sys.exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why catch, then sys.exit? If we simply allow the exception to be raised, we'll still have the script halt execution, as well as exit non-zero. Does the syntax here prevent preventing the traceback? (Even if it does, not sure it's worth the extra lines.)

Runs provision-all to apply the salt state.highstate on dom0 and all VMs
"""
try:
subprocess.check_call([os.path.join("./scripts/provision-all")])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indirection, oh my!! Nit: shouldn't there be a SCRIPTS_PATH in the .join()? Would prefer that we integrate provision-all directly into securedrop-admin, but now is not the time... looking forward to circling back and converting to Python Qubes API wherever possible.

@emkll emkll moved this from Ready for Review to In Development in SecureDrop Team Board Feb 3, 2020
@conorsch
Copy link
Contributor Author

conorsch commented Feb 4, 2020

Looking good with the conditional logic. The latest changes show a minor test failure, related to the updated prod/test paths for keyfiles:

======================================================================
ERROR: test_rpm_repo_public_key (test_dom0_rpm_repo.SD_Dom0_Rpm_Repo_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/securedrop-workstation/tests/test_dom0_rpm_repo.py", line 33, in test_rpm_repo_public_key
    with open(self.pubkey_actual, "r") as f:
FileNotFoundError: [Errno 2] No such file or directory: '/etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation'

======================================================================
FAIL: test_rpm_repo_config (test_dom0_rpm_repo.SD_Dom0_Rpm_Repo_Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/securedrop-workstation/tests/test_dom0_rpm_repo.py", line 54, in test_rpm_repo_config
    self.assertEqual(found_lines, wanted_lines)
AssertionError: Lists differ: ['[se[104 chars]ation-prod', 'file:///etc/pki/rpm-gpg/RPM-GPG-[152 chars]epo'] != ['[se[104 chars]ation', 'enabled=1', 'baseurl=https://yum-test[80 chars]epo']

First differing element 2:
'gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation-prod'
'gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation'

First list contains 1 additional elements.
First extra element 6:
'name=SecureDrop Workstation Qubes dom0 repo'

  ['[securedrop-workstation-dom0]',
   'gpgcheck=1',
-  'gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation-prod',
?                                                                    -----

+  'gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation',
-  'file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation-test',
   'enabled=1',
   'baseurl=https://yum-test.securedrop.org/workstation/dom0/f25',
   'name=SecureDrop Workstation Qubes dom0 repo']

----------------------------------------------------------------------
Ran 49 tests in 237.949s

FAILED (failures=1, errors=1)
Makefile:114: recipe for target 'test' failed
make: *** [test] Error 1

I believe all that's necessary is a few finishing touches on the salt logic. The conditionals for test/prod state look solid on visual review, it's mostly the corresponding filepaths that should be unique across the board, effectively overturning the simplification in c378758 in favor of using explicit prod/test filepaths.

@emkll
Copy link
Contributor

emkll commented Feb 4, 2020

@conorsch I have removed the logic for trusting the prod key in non prod environments. In the changes you've reviewed, I have also omitted changes to sys-firewall. After implementing changes to sys-firewall, it seemed to me like the logic was overly complicated for little benefit (templating the script in /rw/config/ for example, handling the clean-up case), and may introduce unnecessary risk. If we have a compelling reason to use it in the future, we could revisit this.

I think, before merging this, we should focus on the prod-first-install story. Can we get away with not using git to ship software to the dev vm / cloned to dom0?

@emkll emkll force-pushed the 406-prod-make-targets branch 2 times, most recently from 7edccf4 to b1a6877 Compare February 4, 2020 18:18
@emkll emkll moved this from In Development to Ready for Review in SecureDrop Team Board Feb 4, 2020
Conor Schaefer and others added 16 commits February 4, 2020 13:27
Using state file includes doesn't appear to expose vars imported via the
include to the state performing the include. If this doesn't work for
gathering dev/prod vars, we'll likely have to use pillars.
rely on config.json instead of env vars. We can rely on
env vars as jinja templates are evaluated before runtime
(https://docs.saltstack.com/en/latest/topics/jinja/index.html)

Adds production SD release signing key

Necessary for the prod logic. We don't have packages hosted in these
prod URLs yet.

Adds validate check for target config item

Determines the "environment", e.g. dev/prod
Using a single filename for the RPM repo configuration, same as for the
apt repo, regardless of dev/prod URL within that file.
Script performs one operation:
We've already folded the "apt-test-qubes" Buster-based SDW deb packages
into the "apt-test" repository, backed by the dev lfs repo [0].
There's no longer any reason to maintain a Qubes-specific test apt repo;
we'll instead reduce the number of apt URLs from 3 to 2:

  * https://apt-test.freedom.press/ (current default)
  * https://apt.freedom.press/ (prod, future default)

[0] https://github.com/freedomofpress/securedrop-dev-packages-lfs/
The prod endpoints aren't properly configured yet, but at least we're
working on it.
Ensure we can load the environment config variable
Only when target is set to staging or prod in the config.json
The cleanup here is mostly an artifact of branch collaboration. Adding
back in comments that were lost in rebases, and removing extraneous
comments that were left over from debugging.
If environment=dev, then we DON'T want the salt RPM installed in dom0,
because it'll conflict with the direct-copy-from-git-repo strategy used
by the "dev" environment. So if we're installing from dev, let's make
sure that package is absent.

We might want to do this earlier in the run, e.g. in the Makefile, but I
think running it at all is good enough.
Superificial change. Mostly making the change because we had examples of
*both* to due rebase conflicts, and we need one. Given how many
occurrences of "target" elsewhere in the repo (e.g. in the qubesctl
--targets flag), I argue it's more intuitive to discuss the
"environment" as dev/staging/prod.
Conor Schaefer and others added 6 commits February 4, 2020 13:27
Staging environment uses CI-built packages, so let's not install those
in dom0 unless we're on test-only hardware. Prompt for confirmation,
defaulting to "no", if staging env is requested.
- Reduce indirection and direcly provision securedrop-admin via rpm spec
- Better exception handling
* Add SCRIPTS_PATH to sys path to re-use validation script
* Properly copy config.json to /srv/salt/sd
Left out staging specific variables (key, url) to ensure they are not accidentally installed in production.
@emkll
Copy link
Contributor

emkll commented Feb 4, 2020

Rebased on latest master and added docs for the initial provisioning case. This should now be ready for final review

@conorsch
Copy link
Contributor Author

conorsch commented Feb 5, 2020

Latest changes look great, particularly the docs additions explaining the first-run steps for fetching, verifying, and installing the RPM. I'm completely satisfied with the changes here, @emkll, so over to you for a formal approval when you're ready!

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @conorsch , your changes look good to me.

In order to completely test this end-to-end, I've manually built and uploaded a package built on this branch to freedomofpress/securedrop-yum-test#6

Due to the lack of packages on apt.freedom.press and yum.securedrop.org, it will not be a complete test, as the installer will fail on prod scenarios. Given the dependencies on infrastructure (and us actually releasing a beta RC, I think this PR is good to merge for now, since there should be no impact to developers. I'll approve, but not merge, to let others chime in if they disagree

@conorsch conorsch merged commit 2c45093 into master Feb 5, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants