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

More robust config validation logic #379

Merged
merged 3 commits into from
Dec 20, 2019
Merged

More robust config validation logic #379

merged 3 commits into from
Dec 20, 2019

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Dec 16, 2019

Retains the two previous checks:

  1. Does config.json exist?
  2. Does sd-journalist.sec exist?

Adds several checks verifying the contents of config.json.
There's a minimum of user-friendly messages explaining specifically what
the problem is. We can add those messages over time, aiming to balance
utility for Admins and developers.

Assumes that a v2 Onion URL is used. We can update that logic when we
add support for v3 Onion URLs.

Closes #240.

Testing

  • make validate passes in dom0 with v3 Onion URL info
  • make validate passes in dom0 with v2 Onion URL info
  • make validate fails when a known-bad value is in config.json as appending characters to the Onion URL

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

Suggested verifying the GPG key, flagged that the USB device check is almost obsolete. 🙂

If this is something that will eventually be reporting to journalists, we might want to print nicer messages. If it's just for us, asserts are OK.

scripts/validate-config Outdated Show resolved Hide resolved
scripts/validate-config Show resolved Hide resolved
Conor Schaefer added 3 commits December 19, 2019 16:26
Retains the two previous checks:

  1. Does config.json exist?
  2. Does sd-journalist.sec exist?

Adds several checks verifying the contents of config.json.
There's a minimum of user-friendly messages explaining specifically what
the problem is. We can add those messages over time, aiming to balance
utility for Admins and developers.

Assumes that a v2 Onion URL is used. We can update that logic when we
add support for v3 Onion URLs.
Since both v2 and v3 are supported, check v3 first, but if validation
fails, continue validation by trying matches for v2. If both return
invalid, then fail validation.
Suggested by @rmol during review. Calling `gpg <key_file>` will return
non-zero if the key isn't valid.
@conorsch conorsch force-pushed the 240-validate-config branch from 5267415 to 91a36b9 Compare December 19, 2019 21:29
@conorsch
Copy link
Contributor Author

@rmol Great comments, thanks for taking a look. I've rebased and appended a few changes here, namely to add v3 support, and implement the gpg call you suggested. Also updated the test plan to ensure that both v2 & v3 config files work as expected.

If this is something that will eventually be reporting to journalists, we might want to print nicer messages. If it's just for us, asserts are OK.

Very much dev-focused right now. Deferring a friendlier experience for when we tackle the overall admin provisioning story. At that time, we'll likely crib a fair amount of code from https://github.com/freedomofpress/securedrop/blob/43245829ff93e0a84999526f7ecd0435f89eb52a/admin/securedrop_admin/__init__.py#L60

Ready for re-review!

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

Realized one problem with the USB device check, but don't think it's critical as that's going away. Approving as is, happy to make the change if necessary.

scripts/validate-config Show resolved Hide resolved
@rmol rmol merged commit 0d73914 into master Dec 20, 2019
@rmol rmol deleted the 240-validate-config branch December 20, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate onion URLs before provisioning
3 participants