<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi guys,</p>
<p>I encountered the issue of <a moz-do-not-send="true"
href="https://dev.gnupg.org/T3416">T3416 gpg should select
available signing key on card (even with -u option)</a> 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 :)<br>
</p>
<p>Basically what I did was simply copying the code about smart card
from <tt>build_sk_list</tt> in <tt>skclist.c</tt> to <tt>finish_lookup</tt>
in <tt>getkey.c</tt> so that cards are always checked unless a
user specifies a subkey suffixed with an exclamation mark (patch
based on <span class="transaction-comment"
data-sigil="transaction-comment" data-meta="0_39">v2.2.15)</span>:</p>
<p><tt>diff --git a/g10/getkey.c b/g10/getkey.c</tt><tt><br>
</tt><tt>index c4afe4510..0f9839f73 100644</tt><tt><br>
</tt><tt>--- a/g10/getkey.c</tt><tt><br>
</tt><tt>+++ b/g10/getkey.c</tt><tt><br>
</tt><tt>@@ -3443,8 +3443,28 @@ finish_lookup (kbnode_t keyblock,
unsigned int req_usage, int want_exact,</tt><tt><br>
</tt><tt> {</tt><tt><br>
</tt><tt> kbnode_t nextk;</tt><tt><br>
</tt><tt> int n_subkeys = 0;</tt><tt><br>
</tt><tt> int n_revoked_or_expired = 0;</tt><tt><br>
</tt><tt>+ struct agent_card_info_s card_info;</tt><tt><br>
</tt><tt>+ gpg_error_t card_err = 0;</tt><tt><br>
</tt><tt>+ int pk_card = 0;</tt><tt><br>
</tt><tt>+ int latest_key_card = 0;</tt><tt><br>
</tt><tt>+ char fpr[MAX_FINGERPRINT_LEN];</tt><tt><br>
</tt><tt>+ if (!foundk)</tt><tt><br>
</tt><tt>+ {</tt><tt><br>
</tt><tt>+ /* Check if a card is available. If any,
use the key as a hint. */</tt><tt><br>
</tt><tt>+ char *serialno;</tt><tt><br>
</tt><tt>+ memset (&card_info, 0,
sizeof(card_info));</tt><tt><br>
</tt><tt>+ card_err = agent_scd_serialno
(&serialno, NULL);</tt><tt><br>
</tt><tt>+ if (!card_err)</tt><tt><br>
</tt><tt>+ {</tt><tt><br>
</tt><tt>+ xfree (serialno);</tt><tt><br>
</tt><tt>+ card_err = agent_scd_getattr
("KEY-FPR", &card_info);</tt><tt><br>
</tt><tt>+ if (card_err)</tt><tt><br>
</tt><tt>+ log_error ("error retrieving key
fingerprint from card: %s\n",</tt><tt><br>
</tt><tt>+ gpg_strerror (card_err));</tt><tt><br>
</tt><tt>+ }</tt><tt><br>
</tt><tt>+ }</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt> /* Either start a loop or check just this one
subkey. */</tt><tt><br>
</tt><tt> for (k = foundk ? foundk : keyblock; k; k = nextk)</tt><tt><br>
</tt><tt> {</tt><tt><br>
</tt><tt>@@ -3513,12 +3533,19 @@ finish_lookup (kbnode_t keyblock,
unsigned int req_usage, int want_exact,</tt><tt><br>
</tt><tt> /* In case a key has a timestamp of 0 set, we make
sure</tt><tt><br>
</tt><tt> that it is used. A better change would be to
compare</tt><tt><br>
</tt><tt> ">=" but that might also change the selected
keys and</tt><tt><br>
</tt><tt> is as such a more intrusive change. */</tt><tt><br>
</tt><tt>- if (pk->timestamp > latest_date ||
(!pk->timestamp && !latest_date))</tt><tt><br>
</tt><tt>+ fingerprint_from_pk(pk, fpr, NULL);</tt><tt><br>
</tt><tt>+ if (!(foundk || card_err))</tt><tt><br>
</tt><tt>+ pk_card = (card_info.fpr1valid &&
!strncmp(card_info.fpr1, fpr, MAX_FINGERPRINT_LEN))</tt><tt><br>
</tt><tt>+ || (card_info.fpr2valid &&
!strncmp(card_info.fpr2, fpr, MAX_FINGERPRINT_LEN))</tt><tt><br>
</tt><tt>+ || (card_info.fpr3valid &&
!strncmp(card_info.fpr3, fpr, MAX_FINGERPRINT_LEN));</tt><tt><br>
</tt><tt>+ if ((pk->timestamp > latest_date ||
(!pk->timestamp && !latest_date))</tt><tt><br>
</tt><tt>+ && (pk_card ||
!latest_key_card))</tt><tt><br>
</tt><tt> {</tt><tt><br>
</tt><tt> latest_date = pk->timestamp;</tt><tt><br>
</tt><tt> latest_key = k;</tt><tt><br>
</tt><tt>+ latest_key_card = pk_card;</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> if (n_subkeys == n_revoked_or_expired &&
r_flags)</tt><tt><br>
</tt><tt> *r_flags |= LOOKUP_ALL_SUBKEYS_EXPIRED;</tt><tt><br>
</tt><tt>diff --git a/g10/gpgv.c b/g10/gpgv.c</tt><tt><br>
</tt><tt>index c142cef86..c01d66425 100644</tt><tt><br>
</tt><tt>--- a/g10/gpgv.c</tt><tt><br>
</tt><tt>+++ b/g10/gpgv.c</tt><tt><br>
</tt><tt>@@ -620,8 +620,16 @@ agent_scd_getattr (const char *name,
struct agent_card_info_s *info)</tt><tt><br>
</tt><tt> (void)name;</tt><tt><br>
</tt><tt> (void)info;</tt><tt><br>
</tt><tt> return 0;</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt>+</tt><tt><br>
</tt><tt>+int</tt><tt><br>
</tt><tt>+agent_scd_serialno (char **r_serialno, const char
*demand)</tt><tt><br>
</tt><tt>+{</tt><tt><br>
</tt><tt>+ (void)r_serialno;</tt><tt><br>
</tt><tt>+ (void)demand;</tt><tt><br>
</tt><tt>+ return 0;</tt><tt><br>
</tt><tt>+}</tt><tt><br>
</tt><tt> #endif /* ENABLE_CARD_SUPPORT */</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt> /* We do not do any locking, so use these stubs here */</tt><tt><br>
</tt><tt> void</tt><tt><br>
</tt><tt>diff --git a/g10/test-stubs.c b/g10/test-stubs.c</tt><tt><br>
</tt><tt>index 1e1363266..e53e49a78 100644</tt><tt><br>
</tt><tt>--- a/g10/test-stubs.c</tt><tt><br>
</tt><tt>+++ b/g10/test-stubs.c</tt><tt><br>
</tt><tt>@@ -383,8 +383,16 @@ agent_scd_getattr (const char *name,
struct agent_card_info_s *info)</tt><tt><br>
</tt><tt> (void)name;</tt><tt><br>
</tt><tt> (void)info;</tt><tt><br>
</tt><tt> return 0;</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt>+</tt><tt><br>
</tt><tt>+int</tt><tt><br>
</tt><tt>+agent_scd_serialno (char **r_serialno, const char
*demand)</tt><tt><br>
</tt><tt>+{</tt><tt><br>
</tt><tt>+ (void)r_serialno;</tt><tt><br>
</tt><tt>+ (void)demand;</tt><tt><br>
</tt><tt>+ return 0;</tt><tt><br>
</tt><tt>+}</tt><tt><br>
</tt><tt> #endif /* ENABLE_CARD_SUPPORT */</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt> /* We do not do any locking, so use these stubs here */</tt><tt><br>
</tt><tt> void</tt><br>
</p>
<p>Then it seems that there's actually no need to check smart cards
in <tt>build_sk_list</tt> as the program goes through <tt>finish_lookup</tt>
anyway, so to remove those codes:</p>
<p><tt>diff --git a/g10/skclist.c b/g10/skclist.c</tt><tt><br>
</tt><tt>index 78890dc42..8bc1300b6 100644</tt><tt><br>
</tt><tt>--- a/g10/skclist.c</tt><tt><br>
</tt><tt>+++ b/g10/skclist.c</tt><tt><br>
</tt><tt>@@ -128,29 +128,14 @@ build_sk_list (ctrl_t ctrl,</tt><tt><br>
</tt><tt> are in batch mode, die. */</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt> if (!locusr) /* No user ids given - use the card key
or the default key. */</tt><tt><br>
</tt><tt> {</tt><tt><br>
</tt><tt>- struct agent_card_info_s info;</tt><tt><br>
</tt><tt> PKT_public_key *pk;</tt><tt><br>
</tt><tt>- char *serialno;</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt>- memset (&info, 0, sizeof(info));</tt><tt><br>
</tt><tt> pk = xmalloc_clear (sizeof *pk);</tt><tt><br>
</tt><tt> pk->req_usage = use;</tt><tt><br>
</tt><tt> </tt><tt><br>
</tt><tt>- /* Check if a card is available. If any, use the
key as a hint. */</tt><tt><br>
</tt><tt>- err = agent_scd_serialno (&serialno, NULL);</tt><tt><br>
</tt><tt>- if (!err)</tt><tt><br>
</tt><tt>- {</tt><tt><br>
</tt><tt>- xfree (serialno);</tt><tt><br>
</tt><tt>- err = agent_scd_getattr ("KEY-FPR",
&info);</tt><tt><br>
</tt><tt>- if (err)</tt><tt><br>
</tt><tt>- log_error ("error retrieving key fingerprint
from card: %s\n",</tt><tt><br>
</tt><tt>- gpg_strerror (err));</tt><tt><br>
</tt><tt>- }</tt><tt><br>
</tt><tt>-</tt><tt><br>
</tt><tt>- err = get_seckey_default_or_card (ctrl, pk,</tt><tt><br>
</tt><tt>- info.fpr1valid?
info.fpr1 : NULL, 20);</tt><tt><br>
</tt><tt>+ err = get_seckey_default (ctrl, pk);</tt><tt><br>
</tt><tt> if (err)</tt><tt><br>
</tt><tt> {</tt><tt><br>
</tt><tt> free_public_key (pk);</tt><tt><br>
</tt><tt> pk = NULL;</tt><br>
</p>
<p>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.<br>
</p>
<pre class="moz-signature" cols="72">--
Best Regards,
Frederick Zhang
Email: <a class="moz-txt-link-abbreviated" href="mailto:frederick888@tsundere.moe">frederick888@tsundere.moe</a></pre>
</body>
</html>