[gpg] Fix reading ed25519 GPG keys

The S-expression parser from Bouncy Castle parsed such keys wrongly;
there is a "flags" sub-list before the "q" value. Additionally, the
parser validates the key read against the given public key, this failed
because Bouncy Castle does not know the OID of curve name "Ed25519".

Fix this and add a test for reading an ed25519 GPG key.

Bug: jgit-27
Change-Id: Ia50445b88759927d2e80b9871d498fbe5ad201bc
Signed-off-by: Thomas Wolf <twolf@apache.org>

3 files changed, 149 insertions, 25 deletions

diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java
index 3eee18aef5..9b5d592fa8 100644
--- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java
@@ -43,17 +43,6 @@ import org.eclipse.jgit.util.sha1.SHA1;
  */
 public final class KeyGrip {
 
-	// Some OIDs apparently unknown to BouncyCastle.
-
-	private static String OID_OPENPGP_ED25519 = "1.3.6.1.4.1.11591.15.1"; //$NON-NLS-1$
-
-	private static String OID_RFC8410_CURVE25519 = "1.3.101.110"; //$NON-NLS-1$
-
-	private static String OID_RFC8410_ED25519 = "1.3.101.112"; //$NON-NLS-1$
-
-	private static ASN1ObjectIdentifier CURVE25519 = ECNamedCurveTable
-			.getOID("curve25519"); //$NON-NLS-1$
-
 	private KeyGrip() {
 		// No instantiation
 	}
@@ -99,20 +88,15 @@ public final class KeyGrip {
 			break;
 		case PublicKeyAlgorithmTags.ECDH:
 		case PublicKeyAlgorithmTags.ECDSA:
-		case PublicKeyAlgorithmTags.EDDSA:
+		case PublicKeyAlgorithmTags.EDDSA_LEGACY:
+		case PublicKeyAlgorithmTags.Ed25519:
 			ECPublicBCPGKey ec = (ECPublicBCPGKey) publicKey
 					.getPublicKeyPacket().getKey();
 			ASN1ObjectIdentifier curveOID = ec.getCurveOID();
 			// BC doesn't know these OIDs.
-			if (OID_OPENPGP_ED25519.equals(curveOID.getId())
-					|| OID_RFC8410_ED25519.equals(curveOID.getId())) {
+			if (ObjectIds.isEd25519(curveOID)) {
 				return hashEd25519(grip, ec.getEncodedPoint());
-			} else if (CURVE25519.equals(curveOID)
-					|| OID_RFC8410_CURVE25519.equals(curveOID.getId())) {
-				// curvey25519 actually is the OpenPGP OID for Curve25519 and is
-				// known to BC, but the parameters are for the short Weierstrass
-				// form. See https://github.com/bcgit/bc-java/issues/399 .
-				// libgcrypt uses Montgomery form.
+			} else if (ObjectIds.isCurve25519(curveOID)) {
 				return hashCurve25519(grip, ec.getEncodedPoint());
 			}
 			X9ECParameters params = getX9Parameters(curveOID);
@@ -141,7 +125,9 @@ public final class KeyGrip {
 			hash(grip, b.toByteArray(), 'b', false);
 			hash(grip, g, 'g', false);
 			hash(grip, n.toByteArray(), 'n', false);
-			if (publicKey.getAlgorithm() == PublicKeyAlgorithmTags.EDDSA) {
+			int algorithm = publicKey.getAlgorithm();
+			if (algorithm == PublicKeyAlgorithmTags.EDDSA_LEGACY
+					|| algorithm == PublicKeyAlgorithmTags.Ed25519) {
 				hashQ25519(grip, q);
 			} else {
 				hash(grip, q.toByteArray(), 'q', false);
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java
new file mode 100644
index 0000000000..3d3098158a
--- /dev/null
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2024, Thomas Wolf <twolf@apache.org> and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+package org.eclipse.jgit.gpg.bc.internal.keys;
+
+import org.bouncycastle.asn1.ASN1ObjectIdentifier;
+import org.bouncycastle.asn1.x9.ECNamedCurveTable;
+
+/**
+ * Some OIDs apparently unknown to Bouncy Castle.
+ */
+public final class ObjectIds {
+
+	/**
+	 * Legacy OID for ed25519 used in OpenPGP.
+	 */
+	public static final ASN1ObjectIdentifier OID_OPENPGP_ED25519 =
+			new ASN1ObjectIdentifier("1.3.6.1.4.1.11591.15.1"); //$NON-NLS-1$
+
+	/**
+	 * OID for ed25519 according to RFC 8410.
+	 */
+	public static final ASN1ObjectIdentifier OID_RFC8410_ED25519 =
+			new ASN1ObjectIdentifier("1.3.101.112"); //$NON-NLS-1$
+
+	/**
+	 * Legacy OID for curve25519 used in OpenPGP.
+	 */
+	public static final ASN1ObjectIdentifier OID_OPENPGP_CURVE25519 =
+			ECNamedCurveTable.getOID("curve25519"); //$NON-NLS-1$
+	// This is 1.3.6.1.4.1.3029.1.5.1
+
+	/**
+	 * OID for curve25519 according to RFC 8410.
+	 */
+	public static final ASN1ObjectIdentifier OID_RFC8410_CURVE25519 = new ASN1ObjectIdentifier(
+			"1.3.101.110"); //$NON-NLS-1$
+
+	/**
+	 * Determines whether the given {@code oid} denoted ed25519.
+	 *
+	 * @param oid
+	 *            to test
+	 * @return {@code true} if it is ed25519, {@code false} otherwise
+	 */
+	public static boolean isEd25519(ASN1ObjectIdentifier oid) {
+		return OID_OPENPGP_ED25519.equals(oid)
+				|| OID_RFC8410_ED25519.equals(oid);
+	}
+
+	/**
+	 * Determines whether the given {@code oid} denoted curve25519.
+	 *
+	 * @param oid
+	 *            to test
+	 * @return {@code true} if it is curve25519, {@code false} otherwise
+	 */
+	public static boolean isCurve25519(ASN1ObjectIdentifier oid) {
+		return OID_RFC8410_CURVE25519.equals(oid)
+				|| OID_OPENPGP_CURVE25519.equals(oid);
+	}
+
+	/**
+	 * Retrieves an OID by name.
+	 *
+	 * @param name
+	 *            to get the OID for
+	 * @return the OID, or {@code null} if unknown.
+	 */
+	public static ASN1ObjectIdentifier getByName(String name) {
+		if (name != null) {
+			switch (name) {
+			case "Ed25519": //$NON-NLS-1$
+				return OID_OPENPGP_ED25519;
+			case "Curve25519": //$NON-NLS-1$
+				return OID_OPENPGP_CURVE25519;
+			default:
+				break;
+			}
+		}
+		return null;
+	}
+
+	/**
+	 * Checks whether two OIDs denote the same algorithm.
+	 *
+	 * @param oid1
+	 *            First OID to test
+	 * @param oid2
+	 *            Second OID to test
+	 * @return {@code true} if the two OIDs match, {@code false} otherwise
+	 */
+	public static boolean match(ASN1ObjectIdentifier oid1,
+			ASN1ObjectIdentifier oid2) {
+		if (isEd25519(oid1)) {
+			return isEd25519(oid2);
+		}
+		if (isCurve25519(oid1)) {
+			return isCurve25519(oid2);
+		}
+		return oid1 != null && oid1.equals(oid2);
+	}
+}
diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java
index c93c2164c9..fd030ee032 100644
--- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java
+++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java
@@ -30,6 +30,7 @@ import java.io.OutputStream;
 import java.math.BigInteger;
 import java.util.Date;
 
+import org.bouncycastle.asn1.ASN1ObjectIdentifier;
 import org.bouncycastle.asn1.x9.ECNamedCurveTable;
 import org.bouncycastle.bcpg.DSAPublicBCPGKey;
 import org.bouncycastle.bcpg.DSASecretBCPGKey;
@@ -62,9 +63,12 @@ import org.bouncycastle.util.Strings;
  * modified by the JGit team to:
  * <ul>
  * <li>handle unencrypted DSA, EC, and ElGamal keys (upstream only handles
- * unencrypted RSA), and</li>
+ * unencrypted RSA)</li>
  * <li>handle secret keys using AES/OCB as encryption (those don't have a
- * hash).</li>
+ * hash)</li>
+ * <li>fix EC parsing to account for "flags" sub-list present for ed25519 and
+ * curve25519</li>
+ * <li>add support for ed25519 OIDs unknown to BouncyCastle</li>
  * </ul>
  */
 @SuppressWarnings("nls")
@@ -128,6 +132,15 @@ public class SExprParser {
 				SXprUtils.skipOpenParenthesis(inputStream);
 
 				type = SXprUtils.readString(inputStream, inputStream.read());
+				// JGit: c.f. https://github.com/bcgit/bc-java/issues/1590.
+				// There may be a flags sub-list here for ed25519 or curve25519.
+				if (type.equals("flags")) {
+					SXprUtils.readString(inputStream, inputStream.read());
+					SXprUtils.skipCloseParenthesis(inputStream);
+					SXprUtils.skipOpenParenthesis(inputStream);
+					type = SXprUtils.readString(inputStream,
+							inputStream.read());
+				}
 				if (type.equals("q")) {
 					qVal = SXprUtils.readBytes(inputStream, inputStream.read());
 				} else {
@@ -143,12 +156,19 @@ public class SExprParser {
 					curveName = curveName.substring("NIST ".length());
 				}
 
+				// JGit: BC doesn't know Ed25519 curve name.
+				ASN1ObjectIdentifier curveOid = ECNamedCurveTable
+						.getOID(curveName);
+				if (curveOid == null) {
+					curveOid = ObjectIds.getByName(curveName);
+				}
 				ECPublicBCPGKey basePubKey = new ECDSAPublicBCPGKey(
-						ECNamedCurveTable.getOID(curveName),
+						curveOid,
 						new BigInteger(1, qVal));
 				ECPublicBCPGKey assocPubKey = (ECPublicBCPGKey) pubKey
 						.getPublicKeyPacket().getKey();
-				if (!basePubKey.getCurveOID().equals(assocPubKey.getCurveOID())
+				if (!ObjectIds.match(basePubKey.getCurveOID(),
+						assocPubKey.getCurveOID())
 						|| !basePubKey.getEncodedPoint()
 								.equals(assocPubKey.getEncodedPoint())) {
 					throw new PGPException(
@@ -292,6 +312,15 @@ public class SExprParser {
 				SXprUtils.skipOpenParenthesis(inputStream);
 
 				type = SXprUtils.readString(inputStream, inputStream.read());
+				// JGit: c.f. https://github.com/bcgit/bc-java/issues/1590.
+				// There may be a flags sub-list here for ed25519 or curve25519.
+				if (type.equals("flags")) {
+					SXprUtils.readString(inputStream, inputStream.read());
+					SXprUtils.skipCloseParenthesis(inputStream);
+					SXprUtils.skipOpenParenthesis(inputStream);
+					type = SXprUtils.readString(inputStream,
+							inputStream.read());
+				}
 				if (type.equals("q")) {
 					qVal = SXprUtils.readBytes(inputStream, inputStream.read());
 				} else {