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

Validate an exported secret key is decryptable by Sequoia #7026

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Oct 25, 2023

Status

Ready for review

Description of Changes

When we export a secret key in EncryptionManager, validate the output by checking that it is decryptable by Sequoia using the given passphrase and has the expected fingerprint.

A new redwood function, is_valid_secret_key() is the sibling to the eixsting is_valid_public_key(), except that it also takes a passphrase and verifies the secret key can be unlocked using the passphrase.

If the key passes all the conditions, only then is it returned by EncryptionManager to be saved in the database, and deleted out of GPG. If, for whatever reason, GPG fails at exporting the key, or exports something Sequoia can't handle, the key will not be stored and it'll continue to fall back to using GPG for decryption.

While we're at it, the export function is now named get_source_secret_key_from_gpg, to highlight that this specifically just exports the key from GPG and won't work for Sequoia based sources.

Refs #7025.

Testing

I don't have any manual steps because this is to guard against an edge case in which GPG export fails, so it's easiest to prove with mocks and test cases, but if people have suggestions, they could be added to the test plan!

  • Visual review & CI passes

Deployment

Any special considerations for deployment? No

Checklist

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

@legoktm legoktm added this to the SecureDrop 2.7.0 milestone Oct 25, 2023
@legoktm legoktm requested a review from a team as a code owner October 25, 2023 21:15
zenmonkeykstop
zenmonkeykstop previously approved these changes Oct 27, 2023
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM based on visual review

@zenmonkeykstop
Copy link
Contributor

Tried using GH's conflict resolution tool, but now the rebase in CI is b0rked - that's on me. Will back that out, rebase locally, and force-push.

When we export a secret key in EncryptionManager, validate the output by
checking that it is decryptable by Sequoia using the given passphrase
and has the expected fingerprint.

A new redwood function, is_valid_secret_key() is the sibling to the
eixsting is_valid_public_key(), except that it also takes a passphrase
and verifies the secret key can be unlocked using the passphrase.

If the key passes all the conditions, only then is it returned by
EncryptionManager to be saved in the database, and deleted out of GPG.
If, for whatever reason, GPG fails at exporting the key, or exports
something Sequoia can't handle, the key will not be stored and it'll
continue to fall back to using GPG for decryption.

While we're at it, the export function is now named
`get_source_secret_key_from_gpg`, to highlight that this specifically
just exports the key from GPG and won't work for Sequoia based sources.

Refs #7025.
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM once more

@zenmonkeykstop zenmonkeykstop merged commit 83be8c0 into develop Oct 27, 2023
12 checks passed
@zenmonkeykstop zenmonkeykstop deleted the redwood-check-secret branch October 27, 2023 16:19
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.

None yet

2 participants