Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli · Pull Request #14032 · bitcoin/bitcoin

jonasschnelli · 2018-08-23T15:11:14Z

This PR adds encryption to the p2p communication after a slightly overhauled version of BIP151 defined here (there is the plan to change BIP151 or to propose this protocol in a new BIP)

The encryption is optional and by default disabled (-netencryption).

If enabled, a peer connecting to another peer signalling NODE_ENCRYPTED (or added via -connect=) will try to do the proposed key handshake and continue with encrypted communication.

If enabled, peers can request (and perform) encrypted communications by sending a handshake request.

Peers not supporting encryption are still accepted (no option to enforce encrypted communication).

There is a plan to make the handshake quantum resistance by adding NewHope to the key handshake (https://newhopecrypto.org). But since this PR is already very large, it's unclear wether this should be an independent patch (probably another ~600 lines of code).

Out of scope:

optimized ChaCha20 implementation (for review and security reasons, the implementation is extracted from openssh)
benchmarks added to bench (I have done comparison against the v1 protocol with dbl-SHA256 the performance seems very similar)
Please no discussion about the used crypto scheme or the proposal itself (better place would be the mailing list)

TODO:

add option to -connect= RPC addnode where it is possible to specify the expected service flags (currently -connect= will always try for encrypted coms).

practicalswift · 2018-08-23T18:03:45Z

src/net.cpp

Nit: Should this be ”envelope” instead of ”envelop”? If so, change applies throughout this PR and also the BIP.

practicalswift · 2018-08-23T18:07:22Z

src/init.cpp

If I’m reading the logic correct then this should be ”every 10 seconds or after 10kb of data”?

practicalswift · 2018-08-23T18:08:57Z

src/net.cpp

Nit: “treat”

practicalswift · 2018-08-23T18:11:13Z

src/net_encryption.cpp

Nit: “therefore”?

practicalswift · 2018-08-23T18:13:18Z

src/net_message.h

Nit: “has”?

DrahtBot · 2018-08-23T18:28:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16362 (gui: Bilingual translation by hebasto)
#16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
#16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
#16273 (refactor: Remove unused includes by practicalswift)
#16248 (Make whitebind/whitelist permissions more flexible by NicolasDorier)
#16224 (gui: Bilingual GUI error messages by hebasto)
#16202 (Refactor network message deserialization by jonasschnelli)
#16097 (WIP: Prevent meaningless negating of arguments by hebasto)
#16060 (Bury bip9 deployments by jnewbery)
#15649 (Add ChaCha20Poly1305@Bitcoin AEAD by jonasschnelli)
#15505 ([p2p] Request NOTFOUND transactions immediately from other outbound peers, when possible by sdaftuar)
#15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
#15197 (Refactor and slightly stricter p2p message processing by jonasschnelli)
#14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonasschnelli · 2018-08-23T19:50:32Z

Fixed @practicalswift points.

gmaxwell · 2018-08-23T20:26:51Z

Great work!

Although optimized crypto is certainly out of scope, we do want to be mindful of making any protocol decisions that would preclude using them. :)

jonasschnelli · 2018-08-24T09:22:11Z

Note to reviewers: please review...

net: Refactor message parsing (CNetMessage), adds flexibility #14046 Refactor message parsing (CNetMessage), adds flexibility
Add HKDF_HMAC256_L32 and method to negate a private key #14047 Add HKDF_HMAC256_L32 and method to negate a private key
Enable libsecp256k1 ecdh module, add ECDH function to CKey #14049 Enable libsecp256k1 ecdh module, add ECDH function to CKey
Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050 Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh
... first (extracted commits from this PR)

luke-jr · 2018-08-29T18:20:38Z

What is the point of NODE_ENCRYPTED? Service bits shouldn't be used for mere protocol negotiation...

jonasschnelli · 2018-08-29T19:11:06Z

What is the point of NODE_ENCRYPTED? Service bits shouldn't be used for mere protocol negotiation...

This has now been discussed on IRC:
https://botbot.me/freenode/bitcoin-core-dev/2018-08-29/?msg=103889728&page=3

practicalswift · 2018-09-01T21:07:03Z

src/net_encryption.cpp

Should be “reset”?

DrahtBot · 2019-07-12T18:54:03Z

Needs rebase

fanquake · 2021-03-12T00:29:06Z

src/net.cpp

-        CNetMessage& msg = vRecvMsg.back();
+        if (vRecvMsg.empty() || vRecvMsg.back()->Complete()) {
+            if (m_encryption_handler && m_encryption_handler->ShouldCryptMsg()) {
+                vRecvMsg.emplace_back(MakeUnique<NetV2Message>(m_encryption_handler, Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));


Please use std::make_unique in new code.

…ate key 8794a4b QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli) 551d489 Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli) 3b64f85 QA: add test for CKey::Negate() (Jonas Schnelli) 463921b CKey: add method to negate the key (Jonas Schnelli) Pull request description: This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256 and length output of 32 bytes (will be required for v2 transport protocol). This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`. Including tests. This is a subset of bitcoin#14032 and a pre-requirement for the v2 transport protocol. ACKs for commit 8794a4: Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a

fanquake · 2021-08-18T12:25:27Z

My understanding is that someone else is helping with / taking over these changes, and that the BIP is still being overhauled.
I think we'll be better off with new PRs, and clean discussion when work on the implementation resumes in this repo.
Changes from here are be cherry-picked if / when needed. So I'm going to close this PR for now.

michaelfolkson · 2021-08-23T12:16:18Z

For additional context on above from @fanquake BIP 151 has been withdrawn, the replacement is BIP 324 and the point of contact for BIP 324 appears to be @dhruv from this point on.

…ate key 8794a4b QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli) 551d489 Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli) 3b64f85 QA: add test for CKey::Negate() (Jonas Schnelli) 463921b CKey: add method to negate the key (Jonas Schnelli) Pull request description: This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256 and length output of 32 bytes (will be required for v2 transport protocol). This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`. Including tests. This is a subset of bitcoin#14032 and a pre-requirement for the v2 transport protocol. ACKs for commit 8794a4: Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a

