When there are multiple active keys, get_best_pubkey_byname may not work correctly as expected.
I have two active keys on tokens, and I recently begin to use new machine.
I import my new key at first, and then, import old key. I configure trust settings.
In this situation, old key is selected, but apparently get_best_pubkey_byname should return new key.
| rG GnuPG | |||
| rG390f597868a5 gpg: Fix selection of key. | |||
| rG286d4c607574 gpg: Fix two other cases in get_best_pubkey_byname. | |||
| rG627a990f8e30 doc: Fix documentation about --locate-keys. | |||
| rGf2734381ae14 gpg: Fix get_best_pubkey_byname to consider the first match. | |||
| rG44604209c1cf gpg: Put the first key in candidates correctly. | |||
| rG66eb953f4380 gpg: The first key should be in candidates. | |||
| rG7535f1d47a35 gpg: The first key should be in candidates. | |||
| Status | Assigned | Task | ||
|---|---|---|---|---|
| Resolved | • werner | T4713 Bug in get_best_pubkey_byname | ||
| Resolved | • werner | T4702 Deadline for the GnuPG 2.3.0 release | ||
| Resolved | aheinecke | T5273 Release Gpg4win 4.x.x |
diff --git a/g10/getkey.c b/g10/getkey.c index de5024198..051b21203 100644 --- a/g10/getkey.c +++ b/g10/getkey.c @@ -1272,6 +1272,48 @@ only_expired_enc_subkeys (kbnode_t keyblock) return any? 1 : 0; } +static int +fill_pubkey_cmp_cookie (ctrl_t ctrl, struct pubkey_cmp_cookie *new, + const char *name, KBNODE keyblock) +{ + kbnode_t n; + + new->creation_time = 0; + for (n = find_next_kbnode (keyblock, PKT_PUBLIC_SUBKEY); + n; n = find_next_kbnode (n, PKT_PUBLIC_SUBKEY)) + { + PKT_public_key *sub = n->pkt->pkt.public_key; + + if ((sub->pubkey_usage & PUBKEY_USAGE_ENC) == 0) + continue; + + if (! subkey_is_ok (sub)) + continue; + + if (sub->timestamp > new->creation_time) + new->creation_time = sub->timestamp; + } + + for (n = find_next_kbnode (keyblock, PKT_USER_ID); + n; n = find_next_kbnode (n, PKT_USER_ID)) + { + PKT_user_id *uid = n->pkt->pkt.user_id; + char *mbox = mailbox_from_userid (uid->name, 0); + int match = mbox ? strcasecmp (name, mbox) == 0 : 0; + + xfree (mbox); + if (! match) + continue; + + new->uid = scopy_user_id (uid); + new->validity = + get_validity (ctrl, keyblock, &new->key, uid, NULL, 0) & TRUST_MASK; + new->valid = 1; + } + + return new->valid; +} + /* Finally this function compares a NEW key to the former candidate * OLD. Returns < 0 if the old key is worse, > 0 if the old key is * better, == 0 if it is a tie. */ @@ -1346,6 +1388,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, struct getkey_ctx_s *ctx = NULL; int is_mbox = is_valid_mailbox (name); int wkd_tried = 0; + kbnode_t keyblock = NULL; if (retctx) *retctx = NULL; @@ -1353,16 +1396,13 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, start_over: if (ctx) /* Clear in case of a start over. */ { - if (ret_keyblock) - { - release_kbnode (*ret_keyblock); - *ret_keyblock = NULL; - } + release_kbnode (keyblock); getkey_end (ctrl, ctx); ctx = NULL; + keyblock = NULL; } err = get_pubkey_byname (ctrl, mode, - &ctx, pk, name, ret_keyblock, + &ctx, pk, name, &keyblock, NULL, include_unusable); if (err) { @@ -1373,10 +1413,10 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, /* If the keyblock was retrieved from the local database and the key * has expired, do further checks. However, we can do this only if * the caller requested a keyblock. */ - if (is_mbox && ctx && ctx->found_via_akl == AKL_LOCAL && ret_keyblock) + if (is_mbox && ctx && ctx->found_via_akl == AKL_LOCAL) { u32 now = make_timestamp (); - PKT_public_key *pk2 = (*ret_keyblock)->pkt->pkt.public_key; + PKT_public_key *pk2 = keyblock->pkt->pkt.public_key; int found; /* If the key has expired and its origin was the WKD then try to @@ -1388,7 +1428,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, * (see import.c:import_one). */ if (!wkd_tried && pk2->keyorg == KEYORG_WKD && (pk2->keyupdate + 3*3600) < now - && (pk2->has_expired || only_expired_enc_subkeys (*ret_keyblock))) + && (pk2->has_expired || only_expired_enc_subkeys (keyblock))) { if (opt.verbose) log_info (_("checking for a fresh copy of an expired key via %s\n"), @@ -1409,6 +1449,12 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, struct pubkey_cmp_cookie new = { 0 }; kbnode_t new_keyblock; + if (pk) + copy_public_key (&best.key, pk); + else + copy_public_key (&best.key, keyblock->pkt->pkt.public_key); + fill_pubkey_cmp_cookie (ctrl, &best, name, keyblock); + while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0) { int diff = pubkey_cmp (ctrl, name, &best, &new, new_keyblock); @@ -1439,50 +1485,48 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, free_user_id (best.uid); best.uid = NULL; - if (best.valid) + if (retctx) { - if (retctx || ret_keyblock) + ctx = xtrycalloc (1, sizeof **retctx); + if (! ctx) + err = gpg_error_from_syserror (); + else { - ctx = xtrycalloc (1, sizeof **retctx); - if (! ctx) - err = gpg_error_from_syserror (); + ctx->kr_handle = keydb_new (ctrl); + if (! ctx->kr_handle) + { + err = gpg_error_from_syserror (); + xfree (ctx); + ctx = NULL; + if (retctx) + *retctx = NULL; + } else { - ctx->kr_handle = keydb_new (ctrl); - if (! ctx->kr_handle) + u32 *keyid = pk_keyid (&best.key); + ctx->exact = 1; + ctx->nitems = 1; + ctx->items[0].mode = KEYDB_SEARCH_MODE_LONG_KID; + ctx->items[0].u.kid[0] = keyid[0]; + ctx->items[0].u.kid[1] = keyid[1]; + + if (ret_keyblock) { - err = gpg_error_from_syserror (); - xfree (ctx); - ctx = NULL; - if (retctx) - *retctx = NULL; - } - else - { - u32 *keyid = pk_keyid (&best.key); - ctx->exact = 1; - ctx->nitems = 1; - ctx->items[0].mode = KEYDB_SEARCH_MODE_LONG_KID; - ctx->items[0].u.kid[0] = keyid[0]; - ctx->items[0].u.kid[1] = keyid[1]; - - if (ret_keyblock) - { - release_kbnode (*ret_keyblock); - *ret_keyblock = NULL; - err = getkey_next (ctrl, ctx, NULL, ret_keyblock); - } + *ret_keyblock = NULL; + err = getkey_next (ctrl, ctx, NULL, ret_keyblock); } } } - - if (pk) - *pk = best.key; - else - release_public_key_parts (&best.key); } + + if (pk) + *pk = best.key; + else + release_public_key_parts (&best.key); } + release_kbnode (keyblock); + if (err && ctx) { getkey_end (ctrl, ctx);
If we can assume ret_keyblock != NULL (it is, in current implementation), it can be as simple as:
diff --git a/g10/getkey.c b/g10/getkey.c index 6802026f6..27bbd354c 100644 --- a/g10/getkey.c +++ b/g10/getkey.c @@ -1354,6 +1354,8 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, int is_mbox = is_valid_mailbox (name); int wkd_tried = 0; + log_assert (ret_keyblock != NULL); + if (retctx) *retctx = NULL; @@ -1416,7 +1418,10 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, struct pubkey_cmp_cookie new = { 0 }; kbnode_t new_keyblock; - while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0) + copy_public_key (&new.key, (*ret_keyblock)->pkt->pkt.public_key); + new_keyblock = clone_kbnode (*ret_keyblock); + + do { int diff = pubkey_cmp (ctrl, name, &best, &new, new_keyblock); release_kbnode (new_keyblock); @@ -1441,6 +1446,8 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, } new.uid = NULL; } + while (getkey_next (ctrl, ctx, &new.key, &new_keyblock) == 0); + getkey_end (ctrl, ctx); ctx = NULL; free_user_id (best.uid);
I believe that constraint of ret_keyblock != NULL is OK.
Pushing the fix.
Perhaps, backport to 2.2 should be done, too.
There are some problems with the definition of --locate-key. Further discussion required.
I found more wrong cases of get_best_pubkey_byname.
For ranking results,
(1) It may return non-encryption primary key as the most relevant key, when its validity is higher.
(2) It may not select encryption primary key even if its creation time is newer.
Those two cases are fixed in rG286d4c607574: gpg: Fix two other cases in get_best_pubkey_byname..
I think that we should apply further change:
diff --git a/g10/getkey.c b/g10/getkey.c index 077209415..1c337149c 100644 --- a/g10/getkey.c +++ b/g10/getkey.c @@ -1369,7 +1369,7 @@ get_best_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, *retctx = NULL; memset (&pk0, 0, sizeof pk0); - pk0.req_usage = pk? pk->req_usage : 0; + pk0.req_usage = pk? pk->req_usage : PUBKEY_USAGE_ENC; start_over: if (ctx) /* Clear in case of a start over. */
But, tests/openpgp/key-selection.scm assumes that --locate-key may return non-encryption key and expired key.
Let me clarify the point.
PROBLEM:
Key which is shown by gpg --locate-key someone should be used when `gpg -r someone -e'. Currently, the key by --locate-key may not be used (because no encryption capability/subkey is available) and fails, which is confusing.
Or... it could be a feature, not bug, so that failure of -e -r someone can be examined by --locate-keys someone.
pubkey_cmp should be symmetric (pubkey_cmp(A,B) == - pubkey_cmp(B,A)), but it was not.