test/BIP324: functional tests for v2 P2P encryption by stratospher · Pull Request #24748 · bitcoin/bitcoin

stratospher · 2022-04-03T20:14:51Z

This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.

commits overview

introduces a new class EncryptedP2PState to store the keys, functions for performing the initial v2 handshake and encryption/decryption.
this class is used by P2PConnection in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection)
- v2_state is the object of class EncryptedP2PState in P2PConnection used to store its keys, session-id etc.
- a node advertising support for v2 P2P is different from a node actually supporting v2 P2P (differ when false advertisement of services occur)
  - introduce a boolean variable supports_v2_p2p in P2PConnection to denote if it supports v2 P2P.
  - introduce a boolean variable advertises_v2_p2p to denote whether P2PConnection which mimics peer behaviour advertises V2 P2P support. Default option is False.
- In the test framework, you can create Inbound and Outbound connections to TestNode
  1. During Inbound Connections, P2PConnection is the initiator [TestNode <--------- P2PConnection]
    - Case 1:
      - if the TestNode advertises/signals v2 P2P support (means self.nodes[i] set up with "-v2transport=1"), different behaviour will be exhibited based on whether:
        
        P2PConnection supports v2 P2P
        
        P2PConnection does not support v2 P2P
      - In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if P2PConnection should support v2 P2P or not. So supports_v2_p2p boolean variable is used as an option to enable support for v2 P2P in P2PConnection.
      - Since the TestNode advertises v2 P2P support (using "-v2transport=1"), our initiator P2PConnection would send:
        
        (if the P2PConnection supports v2 P2P) ellswift + garbage bytes to initiate the connection
        
        (if the P2PConnection does not support v2 P2P) version message to initiate the connection
    - Case 2:
      - if the TestNode doesn't signal v2 P2P support; P2PConnection being the initiator would send version message to initiate a connection.
  2. During Outbound Connections [TestNode --------> P2PConnection]
    - initiator TestNode would send:
      - (if the P2PConnection advertises v2 P2P) ellswift + garbage bytes to initiate the connection
      - (if the P2PConnection advertises v2 P2P) version message to initiate the connection
    - Suppose P2PConnection advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario)
      - TestNode sends ellswift + garbage bytes
      - P2PConnection receives but can't process it and disconnects.
      - TestNode then tries using v1 P2P and sends version message
      - P2PConnection receives/processes this successfully and they communicate on v1 P2P
the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC
includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious
intermediary)

run the tests

functional test - test/functional/p2p_v2_encrypted.py test/functional/p2p_v2_earlykeyresponse.py

I'm also super grateful to @ dhruv for his really valuable feedback on this branch.
Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md

DrahtBot · 2022-04-05T01:31:07Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, theStack, naumenkogs, glozow
Concept ACK	jonatack, brunoerg
Approach ACK	sr-gi

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29279 (test: p2p: check disconnect due to lack of desirable service flags by theStack)
#29016 (RPC: add new listmempooltransactions by niftynei)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)

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.

jonatack · 2022-04-07T07:03:29Z

Concept ACK

brunoerg · 2022-04-08T12:54:02Z

Concept ACK

stratospher · 2022-10-03T17:24:02Z

converting this PR into a draft. I'll push the updated version which includes the new spec changes in BIP 324 soon.

Instantiate this object when the connection supports v2 P2P transport protocol. - When a P2PConnection is opened, perform initiate_v2_handshake() if the connection is an initiator. application layer messages are only sent after the initial v2 handshake is over (for both initiator and responder).

Messages are built, encrypted and sent over the socket in v2 connections. If a race condition happens between python's main thread and p2p thread with both of them trying to send a message, it's possible that the messages get encrypted with wrong keystream. Messages are built and sent over the socket in v1 connections. So there's no problem if messages are sent in the wrong order. Co-authored-by: Martin Zumsande <[email protected]> Co-authored-by: theStack <[email protected]>

…te mismatch - When a v2 TestNode makes an outbound connection to a P2PInterface node which doesn't support v2 but is advertised as v2 by some malicious intermediary, the TestNode sends 64 bytes ellswift. The v1 node doesn't understand this and disconnects. Then the v2 TestNode reconnects by sending a v1/version message.

- Add an optional `supports_v2_p2p` parameter to specify if the inbound and outbound connections support v2 P2P protocol. - In the `addconnection_callback` which gets called when creating outbound connections, call the `addconnection` RPC with v2 P2P protocol support enabled.

