Migrate to static config #3850

heartsucker · 2018-10-07T13:32:32Z

Status

Ready for review

Description of Changes

Fixes #1966

Changes proposed in this pull request:

Define config.json configuration file and fields
Create a migration tool to convert old config.py into new config.json
Set defaults at runtime
Populate missing config values via ansible

Testing

New Installs

Checkout this branch
Build debs, provision staging VM
Check that app works
Back up the configs
dpkg -i the app code deb (or just run postinst)
Check that the it was idempotent and the configs weren't changed

Old Installs (with ansible)

Checkout the commit on develop that this branch
Build debs, provision staging VM
Check out the head of this branch
Build debs, provision new VM
Check that app works
Check that config was migrated and contains the same values as config.py
Back up the new configs to /tmp or ~
dpkg -i the app code deb (or just run postinst)
Check that the it was idempotent and the configs weren't changed

Old Installs (without ansible)

Checkout the commit on develop that this branch
Build debs, provision staging VM
Check out the head of this branch
Build debs, scp to VM
Install debs manually (to simulate cron-apt)
Check that app works
Check that config was migrated and contains the same values as config.py
Back up the new configs to /tmp or ~
dpkg -i the app code deb (or just run postinst)
Check that the it was idempotent and the configs weren't changed

Old Installs Backup/Restore

