Improve protocol-level handshaking protocol #3049

nbougalis · 2019-08-16T02:13:43Z

This commit restructures the HTTP based protocol negotiation that rippled executes and introduces support for negotiation of compression for peer links which, if implemented, should result in significant bandwidth savings for some server roles.

This commit also introduces the new [network_id] configuration option that administrators can use to specify which network the server is part of and intends to join. This makes it possible for servers from different networks to drop the link early.

The changeset also improves the log messages generated when negotiation of a peer link upgrade fails. In the past, no useful information would be logged, making it more difficult for admins to troubleshoot errors.

nbougalis · 2019-08-16T02:17:03Z

@MarkusTeufelberger, I would appreciate a review from you as well, since this also serves to correct several issues you've previously identified or mentioned in passing, including updating documentation.

ripplelabs-jenkins · 2019-08-16T03:40:44Z

Jenkins Build Summary

Built from this commit

Built at 20190823 - 16:04:16

Test Results

Build Type	Log	Result	Status
msvc.Debug	logfile	1174 cases, 0 failed, t: 587s	PASS ✅
msvc.Debug, NINJA_BUILD=true	logfile	1174 cases, 0 failed, t: 601s	PASS ✅
msvc.Debug -Dunity=OFF	logfile	1173 cases, 0 failed, t: 1087s	PASS ✅
msvc.Release	logfile	1174 cases, 0 failed, t: 418s	PASS ✅

MarkusTeufelberger

Phew, that took much longer to read through and understand than expected.

Thanks for the PR!

I'd really like to see at least some options besides signing the encoded TLS "finished" messages available. RFC5705 sounds more reasonable to me, or also just offering to use actual widely trusted certificates, if available (e.g. via Let's Encrypt).

src/ripple/overlay/README.md

src/ripple/overlay/impl/OverlayImpl.cpp

src/ripple/overlay/impl/ProtocolMessage.h

MarkusTeufelberger · 2019-08-17T21:49:43Z

src/ripple/overlay/impl/ProtocolMessage.h

+        return hdr;
+    }
+
+    // Version 2 header: compressed payload.


A simpler(?) solution might be to add a "header" message type in Protobuf that encodes all these parameters (payload_wire_size, payload_real_size, compression_algorithm) and only have a "header_wire_size" parameter to delimit stuff on the wire (might be possible to just use 1-2 bytes of length data).

You'd then send header_wire_size | header_proto | payload_proto on the wire. You can add the actual version of the protocol used to the header_proto as well to be more flexible and have more options to extend it in the future.

The code below just seems a bit like re-inventing yet another wire protocol, even though you are already using protobuf.

I hear you. I actually don't really like this binary prefix business, and am seriously considering migrating to a pure WebSocket-based transport (which would make the binary header unnecessary) or adopting an approach closer to what you describe (involving coded protobuf streams).

gRPC might be worth a look too then...

I think that gRPC is probably overkill and will require more rearchitecting than I'd like to do at this time. But I will take a look.

I just started looking into libp2p as well (used by Ethereum 2.0, IPFS etc.), but there's no C++ implementation so far and I suspect they are also vulnerable to active MITM with their TLS handshake.

Cap'n'Proto might also be worth a look, gRPC has the advantage that it might also take care of the transport stuff and is probably more widely used and implemented in other software. Wouldn't solve the MITM handshake mitigation, but there were options discussed for that already.

Ok, libp2p's handshake is not vulnerable and also prevents MITM attacks with a standard certificate based approach. The only extra part is that one needs to check that the host key embedded in the certificate did actually sign the public key of the certificate (and that the connection is using this key). That's more related to establishing a connection though, not so much to the stream of protobuf messages here.

While you can certainly address this issue using certificates, I still think the extra work that users have to go through makes it unlikely that they will. The primary objection to the existing MITM mitigation technique seems to be "yuck, I don't like how this is done." And that's fine but it's not really disposative.

No, it is "this is not possible without adding a patch to Python's stdlib" in my case. Accessing these messages is just not possible in a lot of implementations of TLS, verifying a special field in a certificate is.

The certs in libp2p are self-signed and can be created by the application ad-hoc, so there's no work on the user's end...

Hmm. This actually does give me an idea. I may do this, but it'll have to wait for a follow-up PR.

