gpgv: Fix legacy fallback on unavailable keys

If a repository is signed with multiple keys, apt 2.4.0 would
ignore the fallback result if some keys were still missing,
causing signature verification to fail.

Rework the logic such that when checking if fallback was "succesful",
missing keys are ignored - it only matters if we managed to verify
one key now, whether good or bad.

Likewise, simplify the logic when to do the fallback:

If there was a bad signature in trusted.gpg.d, do NOT fallback at all
- this is a minor security issue, as a key in trusted.gpg.d could
fail silently with a bad signature, and then a key in trusted.gpg
might allow the signature to succeed (as trusted.gpg.d key is then
missing).

Only fallback if we are missing a good signature, and there are
keys we have not yet checked.

2 files changed, 18 insertions, 4 deletions

diff --git a/methods/gpgv.cc b/methods/gpgv.cc
index fdd8586b4..0d5707e2a 100644
--- a/methods/gpgv.cc
+++ b/methods/gpgv.cc
@@ -429,7 +429,14 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf
    string const msg = VerifyGetSigners(file, outfile, keyFpts, keyFiles, Signers);
    if (_error->PendingError())
       return msg;
-   if (keyFiles.empty() && (Signers.Good.empty() || !Signers.Bad.empty() || !Signers.NoPubKey.empty()))
+
+   // Bad signature always remains bad, no need to retry against trusted.gpg
+   if (!Signers.Bad.empty())
+      return msg;
+
+   // We do not have a key file pinned, did not find a good signature, but found
+   // missing keys - let's retry with trusted.gpg
+   if (keyFiles.empty() && Signers.Good.empty() && !Signers.NoPubKey.empty())
    {
       std::vector<std::string> legacyKeyFiles{_config->FindFile("Dir::Etc::trusted")};
       if (legacyKeyFiles[0].empty())
@@ -437,14 +444,13 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf
       if (DebugEnabled())
 	 std::clog << "Retrying against " << legacyKeyFiles[0] << "\n";
 
-      // Retry against trusted.gpg
       SignersStorage legacySigners;
 
       string const legacyMsg = VerifyGetSigners(file, outfile, keyFpts, legacyKeyFiles, legacySigners);
       if (_error->PendingError())
 	 return legacyMsg;
-      // Hooray, we found the key now
-      if (not(legacySigners.Good.empty() || !legacySigners.Bad.empty() || !legacySigners.NoPubKey.empty()))
+      // Hooray, we found a key apparently, something verified as good or bad
+      if (!legacySigners.Good.empty() || !legacySigners.Bad.empty())
       {
 	 std::string warning;
 	 strprintf(warning,
diff --git a/test/integration/test-method-gpgv-legacy-keyring b/test/integration/test-method-gpgv-legacy-keyring
index 37a86529a..5af955cdf 100755
--- a/test/integration/test-method-gpgv-legacy-keyring
+++ b/test/integration/test-method-gpgv-legacy-keyring
@@ -25,3 +25,11 @@ testwarningequal "Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InReleas
 Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1420 B]
 Reading package lists...
 W: file:${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/InRelease: Key is stored in legacy trusted.gpg keyring (${TMPWORKINGDIRECTORY}/rootdir/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details." aptget update -q
+
+# 2.4.0 regression: If the InRelease file was signed with two keys, fallback to trusted.gpg did not
+# work: It ran the fallback, but then ignored the result, as keys were still missing.
+signreleasefiles 'Joe Sixpack,Marvin Paranoid'
+testwarningequal "Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1867 B]
+Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1867 B]
+Reading package lists...
+W: file:${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/InRelease: Key is stored in legacy trusted.gpg keyring (${TMPWORKINGDIRECTORY}/rootdir/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details." aptget update -q -omsg=with-two-signatures