-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIP-8: devp2p Forward Compatibility Requirements for Homestead #49
Conversation
packets. If the remote hello packet contains a version greater than the current local | ||
version, the implementation should reply with a hello packet containing the local (lower) | ||
version. Implementations should also ignore any additional list elements at the end of the | ||
hello packet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the hello packets being sent out immediately as the connection is established? Also not sure about it, but AFAIK there are no ordering guarantees between who sends such a packet first or second, so I don't really see a way to send the lower of the two, if both sides have to wait for the remote one to figure out the correct version.
Edit: Perhaps you meant not that they should reply with the lowest, but rather that they should use the lowest version throughout the communication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this requires clarification and that neither side should wait on the other side to send their version and either side can choose to disconnect or continue communication if the version is different. The proposed specifications are ambiguous because they state that either side can do what they want. This can be simplified to:
- Once a secure connection is established a Hello packet or Disconnect packet MUST be sent
- The hello packet MUST contain the highest version supported by the client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clarify this in the text, it was definitely not meant to imply that implementations should wait for the remote hello packet.
In my opinion the proposal is solid apart from the RLP encoding question I've raised above. That should be looked at by someone more capable in cryptography. I would however like to emphasize that this would be a very high priority feature to get into the Homestead fork, as the protocol in it's current state is not upgradable without a hard fork. There are issues with the current protocol already, and as time progresses I'm certain we'll find plenty more, or simply much needed extensions that cannot be implemented due to the forking requirements. The most important aspect IMHO about the solution proposed by Felix is that it's extremely simple. It could be added to any implementation in a matter of minutes, which is imho worth doing now and paving the way forward to easier upgrades afterwards. The upgrade mechanism itself is quite crude as any version bump requires adding fields to the end, no fields can be deleted or semantically changed (to keep backwards compatibility), but as we're not aiming to do many updates, just to facilitate occasional essential ones, I think this is fine. And as we're transitioning through metropolis and serenity, we can take the lessons learned from previous updates and refine/clean in each of these hard forks. Best case scenario we can seamlessly upgrade the devp2p protocol as need arises, worst case scenario we don't use the upgrade mechanism, but it won't hinder anything either. |
version. Implementations should also ignore any additional list elements at the end of the | ||
hello packet. | ||
|
||
Similarly, implementations of the RLPx Discovery Protocol should ignore the version number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be simplified to:
- RLPx UDP packets with unknown type should be ignored
Why the specification to ignore additional list elements?
The version number in Ping/Pong packets shouldn't be ignored. It is to be cached and/or checked against existing metadata and maybe further verified as a compatible version following encrypted handshake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clarify this too. "Ignore" may suggest that the version number should be discarded, which is not the intended meaning. It is simply meant to say that nodes with mismatching version should still be included in the discovery table.
The bit about additional list elements allows for backwards-compatible upgrades in the same manner as for the devp2p hello packet: If any packet is extended to include more information in a later version of the protocol, older nodes will simply ignore these elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a nested tuple? Something like:
auth-body = rlp.list(auth-vsn, rlp.list(sig, initiator-pubk, initiator-nonce OR token))
Thus:
- The first item in auth-body will always be VERSION
- The second item in auth-body will always be a list for HANDSHAKE (3 items)
Notably, any extensions would need to be very strict to ensure they aren't introducing plaintext elements.
Currently the id's are 64 bytes. Can we change them 32 bytes (a compressed public key)? |
|
||
I propose the following changes to the handshake packets: | ||
|
||
* Adding the length of the ciphertext as a plaintext header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No plaintext. The problem with this is that the length will always be greater than X, and likely equal to X, so it can be inferred that the connection is rlpx. Leaking information (aka plaintext) is not within the design rationale of RLPx or any encrypted protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #49 (comment)
@karalabe If homestead is a hard-fork I think we should just fix the protocol. A design rationale behind RLPx is that communication MUST NOT rely on the authentication handshake for metadata, and that metadata is to be exchanged via discovery. The solution is so simple: Try to connect to the node via discovery first! This way both sides will authenticate what version they know they can speak before attempting the handshake – its also easy to write a discovery (UDP) stack which supports multiple versions. Case in point, a peer cannot connect to a node prior to discovery – as discovery is how the public key is obtained. No discovery == no TCP handshake. |
For the handshake, what do we need to consider other than the scenarios below?
Note: This upgrade is more complex than it seems. The necessity of this upgrade is in order to fix the way the KDF used and to enable framing. Ideally we also remove AES padding. What this means is that if new clients are to support old clients they will need to be able to enable the new framing system on a per-session. |
@subtly I'm not planning on supporting backwards compatibility. |
And that's fine. You can always drop connections with lower version (the EIP doc explicitly permits this). |
Test vectors have been added. |
@subtly (aggregating our discussion a bit) Your concerns about the RLPx handshake changes seem to be:
|
111d680
to
c373cba
Compare
After discussing with @fjl and @nagydani I think we need to write up a spec of exact requirements of RLPx in terms of threat models - what attacks are we interested in defending against right now, and which can we leave for future protocol upgrades. It's hard to have a discussion of details such as leaking plaintext and/or ciphertext length without having consensus on threat models & general reqs. |
For anyone following this discussion: A lot has happened. We've had multiple long calls about EIP-8, a counterproposal and a rebuttal of the counterproposal have been written. I've updated the text of EIP-8 to clarify some things. Notably, Postel's Law is now mentioned because it |
So I think in that case the discovery traffic wouldn't be UDP. It could be by written notes sent by pigeons, ect. But the point is just because the normal method for discovery is blocked the transport can still operate. |
The document assumes nodes talking using the currently specified protocols, not a version where arbitrary transport is negotiated using an arbitrary method of discovery. Not sure whether this needs to be made explicit. We can improve on everything in the future, but for the RLPx protocol that is in use right now, correlating UDP traffic is definitely possible (and there are other indicators, including commonly used ports, the fixed-size handshake traffic pattern, the first byte always being |
I think I have a slightly different view here. I view the three protocols (discovery, handshake, transport) as being mutually independent and composable. |
This is my abstract view, too, but when discussing the possibility of filtering RLPx v4 TCP connections |
Anyway, I've fixed up the sentence to state explicitly that TCP connections could be blocked by looking at UDP traffic, which was the intended meaning. |
Another short overview document (as requested by @chriseth): https://gist.github.com/6c6d24dc1c7ab5d77c5c |
Note: test vectors need to be regenerated for the size prefix authentication change. I'll do that and then squash all the commits together. |
Test vectors are now up to date. |
Squashed. |
Could we get this merged into the EIP repo if it was accepted for homestead? (Also specify it as "Accepted"?) |
Done. |
EIP-8: devp2p Forward Compatibility Requirements for Homestead
Add Waves namespace
This is the first Networking EIP, in which I am trying to sneak some changes
into all implementations before the Homestead release. These changes should
make network protocol upgrades a lot easier in the future.
Merry 🎄 everyone.