Keep verified subkey signatures when a GPG key is checked again

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab:jmrp-gpg-key-updater-subkey-rule into master

What does this MR do and why?

This MR builds on !265240 and merges after it. Until then it also carries that MR's two commits, so the change to review is in the last two commits. Once !265240 merges, I'll rebase this MR onto master.

InvalidGpgSignatureUpdater checks again every signature made by a subkey, verified ones included, because a subkey's row never carries gpg_key_id. So after a key update, an email confirmation or a primary email change, every commit signed with a subkey that has expired since turns into "Expired key".

This MR selects a subkey's rows as the primary key's already are: those not linked to it, and those not verified. A verified subkey signature then keeps its status. When the stored key revokes the subkey, every row of it is still checked again, so its commits show that a revoked key signed them. The revocation is read with GPGME::Ctx#keys, since GPGME::Key.find hides a revoked key.

  • It reverses one expectation of !255300 (merged). Its example for a verified subkey signature made under a user ID revoked since now expects verified, as the primary key example next to it does, instead of same_user_different_email.
  • No feature flag: the worker only selects fewer rows. I can add one if you want it.
  • Docs: "Update a GPG key" now says which commits keep their status.
Database

The query keeps WHERE gpg_key_primary_keyid IN (...) on index_gpg_signatures_on_gpg_key_primary_keyid. For a subkey, gpg_key_id IS NULL OR verification_status <> verified becomes gpg_key_subkey_id IS NULL OR verification_status <> verified, a subset of the rows it selected before. For a revoked subkey the condition is dropped, which selects what it selected before.

How I tested it

Ruby 3.3.11 and PostgreSQL 17, without the GDK:

  • bundle exec rspec spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb spec/lib/gitlab/gpg_spec.rb spec/models/gpg_key_spec.rb spec/models/email_spec.rb spec/services/repositories/gpg_keys/update_service_spec.rb spec/workers/invalid_gpg_signature_update_worker_spec.rb: 191 examples, 1 failure, the one below.
  • RuboCop, markdownlint-cli2 and Vale on the changed files: clean.

With the current updater, the changed example, the two examples for a verified subkey signature whose subkey has expired since or whose key revokes another subkey (expired_key and same_user_different_email instead of verified), and every revoked_subkey? example fail. A new revoked_subkey? example fails if it ignores the fingerprint, and another if it counts an expired subkey as revoked. New examples pin the revoked subkey (revoked_key) and an unverified subkey signature that a new primary email verifies; they pass on both, and pin that the narrower rule still checks those rows. The two new fixtures in spec/support/helpers/gpg_helpers.rb come from UserWithRevokedUid.secret_key with GnuPG 2.2.40 under --faked-system-time: --quick-set-expire on the subkey, and --quick-add-key followed by revkey. Key creation does not change, since no row can be linked to a subkey created a moment ago. invalid_gpg_signature_updater_spec.rb:70 fails on master too (a let_it_be_with_reload record keeps the other example's commit double).

References

Related to #296977

Screenshots or screen recordings

No UI change.

How to set up and validate locally

  1. Make a key with a signing subkey that expires in two minutes, and add it in User settings > GPG keys:

    export GNUPGHOME=$(mktemp -d)
g() { gpg --batch --pinentry-mode loopback --passphrase '' "$@"; }
g --quick-gen-key 'Test User <you@example.com>' ed25519 cert 1y
FPR=$(gpg --list-keys --with-colons | awk -F: '/^fpr/ { print $10; exit }')
g --quick-add-key "$FPR" ed25519 sign seconds=120
gpg --armor --export "$FPR"

  2. Push a commit signed with the subkey, and open it: GitLab shows Verified.

  3. After two minutes, add a user ID (g --quick-add-uid "$FPR" 'Test User <other@example.com>') and add the new export to update the key. The commit stays Verified. Without this MR, it turns into an expired key.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by José M. Requena Plens

Merge request reports