search

freedomofpress / securedrop

GitHub repository for the SecureDrop whistleblower platform. Do not submit tips here!
https://securedrop.org/
Other
3.62k stars 686 forks source link

a source whose public key is not migrated at postinst will never be migrated #7055

Open cfm opened 1 year ago

cfm commented 1 year ago

Description

If a source's public key is somehow missed in the upgrade to Alembic version 17c559a7a685, it will never be migrated, even if the source logs in or a journalist interacts with them.

Steps to Reproduce

This is rough and abstract; I need to reproduce it out of my messy testing in #6979.

  1. If a source misses its public-key migration for any reason in... https://github.com/freedomofpress/securedrop/blob/4b954a117a0950013df4108f7387cfd063f92373/securedrop/alembic/versions/17c559a7a685_pgp_public_keys.py#L47
  2. ...then it will not qualify for Sequoia-based regeneration in... https://github.com/freedomofpress/securedrop/blob/4b954a117a0950013df4108f7387cfd063f92373/securedrop/source_app/main.py#L371
  3. ...but nor will it qualify for private-key migration in... https://github.com/freedomofpress/securedrop/blob/4b954a117a0950013df4108f7387cfd063f92373/securedrop/source_app/main.py#L382

Expected Behavior

This should never happen outside of manual keyring-mangling for QA purposes...

...but it's a "stuck" state we should be able to recover from.

Actual Behavior

My QA source stabilized groom can send messages and be sent replies, despite being in this state:

sdadmin@app:~$ export DESIGNATION="stabilized groom"
sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources WHERE journalist_designation=\"$DESIGNATION\";"
10006|0c45b819-750a-4de3-aabc-95e07d79e2d7|GLV3UTT74QNTPENE7OINM7TDG4AFTVZF4NCGLI2M3NB4X3DZUUNPVILI3STPDV3KRSY23W3FTOVHULT5ZEF2BZZJ74TBZIKQJ7EI4TI=|stabilized groom|2023-10-31 23:34:33.953114|0|6||||
sdadmin@app:~$ export ID="GLV3UTT74QNTPENE7OINM7TDG4AFTVZF4NCGLI2M3NB4X3DZUUNPVILI3STPDV3KRSY23W3FTOVHULT5ZEF2BZZJ74TBZIKQJ7EI4TI="
sdadmin@app:~$ sudo -u www-data gpg --homedir /var/lib/securedrop/keys --list-keys --trust-model=direct | grep "$ID" -A3 -B3

Comments

zenmonkeykstop commented 1 year ago

GPG pubkey and fingerprint are likely cached in Redis, if they weren't flushed from there. If it wasn't migrated then the fingerprint field is gonna be blank, so in the absence of cached values my expectation would be that a new reply keypair would be generated at https://github.com/freedomofpress/securedrop/blob/4b954a117a0950013df4108f7387cfd063f92373/securedrop/source_app/main.py#L371 on next login.

legoktm commented 1 year ago

Good spot @cfm, we only attempt public key migration during the alembic upgrade and never again. This should not be a functional issue for sources, because everything will continue to fall back to GPG transparently. We could also try public key migration when we do secret key migration or when a journalist tries to reply, or both? Just need to figure out when to do it.

If it wasn't migrated then the fingerprint field is gonna be blank, so in the absence of cached values my expectation would be that a new reply keypair would be generated at

https://github.com/freedomofpress/securedrop/blob/4b954a117a0950013df4108f7387cfd063f92373/securedrop/source_app/main.py#L371 on next login.

source.fingerprint will fall back to the GPG keyring (source.pgp_fingerprint is the DB storage only) so it won't generate a new keypair.

zenmonkeykstop commented 1 year ago

source.fingerprint will fall back to the GPG keyring (source.pgp_fingerprint is the DB storage only) so it won't generate a new keypair.

Sorry, I meant in the case where he's deleted the key from the GPG keypair. Replies will continue to work there as the first behaviour is to check Redis.

cfm commented 1 year ago

Hypothesis discussed in today's stand-up: This is an edge case emerging out of the logic summarized in https://github.com/freedomofpress/securedrop/issues/7055#issue-1971487161, in particular that source.fingerprint is a property over both the GPG keyring (which may be cached) and the source.pgp_fingerprint column, while source.pgp_secret_key is just a column value. To test this hypothesis:

  1. Does stabilized groom hit the regeneration path with a clear Redis cache (after reboot or FLUSHALL)?
  2. Create a source (with loaddata.py with GPG option), delete its key from the keyring, and log in. → The source should have a new keypair generated.

If we can prove this hypothesis, then this case is recoverable in production, where it shouldn't occur naturally anyway, and we can consider adding a just-in-case and/or on-demand retry of the public-key migration in v2.8.

Either way, this is evidence in support of #7027.

cfm commented 1 year ago

https://github.com/freedomofpress/securedrop/issues/7055#issuecomment-1789303197:

To test this hypothesis:

  1. Does stabilized groom hit the regeneration path with a clear Redis cache (after reboot or FLUSHALL)?

  2. Create a source (with loaddata.py with GPG option), delete its key from the keyring, and log in. → The source should have a new keypair generated.

