-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add SSL support to P2P #8996
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
base: master
Are you sure you want to change the base?
Add SSL support to P2P #8996
Conversation
22ca764 to
5208f25
Compare
|
Added sodium to the dependency list, hopefully builds pass now. |
5208f25 to
d8e410c
Compare
|
Force pushed a change based on my last comment ( |
d8e410c to
6f1e246
Compare
|
Did another force-push with a rebase, to get rid of the |
af7d5c0 to
5d35a9a
Compare
|
I added a crude functional test for SSL. It just tests that autodetection and disabling SSL works. |
|
Started a review and have an initial comment. I didn't get around to fixing Running this code on a node that can get incoming connections, you can see that the back ping to an incoming node always fails (log level 2 you'll see I'm happy to do this since I said I would over in that PR, my bad for not getting to it sooner. |
5d35a9a to
da4fc6c
Compare
|
@j-berman Fixed SSL connections with |
da4fc6c to
3d88cc0
Compare
|
Forgot to include newest code in last push. Trying again! |
3d88cc0 to
a7c49c9
Compare
|
Rebased, and trying to fix a linking issue that I cannot reproduce locally. |
|
Please excuse my ignorance, I'm not familiar with the boost library and I'm therefore wondering what cipher are actually available ? afaiu, SSL namespace include TLS up to TLSv1.3: https://www.boost.org/doc/libs/1_74_0/boost/asio/ssl/context_base.hpp I'm probably blind but i don't find in the commit changes where are cipher options. That would be good to enforce at least TLSv1.2 |
|
The code already forces TLS 1.2+. It also limits the cipher list a few lines below that. |
a7c49c9 to
b3c8322
Compare
|
Bad rebase, will fix. |
b3c8322 to
60925f1
Compare
|
The test that just failed should be spurious. |
|
I rebased against latest master changes, which included a test merge and a seed node merge. |
60925f1 to
0b18c43
Compare
|
Force pushed a rebase. Nothing should have changed. Looking at the diff again. |
0b18c43 to
0bc4259
Compare
|
Force pushed another small update removing some unintended changes. |
|
The failed test is not related. May push a dummy update so that all unit tests get a chance to run. |
0bc4259 to
eb47b3f
Compare
|
Force pushed a rebase merge for p2p functional tests. |
c814bf3 to
ba7a086
Compare
j-berman
left a comment
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.
This generally looks ok to me, though I'm not particularly well versed with SSL. Mostly nits and some questions
| > The server/responder must always be in autodetect SSL mode, so that it can | ||
| handle any possible state that the client/initiator is in. | ||
|
|
||
| > If a node changes certificates, "old" peers will be unable to connect until |
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.
It looks as though if monerod generates a new temporary cert every run, that the node won't share the fingerprint with its peers. Therefore this should only affect nodes that have a persistent cert and then change it, ya?
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.
Correct, if a fingerprint is sent in the peerlist exchange, it is removed by the receiving node. The fingerprint is also exchanged in the handshake, iff the user has a fixed certificate. In this situation, the peer caches it and is equivalent to a unique ip/port pair in terms of treatment and recovery.
src/p2p/net_node.cpp
Outdated
| const command_line::arg_descriptor<std::vector<std::string> > arg_p2p_add_exclusive_node = {"add-exclusive-node", "Specify list of peers to connect to only." | ||
| " If this option is given the options add-priority-node and seed-node are ignored"}; | ||
| const command_line::arg_descriptor<std::vector<std::string> > arg_p2p_seed_node = {"seed-node", "Connect to a node to retrieve peer addresses, and disconnect"}; | ||
| const command_line::arg_descriptor<std::vector<std::string> > arg_p2p_add_peer = {"add-peer", "Manually add peer to local peerlist. Append \",sha-256 fingerprint\" to make authenticated connection." |
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.
Nits:
| const command_line::arg_descriptor<std::vector<std::string> > arg_p2p_add_peer = {"add-peer", "Manually add peer to local peerlist. Append \",sha-256 fingerprint\" to make authenticated connection." | |
| const command_line::arg_descriptor<std::vector<std::string> > arg_p2p_add_peer = {"add-peer", "Manually add peer to local peerlist. Append \",<sha-256 fingerprint>\" to make authenticated connection. " |
Same for other references to the fingerprint
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.
Fixed.
| m_is_income(false), | ||
| m_started(time(NULL)), | ||
| m_ssl(false), | ||
| m_ssl(ssl_support_t::e_ssl_support_disabled), |
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.
nit: the whitespace has tabs here and doesn't line up in my editor
| m_ssl(ssl_support_t::e_ssl_support_disabled), | |
| m_ssl(ssl_support_t::e_ssl_support_disabled), |
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 think I fixed this. Always annoying in editors.
|
|
||
| template<typename ConstBufferSequence, typename WriteHandler> | ||
| void async_write_some(const ConstBufferSequence &buffers, WriteHandler &&handler) | ||
| bool server_handshake(boost::asio::const_buffer buffer) |
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.
This is unused
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.
Removed.
| auto notification = receiver_.get_notification<cryptonote::NOTIFY_NEW_TRANSACTIONS>().second; | ||
| EXPECT_EQ(txs, notification.txs); | ||
| EXPECT_FALSE(notification._.empty()); | ||
| EXPECT_TRUE(notification._.empty()); |
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.
Checking my understanding: because the padding doesn't get serialized into _ as part of the NOTIFY_NEW_TRANSACTIONS request anymore, and instead gets stuffed into the buffer, the _ field is now empty even though this request should have been padded. And the reason you don't pad into a _ field anymore is because you want padding on every message, rather than strictly NOTIFY_NEW_TRANSACTIONS
It would be nice if there was an explicit way to see/test the padding at this level, but not worth changing the internals for it
A comment explaining how these _with_padding tests are just testing smooth notifications even with pad true would be solid, since they're no longer explicitly testing the padding
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.
Commented the best I could in the test file.
| head.m_return_code = SWAP32LE(return_code); | ||
|
|
||
| std::memcpy(buffer.tellp() - buffer.size(), std::addressof(head), sizeof(head)); | ||
| buffer.put_n(0, pad_bytes); |
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.
Just to check my understanding: when de-serializing a message, the de-serializer is unaffected by these padded bytes because these are just dangling bytes that have no associated key, ya? The de-serializer can safely just ignore them
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.
Yes, the epee parser always ignored extra data after the root object. The "noise" mode in Tor uses this "feature" already - each message is padded to a specific length with possible trailing. So we have a working example at least.
Any future parser already has to support this feature due to the noise mode.
ba7a086 to
b2c323f
Compare
|
Force pushed a rebase and @j-berman changes. |
|
@vtnerd Looks like there are conflicts to resolve. |
b2c323f to
bb1eaab
Compare
|
Force pushed a rebase to solve merge conflicts. |
First, my apologies to @moneromooo-monero for NACKing his/her SSL proposal years ago, only for me to bring it back.
This does exactly as the title suggests, with options to disable P2P encryption entirely, and an option to re-use a SSL certificate between "runs". The default generates a new SSL certificate each
monerodrun, so that the node cannot be tracked across IP address changes.Nodes do not trust encryption information from peers. Instead, every peer is assumed to be in autodetect mode, unless overridden on the CLI or via handshake/ping messages directly from the peer.
Possibly bad:
If a node chooses to re-use SSL certificates, a change in certificates will cause connection failures until the node is removed from the white+gray lists OR the node makes a direct connection and provides the new SSL certificate.
EDIT:
I will also attempt some unit test changes; there is enough to review here that I expect this diff to be up a while.