[gpg] Correct finding public keys from pubring.gpg

With a master key not enabled for signing, and a signing sub-key, key
lookup went wrong in several ways and might not find a suitable key for
signing or for signature verification. Fix the code so that it finds
the sub-key, even if user.signingKey is specified not with a key ID but
with an with an e-mail. (Sub-keys don't have user ids, those are
attached only on the master key.)

Change-Id: I9d1f641c49b173d4daffb3fd2e74f5aabd856e39
Signed-off-by: Thomas Wolf <twolf@apache.org>

5 files changed, 173 insertions, 56 deletions

diff --git a/org.eclipse.jgit.gpg.bc/resources/org/eclipse/jgit/gpg/bc/internal/BCText.properties b/org.eclipse.jgit.gpg.bc/resources/org/eclipse/jgit/gpg/bc/internal/BCText.properties
index ab83298c15..77ca2cd0a4 100644
--- a/org.eclipse.jgit.gpg.bc/resources/org/eclipse/jgit/gpg/bc/internal/BCText.properties
+++ b/org.eclipse.jgit.gpg.bc/resources/org/eclipse/jgit/gpg/bc/internal/BCText.properties
@@ -29,6 +29,7 @@ signatureInconsistent=Inconsistent signature; key ID {0} does not match issuer f
 signatureKeyLookupError=Error occurred while looking for public key
 signatureNoKeyInfo=No way to determine a public key from the signature
 signatureNoPublicKey=No public key found to verify the signature