Checkout the commit on develop that this branch
Build debs, provision staging VM
Run backup script
Check out the head of this branch
Build debs, provision new VM
rm /etc/securedrop/*.json
Run restore script
Check that config was migrated and contains the same values as config.py

Deployment

We need to be able to support the following cases:

config.py is not present
config.py present, config.py has partial configuration
config.json present, config.json has partial configuration

Checklist

If you made changes to the server application code:

Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

eloquence · 2018-11-28T20:34:31Z

For the 11/28-12/12 sprint, we will aim to do a first time-boxed review; not planning to merge yet.

kushaldas · 2018-11-29T16:56:55Z

I was planning to make the SDConfig class into a proper strictly defined class (this is just a comment).

redshiftzero

Did a first pass review this afternoon, some comments/thoughts inline!

install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml

redshiftzero · 2018-12-10T23:10:40Z

install_files/ansible-base/roles/backup/files/backup.py

    sd_custom_logo = os.path.join(sd_code, "static/i/logo.png")

    tor_hidden_services = "/var/lib/tor/services"
    torrc = "/etc/tor/torrc"

    with tarfile.open(backup_filename, 'w:gz') as backup:
+        backup.add(sd_config_py)


Possible edge case here:

User backs up SecureDrop on 0.11.0 which uses config.py

User restores to new SecureDrop 0.12.0 install which uses config.json

The migration won't occur in this case, no? Running migrate_config.py as part of the restore logic should resolve.

see the logic in restore.py to see if this does the trick.

securedrop/migrate_config.py

molecule/testinfra/staging/app-code/test_securedrop_app_code.py

securedrop/manage.py

install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml

securedrop/source_app/__init__.py

heartsucker · 2018-12-13T10:30:53Z

Before this is merged, an open question is: should config.json be split into source-config.json and journalist-config.json? I swear there was a reason I didn't do this at first, but I can't for the life of me remember what that might have been. Probably that this was just simpler? The advantage of this would be in the future we could have stricter permissions / AppArmor rules to prevent the source app from reading secret data from the journalist config.

install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml

install_files/securedrop-app-code/DEBIAN/postinst

kushaldas · 2018-12-14T06:57:58Z

Before this is merged, an open question is: should config.json be split into source-config.json and journalist-config.json? I swear there was a reason I didn't do this at first, but I can't for the life of me remember what that might have been. Probably that this was just simpler? The advantage of this would be in the future we could have stricter permissions / AppArmor rules to prevent the source app from reading secret data from the journalist config.

As we discussed last night, please go ahead and split them across into two files. This PR also needs Test steps.

heartsucker · 2018-12-15T16:12:03Z

@kushaldas @redshiftzero I've made a few more changes. Basically all all the config generation logic has been moved into populate_config.py and doesn't exist in ansible (other than as a shell-out to it). There were some tests added for a couple of common cases for the state of the config files, but there are tons of edge cases and I'm not sure how thorough we want to be about the entire space of possibilities for this.

heartsucker · 2018-12-15T16:13:12Z

Ah, also the configs were split into a source and journalist version, both as python classes and different JSON files. I also stripped out all global references to the config and everything is injected (for tests and the application).

codecov-io · 2018-12-15T16:35:58Z

Codecov Report

Merging #3850 into develop will increase coverage by 0.17%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3850      +/-   ##
===========================================
+ Coverage    84.67%   84.85%   +0.17%     
===========================================
  Files           43       44       +1     
  Lines         2760     2918     +158     
  Branches       299      318      +19     
===========================================
+ Hits          2337     2476     +139     
- Misses         355      364       +9     
- Partials        68       78      +10

Impacted Files	Coverage Δ
securedrop/securedrop/source.py	`0% <0%> (-100%)`	⬇️
securedrop/securedrop/journalist.py	`0% <0%> (-100%)`	⬇️
securedrop/securedrop/i18n.py	`92.3% <0%> (-2.57%)`	⬇️
securedrop/securedrop/crypto_util.py	`95.45% <0%> (-0.67%)`	⬇️
securedrop/securedrop/qa_loader.py	`85.62% <0%> (-0.54%)`	⬇️
securedrop/securedrop/create-dev-data.py	`0% <0%> (ø)`	⬆️
securedrop/securedrop/populate_config.py	`75.67% <0%> (ø)`
securedrop/securedrop/manage.py	`80.18% <0%> (+1.41%)`	⬆️
securedrop/securedrop/source_app/__init__.py	`94.25% <0%> (+2.11%)`	⬆️
securedrop/securedrop/journalist_app/__init__.py	`94.11% <0%> (+2.16%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b727a9...e5195dd. Read the comment docs.

install_files/ansible-base/roles/backup/files/backup.py

molecule/builder/tests/test_securedrop_deb_package.py

molecule/testinfra/staging/app-code/test_securedrop_app_code.py

securedrop/i18n.py

securedrop/sdconfig.py

kushaldas · 2018-12-17T15:30:51Z

securedrop/sdconfig.py

-            self.DATABASE_USERNAME = _config.DATABASE_USERNAME  # type: ignore
-        except AttributeError:
-            pass
+        self.SECUREDROP_DATA_ROOT = '/var/lib/securedrop/'


Please add the type annotations.

Can you check again that sufficient type annotations have been added?

install_files/securedrop-app-code/DEBIAN/postinst

eloquence · 2019-01-09T20:03:13Z

Recapping our discussion from sprint planning today:

We want to minimize the risks associated with this big (and important!) change by ensuring we express through automated tests the potential states that the configuration file can be in. The number of states should be manageable given that the file is not manually edited: when have the playbooks been run, and what features have been explicitly enabled? We must assume that there are long-running instances that have not run playbooks since pre-0.4.

@heartsucker -- on the FPF side, we've committed to up to 8 person hours to help get this closer to the finish line, but we should make sure we're on the same page vis-a-vis the testing strategy. Let's discuss w/ @redshiftzero on Gitter, or during standup, next time you're around.

heartsucker · 2019-01-10T12:33:52Z

As a note to reviewers, the test test_populate_config.py is the primary place one would want to look to check for what states of config files we are currently operating under. Additional tests would probably be added here.

we don't want the backup script to error out if config files are missing since we don't know the exact combination that will be present on a prod server

eloquence · 2020-06-18T22:58:53Z

Hi @heartsucker, this has been sitting for a long time and I think it's time to acknowledge that we're probably not going to land a version of this change, which would require significant PR surgery at this point, and which is quite high-risk in terms of potential for breaking existing configs.

Instead our bias is to prioritize moving as many settings as reasonably possible to the database (where we can use alembic migrations for change management) and to expose them via the web-based admin UI.

I'm going to leave this open a few more days in case other folks want to make the case for revisiting this PR, and then close if there are no objections.

eloquence · 2020-06-23T22:07:58Z

Closing PR per the above comment; let's keep the static-config branch around for now in case we want to revisit at a later date.

heartsucker requested review from conorsch, msheiny and redshiftzero as code owners October 7, 2018 13:32

heartsucker force-pushed the static-config branch 4 times, most recently from 02de5cd to 4424c2c Compare October 12, 2018 14:23

redshiftzero requested a review from kushaldas November 28, 2018 18:38

redshiftzero suggested changes Dec 10, 2018

View reviewed changes

kushaldas reviewed Dec 13, 2018

View reviewed changes

install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml Outdated Show resolved Hide resolved

install_files/securedrop-app-code/DEBIAN/postinst Outdated Show resolved Hide resolved

heartsucker force-pushed the static-config branch from c81876a to a808d41 Compare December 13, 2018 21:42

heartsucker force-pushed the static-config branch from a808d41 to 77c2d42 Compare December 14, 2018 09:00

kushaldas suggested changes Dec 17, 2018

View reviewed changes

heartsucker requested a review from emkll as a code owner January 8, 2019 15:52

emkll reviewed Jan 8, 2019

View reviewed changes

install_files/securedrop-app-code/DEBIAN/postinst Outdated Show resolved Hide resolved

heartsucker force-pushed the static-config branch from ad683b3 to dc1c9ca Compare January 9, 2019 10:19

heartsucker force-pushed the static-config branch 2 times, most recently from 79db0b0 to 3c5d2e2 Compare January 14, 2019 16:36

heartsucker added 23 commits March 20, 2019 11:06

added test for file locking

b87068c

remove global config variable, force injection everywhere

da9f287

split SDConfig into {Source,Jouralist}InterfaceConfig

4df763d

update code to work with split configs

080e76a

update tests to work with split configs

86df87e

rename migrate_config.py to populate_config.py

0a8eff2

added secret generation logic to populate_config

857b051

update apparmor rules for the app configs

b9bc26e

full tests of simple case config migration / generation

11d505a

test idempotence of secret generation

62fa9b6

test config population with missing config.py

086374c

update read/write permis for /etc/securedrop

f2772af

update backup script to use new files, avoid errors

58fe9ce

we don't want the backup script to error out if config files are missing since we don't know the exact combination that will be present on a prod server

updated tests for configs in deb packages

3eee206

added tests for config defaults

4c4bdfa

update testinfra tests for multiple configs

0e2a366

added additional type annotations to config class

d0e22ea

add defaults to configs to appeas mypy

f79c976

ensure configs have correct permissions in ansible

8e232ee

use comma separated list for support locale option

d775cc6

ensure journalist key fingerprint is written to configs

8d02a0d

ensure supported locales is a list

56dacb9

ensure old config values are read with correct priority

0ee5747

heartsucker force-pushed the static-config branch from 7ff03ad to 0ee5747 Compare March 20, 2019 10:18

eloquence closed this Jun 23, 2020

eloquence mentioned this pull request Jun 23, 2020

Automated web application configuration migration #1966

Open

eloquence mentioned this pull request Jan 27, 2021

legacy config.py files may prevent the web applications from starting or break functionality #5761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to static config #3850

Migrate to static config #3850

heartsucker commented Oct 7, 2018 •

edited

Loading

eloquence commented Nov 28, 2018 •

edited

Loading

kushaldas commented Nov 29, 2018

redshiftzero left a comment

redshiftzero Dec 10, 2018

heartsucker Dec 13, 2018

heartsucker commented Dec 13, 2018

kushaldas commented Dec 14, 2018

heartsucker commented Dec 15, 2018

heartsucker commented Dec 15, 2018

codecov-io commented Dec 15, 2018 •

edited

Loading

kushaldas Dec 17, 2018

heartsucker Dec 20, 2018

eloquence commented Jan 9, 2019

heartsucker commented Jan 10, 2019

eloquence commented Jun 18, 2020 •

edited

Loading

eloquence commented Jun 23, 2020

Migrate to static config #3850

Migrate to static config #3850

Conversation

heartsucker commented Oct 7, 2018 • edited Loading

Status

Description of Changes

Testing

New Installs

Old Installs (with ansible)

Old Installs (without ansible)

Old Installs Backup/Restore

Deployment

Checklist

If you made changes to the server application code:

If you made changes to the system configuration:

If you made non-trivial code changes:

eloquence commented Nov 28, 2018 • edited Loading

kushaldas commented Nov 29, 2018

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Dec 10, 2018

Choose a reason for hiding this comment

heartsucker Dec 13, 2018

Choose a reason for hiding this comment

heartsucker commented Dec 13, 2018

kushaldas commented Dec 14, 2018

heartsucker commented Dec 15, 2018

heartsucker commented Dec 15, 2018

codecov-io commented Dec 15, 2018 • edited Loading

Codecov Report

kushaldas Dec 17, 2018

Choose a reason for hiding this comment

heartsucker Dec 20, 2018

Choose a reason for hiding this comment

eloquence commented Jan 9, 2019

heartsucker commented Jan 10, 2019

eloquence commented Jun 18, 2020 • edited Loading

eloquence commented Jun 23, 2020

heartsucker commented Oct 7, 2018 •

edited

Loading

eloquence commented Nov 28, 2018 •

edited

Loading

codecov-io commented Dec 15, 2018 •

edited

Loading

eloquence commented Jun 18, 2020 •

edited

Loading