Fix onion service version validation in securedrop-admin install #5335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was fallout from introducing non-interactive configuration validation during the install operation. The SiteConfig._config_in_progress attribute was only being populated in user_prompt_config, so the v3 validator would never see v2 enabled during the install validation step, causing a spurious validation complaint if only v2 were enabled. This change copies the loaded config to _config_in_progress before calling clean_config, so validators can check the values of other configuration values.
zenmonkeykstop
previously approved these changes
Jun 23, 2020
There was a problem hiding this comment.
Fix for #5334 lgtm. There is an unrelated issue as mentioned below but that could be fixed on a more relaxed timescale.
@zenmonkeykstop had a better idea; populating SiteConfig._config_in_progress as we go though clean_config means each validator can trust that other configuration items have been validated.
Build and run the Docker image the same way in local development and CI. Set up admin/Dockerfile with a full copy of the repo to enable more thorough testing of securedrop-admin.
conorsch
approved these changes
Jun 24, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review
Description of Changes
Fixes #5334.
This was fallout from introducing non-interactive configuration validation during the install operation.
The
SiteConfig._config_in_progressattribute was only being populated inuser_prompt_config, so the v3 validator would never see v2 enabled during the install validation step, causing a spurious validation complaint if only v2 were enabled. This changecopies the loaded config tochanges_config_in_progressbefore callingclean_configclean_configto add each validated value to_config_in_progress(thanks @zenmonkeykstop), so validators can check the values of other configuration values.Testing
develop.securedrop-admin sdconfigto enable only v2 onion services.securedrop-admin install. It should abort with this error:fix-5334branch.securedrop-admin install. You should not get the error, but be prompted for the sudo password.Checklist
If you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes: