Replacing the static references of SECP256K1 by an interface #1933

daniel-iobuilders · 2021-02-21T23:12:21Z

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

PR description

At the moment Besu only supports the Ethereum standard SECP256K1 key algorithm. Due to NIST compliance, we would like to add support for SECP256R1 keys.
In this first PR all the static reference calls to SECP256K1 are replaced by an interface. This is a first step to allow different elliptic curves to be added by future PRs.

Fixed Issue(s)

Changelog

I thought about the changelog and included a changelog update if required.

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

atoulme · 2021-02-22T05:13:47Z

This is an intriguing proposal, do you want to change the key wholesale across the whole system? Would you want to allow some compatibility for example at the network layer?

lucassaldanha · 2021-02-22T20:46:53Z

This is an intriguing proposal, do you want to change the key wholesale across the whole system?

The plan is to support different key algorithms across the board. We aren't aiming for interop between nodes using different key algorithms.

Would you want to allow some compatibility for example at the network layer?

I don't think I follow. Would you mind elaborating on this?

shemnon

Partial review, I focused on the main classes and skimmed through the unavoidably long list of import renames.

...tance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/account/Account.java

shemnon · 2021-02-22T20:25:33Z

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

+import java.util.Optional;
+import java.util.function.UnaryOperator;
+
+public interface EllipticCurveSignature {


I feel we should go a bit further than simply pulling out the interface methods. I think we should reconsider all the APIs being pulled through and look to see if better homes exist (calculateECDHKeyAgreement, getHalfCureOrder, enableNarive, etc).

A higher level goal may be to provide SPI classes to make it work with javax.crypto and friends, rather than a single use interface.

I also think we can do better than the EllipticCurveSignature name. Account sigs may not always be ECDSA (DSA and RSA are also listed in FIPS). So perhaps SignatureAlgorithm? Or make it role based AccountSignatures. Also the noun implies it is the signature itself, not the class that does the signing, verification, and key management.

I feel we should go a bit further than simply pulling out the interface methods. I think we should reconsider all the APIs being pulled through and look to see if better homes exist (calculateECDHKeyAgreement, getHalfCureOrder, enableNarive, etc).

I believe the idea is to make incremental changes. And because this class is used pretty much everywhere, pulling out the interface already introduces a bunch of changes to review. Do you feel like it is better to have all changes done at once?

I also think we can do better than the EllipticCurveSignature name. Account sigs may not always be ECDSA (DSA and RSA are also listed in FIPS)

I think supporting "non-EC" have bigger implications that are out of the scope of this PR. I guess this is a small step towards something like https://github.com/EntEthAlliance/client-spec/issues/110. (a really small step)

shemnon · 2021-02-22T20:27:55Z

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

+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class KeyPair {
+


Should we just use javax.security.KeyPair and move the static method elsewhere?

Looking through the code that would require a lot of casting from javax.security.PublicKey -> SECPPublicKey, because SECPPublicKey.getEncodedBytes is used in a lot of places and not part of the interface. Considering this I would leave the class for right now, as it is.

shemnon · 2021-02-22T20:30:48Z

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

+package org.hyperledger.besu.crypto;
+
+public class EllipticCurveSignatureFactory {
+


I'm presuming this is a placeholder for future work to add secp256r1. How will the choice of curve be determined?

Correct. We are considering this being a config in the genesis file. Mostly because it makes it easier for other people deploying nodes in the network (they don't have to remember the right config, just use the same genesis). Thoughts?

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

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

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

shemnon · 2021-02-22T20:47:39Z

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

+
+public class PublicKey implements java.security.PublicKey {
+
+    public static final int BYTE_LENGTH = 64;


We shouldn't presume a key length. What if secp256r1 is un-recommened and we have to go to secp384r1, or secp521r1? or any of the sect series?

I would suggest to do that in a seperate PR. I've looked a bit into it and there are some calculation, e.g. to recover the public key, which are affected by that too. I think a seperate (and much smaller) PR is much easier to review such a bit more difficult change.
I will do that after this one is merged.

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

shemnon · 2021-02-22T20:51:02Z

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

+
+public class Signature {
+
+    public static final int BYTES_REQUIRED = 65;


Again, we shouldn't lock in the signature length. It is reasonable to prepare for chains that may require stronger EC cures.

I guess the impact of supporting stronger keys might add extra work that won't provide much value atm. One of the assumptions for this is that secp256k1 and secp256r1 use keys of the same size. If we make the size dynamic, we might need to handle this across different layers of the system.

I'm not saying that we shouldn't do it, but maybe not in this first iteration.

We can expose the key length as a signature API and the initial batch of factory methods can presume it. But I'm wary of baking in 65 bytes too deeply into the API.

…. Fixing all tests Signed-off-by: Daniel Lehrner <daniel@io.builders>

Renamed PublicKey -> SECPPublicKey Renamed Signature -> SECPSignature Renamed EllipticCurveSignature -> SignatureAlgorithm Refactored call to get SignatureAlgorithm during class loading, to use com.google.common.base.Suppliers#memoize Signed-off-by: Daniel Lehrner <daniel@io.builders>

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

lucassaldanha

I think we can merge this as a first step. This PR is only getting rid of the static references. However, there were some good suggestions that I'd like to see done as well. Let's make sure we work on those next.

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

…dger#1933) * Replacing the static references of SECP256K1 by an interface Signed-off-by: Daniel Lehrner <daniel@io.builders> Signed-off-by: Richard Hart <richardhart92@gmail.com>

matkt · 2021-04-13T09:39:45Z

I have quite a bit of error message while synchronizing my nodes. I got it on Goerli, Ropsten, Mordor.

The error does not seem to prevent the synchronization from doing well but my logs are full of this messsage.

Looking at the stacktrace this seems related to this modification. Any idea ?

{"timestamp":"2021-04-13T09:35:38,122","level":"ERROR","thread":"vert.x-eventloop-thread-2","class":"VertxPeerDiscoveryAgent","message":"Encountered error while handling packet","throwable":" java.lang.IllegalArgumentException: Could not recover public key\n\tat org.hyperledger.besu.crypto.SECP256K1.recoverFromSignatureNative(SECP256K1.java:495)\n\tat org.hyperledger.besu.crypto.SECP256K1.recoverPublicKeyFromSignature(SECP256K1.java:370)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet.(Packet.java:90)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet.decode(Packet.java:126)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.VertxPeerDiscoveryAgent.lambda$handlePacket$6(VertxPeerDiscoveryAgent.java:221)\n\tat io.vertx.core.impl.ContextImpl.lambda$executeBlocking$2(ContextImpl.java:313)\n\tat io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)\n\tat java.base/java.lang.Thread.run(Thread.java:829)\n"}

daniel-iobuilders · 2021-04-13T10:27:18Z

@matkt Thank you for reporting it. Do you use any non-default parameters for starting Besu? Or do you use the default ones, like for example:

besu --network=ropsten

matkt · 2021-04-13T11:36:41Z

@matkt Thank you for reporting it. Do you use any non-default parameters for starting Besu? Or do you use the default ones, like for example:
besu --network=ropsten

I use the default parameters. When I run Besu the error does not appear directly we have to wait (<5 minutes) for the synchronization to progress and then we have the error messages. If I restart Besu it stop and after some time it reappears

daniel-iobuilders · 2021-04-13T14:34:08Z

@matkt Thank you! I could reproduce the error. I look into it and will let you know when I have found the solution.

3nprob · 2021-04-14T06:40:23Z

@daniel-iobuilders Thanks for looking into it. We're getting the same on fast sync nodes on rinkeby/goerli/mainnet, let me know if there's any particular part of config that's relevant to share

Sample trace from besu:21.1.4-openjdk-latest (appearing ~10-30 times per minute):

2021-04-12 18:34:42.924+00:00 | vert.x-eventloop-thread-1 | ERROR | VertxPeerDiscoveryAgent | Encountered error while handling packet
java.lang.IllegalArgumentException: Could not recover public key
	at org.hyperledger.besu.crypto.SECP256K1.recoverFromSignatureNative(SECP256K1.java:420)
	at org.hyperledger.besu.crypto.SECP256K1$PublicKey.recoverFromSignature(SECP256K1.java:539)
	at org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet.<init>(Packet.java:83)
	at org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet.decode(Packet.java:119)
	at org.hyperledger.besu.ethereum.p2p.discovery.VertxPeerDiscoveryAgent.lambda$handlePacket$6(VertxPeerDiscoveryAgent.java:221)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$2(ContextImpl.java:313)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:832)

daniel-iobuilders · 2021-04-14T08:51:20Z

@matkt @3nprob The problem is caused by apparently invalid signatures of some packets that the discovery agent receives. I captured some of these packets and created a test to reproduce the error and tried it with different releases of Besu. This showed that this error is not related to this PR, but also occurs in Besu versions that were released before it, like 21.1.0 or 20.10.4. This leads to the conclusion that the packets are indeed invalid and they don't affect the correct functionality of Besu.

You can verify this by simply running one of the mentioned releases. After a few minutes you will see the same error messages in the logs.

To summarize: This error message appears when a packet of the discovery protocol has an invalid signature. Besu tries to verify it, which throws the exception in question. This is not really an error, because in an open network you can always receive invalid packets and it does not affect any functionality. So what needs to be changed is, how Besu handles those packets. I would suggest that instead of throwing an error a simply debug message in the logs is sufficient. Like this the logs don't get cluttered by it. I will open an issue, share my findings and propose this solution to avoid those messages in the future.

…dger#1933) * Replacing the static references of SECP256K1 by an interface Signed-off-by: Daniel Lehrner <daniel@io.builders>

WIP: Replacing the static references of SECP256K1 by an interface

6942714

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

shemnon reviewed Feb 22, 2021

View reviewed changes

daniel-iobuilders added 3 commits February 23, 2021 22:46

Finished replacing the static references of SECP256K1 by an interface…

bda1d67

…. Fixing all tests Signed-off-by: Daniel Lehrner <daniel@io.builders>

Fixed integration and reference tests

b9c5ef8

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

daniel-iobuilders marked this pull request as ready for review February 25, 2021 10:01

Merge branch 'master' into configurable_elliptic_curve

41edabe

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

daniel-iobuilders requested a review from lucassaldanha February 25, 2021 12:47

lucassaldanha approved these changes Feb 25, 2021

View reviewed changes

daniel-iobuilders and others added 5 commits February 26, 2021 10:10

Merge branch 'master' into configurable_elliptic_curve

996c0da

Merge branch 'master' into configurable_elliptic_curve

8af5149

Added missing javadoc parameters

e8783ac

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

Spotless fix

75f9e0a

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

Merge branch 'master' into configurable_elliptic_curve

6c1988b

lucassaldanha merged commit 8315ba6 into hyperledger:master Mar 1, 2021

daniel-iobuilders mentioned this pull request Mar 2, 2021

Remove hard coded elliptic curve lengths #1967

Closed

daniel-iobuilders mentioned this pull request Mar 11, 2021

Adding SECP256R1 support to be NIST compliant Consensys/discovery#97

Closed

daniel-iobuilders mentioned this pull request Apr 14, 2021

Invalid packets in discovery agent throw an error, but should only be logged as a debug message #2142

Closed

garyschulte mentioned this pull request Nov 1, 2021

Add Daniel as a maintainer #2985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing the static references of SECP256K1 by an interface #1933

Replacing the static references of SECP256K1 by an interface #1933

daniel-iobuilders commented Feb 21, 2021

atoulme commented Feb 22, 2021

lucassaldanha commented Feb 22, 2021

shemnon left a comment

shemnon Feb 22, 2021

lucassaldanha Feb 22, 2021

shemnon Feb 22, 2021

daniel-iobuilders Feb 25, 2021

shemnon Feb 22, 2021

lucassaldanha Feb 22, 2021

shemnon Feb 22, 2021

daniel-iobuilders Feb 24, 2021

shemnon Feb 22, 2021

lucassaldanha Feb 22, 2021

shemnon Feb 22, 2021

lucassaldanha left a comment

matkt commented Apr 13, 2021

daniel-iobuilders commented Apr 13, 2021 •

edited

Loading

matkt commented Apr 13, 2021 •

edited

Loading

daniel-iobuilders commented Apr 13, 2021

3nprob commented Apr 14, 2021 •

edited

Loading

daniel-iobuilders commented Apr 14, 2021

		import static com.google.common.base.Preconditions.checkNotNull;

		public class KeyPair {

		package org.hyperledger.besu.crypto;

		public class EllipticCurveSignatureFactory {


		public class PublicKey implements java.security.PublicKey {

		public static final int BYTE_LENGTH = 64;


		public class Signature {

		public static final int BYTES_REQUIRED = 65;

Replacing the static references of SECP256K1 by an interface #1933

Replacing the static references of SECP256K1 by an interface #1933

Conversation

daniel-iobuilders commented Feb 21, 2021

PR description

Fixed Issue(s)

Changelog

atoulme commented Feb 22, 2021

lucassaldanha commented Feb 22, 2021

shemnon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha left a comment

Choose a reason for hiding this comment

matkt commented Apr 13, 2021

daniel-iobuilders commented Apr 13, 2021 • edited Loading

matkt commented Apr 13, 2021 • edited Loading

daniel-iobuilders commented Apr 13, 2021

3nprob commented Apr 14, 2021 • edited Loading

daniel-iobuilders commented Apr 14, 2021

daniel-iobuilders commented Apr 13, 2021 •

edited

Loading

matkt commented Apr 13, 2021 •

edited

Loading

3nprob commented Apr 14, 2021 •

edited

Loading