+signatureNoSigningKey=No signing key found for key fingerprint {0}
 signatureParseError=Signature cannot be parsed
 signatureVerificationError=Signature verification failed
 unableToSignCommitNoSecretKey=Unable to sign commit. Signing key not available.
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BCText.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BCText.java
index 434998493f..705e195e44 100644
--- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BCText.java
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BCText.java
@@ -59,6 +59,7 @@ public final class BCText extends TranslationBundle {
 	/***/ public String signatureKeyLookupError;
 	/***/ public String signatureNoKeyInfo;
 	/***/ public String signatureNoPublicKey;
+	/***/ public String signatureNoSigningKey;
 	/***/ public String signatureParseError;
 	/***/ public String signatureVerificationError;
 	/***/ public String unableToSignCommitNoSecretKey;
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgKeyLocator.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgKeyLocator.java
index e9032a81fd..970e7df3c9 100644
--- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgKeyLocator.java
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgKeyLocator.java
@@ -27,10 +27,14 @@ import java.nio.file.Paths;
 import java.security.NoSuchAlgorithmException;
 import java.security.NoSuchProviderException;
 import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Locale;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import org.bouncycastle.gpg.keybox.BlobType;
 import org.bouncycastle.gpg.keybox.KeyBlob;
@@ -268,9 +272,11 @@ public class BouncyCastleGpgKeyLocator {
 		return keyId;
 	}
 
-	static PGPPublicKey findPublicKey(String fingerprint, String keySpec)
+	static BouncyCastleGpgPublicKey findPublicKey(String fingerprint,
+			String keySpec)
 			throws IOException, PGPException {
-		PGPPublicKey result = findPublicKeyInPubring(USER_PGP_PUBRING_FILE,
+		BouncyCastleGpgPublicKey result = findPublicKeyInPubring(
+				USER_PGP_PUBRING_FILE,
 				fingerprint, keySpec);
 		if (result == null && exists(USER_KEYBOX_PATH)) {
 			try {
@@ -330,7 +336,8 @@ public class BouncyCastleGpgKeyLocator {
 	 * @throws NoOpenPgpKeyException
 	 *             if the file does not contain any OpenPGP key
 	 */
-	private static PGPPublicKey findPublicKeyInKeyBox(Path keyboxFile,
+	private static BouncyCastleGpgPublicKey findPublicKeyInKeyBox(
+			Path keyboxFile,
 			String keyId, String keySpec)
 			throws IOException, NoSuchAlgorithmException,
 			NoSuchProviderException, NoOpenPgpKeyException {
@@ -343,11 +350,16 @@ public class BouncyCastleGpgKeyLocator {
 				hasOpenPgpKey = true;
 				PGPPublicKey key = findPublicKeyByKeyId(keyBlob, id);
 				if (key != null) {
-					return key;
+					if (!isSigningKey(key)) {
+						return null;
+					}
+					return new BouncyCastleGpgPublicKey(key, true,
+							toStrings(keyBlob.getUserIds()));
 				}
 				key = findPublicKeyByUserId(keyBlob, keySpec);
 				if (key != null) {
-					return key;
+					return new BouncyCastleGpgPublicKey(key, true,
+							toStrings(keyBlob.getUserIds()));
 				}
 			}
 		}
@@ -357,6 +369,14 @@ public class BouncyCastleGpgKeyLocator {
 		return null;
 	}
 
+	private static List<String> toStrings(List<UserID> userIds) {
+		if (userIds == null) {
+			return Collections.emptyList();
+		}
+		return userIds.stream().map(UserID::getUserIDAsString)
+				.collect(Collectors.toList());
+	}
+
 	/**
 	 * If there is a private key directory containing keys, use pubring.kbx or
 	 * pubring.gpg to find the public key; then try to find the secret key in
@@ -387,7 +407,7 @@ public class BouncyCastleGpgKeyLocator {
 			NoSuchAlgorithmException, NoSuchProviderException, PGPException,
 			CanceledException, UnsupportedCredentialItem, URISyntaxException {
 		BouncyCastleGpgKey key;
-		PGPPublicKey publicKey = null;
+		BouncyCastleGpgPublicKey publicKey = null;
 		if (hasKeyFiles(USER_SECRET_KEY_DIR)) {
 			// Use pubring.kbx or pubring.gpg to find the public key, then try
 			// the key files in the directory. If the public key was found in
@@ -397,14 +417,15 @@ public class BouncyCastleGpgKeyLocator {
 					publicKey = findPublicKeyInKeyBox(USER_KEYBOX_PATH, null,
 							signingKey);
 					if (publicKey != null) {
-						key = findSecretKeyForKeyBoxPublicKey(publicKey,
-								USER_KEYBOX_PATH);
+						key = findSecretKeyForKeyBoxPublicKey(
+								publicKey.getPublicKey(), USER_KEYBOX_PATH);
 						if (key != null) {
 							return key;
 						}
 						throw new PGPException(MessageFormat.format(
 								BCText.get().gpgNoSecretKeyForPublicKey,
-								Long.toHexString(publicKey.getKeyID())));
+								Long.toHexString(
+										publicKey.getPublicKey().getKeyID())));
 					}
 					throw new PGPException(MessageFormat.format(
 							BCText.get().gpgNoPublicKeyFound, signingKey));
@@ -427,7 +448,8 @@ public class BouncyCastleGpgKeyLocator {
 					// secring.gpg at all, even if it exists. Which means for us
 					// we have to try both since we don't know which GPG version
 					// the user has.
-					key = findSecretKeyForKeyBoxPublicKey(publicKey,
+					key = findSecretKeyForKeyBoxPublicKey(
+							publicKey.getPublicKey(),
 							USER_PGP_PUBRING_FILE);
 					if (key != null) {
 						return key;
@@ -452,7 +474,7 @@ public class BouncyCastleGpgKeyLocator {
 		if (publicKey != null) {
 			throw new PGPException(MessageFormat.format(
 					BCText.get().gpgNoSecretKeyForPublicKey,
-					Long.toHexString(publicKey.getKeyID())));
+					Long.toHexString(publicKey.getPublicKey().getKeyID())));
 		} else if (hasSecring) {
 			// publicKey == null: user has _only_ pubring.gpg/secring.gpg.
 			throw new PGPException(MessageFormat.format(
@@ -614,40 +636,88 @@ public class BouncyCastleGpgKeyLocator {
 	 * @throws PGPException
 	 *             on BouncyCastle errors
 	 */
-	private static PGPPublicKey findPublicKeyInPubring(Path pubringFile,
+	private static BouncyCastleGpgPublicKey findPublicKeyInPubring(
+			Path pubringFile,
 			String keyId, String keySpec)
 			throws IOException, PGPException {
 		try (InputStream in = newInputStream(pubringFile)) {
 			PGPPublicKeyRingCollection pgpPub = new PGPPublicKeyRingCollection(
 					new BufferedInputStream(in),
 					new JcaKeyFingerprintCalculator());
-
 			String id = keyId != null ? keyId
 					: toFingerprint(keySpec).toLowerCase(Locale.ROOT);
 			Iterator<PGPPublicKeyRing> keyrings = pgpPub.getKeyRings();
+			BouncyCastleGpgPublicKey candidate = null;
 			while (keyrings.hasNext()) {
 				PGPPublicKeyRing keyRing = keyrings.next();
-				Iterator<PGPPublicKey> keys = keyRing.getPublicKeys();
-				while (keys.hasNext()) {
-					PGPPublicKey key = keys.next();
-					// try key id
-					String fingerprint = Hex.toHexString(key.getFingerprint())
-							.toLowerCase(Locale.ROOT);
-					if (fingerprint.endsWith(id)) {
-						return key;
-					}
-					// try user id
-					Iterator<String> userIDs = key.getUserIDs();
-					while (userIDs.hasNext()) {
-						String userId = userIDs.next();
-						if (containsSigningKey(userId, keySpec)) {
-							return key;
-						}
+				BouncyCastleGpgPublicKey newCandidate = findPublicKeyInPubring(
+						keyRing, id, keySpec);
+				if (newCandidate != null) {
+					if (newCandidate.isExactMatch()) {
+						return newCandidate;
+					} else if (candidate == null) {
+						candidate = newCandidate;
 					}
 				}
 			}
+			return candidate;
 		} catch (FileNotFoundException | NoSuchFileException e) {
-			// Ignore and return null
+			return null;
+		}
+	}
+
+	private static BouncyCastleGpgPublicKey findPublicKeyInPubring(
+			PGPPublicKeyRing keyRing, String keyId, String keySpec) {
+		Iterator<PGPPublicKey> keys = keyRing.getPublicKeys();
+		if (!keys.hasNext()) {
+			return null;
+		}
+		PGPPublicKey masterKey = keys.next();
+		String fingerprint = Hex.toHexString(masterKey.getFingerprint())
+				.toLowerCase(Locale.ROOT);
+		boolean masterFingerprintMatch = false;
+		boolean userIdMatch = false;
+		List<String> userIds = new ArrayList<>();
+		masterKey.getUserIDs().forEachRemaining(userIds::add);
+		if (fingerprint.endsWith(keyId)) {
+			masterFingerprintMatch = true;
+		} else {
+			// Check the user IDs
+			for (String userId : userIds) {
+				if (containsSigningKey(userId, keySpec)) {
+					userIdMatch = true;
+					break;
+				}
+			}
+		}
+		if (masterFingerprintMatch) {
+			if (isSigningKey(masterKey)) {
+				return new BouncyCastleGpgPublicKey(masterKey, true, userIds);
+			}
+		}
+		// Check subkeys -- they have no user ids, so only check for a
+		// fingerprint match (unless the master key matched).
+		PGPPublicKey candidate = null;
+		while (keys.hasNext()) {
+			PGPPublicKey subKey = keys.next();
+			if (!isSigningKey(subKey)) {
+				continue;
+			}
+			if (masterFingerprintMatch) {
+				candidate = subKey;
+				break;
+			}
+			fingerprint = Hex.toHexString(subKey.getFingerprint())
+					.toLowerCase(Locale.ROOT);
+			if (fingerprint.endsWith(keyId)) {
+				return new BouncyCastleGpgPublicKey(subKey, true, userIds);
+			}
+			if (candidate == null) {
+				candidate = subKey;
+			}
+		}
+		if (candidate != null && (masterFingerprintMatch || userIdMatch)) {
+			return new BouncyCastleGpgPublicKey(candidate, false, userIds);
 		}
 		return null;
 	}
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgPublicKey.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgPublicKey.java
new file mode 100644
index 0000000000..d736536fd7
--- /dev/null
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgPublicKey.java
@@ -0,0 +1,36 @@
+package org.eclipse.jgit.gpg.bc.internal;
+
+import java.util.List;
+
+import org.bouncycastle.openpgp.PGPPublicKey;
+
+/**
+ * Container for GPG public keys.
+ */
+class BouncyCastleGpgPublicKey {
+
+	private final PGPPublicKey publicKey;
+
+	private final boolean exactMatch;
+
+	private final List<String> userIds;
+
+	BouncyCastleGpgPublicKey(PGPPublicKey publicKey, boolean exactMatch,
+			List<String> userIds) {
+		this.publicKey = publicKey;
+		this.exactMatch = exactMatch;
+		this.userIds = userIds;
+	}
+
+	PGPPublicKey getPublicKey() {
+		return publicKey;
+	}
+
+	boolean isExactMatch() {
+		return exactMatch;
+	}
+
+	List<String> getUserIds() {
+		return userIds;
+	}
+}
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgSignatureVerifier.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgSignatureVerifier.java
index f4fed40973..3378bb3969 100644
--- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgSignatureVerifier.java
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgSignatureVerifier.java
@@ -16,7 +16,7 @@ import java.security.Security;
 import java.text.MessageFormat;
 import java.time.Instant;
 import java.util.Date;
-import java.util.Iterator;
+import java.util.List;
 import java.util.Locale;
 
 import org.bouncycastle.bcpg.sig.IssuerFingerprint;
@@ -121,7 +121,7 @@ public class BouncyCastleGpgSignatureVerifier
 			} else {
 				throw new JGitInternalException(BCText.get().nonSignatureError);
 			}
-		} catch (PGPException e) {
+		} catch (NumberFormatException | PGPException e) {
 			throw new JGitInternalException(BCText.get().signatureParseError,
 					e);
 		}
@@ -134,7 +134,7 @@ public class BouncyCastleGpgSignatureVerifier
 		if (fingerprint != null && keyId != null
 				&& !fingerprint.endsWith(keyId)) {
 			return new VerificationResult(signatureCreatedAt, signer, fingerprint,
-					null, false, false, TrustLevel.UNKNOWN,
+					signer, false, false, TrustLevel.UNKNOWN,
 					MessageFormat.format(BCText.get().signatureInconsistent,
 							keyId, fingerprint));
 		}
@@ -144,18 +144,18 @@ public class BouncyCastleGpgSignatureVerifier
 		// Try to find the public key
 		String keySpec = '<' + signer + '>';
 		Object cached = null;
-		PGPPublicKey publicKey = null;
+		BouncyCastleGpgPublicKey publicKey = null;
 		try {
 			cached = byFingerprint.get(fingerprint);
 			if (cached != null) {
-				if (cached instanceof PGPPublicKey) {
-					publicKey = (PGPPublicKey) cached;
+				if (cached instanceof BouncyCastleGpgPublicKey) {
+					publicKey = (BouncyCastleGpgPublicKey) cached;
 				}
 			} else if (!StringUtils.isEmptyOrNull(signer)) {
 				cached = bySigner.get(signer);
 				if (cached != null) {
-					if (cached instanceof PGPPublicKey) {
-						publicKey = (PGPPublicKey) cached;
+					if (cached instanceof BouncyCastleGpgPublicKey) {
+						publicKey = (BouncyCastleGpgPublicKey) cached;
 					}
 				}
 			}
@@ -176,9 +176,17 @@ public class BouncyCastleGpgSignatureVerifier
 				}
 			}
 			return new VerificationResult(signatureCreatedAt, signer,
-					fingerprint, null, false, false, TrustLevel.UNKNOWN,
+					fingerprint, signer, false, false, TrustLevel.UNKNOWN,
 					BCText.get().signatureNoPublicKey);
 		}
+		if (fingerprint != null && !publicKey.isExactMatch()) {
+			// We did find _some_ signing key for the signer, but it doesn't
+			// match the given fingerprint.
+			return new VerificationResult(signatureCreatedAt, signer,
+					fingerprint, signer, false, false, TrustLevel.UNKNOWN,
+					MessageFormat.format(BCText.get().signatureNoSigningKey,
+							fingerprint));
+		}
 		if (cached == null) {
 			byFingerprint.put(fingerprint, publicKey);
 			byFingerprint.put(keyId, publicKey);
@@ -187,27 +195,28 @@ public class BouncyCastleGpgSignatureVerifier
 			}
 		}
 		String user = null;
-		Iterator<String> userIds = publicKey.getUserIDs();
-		if (!StringUtils.isEmptyOrNull(signer)) {
-			while (userIds.hasNext()) {
-				String userId = userIds.next();
-				if (BouncyCastleGpgKeyLocator.containsSigningKey(userId,
-						keySpec)) {
-					user = userId;
-					break;
+		List<String> userIds = publicKey.getUserIds();
+		if (userIds != null && !userIds.isEmpty()) {
+			if (!StringUtils.isEmptyOrNull(signer)) {
+				for (String userId : publicKey.getUserIds()) {
+					if (BouncyCastleGpgKeyLocator.containsSigningKey(userId,
+							keySpec)) {
+						user = userId;
+						break;
+					}
 				}
 			}
-		}
-		if (user == null) {
-			userIds = publicKey.getUserIDs();
-			if (userIds.hasNext()) {
-				user = userIds.next();
+			if (user == null) {
+				user = userIds.get(0);
 			}
+		} else if (signer != null) {
+			user = signer;
 		}
+		PGPPublicKey pubKey = publicKey.getPublicKey();
 		boolean expired = false;
-		long validFor = publicKey.getValidSeconds();
+		long validFor = pubKey.getValidSeconds();
 		if (validFor > 0 && signatureCreatedAt != null) {
-			Instant expiredAt = publicKey.getCreationTime().toInstant()
+			Instant expiredAt = pubKey.getCreationTime().toInstant()
 					.plusSeconds(validFor);
 			expired = expiredAt.isBefore(signatureCreatedAt.toInstant());
 		}
@@ -215,14 +224,14 @@ public class BouncyCastleGpgSignatureVerifier
 		// specific. We don't use the GPG trustdb but simply the trust packet
 		// on the public key, if present. Even if present, it may or may not
 		// be set.
-		byte[] trustData = publicKey.getTrustData();
+		byte[] trustData = pubKey.getTrustData();
 		TrustLevel trust = parseGpgTrustPacket(trustData);
 		boolean verified = false;
 		try {
 			signature.init(
 					new JcaPGPContentVerifierBuilderProvider()
 							.setProvider(BouncyCastleProvider.PROVIDER_NAME),
-					publicKey);
+					pubKey);
 			signature.update(data);
 			verified = signature.verify();
 		} catch (PGPException e) {