Suggestion needed: fix T3416 gpg should select available signing key on card (even with -u option)

[Frederick Zhang]

Hi guys,

I encountered the issue of T3416 gpg should select available signing key
on card (even with -u option) <https://dev.gnupg.org/T3416> recently and
I've been working on a fix for it. But since I've got little knowledge
about GPG internals, it'd be nice if I can have some feedback and
suggestions from you and maybe get the changes merged if they do make
sense :)

Basically what I did was simply copying the code about smart card from
build_sk_list in skclist.c to finish_lookup in getkey.c so that cards
are always checked unless a user specifies a subkey suffixed with an
exclamation mark (patch based on v2.2.15):

diff --git a/g10/getkey.c b/g10/getkey.c
index c4afe4510..0f9839f73 100644
--- a/g10/getkey.c
+++ b/g10/getkey.c
@@ -3443,8 +3443,28 @@ finish_lookup (kbnode_t keyblock, unsigned int
req_usage, int want_exact,
     {
       kbnode_t nextk;
       int n_subkeys = 0;
       int n_revoked_or_expired = 0;
+      struct agent_card_info_s card_info;
+      gpg_error_t card_err = 0;
+      int pk_card = 0;
+      int latest_key_card = 0;
+      char fpr[MAX_FINGERPRINT_LEN];
+      if (!foundk)
+          {
+              /* Check if a card is available.  If any, use the key as
a hint.  */
+              char *serialno;
+              memset (&card_info, 0, sizeof(card_info));
+              card_err = agent_scd_serialno (&serialno, NULL);
+              if (!card_err)
+                {
+                  xfree (serialno);
+                  card_err = agent_scd_getattr ("KEY-FPR", &card_info);
+                  if (card_err)
+                    log_error ("error retrieving key fingerprint from
card: %s\n",
+                            gpg_strerror (card_err));
+                }
+          }
 
       /* Either start a loop or check just this one subkey.  */
       for (k = foundk ? foundk : keyblock; k; k = nextk)
     {
@@ -3513,12 +3533,19 @@ finish_lookup (kbnode_t keyblock, unsigned int
req_usage, int want_exact,
       /* In case a key has a timestamp of 0 set, we make sure
          that it is used.  A better change would be to compare
          ">=" but that might also change the selected keys and
          is as such a more intrusive change.  */
-      if (pk->timestamp > latest_date || (!pk->timestamp && !latest_date))
+          fingerprint_from_pk(pk, fpr, NULL);
+          if (!(foundk || card_err))
+              pk_card = (card_info.fpr1valid &&
!strncmp(card_info.fpr1, fpr, MAX_FINGERPRINT_LEN))
+                  || (card_info.fpr2valid && !strncmp(card_info.fpr2,
fpr, MAX_FINGERPRINT_LEN))
+                  || (card_info.fpr3valid && !strncmp(card_info.fpr3,
fpr, MAX_FINGERPRINT_LEN));
+          if ((pk->timestamp > latest_date || (!pk->timestamp &&
!latest_date))
+                  && (pk_card || !latest_key_card))
         {
           latest_date = pk->timestamp;
           latest_key = k;
+          latest_key_card = pk_card;
         }
     }
       if (n_subkeys == n_revoked_or_expired && r_flags)
         *r_flags |= LOOKUP_ALL_SUBKEYS_EXPIRED;
diff --git a/g10/gpgv.c b/g10/gpgv.c
index c142cef86..c01d66425 100644
--- a/g10/gpgv.c
+++ b/g10/gpgv.c
@@ -620,8 +620,16 @@ agent_scd_getattr (const char *name, struct
agent_card_info_s *info)
   (void)name;
   (void)info;
   return 0;
 }
+
+int
+agent_scd_serialno (char **r_serialno, const char *demand)
+{
+  (void)r_serialno;
+  (void)demand;
+  return 0;
+}
 #endif /* ENABLE_CARD_SUPPORT */
 
 /* We do not do any locking, so use these stubs here */
 void
diff --git a/g10/test-stubs.c b/g10/test-stubs.c
index 1e1363266..e53e49a78 100644
--- a/g10/test-stubs.c
+++ b/g10/test-stubs.c
@@ -383,8 +383,16 @@ agent_scd_getattr (const char *name, struct
agent_card_info_s *info)
   (void)name;
   (void)info;
   return 0;
 }
+
+int
+agent_scd_serialno (char **r_serialno, const char *demand)
+{
+  (void)r_serialno;
+  (void)demand;
+  return 0;
+}
 #endif /* ENABLE_CARD_SUPPORT */
 
 /* We do not do any locking, so use these stubs here */
 void

Then it seems that there's actually no need to check smart cards in
build_sk_list as the program goes through finish_lookup anyway, so to
remove those codes:

diff --git a/g10/skclist.c b/g10/skclist.c
index 78890dc42..8bc1300b6 100644
--- a/g10/skclist.c
+++ b/g10/skclist.c
@@ -128,29 +128,14 @@ build_sk_list (ctrl_t ctrl,
      are in batch mode, die.  */
 
   if (!locusr) /* No user ids given - use the card key or the default
key.  */
     {
-      struct agent_card_info_s info;
       PKT_public_key *pk;
-      char *serialno;
 
-      memset (&info, 0, sizeof(info));
       pk = xmalloc_clear (sizeof *pk);
       pk->req_usage = use;
 
-      /* Check if a card is available.  If any, use the key as a hint.  */
-      err = agent_scd_serialno (&serialno, NULL);
-      if (!err)
-        {
-          xfree (serialno);
-          err = agent_scd_getattr ("KEY-FPR", &info);
-          if (err)
-            log_error ("error retrieving key fingerprint from card: %s\n",
-                       gpg_strerror (err));
-        }
-
-      err = get_seckey_default_or_card (ctrl, pk,
-                                        info.fpr1valid? info.fpr1 :
NULL, 20);
+      err = get_seckey_default (ctrl, pk);
       if (err)
     {
       free_public_key (pk);
       pk = NULL;

I tested these patches locally and everything seemed to work fine. But
please do let me know if there are any problems and it'd be great if you
can help me improve them. Thanks.

-- 
Best Regards,
Frederick Zhang

Email:      frederick888 at tsundere.moe

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnupg-devel/attachments/20190410/ceb59576/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.gnupg.org/pipermail/gnupg-devel/attachments/20190410/ceb59576/attachment-0001.sig>