Implement P2P auth handshake #96

gsalgado · 2017-09-22T10:09:19Z

I tried to use coincurve for the ecdh exchange, but apparently it is not compatible with geth's implementation, so had to resort to pyca/cryptography.

pipermerriam

My brain hurts a little trying to ingest all of this stuff.

pipermerriam · 2017-09-22T17:20:51Z

evm/p2p/auth.py

+    initiator = HandshakeInitiator(remote, privkey)
+    reader, writer = yield from initiator.connect()
+
+    initiator_nonce = keccak(int_to_big_endian(random.randint(0, 2 ** 256 - 1)))


Is it safe for us to be using random.random? My understanding has always been no.

With pycryptodome we can use Crypto.Random.get_random_bytes(n) which I understand to be appropriate for key generation. thoughts?

pipermerriam · 2017-09-22T17:22:27Z

evm/p2p/auth.py

+
+    ephemeral_pubkey, responder_nonce, auth_ack = initiator.decode_auth_ack_message(ack_data)
+    aes_secret, mac_secret, egress_mac, ingress_mac = initiator.derive_secrets(
+        initiator_nonce, responder_nonce, ephemeral_pubkey, auth_init, auth_ack)


formatting nitpick

I find this style hard to read. Can you drop these all onto their own lines and maybe even call it with keyword arguments to ensure that argument ordering doesn't ever get subtly borked.

Sure, but as I said below I'm moving this to another branch

pipermerriam · 2017-09-22T17:23:43Z

evm/p2p/auth.py