Based on what was described, my vote would be for the "header" message type & header_wire_size, seems like a simple solution that does the job. Since 'finished' and 'peer_finished' messages are part of the public openssl API, higher level wrappers should expose this functionality, if they do not we can push for this as a community (it's FOSS afterall!).

Reading about Cap'n'Proto, it seems cool though is a binary protocol which has downsides (additional debugging complexity, etc). Didn't look into libp2p as @MarkusTeufelberger said there was no C++ implementation. Self-signed certs would work but introduce additional management complexity (yes rippled could implement it internally but so would each protocol implementation). And what would we get beyond what is already provided in that case?

src/ripple/overlay/impl/ProtocolMessage.h

movitto · 2019-08-20T14:19:21Z

Phew, that took much longer to read through and understand than expected.

Thanks for the PR!

I'd really like to see at least some options besides signing the encoded TLS "finished" messages available. RFC5705 sounds more reasonable to me, or also just offering to use actual widely trusted certificates, if available (e.g. via Let's Encrypt).

Just jumping in here as I start to take a look at this. We've already implemented the handshake protocol in the ruby library and I'd like to keep that up to date.

Reading RFC5705, it specified the necessity for a 'master_secret' and 'label' for the EKM context, what were you thinking for those?

I'd personally vote against certificates as it's a centralizing component and the whole point on the P2P protocol is to be decentralized.

MarkusTeufelberger · 2019-08-20T14:24:04Z

master_secret and label can be public values as far as I understand it.

Certificates don't have to be signed by a CA, they can be self-signed. Take a look at libp2p's TLS 1.3 handshake protocol for example. Nothing centralized about that one.

nbougalis · 2019-08-22T01:47:05Z

Just jumping in here as I start to take a look at this. We've already implemented the handshake protocol in the ruby library and I'd like to keep that up to date.

That's neat. Even if we implement something different, it's likely that this mechanism will stick around for a while, and you'll have plenty of time to implement it. Also, hopefully it'll be something that doesn't require custom ruby gems.

Reading RFC5705, it specified the necessity for a 'master_secret' and 'label' for the EKM context, what were you thinking for those?

I'm not convinced the RFC5705 solution(s) prevent the kind of attack that our simple construct prevents anyways. I have technique that can do it and which doesn't require reaching into the SSL session and pulling something out of it, but it's a more complicated process than what we do currently requiring at least 5 messages (3 in one direction, 2 in the other direction) to be exchanged.

I'd personally vote against certificates as it's a centralizing component and the whole point on the P2P protocol is to be decentralized.

I think that we could implement something where the server creates a self-signed certificate (if none exists) and it would be no more difficult for the user, but I need to carefully consider the ramifications and ensure that we get the security guarantees we want.

codecov-io · 2019-08-27T02:27:51Z

Codecov Report

Merging #3049 into develop will increase coverage by 0.14%.
The diff coverage is 13.13%.

@@             Coverage Diff             @@
##           develop    #3049      +/-   ##
===========================================
+ Coverage    68.94%   69.08%   +0.14%     
===========================================
  Files          688      690       +2     
  Lines        55743    55636     -107     
===========================================
+ Hits         38431    38436       +5     
+ Misses       17312    17200     -112

Impacted Files	Coverage Δ
src/ripple/protocol/impl/BuildInfo.cpp	`91.66% <ø> (+12.5%)`	⬆️
src/ripple/overlay/Message.h	`0% <ø> (ø)`	⬆️
src/ripple/protocol/SecretKey.h	`77.77% <ø> (ø)`	⬆️
src/ripple/protocol/PublicKey.h	`100% <ø> (ø)`	⬆️
src/ripple/overlay/Peer.h	`0% <ø> (ø)`	⬆️
src/ripple/overlay/Overlay.h	`78.94% <ø> (ø)`	⬆️
src/ripple/rpc/handlers/WalletPropose.cpp	`90.62% <ø> (ø)`	⬆️
src/ripple/protocol/KeyType.h	`91.66% <ø> (ø)`
src/ripple/peerfinder/PeerfinderManager.h	`100% <ø> (ø)`	⬆️
src/ripple/overlay/impl/PeerImp.h	`0% <ø> (ø)`	⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98d55b9...c83f543. Read the comment docs.

src/ripple/overlay/impl/Handshake.cpp

src/ripple/overlay/README.md

