Allow error code extraction from GPG by including debug output

[drmoose]

This PR is a backend change in support of future work on passff/passff#544.

It enables --debug=ipc to override the --quiet on the command line that pass sends to gpg, and --status-fd=2 to enable non-localized messages sent to stderr. The stderr output is then parsed looking for error codes and filtering out the excessive detail that causes before passing it off to the UI. It adds an "errorCode": key to the json output that's sent to firefox, with the value being an integer code from err-codes.h.in if detected, or 0. "No code detected" and "No error" use the same code intentionally. The "exitCode": key has been left in place for compatibility (with mismatched addon+host versions) and to disambiguate those cases.

Because the details about how these errors are returned in the GPG output vary from version to version, I've created a test harness to test every supportable gpg version in an assortment of docker containers. This test code is... very ad-hoc. I've committed it to my fork if you want to peruse the code or reproduce my results but haven't included it in this PR because re-running it in the future strikes me as being of questionable utility without something like Github Actions.

The output of my testing is the table below, which has columns for each of the conditions I've endeavored to detect. Columns >= 4 represent test runs where I expect a particular error code. Icons are described in the legend below.

distro	gpg version	libgcrypt	Found	Cancel (99)	Bad Pin (11)	No Pinentry (85)	No Secret (17)
fedora:38	2.4.0	1.10.2	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸
fedora:37	2.3.8	1.10.1	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸
rockylinux:9	2.3.3	1.10.0	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸	🇪🇸 🇯🇵 🇺🇸
rockylinux:8	2.2.20	1.8.5	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
centos:7	2.0.22	1.5.3	🇪🇸 🇯🇵 🇺🇸
ubuntu	2.2.27	1.9.4	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
ubuntu:rolling	2.2.40	1.10.1	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸 ⚠️
ubuntu:jammy	2.2.27	1.9.4	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
ubuntu:focal	2.2.19	1.8.5	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
ubuntu:bionic	2.2.4	1.8.1	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
debian:sid	2.2.40	1.10.2	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸 ⚠️
debian:12	2.2.40	1.10.1	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸 ⚠️
debian:11	2.2.27	1.8.8	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸
debian:10	2.2.12	1.8.4	🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸	🐛 🇪🇸 🇯🇵 🇺🇸

Legend:

• 🇺🇸 Works in Engilsh
• 🇪🇸 Works in Spanish
• 🇯🇵 Works in Japanese
• 🐛 Only works with --debug=ipc
• ⚠️ Substantially different stderr output

The "Substantially different" stderr output on the three ⚠️ cells differs from the original output by including the GPG key ID that was used to encrypt the file when "No Secret" occurs. This seems like useful information to me, so I've left it in.

Earlier versions of this table that I posted in the ticket as I went along had more entries for centos:7, but I deliberately removed those. There does not appear to be any way at all to get gnupg 2.0.22 to meaningfully distinguish among "Cancel", "Bad Pin" and "No Pinentry" conditions, and adding a check for one creates false-positives for others. Since passff/passff#544 is about optionally not showing the error modal, I decided the best option here would be to not have any false positives anywhere.

[tuxor1337]

Thanks for your efforts, this is very much appreciated!

I pushed some code refactoring related to some of my personal preferences regarding coding style.

I think that your stderr cleaning currently removes too many lines. Is there a particular reason why you would remove lines that are not related to GPG if the "BEGIN_DECRYPTION" keyword is found? I would prefer to leave all lines untouched that don't start with either "[GNUPG:]" or "gpg: DBG:" (or are continuations of previous GPG statements). What do you think?

[drmoose]

I pushed some code refactoring related to some of my personal preferences regarding coding style.

I like most of these changes. This specific change, however:

-        else:
+        elif not line.startswith('gpg: DBG:'):
             preserve.append(line)
     # Filter out any gpg: DBG: messages that might've slipped through.
-    return (
-        '\n'.join(x for x in preserve if not x.startswith('gpg: DBG:')),
-        error_code
-    ) 
+    return '\n'.join(preserve), error_code

introduces a bug. Stripping out gpg: DBG: lines as a post process, rather than inside the main loop, was done to prevent line continuations of DBG lines from being attached to unrleated non-DBG lines above them.

I think that your stderr cleaning currently removes too many lines. Is there a particular reason why you would remove lines that are not related to GPG if the "BEGIN_DECRYPTION" keyword is found? I would prefer to leave all lines untouched that don't start with either "[GNUPG:]" or "gpg: DBG:" (or are continuations of previous GPG statements). What do you think?