Also allow P2PConnection::send_message() to send decoy messages for writing tests.

…P 324 - A node initiates a v2 connection by sending 64 bytes ellswift - In BIP 324 "The responder waits until one byte is received which does not match the V1_PREFIX (16 bytes consisting of the network magic followed by "version\x00\x00\x00\x00\x00".)" - It's possible that the 64 bytes ellswift sent by an initiator starts with a prefix of V1_PREFIX - Example form of 64 bytes ellswift could be: 4 bytes network magic + 60 bytes which aren't prefixed with remaining V1_PREFIX - We test this behaviour: - when responder receives 4 byte network magic -> no response received by initiator - when first mismatch happens -> response received by initiator

stratospher · 2024-01-25T05:59:27Z

There are many more tests possible, right? E.g. sending more than 4095 bytes of garbage.
I assume the goal here wasn't to cover everything?

yes! goal here was to just introduce support for v2 in the test framework. we need more tests after this.

mzumsande

Code Review ACK bc9283c

theStack

Code-review ACK bc9283c

Went through another review round and only found some minor nits, mostly naming suggestions. If you retouch (or follow-up material):

should use multi-line imports (see style guidelines in test/functional/README.md)
commit 382894c has a leading space in the commit subject

test/functional/test_framework/v2_p2p.py

test/functional/test_framework/p2p.py

naumenkogs · 2024-01-29T09:49:20Z

ACK bc9283c

test/functional/p2p_v2_earlykeyresponse.py

glozow

ACK bc9283c

Did some light code review of the P2PConnection functions, mutation testing of EncryptedP2PState, and manually changing other functional tests to use v2 connections.

src/rpc/net.cpp

maflcko · 2024-01-30T14:29:36Z

The test fails for me:

146/294 - p2p_v2_earlykeyresponse.py failed, Duration: 1 s

stdout:
2024-01-30T13:33:10.051000Z TestFramework (INFO): PRNG seed is: 5518845862592986813
2024-01-30T13:33:10.052000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240130_142948/p2p_v2_earlykeyresponse_133
2024-01-30T13:33:10.370000Z TestFramework (INFO): Sending ellswift bytes in parts to ensure that response from responder is received only when
2024-01-30T13:33:10.370000Z TestFramework (INFO): ellswift bytes have a mismatch from the 16 bytes(network magic followed by "version\x00\x00\x00\x00\x00")
2024-01-30T13:33:10.371000Z TestFramework (INFO): Sending first 4 bytes of ellswift which match network magic
2024-01-30T13:33:10.371000Z TestFramework (INFO): If a response is received, assertion failure would happen in our custom data_received() function
2024-01-30T13:33:10.429000Z TestFramework (INFO): Sending remaining ellswift and garbage which are different from V1_PREFIX. Since a response is
2024-01-30T13:33:10.429000Z TestFramework (INFO): expected now, our custom data_received() function wouldn't result in assertion failure
2024-01-30T13:33:10.480000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:12596 due to 
2024-01-30T13:33:10.518000Z TestFramework (INFO): successful disconnection when MITM happens in the key exchange phase
2024-01-30T13:33:10.519000Z TestFramework (INFO): Stopping nodes
2024-01-30T13:33:10.625000Z TestFramework (INFO): Cleaning up /tmp/test_runner_₿_🏃_20240130_142948/p2p_v2_earlykeyresponse_133 on exit
2024-01-30T13:33:10.625000Z TestFramework (INFO): Tests successful


stderr:
Fatal error: protocol.data_received() call failed.
protocol: <__main__.PeerEarlyKey object at 0x7fe7925dccb0>
transport: <_SelectorSocketTransport fd=10 read=polling write=<idle, bufsize=0>>
Traceback (most recent call last):
  File "python3.12/asyncio/selector_events.py", line 1023, in _read_ready__data_received
    self._protocol.data_received(data)
  File "test/functional/p2p_v2_earlykeyresponse.py", line 60, in data_received
    assert self.v2_state.can_data_be_received and not self.v2_state.send_net_magic
AssertionError

stratospher · 2024-01-30T15:32:03Z

hmm weird, looking into it.

i also have a WIP branch which uses this test file and adds more v2 tests for sending excess garbage bytes, wrong garbage terminator, incorrect ellswift and non-empty version packet.