jonasschnelli added the P2P label Aug 23, 2018

practicalswift reviewed Aug 23, 2018

View reviewed changes

src/init.cpp Outdated

Copy link

Contributor

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I’m reading the logic correct then this should be ”every 10 seconds or after 10kb of data”?

practicalswift reviewed Aug 23, 2018

View reviewed changes

src/net.cpp Outdated

Copy link

Contributor

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: “treat”

practicalswift reviewed Aug 23, 2018

View reviewed changes

src/net_encryption.cpp Outdated

Copy link

Contributor

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: “therefore”?

practicalswift reviewed Aug 23, 2018

View reviewed changes

src/net_message.h Outdated

Copy link

Contributor

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: “has”?

This was referenced Aug 23, 2018

Dandelion transaction relay (BIP 156) #13947

Closed

p2p: Clarify control flow in ProcessMessage #13946

Merged

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

jonasschnelli force-pushed the 2018/08/bip151 branch from e654fd6 to 4e4aedd Compare August 23, 2018 19:06

DrahtBot mentioned this pull request Aug 23, 2018

Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

Closed

jonasschnelli mentioned this pull request Aug 23, 2018

ci: Add Appveyor CI #13964

Merged

jonasschnelli force-pushed the 2018/08/bip151 branch from 4e4aedd to 603490f Compare August 24, 2018 08:50

This was referenced Aug 24, 2018

net: Refactor message parsing (CNetMessage), adds flexibility #14046

Closed

Add HKDF_HMAC256_L32 and method to negate a private key #14047

Merged

This was referenced Aug 24, 2018

Enable libsecp256k1 ecdh module, add ECDH function to CKey #14049

Closed

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

Closed

DrahtBot added the Needs rebase label Aug 25, 2018

jonasschnelli force-pushed the 2018/08/bip151 branch from 603490f to c9b3c58 Compare August 29, 2018 13:28

DrahtBot removed the Needs rebase label Aug 29, 2018

DrahtBot mentioned this pull request Aug 29, 2018

doc: Change documentation for =0 for non-boolean options #14100

Merged

DrahtBot mentioned this pull request Aug 30, 2018

export der always compressed #14102

Closed

jonasschnelli force-pushed the 2018/08/bip151 branch from c9b3c58 to 93f64ad Compare August 31, 2018 19:47

practicalswift reviewed Sep 1, 2018

View reviewed changes

src/net_encryption.cpp Outdated

Copy link

Contributor

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be “reset”?

jonasschnelli added 13 commits May 20, 2019 12:02

Split off format specific details from CNetMessage

99c378d

Move NetMessage (NetMessageBase) to net_message.h/cpp

29a3ac1

Factor out RecordRecvBytesPerMsgCmd()

bd42fbd

==== V2 P2P implementation ====

1c36680

Add new p2p message format for encrypted communication

bae1ec2

Add v2 message transport protocol encryption with static key

d1138ba

Add v2 message transport protocol encryption handshake

3459cc9

Add and respect service flag NODE_P2P_V2

59609e6

Implement encryption rekeying

2568074

Avoid ephemeral key->pubkeys that start with net magic

3de5d24

Add encryption state and session id to getpeerinfo

c45e2a2

[QA] add encryption functional test

11f4b77

Implement v2 p2p protocol command short IDs

39bf10f

jonasschnelli force-pushed the 2018/08/bip151 branch from bdc9b75 to 39bf10f Compare May 20, 2019 10:28

DrahtBot removed the Needs rebase label May 20, 2019

DrahtBot added the Needs rebase label Jul 12, 2019

PastaPastaPasta mentioned this pull request Jul 18, 2019

Backport chacha-poly1305, prepare for V2 P2P Encrypted Messaging dashpay/dash#3032

Merged

cloudhead mentioned this pull request Jan 23, 2021

BIP 151 cloudhead/nakamoto#13

Open

fanquake reviewed Mar 12, 2021

View reviewed changes

fanquake closed this Aug 18, 2021

DeckerSU mentioned this pull request Dec 21, 2021

Implement TLS support for encrypted p2p communication GLEECBTC/komodo-daemon#522

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 24, 2022

Conversation

jonasschnelli commented Aug 23, 2018

Uh oh!

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Aug 23, 2018

Uh oh!

gmaxwell commented Aug 23, 2018

Uh oh!

jonasschnelli commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 29, 2018

Uh oh!

jonasschnelli commented Aug 29, 2018

Uh oh!

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 12, 2019

Uh oh!

fanquake Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 18, 2021

Uh oh!

michaelfolkson commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Aug 23, 2018 •

edited

Loading

jonasschnelli commented Aug 24, 2018 •

edited

Loading

michaelfolkson commented Aug 23, 2021 •

edited

Loading