movitto · 2019-08-27T18:56:50Z

src/ripple/overlay/impl/ProtocolMessage.h

+
+    // Version 1 header: uncompressed payload.
+    // The top six bits of the first byte are 0.
+    if ((*iter & 0xFC) == 0)


Any reason as to use the top 6 bits of the first byte (part of the payload_wire_size field) for the compression flag? Is it to maintain compatibility w/ the current header format (size)?

We could certainly do that. I have a different plan for compression in mind though and I'll probably repurpose the high bit to indicate compression when I do that PR.

movitto · 2019-08-27T19:17:36Z

Great job @nbougalis, this certainly looks like it sets the stage for some awesome changes when it comes to compression! As @MarkusTeufelberger said it was a little tricky to review this one though due to all the features / changes spread out in the many patches.

TMHello -> Handshake 👍
ProtocolVersion representation 👍 (fan of this though just to make sure I didn't miss anything, the different versions are not being used / distinguished yet correct?)
network_id 👍
message encapsulation changes (see comments on inline discussion above)
README update (few nits, see inline comments above)
Additional handshake fields (again see comments above)
Refactoring (logging, style, etc) 👍

seelabs

@nbougalis I like these changes a lot. I did a pass for nits/language-level feedback. I still need to do a pass and think about the meat of these changes.

src/ripple/overlay/impl/Handshake.cpp

src/ripple/overlay/impl/PeerImp.cpp

src/ripple/overlay/impl/ProtocolVersion.cpp

src/ripple/overlay/README.md

src/ripple/overlay/impl/OverlayImpl.cpp

seelabs · 2019-09-04T21:36:12Z

src/ripple/overlay/impl/OverlayImpl.cpp

    msg.insert("Server", BuildInfo::getFullVersionString());
    msg.insert("Remote-Address", remote_address.to_string());
    msg.insert(boost::beast::http::field::connection, "close");
-    msg.body() = text;
    msg.prepare_payload();
    return std::make_shared<SimpleWriter>(msg);


SimpleWriter is also deprecated, tho I don't know what replaces it (and obviously not part of this patch).

src/ripple/overlay/impl/ProtocolVersion.cpp

ximinez · 2019-10-23T22:16:14Z

Just FYI, VS doesn't like the make_function_output_iterator
https://travis-ci.com/ripple/rippled/jobs/231590174#L860. I'm not sure how to fix it.

ximinez · 2019-11-01T14:43:03Z

This commit works around the compile error I pointed out above by doing it "the hard way" for MSVC builds. ximinez@c974971

ximinez · 2019-11-01T18:22:29Z

src/test/overlay/TMHello_test.cpp

-    }
-};
-
-BEAST_DEFINE_TESTSUITE(TMHello,overlay,ripple);


I am always reluctant to get rid of unit tests without a really good reason. I don't think you've entirely removed the functionality being tested here, so why remove the tests?

ximinez · 2019-11-01T18:43:59Z

src/test/protocol/BuildInfo_test.cpp

-    }
-};
-
-BEAST_DEFINE_TESTSUITE(BuildInfo,ripple_data,ripple);


As above, I am always reluctant to get rid of unit tests without a really good reason. I don't think you've entirely removed the functionality being tested here, so why remove the tests?

This code changes ProtocolVersion to be a simple std::pair and all the relevant operators that were being tested here before are now provided by std so I don't believe that testing that functionality is necessary.

The only test that is no longer there is BEAST_EXPECT(current_protocol >= minimum_protocol) and the new implementation of this code simply doesn't have such values in a way that makes sense to test. Rather the implementation requires that all supported versions are listed (in sorted order, with no duplicates) in the supportedProtocolList array.

I had a static_assert that was meant to test the sorted & no dupes requirement, but it turns out that it was broken and I didn't know because I had incorrectly gated it so that it would require compiling with C++20. So your comment did help me find that, thanks!

I rewrote the static_assert to execute under C++17 and verified that it works correctly.

This commit restructures the HTTP based protocol negotiation that `rippled` executes and introduces support for negotiation of compression for peer links which, if implemented, should result in significant bandwidth savings for some server roles. This commit also introduces the new `[network_id]` configuration option that administrators can use to specify which network the server is part of and intends to join. This makes it possible for servers from different networks to drop the link early. The changeset also improves the log messages generated when negotiation of a peer link upgrade fails. In the past, no useful information would be logged, making it more difficult for admins to troubleshoot errors. This commit also fixes RIPD-237 and RIPD-451

)

