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

Have JI validate journalist key is valid #7035

Merged
merged 1 commit into from Oct 27, 2023
Merged

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Oct 26, 2023

Status

Ready for review

Description of Changes

During the Sequoia migration, we need to export the journalist public key from the GPG keyring into a file on disk. We also needed to validate the key was usable by Sequoia (e.g. no SHA-1 binding signatures).

Previously the plan was to validate it during the postinst and error out if it wasn't valid, but if validation fails for whatever reason, then we abort, which interrupts the postinst, so, e.g. database upgrades won't be applied. In retrospect having the validation fail at package install time is bad, because it requires even more manual work to get an instance in a working state since you need to manually apply the updates after doing a key rotation.

Now we validate the journalist key during startup of the Journalist Interface, printing and logging an error if it doesn't validate and then exiting. This should bring attention to journalists and therefore the admin that the instance needs manual attention. We will also include information about this change in the pre-release and release announcements.

Fixes #7030.

Testing

  • Visual review & CI passes
  • In the dev environment:
    • git checkout 40a53c07bae7fee643bddf6db62118638ad0b421 securedrop/tests/files/test_journalist_key.pub (get a SHA-1 signed version of the test key)
    • Edit loaddata.py to make it a no-op (comment out the if name == main block)
    • Run make dev, the JI should immediately exit with an error message about the key not being supported.
  • In a staging environment: (might be OK to defer this to rc3 QA?)
    • Set up a 2.6.0 staging environment, which should use the weak test key. Download it from /public-key and run it through sq-keyring-linter to verify it is using a SHA-1 signature.
    • Upgrade to 2.7.0. The upgrade should succeed, but the JI should be down

Deployment

Any special considerations for deployment? Yes, see description.

Checklist

  • Linting (make lint) and tests (make test) pass in the development container

During the Sequoia migration, we need to export the journalist public
key from the GPG keyring into a file on disk. We also needed to validate
the key was usable by Sequoia (e.g. no SHA-1 binding signatures).

Previously the plan was to validate it during the postinst and error out
if it wasn't valid, but if validation fails for whatever reason, then we
abort, which interrupts the postinst, so, e.g. database upgrades won't
be applied. In retrospect having the validation fail at package install
time is bad, because it requires even more manual work to get an
instance in a working state since you need to manually apply the updates
after doing a key rotation.

Now we validate the journalist key during startup of the Journalist
Interface, printing and logging an error if it doesn't validate and then
exiting. This should bring attention to journalists and therefore the
admin that the instance needs manual attention. We will also include
information about this change in the pre-release and release
announcements.

Fixes #7030.
@legoktm legoktm requested a review from a team as a code owner October 26, 2023 20:58
@cfm cfm added this to the SecureDrop 2.7.0 milestone Oct 26, 2023
@cfm cfm self-assigned this Oct 27, 2023
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

The development-environment test plan looks good:

  • In the dev environment:
    • git checkout 40a53c07bae7fee643bddf6db62118638ad0b421 securedrop/tests/files/test_journalist_key.pub (get a SHA-1 signed version of the test key)
    • Edit loaddata.py to make it a no-op (comment out the if name == main block)
    • Run make dev, the JI should immediately exit with an error message about the key not being supported.
user@sd-dev:~/securedrop$ git checkout 40a53c07bae7fee643bddf6db62118638ad0b421 securedrop/tests/files/test_journalist_key.pub
Updated 1 path from 7b9207952
user@sd-dev:~/securedrop$ make dev
[...]
 => Journalist Interface <= 
ERROR: Journalist public key is not valid: NoSupportedKeys("65A1B5FF195B56353CC63DFFCC40EF1228271441")

 => Source Interface <= 

 * Serving Flask app 'source_app' (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: off
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.

 => Journalist Interface <= 

[2023-10-27 00:21:33,390] ERROR in journalist: ERROR: Journalist public key is not valid: NoSupportedKeys("65A1B5FF195B56353CC63DFFCC40EF1228271441")

One of the development servers exited unexpectedly. See the traceback above for details.
Once you have resolved the issue, you can re-run './manage.py run' to continue developing.

Restoration unbreaks the Journalist Interface:

user@sd-dev:~/securedrop$ git reset --hard
HEAD is now at 7fd01d908 Have JI validate journalist key is valid
user@sd-dev:~/securedrop$ make dev
[...]
 => Source Interface <= 

 * Serving Flask app 'source_app' (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: off
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.

 => Journalist Interface <= 

 * Serving Flask app 'journalist_app' (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: off
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on all addresses (0.0.0.0)
 * Running on http://127.0.0.1:8081
 * Running on http://172.17.0.2:8081

I agree that we can defer staging/production-environment testing to RC3 QA, but I'll leave this for @zenmonkeykstop to merge as Release Manager.

Comment on lines +35 to +36
print(f"ERROR: Unable to read journalist public key: {e}", file=sys.stderr)
app.logger.error(f"ERROR: Unable to read journalist public key: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why both print() and app.logger.error() here? Is that for capsys's benefit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to scream as loudly as possible that there's an issue. I started with print because I thought it might be to early to use a logger, but no, the logger seemed fine, so then I added it in as an extra thing but figured leaving in the print could help and wouldn't be harmful.

@zenmonkeykstop
Copy link
Contributor

Agreed on testing, we can add the non-dev test to the RC3 test plan.

@zenmonkeykstop zenmonkeykstop merged commit 4c883e1 into develop Oct 27, 2023
12 checks passed
@legoktm legoktm deleted the ji-validate-key branch October 27, 2023 19:11
@zenmonkeykstop zenmonkeykstop mentioned this pull request Nov 2, 2023
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

determine post-upgrade failure-mode for a SHA-1-signed submission key
3 participants