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

Adds new pubkey for Release Signing Key #5930

Merged
merged 2 commits into from
May 11, 2021

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented May 6, 2021

Status

Ready for review

Description of Changes

Towards #5923

Bumps the version of the securedrop-keyring package, preserving the
old/current release signing key, but adding a new pubkey.

  • Old/current fingerprint: 22245C81E3BAEB4138B36061310F561200F4AD77
  • New/next fingerprint: 2359E6538C0613E652955E6C188EDD3B7B22E6A3

This this is a soft rotation, we'll make sure that all instances have
the new key first, then later remove reference to the old key.

As with previous updates, here's the command I used to import the new
key into the keyring:

gpg --no-default-keyring --keyring \
install_files/securedrop-keyring/etc/apt/trusted.gpg.d/securedrop-keyring.gpg \
--import \
install_files/ansible-base/roles/install-fpf-repo/files/fpf-signing-key-2021.pub

Note the tweak to the target file to import, i.e. the "2021" suffix.

Testing

  1. Review the transition statement in Rotate the SecureDrop Release Signing Key #5923 (comment)
  2. Ensure that the new key introduced in this PR, i.e. 2359E6538C0613E652955E6C188EDD3B7B22E6A3, was used to sign that statement.
  3. Ensure that the current/old key, 22245C81E3BAEB4138B36061310F561200F4AD77, was used to sign that statement.
  4. Ensure that the keyring changes in this PR preserve the old pubkey while adding the new pubkey.
  5. Inspect the GUI updater logic, and confirm it's appropriate for the rotation plan.

Deployment

We intend to include these changes as part of 1.8.2: https://github.com/freedomofpress/securedrop/milestone/73 These changes should be reviewed in the context of ensuring continued updates for both servers and Journalist/Admin Workstations.

@conorsch conorsch force-pushed the 5923-add-new-signing-pubkey branch 2 times, most recently from d63dc6d to 65f5ea6 Compare May 6, 2021 21:53
@conorsch conorsch requested a review from zenmonkeykstop May 6, 2021 21:56
@conorsch conorsch marked this pull request as ready for review May 6, 2021 21:56
@conorsch conorsch requested a review from a team as a code owner May 6, 2021 21:56
@conorsch
Copy link
Contributor Author

conorsch commented May 6, 2021

Note that the admin tests will fail during retrieval from keyservers, e.g.

>       assert b'Signature verification failed' in output
E       AssertionError: assert b'Signature verification failed' in b'INFO: Applying SecureDrop updates...\r\nINFO: Checking for SecureDrop updates...\r\nFetching origin\r\nINFO: Update ...r\', \'hkps://keys.openpgp.org\', \'324C978C1CD14C0C2929D7D96FE1D5E9814BE242\']\' returned non-zero exit status 2.\r\n'

That's because the new pubkey has not yet been pushed to keys.openpgp.org. Once this PR and the associated dual-signed transition statement in #5923 are reviewed, we can push as part of review, and confirm all tests are passing.

@conorsch conorsch force-pushed the 5923-add-new-signing-pubkey branch 2 times, most recently from a8b954f to 054b3f5 Compare May 7, 2021 00:22
@zenmonkeykstop zenmonkeykstop added this to the 1.8.2 milestone May 7, 2021
zenmonkeykstop
zenmonkeykstop previously approved these changes May 7, 2021
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.

✔️ Review the transition statement in #5923 (comment)
✔️ Ensure that the new key introduced in this PR, i.e. 324C978C1CD14C0C2929D7D96FE1D5E9814BE242, was used to sign that statement.
✔️ Ensure that the current/old key, 22245C81E3BAEB4138B36061310F561200F4AD77, was used to sign that statement.
✔️ Ensure that the keyring changes in this PR preserve the old pubkey while adding the new pubkey. (built and installed securedrop-keyring, both keys are present on the server and current key is used when checking Release file on apt.fp
✔️ Inspect the GUI updater logic, and confirm it's appropriate for the rotation plan.

Good to go once the new pubkey is available and tests pass.

@conorsch conorsch requested a review from zenmonkeykstop May 7, 2021 22:26
@conorsch conorsch dismissed zenmonkeykstop’s stale review May 7, 2021 22:27

Publishing pubkey is complicated, let's discuss further

@conorsch
Copy link
Contributor Author

conorsch commented May 7, 2021

Thanks for the careful review here, @zenmonkeykstop. I've dismissed your review for now, pending further discussion with the team. Since we're using keys.openpgp.org, which only supports one (1) key per uid, we may need to alter the uid on the next-up pubkey. That'd require regeneration if so.

@conorsch conorsch added blocked needs/discussion queued up for discussion at future team meeting. Use judiciously. release blocker labels May 7, 2021
Conor Schaefer added 2 commits May 10, 2021 10:51
Bumps the version of the `securedrop-keyring` package, preserving the
old/current release signing key, but adding a new pubkey.

  * Old/current fingerprint: 22245C81E3BAEB4138B36061310F561200F4AD77
  * New/next fingerprint: 2359E6538C0613E652955E6C188EDD3B7B22E6A3

This this is a soft rotation, we'll make sure that all instances have
the new key first, then later remove reference to the old key.

As with previous updates, here's the command I used to import the new
key into the keyring:

    gpg --no-default-keyring --keyring \
    install_files/securedrop-keyring/etc/apt/trusted.gpg.d/securedrop-keyring.gpg \
    --import \
    install_files/ansible-base/roles/install-fpf-repo/files/fpf-signing-key-2021.pub

Note the tweak to the target file to import, i.e. the "2021" suffix.
We still need to support both keys, during the transition period. Let's
make sure that the new key is added, and a signature from either is
considered valid.
@conorsch conorsch force-pushed the 5923-add-new-signing-pubkey branch from 054b3f5 to dd84f81 Compare May 10, 2021 17:52
@conorsch
Copy link
Contributor Author

conorsch commented May 10, 2021

OK, @zenmonkeykstop, ready for another look! I've rebased and amended the PR to use a different "new" key. The transition statement in the corresponding issue has been updated with dual-sigs from the proper pair, old and new. Note the slightly altered uid, as you sagely suggested. Ready for another look. If you confirm these changes are well structured, we can proceed with publishing the new pubkey to hagrid and testing Tails workstation behavior.

@zenmonkeykstop
Copy link
Contributor

The transition statement looks good, and both keys are present on a test server, with apt operations that use the old key still working fine, after building and installing the securedrop-keyring package off this branch. IMO we're ok to go with the keyserver upload and final checks on Tails.

@conorsch
Copy link
Contributor Author

conorsch commented May 11, 2021

Pushed 2359E6538C0613E652955E6C188EDD3B7B22E6A3 : https://keys.openpgp.org/[email protected] Re-ran the admin tests, and they're passing now. In Tails (4.18), I ran the GUI updater, and it successfully pulled in the new key.

@conorsch conorsch removed blocked needs/discussion queued up for discussion at future team meeting. Use judiciously. labels May 11, 2021
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.

Code changes LGTM, and key and transition statement check out. Approved.

conorsch added a commit that referenced this pull request May 11, 2021
backports #5930 to 1.8.2 release branch and updates to RC2
@zenmonkeykstop zenmonkeykstop mentioned this pull request May 13, 2021
32 tasks
@rmol rmol deleted the 5923-add-new-signing-pubkey branch June 23, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants