Migrate public keys from GPG to database-backed storage #6946
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
legoktm
force-pushed
the
sequoia-migrate-public
branch
2 times, most recently
from
49f6ec7
to
1616478
Compare
legoktm
commented
Sep 27, 2023
legoktm
force-pushed
the
sequoia-migrate-public
branch
3 times, most recently
from
fe26e56
to
92fc742
Compare
3 tasks
cfm
requested changes
Oct 2, 2023
There was a problem hiding this comment.
This looks great, @legoktm. I've taken the liberty of being a little aggressive in my testing just to make sure we're hitting the code-paths you intend. :-)
I've left a couple of questions inline, but otherwise I'll be happy to approve and merge this once it's rebased from develop after #6892.
- Start the dev server (
make dev) and open a shell into the container, e.g.podman exec --user=root -it $(podman ps --filter name=securedrop --format '{{.ID}}') bash - Run
alembic stamp 811334d7105f(ID of the migration before this one) - Run
./loaddata.py --gpgto add some GPG sources - Run
select filesystem_id, pgp_fingerprint from sources;in SQLite to verify there are some sources without a fingerprint set in the database - My addition: Choose one of the new sources and smoke-test a round trip: source submits; journalist replies
- Run
alembic upgrade head, completes successfully. Then run the database query again to see that all fingerprints have been populated. - My addition, aggressive:
gpg --homedir /var/lib/securedrop/keys --list-secret-keys, thengpg --homedir /var/lib/securedrop/keys --delete-secret-keyeach one - My addition: Choose the same source as before and smoke-test a round trip: source submits; journalist replies
- Obligatory CI passes & visual review
legoktm
force-pushed
the
sequoia-migrate-public
branch
from
92fc742
to
d2828d8
Compare
|
Rebased and integrated is_valid_public_key() checks. |
legoktm
force-pushed
the
sequoia-migrate-public
branch
from
d2828d8
to
a67c243
Compare
New sources are created using Sequoia by default, but we need GPG-backed sources to test the migration process. This function is mostly copied from tests.utils.create_legacy_gpg_key.
Add an alembic migration that iterates over the GPG keyring, identifies source keys, exports them from GPG and saves them into the database. The main failure risks are the interactions with GPG. We already run `gpg.list_keys()` on startup, so it's unlikely that's broken (and if it is, we have bigger problems). So the main concern is exporting the key might fail. The export operation is wrapped in a try/except and we validate the exported key we get from GPG. Notably, this does not increase our footprint of pretty_bad_protcol usage as the two functions being used are already in use elsewhere in SecureDrop. Some higher-level design information is at <#6946 (comment)>. Fixes #6800.
legoktm
force-pushed
the
sequoia-migrate-public
branch
from
a67c243
to
1964032
Compare
|
Rebased + signed. |
cfm
approved these changes
Oct 4, 2023
There was a problem hiding this comment.
Thanks, @legoktm! We've resolved all our discussions from #6946 (review), so I'll merge as soon as CI passes.
8 tasks
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, depends on
#6892 and#6948.Description of Changes
Commit messages have more detail on each.
Fixes #6800.
Testing
make dev) and open a shell into the container, e.g.podman exec --user=root -it $(podman ps --filter name=securedrop --format '{{.ID}}') bashalembic stamp 811334d7105f(ID of the migration before this one)./loaddata.py --gpgto add some GPG sourcesselect filesystem_id, pgp_fingerprint from sources;in SQLite to verify there are some sources without a fingerprint set in the databasealembic upgrade head, completes successfully. Then run the database query again to see that all fingerprints have been populated.Deployment
Any special considerations for deployment?
This is a non-destructive migration. There is some risk the migration fails, which would halt package upgrade, but the main point where we expect that to happen (
gpg.export_key()) is wrapped in a try/except.Checklist
make lint) and tests (make test) pass in the development container