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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eliminate flag-for-reply process #5954
Conversation
The changes in sources of randomness in Linux, and libgcrypt's use of them, have eliminated any need there might have been to check the amount of entropy available. Keypair generation takes less than a second on current CPUs. There's no need to defer key generation or to perform it asynchronously.
source.flagged = True | ||
db.session.commit() | ||
return jsonify({'message': 'Source flagged for reply'}), 200 | ||
return jsonify({'message': 'Sources no longer need to be flagged for reply'}), 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth marking the endpoint explicitly as deprecated, to be removed in a future release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can mark it as deprecated in the documentation, then we don't have to update this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the whole test steps, and also visually inspected the code changes.
We should mark the API deprecation in the documentation, other than this I am okay with this change.
source.flagged = True | ||
db.session.commit() | ||
return jsonify({'message': 'Source flagged for reply'}), 200 | ||
return jsonify({'message': 'Sources no longer need to be flagged for reply'}), 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can mark it as deprecated in the documentation, then we don't have to update this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visually reviewed the diff and am relying on kushal's functional review, this LGTM!
Status
Ready for review
Description of Changes
Fixes #1584.
This PR removes the
haveged
package, the checks of the entropy pool before generating source keypairs, and asynchronous source key generation. Source keys are now generated if they don't exist whenever a source visits/lookup
or submits something, and theSource.flagged
column and all associated code have been removed. Theflag
route in the journalist API is now a no-op, always returning a 200 status with a message explaining that the action is no longer needed. Theis_flagged
attribute of the JSON representation of sources returned from the API is now always false.Testing
git checkout 1584-trust-in-disorder
make dev
docker exec -it securedrop-dev-0 bash
) runsqlite3 /var/lib/securedrop/db.sqlite "select id, filesystem_id from sources order by id"
andgpg --homedir /var/lib/securedrop/keys/ --list-keys
to verify that the new source has a key.docker exec -it securedrop-dev-0 bash
.sqlite3 /var/lib/securedrop/db.sqlite "select filesystem_id from sources where journalist_designation = 'its-journalist-designation'"
gpg --homedir /var/lib/securedrop/keys --yes --pinentry-mode=loopback --delete-secret-keys FILESYSTEM_ID
gpg --homedir /var/lib/securedrop/keys --yes --pinentry-mode=loopback --delete-keys FILESYSTEM_ID
redis-cli del sd/crypto-util/fingerprints
Deployment
What could go wrong? 馃檪
I believe the changed logic will cover existing cases of sources without keys. Any source that was flagged for reply should get a key the next time they log in, simply because they don't have a key. The changed messaging in the journalist interface could be confusing for journalists who've had to flag sources for reply, but the effective flow is the same: you just have to wait for the source to return.
Checklist
If you made changes to the server application code:
make lint
) and tests (make test
) pass in the development containerIf you made changes to the system configuration:
If you added or removed a file deployed with the application:
If you made non-trivial code changes:
Choose one of the following: