Removed hardcoded key lengths and added test with different key lengt…

…hs and elliptic curves.

Signed-off-by: Daniel Lehrner <daniel@io.builders>

consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueBlockHashing.java

@@ -14,7 +14,7 @@
  */
 package org.hyperledger.besu.consensus.clique;
 
-import org.hyperledger.besu.crypto.SECPSignature;
+import org.hyperledger.besu.crypto.SignatureAlgorithmFactory;
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.Hash;
@@ -69,7 +69,9 @@ private static Bytes serializeHeaderWithoutProposerSeal(
   private static Bytes encodeExtraDataWithoutProposerSeal(final CliqueExtraData cliqueExtraData) {
     final Bytes extraDataBytes = cliqueExtraData.encode();
     // Always trim off final 65 bytes (which maybe zeros)
-    return extraDataBytes.slice(0, extraDataBytes.size() - SECPSignature.BYTES_REQUIRED);
+    return extraDataBytes.slice(
+        0,
+        extraDataBytes.size() - SignatureAlgorithmFactory.getInstance().getSignatureByteLength());
   }
 
   private static Bytes serializeHeader(

consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueExtraData.java

@@ -18,6 +18,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import org.hyperledger.besu.crypto.SECPSignature;
+import org.hyperledger.besu.crypto.SignatureAlgorithm;
 import org.hyperledger.besu.crypto.SignatureAlgorithmFactory;
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
@@ -39,6 +40,8 @@
  */
 public class CliqueExtraData implements ParsedExtraData {
   private static final Logger LOG = LogManager.getLogger();
+  private static final Supplier<SignatureAlgorithm> SIGNATURE_ALGORITHM =
+      Suppliers.memoize(SignatureAlgorithmFactory::getInstance);
   public static final int EXTRA_VANITY_LENGTH = 32;
 
   private final Bytes vanityData;
@@ -82,13 +85,14 @@ public static CliqueExtraData decode(final BlockHeader header) {
 
   static CliqueExtraData decodeRaw(final BlockHeader header) {
     final Bytes input = header.getExtraData();
-    if (input.size() < EXTRA_VANITY_LENGTH + SECPSignature.BYTES_REQUIRED) {
+    final int signatureByteLength = SIGNATURE_ALGORITHM.get().getSignatureByteLength();
+
+    if (input.size() < EXTRA_VANITY_LENGTH + signatureByteLength) {
       throw new IllegalArgumentException(
           "Invalid Bytes supplied - too short to produce a valid Clique Extra Data object.");
     }
 
-    final int validatorByteCount =
-        input.size() - EXTRA_VANITY_LENGTH - SECPSignature.BYTES_REQUIRED;
+    final int validatorByteCount = input.size() - EXTRA_VANITY_LENGTH - signatureByteLength;
     if ((validatorByteCount % Address.SIZE) != 0) {
       throw new IllegalArgumentException("Bytes is of invalid size - i.e. contains unused bytes.");
     }
@@ -97,7 +101,7 @@ static CliqueExtraData decodeRaw(final BlockHeader header) {
     final List<Address> validators =
         extractValidators(input.slice(EXTRA_VANITY_LENGTH, validatorByteCount));
 
-    final int proposerSealStartIndex = input.size() - SECPSignature.BYTES_REQUIRED;
+    final int proposerSealStartIndex = input.size() - signatureByteLength;
     final SECPSignature proposerSeal = parseProposerSeal(input.slice(proposerSealStartIndex));
 
     return new CliqueExtraData(vanityData, proposerSeal, validators, header);
@@ -141,7 +145,7 @@ private static Bytes encode(
         validatorData,
         proposerSeal
             .map(SECPSignature::encodedBytes)
-            .orElse(Bytes.wrap(new byte[SECPSignature.BYTES_REQUIRED])));
+            .orElse(Bytes.wrap(new byte[SIGNATURE_ALGORITHM.get().getSignatureByteLength()])));
   }
 
   public Bytes getVanityData() {

consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/CliqueExtraDataTest.java

@@ -91,7 +91,10 @@ public void parseRinkebyGenesisBlockExtraData() {
   public void insufficientDataResultsInAnIllegalArgumentException() {
     final Bytes illegalData =
         Bytes.wrap(
-            new byte[SECPSignature.BYTES_REQUIRED + CliqueExtraData.EXTRA_VANITY_LENGTH - 1]);
+            new byte
+                [SIGNATURE_ALGORITHM.get().getSignatureByteLength()
+                    + CliqueExtraData.EXTRA_VANITY_LENGTH
+                    - 1]);
 
     assertThatThrownBy(() -> CliqueExtraData.decodeRaw(createHeaderWithExtraData(illegalData)))
         .isInstanceOf(IllegalArgumentException.class)
@@ -104,7 +107,7 @@ public void sufficientlyLargeButIllegallySizedInputThrowsException() {
     final Bytes illegalData =
         Bytes.wrap(
             new byte
-                [SECPSignature.BYTES_REQUIRED
+                [SIGNATURE_ALGORITHM.get().getSignatureByteLength()
                     + CliqueExtraData.EXTRA_VANITY_LENGTH
                     + Address.SIZE
                     - 1]);

crypto/src/main/java/org/hyperledger/besu/crypto/KeyPair.java

@@ -16,14 +16,9 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import java.math.BigInteger;
-import java.security.KeyPairGenerator;
-import java.util.Arrays;
 import java.util.Objects;
 
 import org.bouncycastle.crypto.params.ECDomainParameters;
-import org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPrivateKey;
-import org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPublicKey;
 
 public class KeyPair {
 
@@ -42,25 +37,6 @@ public static KeyPair create(
     return new KeyPair(privateKey, SECPPublicKey.create(privateKey, curve, algorithm));
   }
 
-  public static KeyPair generate(final KeyPairGenerator keyPairGenerator, final String algorithm) {
-    final java.security.KeyPair rawKeyPair = keyPairGenerator.generateKeyPair();
-    final BCECPrivateKey privateKey = (BCECPrivateKey) rawKeyPair.getPrivate();
-    final BCECPublicKey publicKey = (BCECPublicKey) rawKeyPair.getPublic();
-
-    final BigInteger privateKeyValue = privateKey.getD();
-
-    // Ethereum does not use encoded public keys like bitcoin - see
-    // https://en.bitcoin.it/wiki/Elliptic_Curve_Digital_Signature_Algorithm for details
-    // Additionally, as the first bit is a constant prefix (0x04) we ignore this value
-    final byte[] publicKeyBytes = publicKey.getQ().getEncoded(false);
-    final BigInteger publicKeyValue =
-        new BigInteger(1, Arrays.copyOfRange(publicKeyBytes, 1, publicKeyBytes.length));
-
-    return new KeyPair(
-        SECPPrivateKey.create(privateKeyValue, algorithm),
-        SECPPublicKey.create(publicKeyValue, algorithm));
-  }
-
   @Override
   public int hashCode() {
     return Objects.hash(privateKey, publicKey);

crypto/src/main/java/org/hyperledger/besu/crypto/KeyPairUtil.java

@@ -19,18 +19,23 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.math.BigInteger;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.security.KeyPairGenerator;
+import java.util.Arrays;
 import java.util.List;
 
 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
 import com.google.common.io.Resources;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.apache.tuweni.bytes.Bytes32;
+import org.apache.tuweni.bytes.Bytes;
+import org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPrivateKey;
+import org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPublicKey;
 
 public class KeyPairUtil {
   private static final Logger LOG = LogManager.getLogger();
@@ -53,7 +58,7 @@ public static KeyPair loadKeyPairFromResource(final String resourcePath) {
       throw new IllegalArgumentException("Unable to load resource: " + resourcePath);
     }
     SECPPrivateKey privateKey =
-        SIGNATURE_ALGORITHM.get().createPrivateKey(Bytes32.fromHexString((keyData)));
+        SIGNATURE_ALGORITHM.get().createPrivateKey(Bytes.fromHexString((keyData)));
     keyPair = SIGNATURE_ALGORITHM.get().createKeyPair(privateKey);
 
     LOG.info("Loaded keyPair {} from {}", keyPair.getPublicKey().toString(), resourcePath);
@@ -101,7 +106,7 @@ static SECPPrivateKey loadPrivateKey(final File file) {
       if (info.size() != 1) {
         throw new IllegalArgumentException("Supplied file does not contain valid keyPair pair.");
       }
-      return SIGNATURE_ALGORITHM.get().createPrivateKey(Bytes32.fromHexString((info.get(0))));
+      return SIGNATURE_ALGORITHM.get().createPrivateKey(Bytes.fromHexString((info.get(0))));
     } catch (IOException ex) {
       throw new IllegalArgumentException("Supplied file does not contain valid keyPair pair.");
     }
@@ -116,4 +121,25 @@ static void storeKeyPair(final KeyPair keyKair, final File file) throws IOExcept
         keyKair.getPrivateKey().getEncodedBytes().toString().getBytes(StandardCharsets.UTF_8));
     Files.move(tempPath, file.toPath(), REPLACE_EXISTING, ATOMIC_MOVE);
   }
+
+  public static KeyPair generate(final KeyPairGenerator keyPairGenerator, final String algorithm) {
+    final java.security.KeyPair rawKeyPair = keyPairGenerator.generateKeyPair();
+    final BCECPrivateKey privateKey = (BCECPrivateKey) rawKeyPair.getPrivate();
+    final BCECPublicKey publicKey = (BCECPublicKey) rawKeyPair.getPublic();
+
+    final BigInteger privateKeyValue = privateKey.getD();
+
+    // Ethereum does not use encoded public keys like bitcoin - see
+    // https://en.bitcoin.it/wiki/Elliptic_Curve_Digital_Signature_Algorithm for details
+    // Additionally, as the first bit is a constant prefix (0x04) we ignore this value
+    final byte[] publicKeyBytes = publicKey.getQ().getEncoded(false);
+    final BigInteger publicKeyValue =
+        new BigInteger(1, Arrays.copyOfRange(publicKeyBytes, 1, publicKeyBytes.length));
+
+    return new KeyPair(
+        SECPPrivateKey.create(
+            privateKeyValue, algorithm, SIGNATURE_ALGORITHM.get().getPrivateKeyByteLength()),
+        SECPPublicKey.create(
+            publicKeyValue, algorithm, SIGNATURE_ALGORITHM.get().getPublicKeyByteLength()));
+  }
 }

crypto/src/main/java/org/hyperledger/besu/crypto/SECP256K1.java

@@ -65,6 +65,9 @@
 public class SECP256K1 implements SignatureAlgorithm {
 
   private static final Logger LOG = LogManager.getLogger();
+  private static final int PRIVATE_KEY_BYTE_LENGTH = 32;
+  private static final int PUBLIC_KEY_BYTE_LENGTH = 64;
+  private static final int SIGNATURE_BYTE_LENGTH = 65;
 
   private boolean useNative = true;
 
@@ -273,7 +276,7 @@ public SECPSignature normaliseSignature(
           "Could not construct a recoverable key. This should never happen.");
     }
 
-    return new SECPSignature(nativeR, s, (byte) recId);
+    return new SECPSignature(nativeR, s, (byte) recId, PRIVATE_KEY_BYTE_LENGTH);
   }
 
   private boolean verifyDefault(
@@ -340,11 +343,11 @@ public Bytes32 calculateECDHKeyAgreement(
 
   @Override
   public SECPPrivateKey createPrivateKey(final BigInteger key) {
-    return SECPPrivateKey.create(key, ALGORITHM);
+    return SECPPrivateKey.create(key, ALGORITHM, PRIVATE_KEY_BYTE_LENGTH);
   }
 
   @Override
-  public SECPPrivateKey createPrivateKey(final Bytes32 key) {
+  public SECPPrivateKey createPrivateKey(final Bytes key) {
     return SECPPrivateKey.create(key, ALGORITHM);
   }
 
@@ -355,7 +358,7 @@ public SECPPublicKey createPublicKey(final SECPPrivateKey privateKey) {
 
   @Override
   public SECPPublicKey createPublicKey(final BigInteger key) {
-    return SECPPublicKey.create(key, ALGORITHM);
+    return SECPPublicKey.create(key, ALGORITHM, PUBLIC_KEY_BYTE_LENGTH);
   }
 
   @Override
@@ -371,7 +374,7 @@ public Optional<SECPPublicKey> recoverPublicKeyFromSignature(
     } else {
       final BigInteger publicKeyBI =
           recoverFromSignature(signature.getRecId(), signature.getR(), signature.getS(), dataHash);
-      return Optional.of(SECPPublicKey.create(publicKeyBI, ALGORITHM));
+      return Optional.of(SECPPublicKey.create(publicKeyBI, ALGORITHM, PUBLIC_KEY_BYTE_LENGTH));
     }
   }
 
@@ -387,17 +390,17 @@ public KeyPair createKeyPair(final SECPPrivateKey privateKey) {
 
   @Override
   public KeyPair generateKeyPair() {
-    return KeyPair.generate(keyPairGenerator, ALGORITHM);
+    return KeyPairUtil.generate(keyPairGenerator, ALGORITHM);
   }
 
   @Override
   public SECPSignature createSignature(final BigInteger r, final BigInteger s, final byte recId) {
-    return SECPSignature.create(r, s, recId, curveOrder);
+    return SECPSignature.create(r, s, recId, curveOrder, PRIVATE_KEY_BYTE_LENGTH);
   }
 
   @Override
   public SECPSignature decodeSignature(final Bytes bytes) {
-    return SECPSignature.decode(bytes, curveOrder);
+    return SECPSignature.decode(bytes, curveOrder, PRIVATE_KEY_BYTE_LENGTH);
   }
 
   @Override
@@ -415,6 +418,21 @@ public String getCurveName() {
     return CURVE_NAME;
   }
 
+  @Override
+  public int getPrivateKeyByteLength() {
+    return PRIVATE_KEY_BYTE_LENGTH;
+  }
+
+  @Override
+  public int getPublicKeyByteLength() {
+    return PUBLIC_KEY_BYTE_LENGTH;
+  }
+
+  @Override
+  public int getSignatureByteLength() {
+    return SIGNATURE_BYTE_LENGTH;
+  }
+
   private SECPSignature signNative(final Bytes32 dataHash, final KeyPair keyPair) {
     final LibSecp256k1.secp256k1_ecdsa_recoverable_signature signature =
         new secp256k1_ecdsa_recoverable_signature();
@@ -443,7 +461,11 @@ private SECPSignature signNative(final Bytes32 dataHash, final KeyPair keyPair)
     final Bytes32 r = Bytes32.wrap(sig, 0);
     final Bytes32 s = Bytes32.wrap(sig, 32);
     return SECPSignature.create(
-        r.toUnsignedBigInteger(), s.toUnsignedBigInteger(), (byte) recId.getValue(), curveOrder);
+        r.toUnsignedBigInteger(),
+        s.toUnsignedBigInteger(),
+        (byte) recId.getValue(),
+        curveOrder,
+        PRIVATE_KEY_BYTE_LENGTH);
   }
 
   private boolean verifyNative(

crypto/src/main/java/org/hyperledger/besu/crypto/SECPKeyUtil.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.crypto;
+
+import java.math.BigInteger;
+import java.util.Arrays;
+
+import org.apache.tuweni.bytes.Bytes;
+import org.apache.tuweni.bytes.MutableBytes;
+import org.bouncycastle.crypto.params.ECDomainParameters;
+import org.bouncycastle.math.ec.ECPoint;
+import org.bouncycastle.math.ec.FixedPointCombMultiplier;
+
+public class SECPKeyUtil {
+
+  public static Bytes toBytes(final BigInteger key, final int byteLength) {
+    final byte[] backing = key.toByteArray();
+
+    if (backing.length == byteLength) {
+      return Bytes.wrap(backing);
+    }
+
+    // if the input is too long it is shifted to the left
+    if (backing.length > byteLength) {
+      return Bytes.wrap(backing, backing.length - byteLength, byteLength);
+    }
+
+    // if the input is too short it is shifted to the right
+    final MutableBytes res = MutableBytes.create(byteLength);
+    Bytes.wrap(backing).copyTo(res, byteLength - backing.length);
+    return res;
+  }
+
+  public static Bytes toPublicKey(final SECPPrivateKey privateKey, final ECDomainParameters curve) {
+    BigInteger privKey = privateKey.getEncodedBytes().toUnsignedBigInteger();
+
+    // ECDSA public keys are twice as long as the private keys
+    final int publicKeyLength = privateKey.getEncodedBytes().size() * 2;
+
+    /*
+     * TODO: FixedPointCombMultiplier currently doesn't support scalars longer than the group
+     * order, but that could change in future versions.
+     */
+    if (privKey.bitLength() > curve.getN().bitLength()) {
+      privKey = privKey.mod(curve.getN());
+    }
+
+    final ECPoint point = new FixedPointCombMultiplier().multiply(curve.getG(), privKey);
+
+    return Bytes.wrap(
+        // point.getEncoded returns as its first byte a constant prefix and the rest of the
+        // bytes are the public key.
+        // The to parameter of Arrays.copyOfRange must the publicKeyLength + 1, because it is
+        // exclusive
+        Arrays.copyOfRange(point.getEncoded(false), 1, publicKeyLength + 1));
+  }
+}