GNOME Bugzilla – Bug 719872
Seahorse does not show fingerprint in key import dialog
Last modified: 2014-07-29 10:02:25 UTC
Using the seahorse option to find and import remote public keys, the properties dialog does not show the correct fingerprint but the key ID instead go to remote, find remote keys, selecting a key from the results and select properties in the context menu of the selected key tested versions: 3.10.1 ArchLinux 3.2.2 Ubuntu 12.04.3
This happens because parse_hkp_index() only parses the fingerprint suffix from the lines that look like pub 2048R/1234ABCD 2010-01-22 John Doe <john@example.com> i.e. it doesn't actually compute the fingerprint from an actual key.
I believe we can just pass a "fingerprint=on" variable to the GET request in seahorse_hkp_source_search_async(); that instructs the keyserver to return a full fingerprint for the found keys. In turn, parse_hkp_index() will need to parse *that* full fingerprint instead of just picking up the 8 digits from the key ID.
Okay, here is a series of patches to fix this.
Created attachment 273943 [details] [review] Request the full fingerprint from the keyserver
Created attachment 273944 [details] [review] Pretty-print fingerprints in a smarter way
Created attachment 273945 [details] [review] Pretty-print the downloaded fingerprint
Created attachment 273946 [details] [review] Don't overwrite a potentially random character in the fingerprint
Review of attachment 273943 [details] [review]: Cool. Thanks for working on fixing this. ::: pgp/seahorse-hkp-source.c @@ +408,3 @@ + char *str; + + g_assert (subkey_with_id != NULL); We shouldn't assert based on data received from outside the process. I think it would be better to just use subkey_with_id in the 'else if' condition above.
Review of attachment 273944 [details] [review]: This looks good and makes sense.
Review of attachment 273945 [details] [review]: Looks good, once the dependent patch is fixed up.
Review of attachment 273946 [details] [review]: Thanks. looks good.
(In reply to comment #8) > ::: pgp/seahorse-hkp-source.c > @@ +408,3 @@ > + char *str; > + > + g_assert (subkey_with_id != NULL); > > We shouldn't assert based on data received from outside the process. I think it > would be better to just use subkey_with_id in the 'else if' condition above. subkey_with_id gets initialized here: /* Add all the info to the key */ subkey = seahorse_pgp_subkey_new (); seahorse_pgp_subkey_set_keyid (subkey, fpr); subkey_with_id = subkey; In turn, subkey is always created as part of the initialization of a key. So, subkey_with_id should not be null once we get past the stage of initializing the key - hence the assert. It's mostly a sanity check against my own understanding of the code; I can remove it if you want.
(In reply to comment #12) > (In reply to comment #8) > > ::: pgp/seahorse-hkp-source.c > > @@ +408,3 @@ > > + char *str; > > + > > + g_assert (subkey_with_id != NULL); > > > > We shouldn't assert based on data received from outside the process. I think it > > would be better to just use subkey_with_id in the 'else if' condition above. > > subkey_with_id gets initialized here: > > /* Add all the info to the key */ > subkey = seahorse_pgp_subkey_new (); > seahorse_pgp_subkey_set_keyid (subkey, fpr); > subkey_with_id = subkey; > > In turn, subkey is always created as part of the initialization of a key. So, > subkey_with_id should not be null once we get past the stage of initializing > the key - hence the assert. It's mostly a sanity check against my own > understanding of the code; I can remove it if you want. I don't think we should g_assert() (and thus possibly abort() the process) based on input from outside the process. This would occur if a fingerprint came before a "pub " line. Could you put a g_message() here about invalid input, then that would be better.
Woop, I didn't notice your reply :( Will do that.
Created attachment 281924 [details] [review] When searching for keys, request the full fingerprint Updated patch without the g_assert()
Created attachment 281925 [details] [review] Prettify the downloaded fingerprint The other remaining patch, rebased against the previous one.
Thanks. Merged.