This case reproduces cleanly as follows:

# Create a new source with a legacy keypair:
sdadmin@app:~$ sudo -u www-data /var/www/securedrop/loaddata.py --gpg --source-count 1
[...]
Created source 3/3 (codename: 'oxidation rebound persuaded unimpeded throttle eardrum supermom', journalist designation 'lumbar induction', files: 2, messages: 2, replies: 2)

# Delete that keypair from the keyring:
sdadmin@app:~$ export DESIGNATION="lumbar induction"
sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources WHERE journalist_designation=\"$DESIGNATION\";"
10011|70ccfe82-20fc-401f-8e9b-fab1d5752124|WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I=|lu
mbar induction|2023-11-01 18:39:48.449972|0|6||||
sdadmin@app:~$ export ID="WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I="
sdadmin@app:~$ sudo -u www-data gpg --homedir /var/lib/securedrop/keys --list-keys --trust-model=direct | grep "$ID" -A3 -B3

pub   rsa4096 2013-05-14 [SCEA]
      21C69F6EB1AF6CAA681EC54C9A7F9862BC64A87B
uid           [ultimate] Source Key <WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I=>
sdadmin@app:~$ sudo -u www-data gpg --homedir /var/lib/securedrop/keys --trust-model direct --yes --delete-secret-keys 21C69F6EB1AF6CAA681EC54C9A7F986
2BC64A87B
gpg (GnuPG) 2.2.19; Copyright (C) 2019 Free Software Foundation, Inc.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

sec  rsa4096/9A7F9862BC64A87B 2013-05-14 Source Key <WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXK
WMPB6I=>

Delete this key from the keyring? (y/N) y
This is a secret key! - really delete? (y/N) y
sdadmin@app:~$ sudo -u www-data gpg --homedir /var/lib/securedrop/keys --trust-model direct --yes --delete-key 21C69F6EB1AF6CAA681EC54C9A7F9862BC64A87
B
gpg (GnuPG) 2.2.19; Copyright (C) 2019 Free Software Foundation, Inc.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

pub  rsa4096/9A7F9862BC64A87B 2013-05-14 Source Key <WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXK
WMPB6I=>

Delete this key from the keyring? (y/N) y
sdadmin@app:~$ sudo -u www-data gpg --homedir /var/lib/securedrop/keys --list-keys --trust-model=direct | grep "$ID" -A3 -B3
sdadmin@app:~$ 

sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources WHERE journalist_designation=\"$DESIGNATION\";"
10011|70ccfe82-20fc-401f-8e9b-fab1d5752124|WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I=|lumbar induction|2023-11-01 18:39:48.449972|0|6||||

# The source logs in again.
sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources WHERE journalist_designation=\"$DESIGNATION\";"
10011|70ccfe82-20fc-401f-8e9b-fab1d5752124|WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I=|lumbar induction|2023-11-01 18:39:48.449972|0|6||||

# Explicitly flush the Redis cache:
sdadmin@app:~$ redis-cli FLUSHALL                                           
OK

# The source logs in again.
sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources WHERE journalist_designation=\"$DESIGNATION\";" | grep "BEGIN"
10011|70ccfe82-20fc-401f-8e9b-fab1d5752124|WF7FDVA4NMIHLXVORBOVWZL6QUNITBIVQLNBJF3LIJFLSTMGT4QSLVPPLUDAJCH4JSIIYYC6CLICOXBUJ52BU45JI3CADKEXKWMPB6I=|lumbar induction|2023-11-01 18:39:48.449972|0|6||F90A811319324865FF9770C1F98322309BE8C807|-----BEGIN PGP PUBLIC KEY BLOCK-----
|-----BEGIN PGP PRIVATE KEY BLOCK-----
cfm commented 1 year ago

Deferred to v2.8 based on discussion today.

legoktm commented 1 year ago

Prior to the Sequoia work, sources were conceptually in 3 states: A) created, no gpg key pair B) gpg key pair created C) deleted. In that sense the data is static (until deletion), so having the cache never expire is fine.

In the GPG->Sequoia migration we have edge case scenarios between B and C that are B1) GPG key pair is lost during migration, B2) new Sequoia keypair is generated upon login. Because of these, the assumption that keypair data is static is no longer true. So there is the possibility that the redis cache could be out of sync with the GPG keypair on disk.

That said, I do think B1 is pretty theoretical (there's no code that deletes any keys until everything has been validated and saved in the database) but not zero.

Clearing the cache is also problematic because if the migration failed (which is the edge case we're already focusing on) then there's a concern looking up a source's fingerprint will be too slow because it needs to iterate over the whole keyring. If/when we do #7027 we can optimize EncryptionManager._get_source_key_details() by having GPG.list_keys() (in p-b-p) take a filter parameter or something to let GPG do the filtering instead of us.

cfm commented 10 months ago

I've modeled (an abstraction of) this bug in https://gist.github.com/cfm/1a1881c65160eb0e9497b47b97669768. I'm happy to report that it's just a cache-invalidation problem. Eliminating the cache, as proposed in #7027, should do the trick nicely. :-)