That logic is intended to strip out lines that were not present in the stderr output sent to the addon when opts.quiet was in effect. On some platforms, letting through all non-DBG code results in a much, much longer stderr output which we'd have to show in the addon's UI. For example, on the "No Secret Key" case on Debian Sid, allowing these lines through results in

+gpg: reading options from '[cmdline]'
+gpg: enabled debug flags: ipc
+gpg: public key is 5C1684312328219A
+gpg: no running gpg-agent - starting '/usr/bin/gpg-agent'
+gpg: waiting for the agent to come up ... (5s)
+gpg: connection to agent established 
 gpg: encrypted with 1024-bit RSA key, ID 5C1684312328219A, created 2023-07-05
       "Unrecoverable <unrecoverable@example.com>"
 gpg: decryption failed: No secret key
+gpg: secmem usage: 32/65536 bytes in 1 blocks

none of which is helpful to the user, and which would overflow the <pre> in the passff error modal. passs decision to inject --quiet to all gpg calls results in a much cleaner UI.

41fe4c1

Introduces another bug, in that it renames some, but not all, instances of error_code to gpg_error_code.

[tuxor1337]

The example you posted doesn't look that bad. If it's not possible to reduce the number of lines further without potentially losing information, I would rather go with additional output.

[drmoose]

I added one minor tweak and re-ran my tests. Everything passes now post-refactor, with the caveat that every condition now has much more stderr output than it used to.

Which... that really feels like noise / worse UX. The "decryption failed" message is no longer intelligible at a glance, real error now buried in the middle of a bunch of lines that make the entire message look useless. (Arguably this is an issue with GPG itself, which seems to go out of its way to be difficult to use unless you've taken a graduate-level course in cryptography, but browsers are used by normal people.)

[tuxor1337]

I think we can still handle this in PassFF. If the error code is known there is no need to show the stderr to the user anyway. We can either suppress the dialog altogether (e.g. when the user deliberately cancelled the operation) or formulate our own custom warning message describing the kind of error. If the error code is not known, or if the error is not caused by GnuPG, we are in a very special situation anyways, and additional logs are rather useful than not.

Do you have a concrete situation in mind where this logic cannot be applied, and where the user will be flooded by gibberish log output even though the situation is actually quite clear (or would clarify immediately from the reduced log output that we used to have)?

[drmoose]

Non-English-speaking users are the situation I have in mind. Passing through the relevant part of the gpg error in all non-cancel cases lets passff intelligibly report errors in languages none of its contributors can speak.

[tuxor1337]

I don't understand, can you elaborate? A non-english speaking user will usually only use PassFF if a translation of PassFF is already available for their language, right? And in that case PassFF should be able to provide meaningful error message in their language, right?

[drmoose]

I'm accustomed to assuming translations (at least, reliable ones) are very difficult to obtain. A quick scan of passff's _locale folder shows us:

de: 78 strings defined
en: 97 strings defined
fr: 80 strings defined
pl: 91 strings defined
pt: 97 strings defined
ru: 76 strings defined
tr: 97 strings defined

The fact that they're not all the same number suggests that at least some of these translations is behind or out of date. By comparison debian:sid has 30 languages for gnupg.

non-english speaking user will usually only use PassFF if a translation of PassFF is already available for their language, right?

A user of one of the languages in the second list, but not in the first list, might still find passff useful in its current form (certainly preferable to "nothing"). Even if the passff-specific strings are written in gibberish, this is still an intelligible error:

All that said, passff-specific error messages for these most common error states would be nice to have (and may reduce the frequency of duplicate issues on here, which are pretty reliably written in English), so it might be a good tradeoff. Naturally, I have no way of knowing how many users are muddling through with the quoted error text alone, if any. I may be worrying about nothing.

[tuxor1337]

You put so much work into this, and it has so much potential of improving PassFF. What would you think about skipping all the filtering and interpretation of the debug outputs in the host app, but instead send all the unfiltered stderr to the extension? There is nothing in the current filtering procedure that could not just as well be implemented in JavaScript from within a WebExtension, right? I think, it's in general much more flexible to move as much of the logic to JavaScript as possible. That way, we can avoid future changes to the host app (which is nice because host app updates require user action).

Here is my JavaScript implementation of the filtering and error code extraction:
https://github.com/passff/passff/blob/ac1c064896b337f793cf3074f5c78f240bea1233/src/modules/pass.js#L246-L283

[drmoose]

Makes sense to me. I see you've already committed e45d99d to my fork. 👍