ximinez

I haven't done a full review, but did check out the more recently added commits.

I also fixed the Windows build by ripping off the boost::function_output_iterator with an iterator that skips the lambda and does the assignment directly. It's probably not the best option, but it works.

Edit: The branch is https://github.com/ximinez/rippled/tree/nb--handshake

src/ripple/overlay/impl/ProtocolVersion.cpp

src/ripple/overlay/impl/ProtocolMessage.h

nbougalis · 2019-11-13T20:55:23Z

I also fixed the Windows build by ripping off the boost::function_output_iterator with an iterator that skips the lambda and does the assignment directly. It's probably not the best option, but it works.

The change in question is: ximinez@878b1d8

I appreciate you doing this, but it seems to me that it is overkill to address what is, at the end of the day, either a compiler deficiency or an issue with Boost against Windows. The fix that I put in place after your initial report (creating a std::function to wrap the lambda) is more compact and easy to understand IMO. So unless you feel particularly strong about your approach, I'll stick with what I have, @ximinez.

ximinez · 2019-11-13T22:57:23Z

I appreciate you doing this, but it seems to me that it is overkill to address what is, at the end of the day, either a compiler deficiency or an issue with Boost against Windows. The fix that I put in place after your initial report (creating a std::function to wrap the lambda) is more compact and easy to understand IMO. So unless you feel particularly strong about your approach, I'll stick with what I have, @ximinez.

Welp, egg on my face, because I'm having no problems building at all now. I have no idea what changed, but appears to be a moot point either way.

ximinez

I went through the whole thing this time, and it looks good. I have a couple of very minor and optional suggestions.

I notice there's a CI failure building the validator keys tool, which is because KeyType.h got moved. I think that'll have to be addressed separately, but probably before this change is released.

src/ripple/overlay/impl/ProtocolVersion.cpp

src/ripple/overlay/impl/ProtocolVersion.h

nbougalis requested review from thejohnfreeman, seelabs, scottschurr and jwbusch August 16, 2019 02:13

MarkusTeufelberger mentioned this pull request Aug 17, 2019

peer handshake violates TLS transparency #2413

Closed

MarkusTeufelberger reviewed Aug 17, 2019

View reviewed changes

MarkusTeufelberger mentioned this pull request Aug 18, 2019

src/ripple/overlay/README.md incorrect #2449

Closed

scottschurr assigned MarkusTeufelberger, thejohnfreeman, seelabs, scottschurr and jwbusch Aug 21, 2019

mDuo13 mentioned this pull request Aug 26, 2019

Add Devnet description and disambiguation table XRPLF/xrpl-dev-portal#674

Merged

movitto reviewed Aug 27, 2019

View reviewed changes

src/ripple/overlay/impl/Handshake.cpp Outdated Show resolved Hide resolved

movitto reviewed Aug 27, 2019

View reviewed changes

src/ripple/overlay/README.md Show resolved Hide resolved

movitto reviewed Aug 27, 2019

View reviewed changes

seelabs reviewed Sep 4, 2019

View reviewed changes

src/ripple/overlay/README.md Outdated Show resolved Hide resolved

seelabs reviewed Sep 4, 2019

View reviewed changes

src/ripple/overlay/impl/OverlayImpl.cpp Show resolved Hide resolved

seelabs reviewed Sep 4, 2019

View reviewed changes

seelabs reviewed Sep 5, 2019

View reviewed changes

src/ripple/overlay/impl/ProtocolVersion.cpp Outdated Show resolved Hide resolved

src/ripple/overlay/impl/ProtocolVersion.cpp Show resolved Hide resolved

seelabs reviewed Sep 5, 2019

View reviewed changes

src/ripple/overlay/impl/ProtocolVersion.cpp Outdated Show resolved Hide resolved

nbougalis mentioned this pull request Sep 9, 2019

Propagate validator lists (VLs or UNLs) over the peer network: #3072

Closed

ximinez reviewed Nov 1, 2019

View reviewed changes

nbougalis added 7 commits November 1, 2019 19:47

[FOLD] Address review comments by @movitto and @seelabs

2324ff0