+    aes_secret, mac_secret, egress_mac, ingress_mac = initiator.derive_secrets(
+        initiator_nonce, responder_nonce, ephemeral_pubkey, auth_init, auth_ack)
+
+    peer = Peer(remote, privkey, reader, writer, aes_secret,


I've always been a bit frightened of function calls and object instantaition that uses more than 3-4 positional arguments given the ease of transposing them or just plain getting the order wrong. Thoughts on converting this to use keyword arguments?

Yeah, I'm also a bit weary of them so I'm happy to do that. But I'll move this whole function and the creation of the Peer class to another branch I have, as this really belongs there and will mean less conflicts when I rebase the other branch after we're done with the code review

pipermerriam · 2017-09-22T17:32:37Z

evm/p2p/auth.py

+        except AuthenticationError:
+            eph_pubkey, nonce, version = decode_ack_eip8(ciphertext, self.privkey)
+            self.got_eip8_auth = True
+        auth_ack = ciphertext


this assignment struck me as strange.

Good catch; it makes no sense for this method to do this and then return it afterwards, as this is the very argument passed into the method. Fixed so it no longer returns auth_ack and dropped the assignment

pipermerriam · 2017-09-22T17:33:22Z

evm/p2p/auth.py

+        # S || H(ephemeral-pubk) || pubk || nonce || 0x0
+        flag = 0x0
+        auth_message = S + keccak(self.ephemeral_pubkey) + self.pubkey + \
+            nonce + ascii_chr(flag)


The ascii_chr(flag seems like strange indirection for the literal b'\x00'.

Right, fixed

pipermerriam · 2017-09-22T17:35:33Z

evm/p2p/auth.py

+        if self.got_eip8_auth:
+            # The EIP-8 version has an authenticated length prefix.
+            prefix = struct.pack('>H', len(ack_message) + ecies.encrypt_overhead_length)
+            auth_ack = ecies.encrypt(ack_message, self.remote.pubkey, shared_mac_data=prefix)


Maybe rename to suffix to avoid the overwriting of this value on the next line.

pipermerriam · 2017-09-22T17:37:03Z

evm/p2p/ecies.py

+mode = modes.CTR
+curve = ec.SECP256K1()
+# ECIES using AES256 and HMAC-SHA-256-32
+key_len = 32


Thoughts on renaming this and the others above it to KEY_LEN?

I find that casing helps distinguish top level module variables from vars in the local context.

pipermerriam · 2017-09-22T17:38:27Z

evm/p2p/constants.py

+encrypted_auth_msg_len = auth_msg_len + encrypt_overhead_length
+
+# Length of encrypted pre-EIP-8 handshake reply
+encrypted_auth_ack_len = auth_ack_len + encrypt_overhead_length


Can all of these be changed to CAP_CASE so that it's a bit easier to tell they are constants when they are used in other modules.

pipermerriam · 2017-09-22T17:40:59Z

evm/p2p/kademlia.py

@@ -81,7 +81,7 @@ def __init__(self, pubkey, address):
    @classmethod
    def from_uri(cls, uri):
        ip, port, pubkey = host_port_pubkey_from_uri(uri)
-        return cls(pubkey, Address(ip.decode(), int(port)))
+        return cls(pubkey, Address(ip.decode(), int(port), int(port)))


Just one line but this seems like it could get extracted and merged separately.

This was left by accident, I'll remove it

pipermerriam · 2017-09-22T17:49:42Z

evm/p2p/utils.py

+def sxor(s1, s2):
+    if len(s1) != len(s2):
+        raise ValueError("Cannot sxor strings of different length")
+    return b''.join(ascii_chr(safe_ord(a) ^ safe_ord(b)) for a, b in zip(s1, s2))


I don't know if this is much better in terms of readability. but...

bytes((itertools.starmap(operator.xor, zip(a, b)))) # or just bytes(x ^ y for x, y in zip(a, b))

The ascii_chr and safe_ord are python2.7 compat right? Those can be dropped?

Yep, leftover from stuff I copied from pydevp2p

pipermerriam · 2017-09-25T21:20:55Z

This should be rebased now that https://github.com/pipermerriam/py-evm/pull/95 is merged.

gsalgado · 2017-09-26T08:00:33Z

Rebased and I think I've addressed all your comments, so should be ready for another pass

pipermerriam

Good to merge as is, but I did leave a few more comments.

pipermerriam · 2017-09-27T03:04:22Z

evm/p2p/auth.py

+        S = self.ephemeral_privkey.sign_msg_hash(secret_xor_nonce).to_bytes()
+
+        # S || H(ephemeral-pubk) || pubk || nonce || 0x0
+        auth_message = S + keccak(self.ephemeral_pubkey.to_bytes()) + self.pubkey.to_bytes() + \


I've found this format a bit more readable than using \ to split across multiple lines.

auth_message = ( S + keccak(self.ephemeral_pubkey.to_bytes()) + self.pubkey.to_bytes() + nonce + b'\x00' )

Or:

auth_message = b''.join(( S, keccak(self.ephemeral_pubkey.to_bytes()), self.pubkey.to_bytes(), nonce, b'\x00', ))

pipermerriam · 2017-09-27T03:19:43Z

evm/p2p/test_ecies.py

+    privkey = ecies.generate_privkey()
+    ciphertext = ecies.encrypt(msg, privkey.public_key)
+    decrypted = ecies.decrypt(ciphertext, privkey)
+    assert decrypted == msg


This is the type of test that I've historically used hypothesis for (things that have a round trip operation like encrypt/decrypt, encode/decode)

pipermerriam · 2017-09-27T03:20:55Z

evm/p2p/test_ecies.py

+    assert hmac == tag_expected
+
+
+# FIXME: Document those values; this test was lifted from pydevp2p


Couple of remaining FIXME left in. Fine to fix them later but it might be good to at least provide a link here for now.

Included the links in the comments

pipermerriam · 2017-09-27T03:21:57Z

evm/p2p/utils.py

@@ -0,0 +1,4 @@
+def sxor(s1, s2):
+    if len(s1) != len(s2):


Since this is used in cryptographic operations, should this use a constant time compare?

Isn't that the case already, since we're comparing their lengths?

I should not review code late at night. 👍

pipermerriam · 2017-09-27T03:23:17Z

evm/utils/keccak.py

@@ -10,4 +10,9 @@ def keccak(value):
    return keccak_256(value).digest()


+# FIXME: this is a terrible name.
+def keccak_updateable(value):


Thinking that it might be better to just use keccak_256 directly from the sha3 library instead of this proxy.

Agreed, I'll do that

gsalgado added the Needs Review label Sep 22, 2017

gsalgado force-pushed the p2p-auth branch from a31e70d to ae4c01b Compare September 22, 2017 16:12

pipermerriam requested changes Sep 22, 2017

View reviewed changes

gsalgado force-pushed the p2p-auth branch from eb5e92e to 504dc9b Compare September 25, 2017 09:42

gsalgado mentioned this pull request Sep 25, 2017

Implement auth.handshake() and Peer #98

Merged

gsalgado force-pushed the p2p-auth branch from 584484c to ca57921 Compare September 26, 2017 07:59

ethereum deleted a comment from gsalgado Sep 27, 2017

pipermerriam reviewed Sep 27, 2017

View reviewed changes

pipermerriam added PR state: merge ready and removed Needs Review labels Sep 27, 2017

pipermerriam approved these changes Sep 27, 2017

View reviewed changes

gsalgado force-pushed the p2p-auth branch from eb82f23 to 683226f Compare September 27, 2017 06:57

Implement P2P auth handshake

54145a9

gsalgado force-pushed the p2p-auth branch from 6969b1e to 54145a9 Compare September 27, 2017 07:33

gsalgado merged commit 52409ec into ethereum:master Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement P2P auth handshake #96

Implement P2P auth handshake #96

gsalgado commented Sep 22, 2017 •

edited

Loading

pipermerriam left a comment

pipermerriam Sep 22, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam Sep 22, 2017

pipermerriam Sep 22, 2017

pipermerriam Sep 22, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam Sep 22, 2017

gsalgado Sep 25, 2017

pipermerriam commented Sep 25, 2017

gsalgado commented Sep 26, 2017

pipermerriam left a comment

pipermerriam Sep 27, 2017

pipermerriam Sep 27, 2017

gsalgado Sep 27, 2017

pipermerriam Sep 27, 2017

gsalgado Sep 27, 2017

pipermerriam Sep 27, 2017

gsalgado Sep 27, 2017

pipermerriam Sep 27, 2017

pipermerriam Sep 27, 2017

gsalgado Sep 27, 2017

		assert hmac == tag_expected


		# FIXME: Document those values; this test was lifted from pydevp2p

Implement P2P auth handshake #96

Implement P2P auth handshake #96

Conversation

gsalgado commented Sep 22, 2017 • edited Loading

pipermerriam 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

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

pipermerriam commented Sep 25, 2017

gsalgado commented Sep 26, 2017

pipermerriam 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

gsalgado commented Sep 22, 2017 •

edited

Loading