sr-gi · 2024-01-30T21:34:47Z

WIP branch

Is this consistently failing for you (or can you at least reproduce it)? I can see it also failing on some of the current open PRs but I can not get my head around why (nor can I reproduce it).

This should only happen if PeerEarlyKey receives any data from the node before sending anything at all, or after sending the network magic but before sending the remaining initial bytes + garbage.

To my understanding, the node should not have been able to parse enough data to be replying (with either v1 or v2) 😕

In case you can reproduce it, it may be useful to log what data_received is receiving

mzumsande · 2024-01-30T23:06:32Z

The test fails for me:

see #29352 for a fix (and a way to reproduce it).

DrahtBot added Build system P2P RPC/REST/ZMQ Upstream Utils/log/libs labels Apr 3, 2022

stratospher force-pushed the p2p-encryption-test branch from 24f028a to 9880108 Compare April 4, 2022 14:10

DrahtBot mentioned this pull request Apr 8, 2022

Update libsecp256k1 subtree to current master #24792

Merged

DrahtBot added the Needs rebase label Apr 8, 2022

stratospher marked this pull request as draft October 3, 2022 17:17

stratospher and others added 11 commits January 25, 2024 11:09

[test] Perform initial v2 handshake

05bddb2

[test] Read v2 P2P messages

5b91fb1

[test] Build v2 P2P messages

a94e350

[test] Ignore BIP324 decoy messages

4115cf9

Also allow P2PConnection::send_message() to send decoy messages for writing tests.

[test] Add functional tests to test v2 P2P behaviour

ba73735

[test] Check whether v2 TestNode performs downgrading

ffe6a56

mzumsande reviewed Jan 25, 2024

View reviewed changes

theStack approved these changes Jan 25, 2024

View reviewed changes

test/functional/test_framework/v2_p2p.py Show resolved Hide resolved

test/functional/test_framework/v2_p2p.py Show resolved Hide resolved

test/functional/test_framework/p2p.py Show resolved Hide resolved

test/functional/test_framework/p2p.py Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 26, 2024

test: p2p: check disconnect due to lack of desirable service flags #29279

Merged

naumenkogs reviewed Jan 29, 2024

View reviewed changes

test/functional/p2p_v2_earlykeyresponse.py Outdated Show resolved Hide resolved

glozow reviewed Jan 29, 2024

View reviewed changes

maflcko reviewed Jan 30, 2024

View reviewed changes

src/rpc/net.cpp Show resolved Hide resolved

mzumsande mentioned this pull request Jan 30, 2024

net: enable v2transport by default #29347

Merged

mzumsande mentioned this pull request Jan 30, 2024

test: fix intermittent failure in p2p_v2_earlykeyresponse #29352

Merged

stratospher mentioned this pull request Jan 31, 2024

test: make v2transport arg in addconnection mandatory and few cleanups #29356

Merged

mzumsande mentioned this pull request Jan 31, 2024

test: use v2 everywhere for P2PConnection if --v2transport is enabled #29358

Merged

stratospher mentioned this pull request May 17, 2024

test/BIP324: disconnection scenarios during v2 handshake #29431

Merged

mzumsande mentioned this pull request Jul 30, 2024

tests: the python test framework mixes/confuses messages to different peers #30543

Closed

1 task

kwvg mentioned this pull request Oct 23, 2024

backport: merge bitcoin#27653, #24748, #29352, #29372, #29460, #29358, #29511, #29390, #29431, bitcoin-core/gui#788 (BIP324 backports: part 4) dashpay/dash#6330

Merged

5 tasks

Conversation

stratospher commented Apr 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

commits overview

run the tests

Uh oh!

DrahtBot commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

jonatack commented Apr 7, 2022

Uh oh!

brunoerg commented Apr 8, 2022

Uh oh!

stratospher commented Oct 3, 2022

Uh oh!

stratospher commented Jan 25, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Jan 29, 2024

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jan 30, 2024

Uh oh!

stratospher commented Jan 30, 2024

Uh oh!

sr-gi commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Jan 30, 2024 • 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!

16 participants

stratospher commented Apr 3, 2022 •

edited

Loading

DrahtBot commented Apr 5, 2022 •

edited

Loading

sr-gi commented Jan 30, 2024 •

edited

Loading

mzumsande commented Jan 30, 2024 •

edited

Loading