From d24eee7d5c502dc63d13e5584b8df8a49c9251e1 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 10 Mar 2024 00:04:41 +0100 Subject: [PATCH] [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 --- .../jgit/gpg/bc/internal/BCText.properties | 1 + .../eclipse/jgit/gpg/bc/internal/BCText.java | 1 + .../internal/BouncyCastleGpgKeyLocator.java | 130 ++++++++++++++---- .../bc/internal/BouncyCastleGpgPublicKey.java | 36 +++++ .../BouncyCastleGpgSignatureVerifier.java | 61 ++++---- 5 files changed, 173 insertions(+), 56 deletions(-) create mode 100644 org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/BouncyCastleGpgPublicKey.java 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 toStrings(List 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 keyrings = pgpPub.getKeyRings(); + BouncyCastleGpgPublicKey candidate = null; while (keyrings.hasNext()) { PGPPublicKeyRing keyRing = keyrings.next(); - Iterator 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 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 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 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 userIds; + + BouncyCastleGpgPublicKey(PGPPublicKey publicKey, boolean exactMatch, + List userIds) { + this.publicKey = publicKey; + this.exactMatch = exactMatch; + this.userIds = userIds; + } + + PGPPublicKey getPublicKey() { + return publicKey; + } + + boolean isExactMatch() { + return exactMatch; + } + + List 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 userIds = publicKey.getUserIDs(); - if (!StringUtils.isEmptyOrNull(signer)) { - while (userIds.hasNext()) { - String userId = userIds.next(); - if (BouncyCastleGpgKeyLocator.containsSigningKey(userId, - keySpec)) { - user = userId; - break; + List 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) { -- 2.39.5