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

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

Open
1 task done
cfm opened this issue Nov 1, 2023 · 8 comments
Open
1 task done
Labels

Comments

@cfm
Copy link
Member

cfm commented Nov 1, 2023

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...
    for keyinfo in gpg.list_keys(secret=True):
  2. ...then it will not qualify for Sequoia-based regeneration in...
    if source.fingerprint is None:
  3. ...but nor will it qualify for private-key migration in...
    elif source.pgp_secret_key is None:

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

@cfm cfm added this to the SecureDrop 2.7.0 milestone Nov 1, 2023
@cfm cfm mentioned this issue Nov 1, 2023
36 tasks
@zenmonkeykstop
Copy link
Contributor

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

if source.fingerprint is None:
on next login.

@legoktm
Copy link
Member

legoktm commented Nov 1, 2023

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

if source.fingerprint is None:

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
Copy link
Contributor

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
Copy link
Member Author

cfm commented Nov 1, 2023

Hypothesis discussed in today's stand-up: This is an edge case emerging out of the logic summarized in #7055 (comment), 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
Copy link
Member Author

cfm commented Nov 1, 2023

#7055 (comment):

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 cfm changed the title [2.7.0] a source whose public key is not migrated at postinst will never be migrated a source whose public key is not migrated at postinst will never be migrated Nov 1, 2023
@cfm cfm modified the milestones: SecureDrop 2.7.0, SecureDrop 2.8.0 Nov 1, 2023
@cfm
Copy link
Member Author

cfm commented Nov 1, 2023

Deferred to v2.8 based on discussion today.

@legoktm
Copy link
Member

legoktm commented Nov 1, 2023

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
Copy link
Member Author

cfm commented Dec 19, 2023

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. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants