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

pretty_bad_protocol: Support exporting encrypted secret keys #6907

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Aug 3, 2023

Status

Ready for review

Description of Changes

As part of the Sequoia migration, we want to export secret keys out of the GPG keyring and into our new database storage.

So, have the gpg.export_keys() function accept a passphrase to export encrypted keys that GPG wants a passphrase for. Internally we switch it to use the _handle_io function since that has support for taking passphrases; the one weird thing is that it requires an input file (e.g. if you were encrypting a file) but since there's no input here an empty temporary file works just fine.

For whatever reason I couldn't get a simple p.stdin.write(passphrase) to work, it would hang on stdout, which is probably related to the pre-existing comment about "stdout will be empty in case of failure"...

A new test generates a new GPG key pair, exports the public and secret keys from the keyring and then encrypts and decrypts a message using Sequoia to test compatibility. It also checks a few error cases
like invalid fingerprint and missing secret key.

Refs #6802.

Testing

How should the reviewer test this PR?

  • Visual review
  • CI passes

Deployment

Any special considerations for deployment? No

Checklist

  • Linting (make lint) and tests (make test) pass in the development container
  • I have written a test plan and validated it for this PR

As part of the Sequoia migration, we want to export secret keys out
of the GPG keyring and into our new database storage.

So, have the gpg.export_keys() function accept a passphrase to export
encrypted keys that GPG wants a passphrase for. Internally we switch it
to use the `_handle_io` function since that has support for taking
passphrases; the one weird thing is that it requires an input file (e.g.
if you were encrypting a file) but since there's no input here an empty
temporary file works just fine.

For whatever reason I couldn't get a simple `p.stdin.write(passphrase)`
to work, it would hang on stdout, which is probably related to the
pre-existing comment about "stdout will be empty in case of failure"...

A new test generates a new GPG key pair, exports the public and secret
keys from the keyring and then encrypts and decrypts a message using
Sequoia to test compatibility. It also checks a few error cases
like invalid fingerprint and missing secret key.

Refs #6802.
@legoktm legoktm force-pushed the pbp-export-secrets branch from bd702d6 to 37a8079 Compare August 7, 2023 18:21
@legoktm legoktm changed the title WIP: pretty_bad_protocol: Support exporting encrypted secret keys pretty_bad_protocol: Support exporting encrypted secret keys Aug 8, 2023
@legoktm legoktm marked this pull request as ready for review August 8, 2023 15:19
@legoktm legoktm requested a review from a team as a code owner August 8, 2023 15:19
@cfm cfm self-assigned this Aug 8, 2023
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

The diff here looks good, @legoktm. A couple of comments:

I assume we'll get an integration test of using an actual source's secret key this way in #6802.

When we originally vendored pretty_bad_protocol in #6836, I wasn't aware (at least, I wasn't tracking) that we'd need to patch it further. In hindsight, I wish we'd used something like vendoring (however unsupported) to add another layer of separation between the upstream code we've vendored and our local patches to it; at the very least, I wish we'd marked or logged the patches in the vendored tree for easy reference. But since (a) we don't plan to vendor anything else and (b) even pretty_bad_protocol will go away in some future (breaking!) release, I'll get over it. :-)

@cfm cfm merged commit 7eb784e into develop Aug 16, 2023
@legoktm legoktm deleted the pbp-export-secrets branch August 23, 2023 06:18
@legoktm
Copy link
Member Author

legoktm commented Aug 23, 2023

I wish we'd used something like vendoring (however unsupported) to add another layer of separation between the upstream code we've vendored and our local patches to it; at the very least, I wish we'd marked or logged the patches in the vendored tree for easy reference.

I know I used the term "vendoring" in the initial task description and PR that brought in pbp but really I think this is more of a forking rather than vendoring. There is no upstream to follow, so I don't see the value in clearly identifying patches or tooling to facilitate importing hypothetical newer versions.

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.

2 participants