Update Handshakers to use NodeKey #666

rain-on · 2020-04-07T06:47:40Z

PR description

Update the Handhshaking classes to use an injected NodeKey object for ECIES encryption.
This has necessitated an update of NodeKey to support the creation of Key and ECIES agreement creation.

Fixed Issue(s)

N/A

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

usmansaleem · 2020-04-07T10:13:19Z

.../java/org/hyperledger/besu/ethereum/p2p/rlpx/connections/netty/HandshakeHandlerOutbound.java

@@ -53,7 +54,8 @@ public HandshakeHandlerOutbound(
        connectionFuture,
        connectionEventDispatcher,
        metricsSystem);
-    handshaker.prepareInitiator(kp, SECP256K1.PublicKey.create(peer.getId()));
+    handshaker.prepareInitiator(


Does it make sense to move the second parameter SECP256K1.PublicKey.create(peer.getId()) as potentially a static method in NodeKey as well?

Thinking not - NodeKey is all about protecting this node's identity/private key - the Peer in this question is someone remote.
I.e. only operations for our node key need to go via the NodeKey interface

usmansaleem · 2020-04-07T10:15:41Z

...2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/ecies/ECIESHandshaker.java

    checkState(
        status.compareAndSet(
            Handshaker.HandshakeStatus.UNINITIALIZED, Handshaker.HandshakeStatus.IN_PROGRESS),
        "handshake was already prepared");

    this.initiator = false;
-    this.identityKeyPair = ourKeypair;
+    this.nodeKey = nodeKey;
    this.ephKeyPair = SECP256K1.KeyPair.generate();


what about ephKeyPair?

This is the ephemeral key used for inter-node communications and is rolling - so no need to protect it alongside our nodekey

usmansaleem · 2020-04-07T10:17:14Z

...p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/ecies/EncryptedMessage.java

   * @return The plaintext.
   * @throws InvalidCipherTextException Thrown if decryption failed.
   */
-  public static Bytes decryptMsgEIP8(final Bytes msgBytes, final SECP256K1.PrivateKey ourKey)
+  public static Bytes decryptMsgEIP8(final Bytes msgBytes, final NodeKey nodeKey)


just wondering, should we name argument as 'ourNodeKey' ?

I'd prefer not to. For 2 reasons:
Thinking "nodeKey" is explicit enough to indicate ownership (on some levels).
Secondly, I'd hate for this class to imply the owner of the key which is doing the decryption - i.e. it simply needs a key to do the decryption - ownership of the key makes no difference as to the operation being conducted.

usmansaleem

Looks good.

jframe · 2020-04-07T23:42:33Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/Handshaker.java

   * @param theirPubKey The public key of the node we're handshaking with.
   * @throws IllegalStateException Indicates that preparation had already occured.
   */
-  void prepareInitiator(SECP256K1.KeyPair ourKeypair, SECP256K1.PublicKey theirPubKey);
+  void prepareInitiator(final NodeKey nodeKey, SECP256K1.PublicKey theirPubKey);


final isn't needed on the interface fields

jframe · 2020-04-07T23:43:27Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/Handshaker.java

   * @throws IllegalStateException Indicates that preparation had already occured.
   */
-  void prepareResponder(SECP256K1.KeyPair ourKeypair);
+  void prepareResponder(final NodeKey nodeKey);


final isn't needed on the interface fields

jframe · 2020-04-07T23:45:02Z

...java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/ecies/InitiatorHandshakeMessageV1.java

@@ -95,11 +95,10 @@ public static InitiatorHandshakeMessageV1 create(
   * Decodes this message.
   *
   * @param bytes The raw bytes.
-   * @param keyPair Our keypair to calculat ECDH key agreements.
+   * @param nodeKey Our keypair to calculat ECDH key agreements.


Can you also update the javadoc since this is no longer a keypair

jframe · 2020-04-07T23:47:42Z

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

 import org.apache.tuweni.bytes.Bytes32;

 public interface NodeKey {

  Signature sign(Bytes32 dataHash);

  PublicKey getPublicKey();
+
+  Bytes32 calculateKeyAgreement(final PublicKey partyKey);


Do we want to be more explicit here and call out that is the ECDH key agreement? I thinking we should.

jframe · 2020-04-07T23:49:51Z

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

 import org.apache.tuweni.bytes.Bytes32;

 public interface NodeKey {

  Signature sign(Bytes32 dataHash);

  PublicKey getPublicKey();
+
+  Bytes32 calculateKeyAgreement(final PublicKey partyKey);


Should this just be publicKey instead of partyKey? I don't know enough ECDH key agreements to say it should be, but elsewhere it seems to be just referred to as publicKey

jframe · 2020-04-07T23:50:52Z

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

+  }
+
+  @Override
+  public Bytes calculateECIESAgreement(final SECP256K1.PublicKey ephPubKey) {


Thinking this should be calculateECIESKeyAgreement

jframe · 2020-04-07T23:53:52Z

.../main/java/org/hyperledger/besu/ethereum/p2p/rlpx/handshake/ecies/ECIESEncryptionEngine.java

-      final CipherParameters privParam,
-      final SECP256K1.PublicKey ephPubKey,
-      final byte[] iv) {
+      final Bytes Z, final SECP256K1.PublicKey ephPubKey, final byte[] iv) {


This can be lower z

changed - but I wonder what it REALLY is .... changed to agreedSecret

jframe · 2020-04-08T00:58:33Z

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

+    final ECDomainParameters dp = SECP256K1.CURVE;
+
+    final CipherParameters pubParam = new ECPublicKeyParameters(ephPubKey.asEcPoint(), dp);
+    final CipherParameters privParam =
+        new ECPrivateKeyParameters(nodeKeys.getPrivateKey().getD(), dp);
+
+    final BasicAgreement agree = new ECDHBasicAgreement();
+    agree.init(privParam);
+    final BigInteger z = agree.calculateAgreement(pubParam);
+    return Bytes.wrap(BigIntegers.asUnsignedByteArray(agree.getFieldSize(), z));


Would be nicer if both of the key agreement functions were in SECP256k1 class

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

Update Handshakers to use NodeKey

16d1e71

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

rain-on requested review from usmansaleem and jframe April 7, 2020 06:47

Merge branch 'master' into handshaker

d3a8736

usmansaleem reviewed Apr 7, 2020

View reviewed changes

usmansaleem approved these changes Apr 7, 2020

View reviewed changes

jframe reviewed Apr 8, 2020

View reviewed changes

Trent Mohay and others added 3 commits April 8, 2020 11:55

Post Review

e6c4dd9

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

Repair test failure

6e44fec

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

Merge remote-tracking branch 'upstream/master' into handshaker

b94d328

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

rain-on force-pushed the handshaker branch from b62646a to b94d328 Compare April 8, 2020 03:50

fixed spotless

620f99f

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

jframe approved these changes Apr 8, 2020

View reviewed changes

rain-on merged commit d757af8 into hyperledger:master Apr 8, 2020

rain-on deleted the handshaker branch April 8, 2020 05:15

Update Handshakers to use NodeKey #666

Update Handshakers to use NodeKey #666

Uh oh!

Conversation

rain-on commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usmansaleem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rain-on commented Apr 7, 2020 •

edited

Loading