Fix empty ENR list in auth-response properly + potentially request new ENR on revalidation

eth/p2p/discoveryv5/encoding.nim

@@ -22,7 +22,7 @@ type
   AuthResponse = object
     version: int
     signature: array[64, byte]
-    record: Record
+    record: Option[enr.Record]
 
   Codec* = object
     localNode*: Node
@@ -104,16 +104,19 @@ proc encodeAuthHeader*(rng: var BrHmacDrbgContext,
   let ln = c.localNode
 
   if challenge.recordSeq < ln.record.seqNum:
-    resp.record = ln.record
-  # else:
-  # an uninitialized record which will get encoded as an empty rlp list.
+    resp.record = some(ln.record)
+  else:
+    resp.record = none(enr.Record)
 
   let ephKeys = KeyPair.random(rng)
   let signature = signIDNonce(c.privKey, challenge.idNonce,
     ephKeys.pubkey.toRaw)
   resp.signature = signature.toRaw
 
-  let secrets = deriveKeys(ln.id, toId, ephKeys.seckey, challenge.pubKey,
+  # Calling `encodePacket` for handshake should always be with a challenge
+  # with the pubkey of the node we are targetting.
+  doAssert(challenge.pubKey.isSome())
+  let secrets = deriveKeys(ln.id, toId, ephKeys.seckey, challenge.pubKey.get(),
     challenge.idNonce)
 
   let respRlp = rlp.encode(resp)
@@ -249,7 +252,8 @@ proc decodeMessage(body: openarray[byte]): DecodeResult[Message] =
 proc decodeAuthResp*(c: Codec, fromId: NodeId, head: AuthHeader,
     challenge: Whoareyou, newNode: var Node): DecodeResult[HandshakeSecrets] =
   ## Decrypts and decodes the auth-response, which is part of the auth-header.
-  ## Requiers the id-nonce from the WHOAREYOU packet that was send.
+  ## Requires the id-nonce from the WHOAREYOU packet that was send.
+  ## newNode can be nil in case node was already known (no was ENR send).
   if head.scheme != authSchemeName:
     warn "Unknown auth scheme"
     return err(HandshakeError)
@@ -270,19 +274,26 @@ proc decodeAuthResp*(c: Codec, fromId: NodeId, head: AuthHeader,
     authResp = rlp.decode(respData.get(), AuthResponse)
   except RlpError, ValueError:
     return err(HandshakeError)
-  # TODO:
-  # Should allow for not having an ENR included, solved for now by sending
-  # whoareyou with always recordSeq of 0
 
-  # Node returned might not have an address or not a valid address
-  newNode = ? newNode(authResp.record).mapErrTo(HandshakeError)
+  var pubKey: PublicKey
+  if authResp.record.isSome():
+    # Node returned might not have an address or not a valid address.
+    newNode = ? newNode(authResp.record.get()).mapErrTo(HandshakeError)
     if newNode.id != fromId:
       return err(HandshakeError)
 
+    pubKey = newNode.pubKey
+  else:
+    if challenge.pubKey.isSome():
+      pubKey = challenge.pubKey.get()
+    else:
+      # We should have received a Record in this case.
+      return err(HandshakeError)
+
   # Verify the id-nonce-sig
   let sig = ? SignatureNR.fromRaw(authResp.signature).mapErrTo(HandshakeError)
   let h = idNonceHash(head.idNonce, head.ephemeralKey)
-  if verify(sig, SkMessage(h.data), newNode.pubkey):
+  if verify(sig, SkMessage(h.data), pubkey):
     ok(secrets)
   else:
     err(HandshakeError)

eth/p2p/discoveryv5/enr.nim

@@ -469,7 +469,4 @@ proc read*(rlp: var Rlp, T: typedesc[Record]):
   rlp.skipElem()
 
 proc append*(rlpWriter: var RlpWriter, value: Record) =
-  if value.raw.len > 0:
   rlpWriter.appendRawBytes(value.raw)
-  else:
-    rlpWriter.startList(0)

eth/p2p/discoveryv5/protocol.nim

@@ -249,7 +249,12 @@ proc decodeWhoAreYou(d: Protocol, packet: openArray[byte]):
 proc sendWhoareyou(d: Protocol, address: Address, toNode: NodeId,
     authTag: AuthTag): DiscResult[void] {.raises: [Exception, Defect].} =
   trace "sending who are you", to = $toNode, toAddress = $address
-  let challenge = Whoareyou(authTag: authTag, recordSeq: 0)
+  let n = d.getNode(toNode)
+  let challenge = if n.isSome():
+                    Whoareyou(authTag: authTag, recordSeq: n.get().record.seqNum,
+                      pubKey: some(n.get().pubkey))
+                  else:
+                    Whoareyou(authTag: authTag, recordSeq: 0)
   brHmacDrbgGenerate(d.rng[], challenge.idNonce)
 
   # If there is already a handshake going on for this nodeid then we drop this
@@ -353,7 +358,7 @@ proc receive*(d: Protocol, a: Address, packet: openArray[byte]) {.gcsafe,
     var pr: PendingRequest
     if d.pendingRequests.take(whoareyou.authTag, pr):
       let toNode = pr.node
-      whoareyou.pubKey = toNode.pubkey # TODO: Yeah, rather ugly this.
+      whoareyou.pubKey = some(toNode.pubkey) # TODO: Yeah, rather ugly this.
       doAssert(toNode.address.isSome())
       let (data, _) = encodePacket(d.rng[], d.codec, toNode.id, toNode.address.get(),
         pr.message, challenge = whoareyou)
@@ -725,8 +730,10 @@ proc revalidateNode*(d: Protocol, n: Node)
 
   if pong.isOK():
     if pong.get().enrSeq > n.record.seqNum:
-      # TODO: Request new ENR
-      discard
+      # Request new ENR
+      let nodes = await d.findNode(n, 0)
+      if nodes.isOk() and nodes[].len > 0:
+        discard d.addNode(nodes[][0])
 
 proc revalidateLoop(d: Protocol) {.async, raises: [Exception, Defect].} =
   # TODO: General Exception raised.

eth/p2p/discoveryv5/types.nim

@@ -23,7 +23,7 @@ type
     authTag*: AuthTag
     idNonce*: IdNonce
     recordSeq*: uint64
-    pubKey* {.rlpIgnore.}: PublicKey
+    pubKey* {.rlpIgnore.}: Option[PublicKey]
 
   Whoareyou* = ref WhoareyouObj
 
@@ -107,3 +107,23 @@ proc hash*(address: Address): Hash {.inline.} =
 proc hash*(key: HandshakeKey): Hash =
   result = key.nodeId.hash !& key.address.hash
   result = !$result
+
+proc read*(rlp: var Rlp, O: type Option[Record]): O
+    {.raises: [ValueError, RlpError, Defect].} =
+  mixin read
+  if not rlp.isList:
+    raise newException(
+      ValueError, "Could not deserialize optional ENR, expected list")
+
+  # The discovery specification states that in case no ENR is send in the
+  # handshake, an empty rlp list instead should be send.
+  if rlp.listLen == 0:
+    none(Record)
+  else:
+    some(read(rlp, Record))
+
+proc append*(writer: var RlpWriter, value: Option[Record]) =
+  if value.isSome:
+    writer.append value.get
+  else:
+    writer.startList(0)

tests/p2p/test_discv5_encoding.nim

@@ -241,7 +241,7 @@ suite "Discovery v5 Additional":
       nodeId = pubKey.toNodeId()
       idNonce = hexToByteArray[idNonceSize](
         "0xa77e3aa0c144ae7c0a3af73692b7d6e5b7a2fdc0eda16e8d5e6cb0d08e88dd04")
-      whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 0, pubKey: pubKey)
+      whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 0, pubKey: some(pubKey))
       c = Codec(localNode: node, privKey: privKey)
 
     let (auth, _) = encodeAuthHeader(rng[], c, nodeId, nonce, whoareyou)