Improve deletion of secret subkeys (don't delete primary key when subkey deletion is requested)
Open, NormalPublic

Description

This command that i would have expected to delete the subkey only actually deletes the primary secret key as well:

gpg --delete-secret-key "0x${SUBKEY_FPR}!"

yikes! It does give you a prompt with the primary fingerprint to confirm that you're willing to delete the whole thing, but unless you directly compare fingerprints, it's not obvious that it's prompting you to do something significantly more than you asked it to do.

afaict, the only supported way to delete a secret subkey without deleting its primary key is to learn its keygrip and do:

gpg-connect-agent "delete_key $KEYGRIP" /bye

This is definitely not intuitive for folks used to operating gpg from the command line.

dkg created this task.Apr 11 2019, 5:27 PM
matheusmoreira added a comment.EditedApr 16 2019, 7:29 AM

I've been studying the source code. When a fingerprint suffixed with ! is given as argument, the do_delete_key function correctly classifies the search descriptor as exact and finds the correct key using keydb_search. However, the handle returned by keydb_get_keyblock apparently includes the primary key and all subkeys associated with it. After confirming the action with the user, the function iterates over all PKT_PUBLIC_KEY and PKT_PUBLIC_SUBKEY packets present in the keyblock, obtains the keygrip of each key and asks gpg-agent to delete it.

If the user specified an exact key with a ! suffix, the function should behave differently. The easiest solution I could think of involves skipping all keys other than the one specified by the user:

if (desc.exact && !same_key(username, node->pkt->pkt.public_key))
  continue; /* Current key not the one specified by the user. */

I can't figure out how to compare the keys though. The public key packet structure contains the fingerprint but notes that it is "only valid if FPRLEN is not 0". Also, username might not be a fingerprint at all so it must be converted it to one first. What is the appropriate way to do these things?

Alternatively: is it possible to operate directly on the handle returned by keydb_search? Debugging output indicates that it is returning a handle to the key specified by the user. It seems like a good starting point.

I think it would also be nice to have a safer --delete-secret-subkeys command that would only operate on PKT_PUBLIC_SUBKEY packets. This would protect the user's primary key from accidental deletion.

I managed to make it work on my branch: gpg --delete-secret-key FPR! deletes just that key and no others! I will prepare a patch for this specific change and then try to implement the --delete-secret-subkeys command.

Ha, fancy that, I just added the method of using gpg-connect-agent to our new handbook, I agree that having a --delete-secret-subkeys command would be incredibly handy here.

When the changes are merged I'll update the handbook to reflect the change.

dkg added a comment.Fri, Apr 26, 6:58 AM

nice, i'm glad to hear you've got something working, @matheusmoreira ! if you can point to your branch, or send patches here so that other folks can review, that would be great.

@dkg Sure! I thought I was supposed to email the patches to the development mailing list. I've uploaded my delete-secret-subkey branch to GitHub. You can see a comparison here. I'll describe my changes.

Commit 0f869d5 introduces the should_skip function which is used to decide whether to skip keys instead of deleting them. should_skip signals that the key is to be skipped if the following conditions are true:

  1. The description is for an exact key.
  2. The given public key packet doesn't exactly match the described key.

When do_delete_key iterates over all public key packets in the keyblock, it uses the should_skip function to decide whether to continue to the next key instead of deleting it. This skips all keys other than the key informed by the user. Here are the commands I used to test this:

$ agent/gpg-agent --daemon --homedir $XDG_RUNTIME_DIR/gnupg-git
$ g10/gpg --homedir $XDG_RUNTIME_DIR/gnupg-git --batch --passphrase '' --quick-gen-key test
$ g10/gpg --homedir $XDG_RUNTIME_DIR/gnupg-git -K --with-subkey-fingerprint
sec   rsa3072 2019-04-26 [SC] [expires: 2021-04-25]
      783228FA37305EA9110A2D4D8F23802430D3276B
uid           [ultimate] test
ssb   rsa3072 2019-04-26 [E]
      54DAF8C722ECB466481E4E9148BC31B3B885937E
$ g10/gpg --homedir $XDG_RUNTIME_DIR/gnupg-git --batch --yes --delete-secret-keys 54DAF8C722ECB466481E4E9148BC31B3B885937E!
$ g10/gpg --homedir $XDG_RUNTIME_DIR/gnupg-git -K
sec   rsa3072 2019-04-26 [SC] [expires: 2021-04-25]
      783228FA37305EA9110A2D4D8F23802430D3276B
uid           [ultimate] test
ssb#  rsa3072 2019-04-26 [E]

The sec and ssb# confirm that the subkey is gone and that deletion of the primary key was skipped. If the user doesn't use a trailing !, the function behaves the same as before: it will delete the primary key and all its subkeys. In any case, gpg still asks the user to confirm the deletion of the primary key. This is due to this code:

if (secret)
  print_seckey_info (ctrl, pk);
else
  print_pubkey_info (ctrl, NULL, pk );

tty_printf( "\n" );

yes = cpr_get_answer_is_yes (secret? "delete_key.secret.okay" : "delete_key.okay",
                             _("Delete this key from the keyring? (y/N) "));

if (!cpr_enabled() && secret && yes)
  {
    /* I think it is not required to check a passphrase; if the
    * user is so stupid as to let others access his secret
    * keyring (and has no backup) - it is up him to read some
    * very basic texts about security.  */
    yes = cpr_get_answer_is_yes ("delete_key.secret.okay",
                                 _("This is a secret key! - really delete? (y/N) "));
  }

This code executes before the key deletion logic, where my changes are focused. I'm trying to come up with a better way. I thought about building a list of keys targeted for deletion so gpg can then ask the user to confirm the deletion of each key individually. Is there a linked list data structure that I can use for this?


Commit 2252b84 extracts the secret key deletion logic out of the do_delete_key function and into a reusable function. I think it will be useful when I start working on the --delete-secret-subkeys command.

I based the should_skip function on the implementation of the exact_subkey_match_p function from g10/export.c. Do you think a reusable function in common/ would be useful?

dkg added a subscriber: werner.Sat, Apr 27, 3:15 AM

Thanks for this work, @matheusmoreira ! I personally think a reusable function in common/ would be preferable, but it's probably up to @werner to decide what's best here.

One thing that i can recommend stylistically is to try to structure your commit messages the way that other upstream commit messages are structured (see doc/HACKING)

@dkg, thanks for the feedback. I read doc/HACKING and revised the commit message so that it contains ChangeLog entries and a marker line before my description. I compared my new message to prior log entries and they seem to match now. Is this appropriate? If so, I will revise my other commits in the same manner.

I also took this opportunity to create a new exact-subkey-deletion branch containing just that commit. Now it can be applied directly on top of master.

I thought about building a list of keys targeted for deletion so gpg can then ask the user to confirm the deletion of each key individually.

Just finished implementing this. Now gpg confirms the deletion of each individual secret key instead of just the primary key once. The key information printed before the confirmation prompts now matches the key that gpg will delete if the user answers yes.

Revised the commit message of 2252b84 too. Placed it on the exact-subkey-deletion branch.

If you have a patch please send it either by mail to gnupg-devel or attach it here. Thanks.

matheusmoreira added a comment.EditedTue, Apr 30, 11:50 AM

@werner Here are the patches:



I created revisions for them: D479 D480 D481

Revision D482 adds an extra confirmation prompt before deleting the secret primary key.
Revision D483 adds portuguese translation for the primary key deletion confirmation message.

dkg added a comment.Tue, May 7, 11:16 PM

@werner could you review the patches posted here by @matheusmoreira ? This looks concretely useful, and i would like to have this fixed.

matheusmoreira added a comment.EditedWed, May 8, 12:51 AM

Diffs downloaded from the revisions don't include commit messages for some reason. Here are all the commits I submitted for review as patch files with messages:

  1. 110a4550179f
matheusmoreira added a comment.EditedThu, May 9, 9:14 PM

It appears this issue was first identified and triaged in 2016: T2879
The subkey deletion feature also showed up in other issues since then:

We need a way to delete a secret subkey.

If the patches are merged, these tasks should be updated as well.

werner claimed this task.Tue, May 21, 7:55 AM
werner triaged this task as Normal priority.
werner added a parent task: T4509: Release GnuPG 2.2.16.
matheusmoreira added a comment.EditedWed, May 22, 2:10 AM

@werner Thanks for merging the --dry-run patch in 110a4550179f !

I've rebased the other commits on top of the latest master. They should now apply without any conflicts. I incorporated the changes from 110a4550179f and 126caa34bbdb .

Actually I have a different approach to fix this bug(let). Please give me a few days.