[FOLD] Address bugs discovered by @MarkusTeufelberger & @seelabs

de8088f

[FOLD] Move KeyType from crypto/ to protocol/

fb6f2dd

[FOLD] Use ugly constexpr lambda in static_assert

277748c

[FOLD] Add unit tests & address issues detected by them (thanks @ximinez

f50093f

)

[FOLD] Account for move of KeyType.h

5ac35b2

ximinez requested changes Nov 4, 2019

View reviewed changes

src/ripple/overlay/impl/ProtocolVersion.cpp Outdated Show resolved Hide resolved

ximinez reviewed Nov 4, 2019

View reviewed changes

src/ripple/overlay/impl/ProtocolMessage.h Show resolved Hide resolved

[FOLD] Misc cleanups from review

d883e80

ximinez approved these changes Nov 18, 2019

View reviewed changes

src/ripple/overlay/impl/ProtocolVersion.cpp Outdated Show resolved Hide resolved

src/ripple/overlay/impl/ProtocolVersion.h Outdated Show resolved Hide resolved

nbougalis added 2 commits November 24, 2019 01:01

[FOLD] Use symbolic constant for legacy protocol version

b1c6edf

[FOLD] Address more review comments by @ximinez

c83f543

nbougalis mentioned this pull request Nov 28, 2019

Proposed 1.5.0-b1 #3167

Merged

nbougalis closed this in #3167 Dec 6, 2019

intelliot mentioned this pull request Apr 29, 2020

Return network_id upon initial websocket connection and/or in server_info #3383

Closed

nbougalis deleted the handshake branch October 16, 2023 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve protocol-level handshaking protocol #3049

Improve protocol-level handshaking protocol #3049

nbougalis commented Aug 16, 2019

nbougalis commented Aug 16, 2019

ripplelabs-jenkins commented Aug 16, 2019 •

edited

Loading

MarkusTeufelberger left a comment

MarkusTeufelberger Aug 17, 2019

nbougalis Aug 18, 2019

MarkusTeufelberger Aug 18, 2019

nbougalis Aug 19, 2019

MarkusTeufelberger Aug 19, 2019

MarkusTeufelberger Aug 19, 2019

nbougalis Aug 19, 2019

MarkusTeufelberger Aug 19, 2019

nbougalis Aug 21, 2019

movitto Aug 27, 2019

movitto commented Aug 20, 2019

MarkusTeufelberger commented Aug 20, 2019

nbougalis commented Aug 22, 2019

codecov-io commented Aug 27, 2019 •

edited

Loading

movitto Aug 27, 2019

nbougalis Sep 4, 2019

movitto commented Aug 27, 2019 •

edited

Loading

seelabs left a comment

seelabs Sep 4, 2019

ximinez commented Oct 23, 2019

ximinez commented Nov 1, 2019

ximinez Nov 1, 2019

ximinez Nov 1, 2019

nbougalis Nov 2, 2019 •

edited

Loading

ximinez left a comment •

edited

Loading

nbougalis commented Nov 13, 2019 •

edited

Loading

ximinez commented Nov 13, 2019

ximinez left a comment

Improve protocol-level handshaking protocol #3049

Improve protocol-level handshaking protocol #3049

Conversation

nbougalis commented Aug 16, 2019

nbougalis commented Aug 16, 2019

ripplelabs-jenkins commented Aug 16, 2019 • edited Loading

Jenkins Build Summary

Test Results

MarkusTeufelberger 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

movitto commented Aug 20, 2019

MarkusTeufelberger commented Aug 20, 2019

nbougalis commented Aug 22, 2019

codecov-io commented Aug 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

movitto commented Aug 27, 2019 • edited Loading

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez commented Oct 23, 2019

ximinez commented Nov 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

ximinez left a comment • edited Loading

Choose a reason for hiding this comment

nbougalis commented Nov 13, 2019 • edited Loading

ximinez commented Nov 13, 2019

ximinez left a comment

Choose a reason for hiding this comment

ripplelabs-jenkins commented Aug 16, 2019 •

edited

Loading

codecov-io commented Aug 27, 2019 •

edited

Loading

movitto commented Aug 27, 2019 •

edited

Loading

nbougalis Nov 2, 2019 •

edited

Loading

ximinez left a comment •

edited

Loading

nbougalis commented Nov 13, 2019 •

edited

Loading