test: extend the SOCKS5 Python proxy to actually connect to a destination

vasild commented at 3:30 pm on February 11, 2024: contributor

If requested, make the SOCKS5 Python proxy redirect connections to a set of given destinations. Actually act as a real proxy, connecting the client to a destination, except that the destination is not what the client asked for.

This would enable us to “connect” to Tor addresses from the functional tests.

Plus a few other minor improvements in the test framework as individual commits.

These changes are part of #29415 but they make sense on their own and would be good to have them, regardless of the fate of #29415. Also, if this is merged, that would reduce the size of #29415, thus the current standalone PR.

DrahtBot commented at 3:30 pm on February 11, 2024: contributor

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	tdb3, mzumsande, achow101
Approach ACK	jonatack
Stale ACK	pinheadmz

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Feb 11, 2024

in test/functional/test_framework/p2p.py:194 in 05f8c032b8 outdated

187@@ -188,6 +188,10 @@ def peer_connect_helper(self, dstaddr, dstport, net, timeout_factor):
188         self.on_connection_send_msg = None
189         self.recvbuf = b""
190         self.magic_bytes = MAGIC_BYTES[net]
191+        if dstport != 0:
192+            self.p2p_connected_to_node = True
193+        else:
194+            self.p2p_connected_to_node = False

Empact commented at 7:13 am on February 27, 2024:

nit: you can assign the comparison directly

vasild commented at 12:57 pm on February 29, 2024:

Done

DrahtBot added the label Needs rebase on Feb 27, 2024

vasild force-pushed on Feb 29, 2024

vasild commented at 12:57 pm on February 29, 2024: contributor

05f8c032b8...4267c206f4: rebase due to conflicts and address #29420 (review)

DrahtBot added the label CI failed on Feb 29, 2024

DrahtBot removed the label Needs rebase on Feb 29, 2024

DrahtBot removed the label CI failed on Feb 29, 2024

vasild force-pushed on Mar 18, 2024

vasild commented at 4:39 pm on March 18, 2024: contributor

4267c206f4...9c50b2fe44: pick changes due to suggestions from https://github.com/bitcoin/bitcoin/pull/29415

in test/functional/test_framework/p2p.py:226 in 9a9b545c7f outdated

221+        info = transport.get_extra_info("socket")
222+        us = info.getsockname()
223+        them = info.getpeername()
224+        logger.debug(f"Connected: us={us[0]}:{us[1]}, them={them[0]}:{them[1]}")
225+        self.dstaddr = them[0]
226+        self.dstport = them[1]

pinheadmz commented at 6:26 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/9a9b545c7fc044242c4dd8a4916b2ba13b3ea850

Why do we need to replace these values? I changed this to:

0        assert self.dstaddr == them[0]
1        assert self.dstport == them[1]

…and all the p2p tests still passed

vasild commented at 3:40 pm on May 7, 2024:

I set dstaddr and dstport here in connection_made() because when accepting connections (from the python node point of view) they are set to "0" and 0 respectively (in master):

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186

When I add those asserts I get:

0    assert self.dstaddr == them[0]
1           ^^^^^^^^^^^^^^^^^^^^^^^
2AssertionError

vasild commented at 10:23 am on July 2, 2024:

from comment #29415 (review):

Why didn’t we need to do this on master? The old log message was Connected & Listening: 127.0.0.1:15689 not Connected & Listening: 0:0

When accepting connections (from the python node point of view), then indeed 0:0 is printed (on master):

0$ ./test/functional/p2p_addr_relay.py -l debug --nocleanup
1$ ./test/functional/combine_logs.py |grep 'Connected & L'
2...
3 test  2024-07-02T10:18:18.494000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
4 test  2024-07-02T10:18:18.609000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
5 test  2024-07-02T10:18:20.556000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0 
6 test  2024-07-02T10:18:20.719000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0 
7 test  2024-07-02T10:18:20.934000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
8 test  2024-07-02T10:18:22.265000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0

in test/functional/test_framework/socks5.py:147 in 1e76be0431 outdated

142+                    while not done:
143+                        rlist, _, xlist = select.select(sockets, [], sockets)
144+                        if len(xlist) > 0:
145+                            raise IOError("Exceptional condition on socket")
146+                        for s in rlist:
147+                            data = s.recv(4096)

pinheadmz commented at 6:48 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

any reason for this particular buffer size? some quick research suggests that 4kB and 8kB are common values for this, I’m just wondering if it really affects us.

vasild commented at 3:44 pm on May 7, 2024:

No, I just picked some value that looks reasonable, anything would work. For example 1 or 5 would still work but do not look reasonable because they will cause a lot of iteration loops.

in test/functional/test_framework/socks5.py:54 in 1e76be0431 outdated

49+        #     "node": python_p2p_node,
50+        #     "requested_to_addr": "the_client_asked_to_connect_to_this_addr.onion",
51+        # }
52+        # Redirect the i'th connection to destinations[i] to_addr:to_port
53+        self.destinations = []
54+        self.destinations_used = 0

pinheadmz commented at 7:54 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

Instead of the counter, you could also use dest = self.destiantions.pop() below and then log your warning once the array is empty

vasild commented at 3:46 pm on May 7, 2024:

I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).

in test/functional/test_framework/socks5.py:143 in 1e76be0431 outdated

138+                    self.conn.setblocking(False)
139+                    conn_to.setblocking(False)
140+                    sockets = [self.conn, conn_to]
141+                    done = False
142+                    while not done:
143+                        rlist, _, xlist = select.select(sockets, [], sockets)

pinheadmz commented at 7:55 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

had to read the docs on this one but I agree it is correct 👍

pinheadmz approved

pinheadmz commented at 8:18 pm on April 17, 2024: member

ACK 9c50b2fe44e4744204c51b82e86174db1d749cfe

Reviewed each commit and left some questions below. Built and ran all unit and functional tests on arm/macos. I am already familiar with this feature from #29415 and have run those tests as well.

One nitty nitty nit nit: commit message 9a9b545c7fc044242c4dd8a4916b2ba13b3ea850 uses c++ syntax instead of python ;-)

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 9c50b2fe44e4744204c51b82e86174db1d749cfe
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmYgKwwACgkQ5+KYS2KJ
 7yTptJQ/+OksRFOpcFGtbEYIaaAaiWAlLJVBQYa2ljpRvSxecoWqmcgP6wvaXGSsA
 8SxBiu7Y+1fZqBEaRLeOLm7GtZiZesc87Cu2BJGM5SvxLkiF6l7YqtwISq51Z0cZu
 9C15s/eSexvpLFp+tGvlcm9au5iZShU7nGMVNAEz5vfFUJOQBDVDJ1mib1hxaLRaV
104U97uoYQcOVZDz+fdzisekP1wpklXZTDQiS9grbHMBmqeT45ugxgUEC+DVWFtm5W
1101JBblBPNLvlOFiNpHCy0MUNXtg7n1ER2ee677ZTP05ck9hOga0gu8g8doemvZ91
12QPs2nLq0dHvaFtlYl2N3Ce3aRBX6I3qfegKWmrBWm54KB3mOSrV3OP2j0UMUGmcM
13nwPQruUg+GnBsEWK9RgvLAmSFbkGEJzS0nqFfOkRLLEwNWAzMUYAooXs4J7xLVcA
14AR8ncBnz9zVBkdhrHbT8wYcf22N/W+k3fwxCgTxHPiAt10UXAzkdfnRfwjjQG7NU
15xmP20eAFsE9FWOnfkVIh/VCzcgS+wT629gaVYJqF9hX16K9uKH2AuOQeCLnRiX3D
16E1B1YnspQklZAy0ejc2BZEuwBc+MjtSEowTPK9ENT8NJoEO2y3YrnGcbXThyVrBQ
17yvLU1dfljFfSq2seaNP4fpKJT8cKXAdCHj/HlowVsYw9hDs/PKU=
18=VgCO
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot added the label Needs rebase on Jul 9, 2024

vasild force-pushed on Jul 24, 2024

vasild commented at 8:08 am on July 24, 2024: contributor

9c50b2fe44...a179b1cbf5: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jul 24, 2024

vasild force-pushed on Aug 2, 2024

vasild commented at 10:08 am on August 2, 2024: contributor

a179b1cbf5...5956668ecd: rebase and pick changes from #29415 (comment), more specifically:

Change the extension of the SOCKS5 proxy - instead of being given a set of predefined destinations to redirect the connections to, it is now given a callback function which returns the destination where to redirect, or it can decide to return None asking the SOCKS5 proxy to close the connection (default behavior). This is more flexible.

test: improve debug log message from P2PConnection::connection_made()

This is used in both cases - TCP server (accept) and TCP client (connect).
The message "Connected & Listening address:port" is confusing.

Print both ends of the TCP connection.

ba621ffb9c

vasild force-pushed on Sep 3, 2024

vasild commented at 10:33 am on September 3, 2024: contributor

5956668ecd...f93fab4c58: rebase to pick CMake (even though this PR only contains changes to .py files)

DrahtBot added the label CI failed on Sep 5, 2024

DrahtBot removed the label CI failed on Sep 11, 2024

achow101 requested review from laanwj on Oct 15, 2024

achow101 requested review from mzumsande on Oct 15, 2024

achow101 commented at 3:45 pm on October 15, 2024: member

cc @tdb3

in test/functional/test_framework/socks5.py:152 in f93fab4c58 outdated

147+                    with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to:
148+                        self.conn.setblocking(False)
149+                        conn_to.setblocking(False)
150+                        sockets = [self.conn, conn_to]
151+                        done = False
152+                        while not done:

laanwj commented at 9:04 am on October 16, 2024:

Maybe factor this shovel-from-socketa-to-socketb (and vice versa) loop out to a function in netutil as well.

vasild commented at 11:23 am on October 22, 2024:

I moved it to its own function, but in test/functional/test_framework/socks5.py because it uses the newly created sendall() (due to the suggestion below) which is very similar to recvall() which already exists in socks5.py.

I guess if this is to be moved to test/functional/test_framework/netutil.py, then all 3 of recvall(), sendall() and forward_sockets() should be moved. Leave it as is or move to netutil.py?

laanwj commented at 10:13 am on October 25, 2024:

thanks! factoring it out was more fo a readablity thing, i don’t really mind if it’s in netutil i’m fine with leaving it where it is, if some other code needs it it can always be moved to a more general place

test: extend the SOCKS5 Python proxy to actually connect to a destination

If requested, make the SOCKS5 Python proxy redirect each connection to a
given destination. Actually act as a real proxy, connecting the
client to a destination, except that the destination is not what the
client asked for.

This would enable us to "connect" to Tor addresses from the functional
tests.

ebe42c00aa

test: support WTX INVs from P2PDataStore and fix a comment 22cd0e888c

test: set P2PConnection.p2p_connected_to_node in peer_connect_helper()

Set `P2PConnection.p2p_connected_to_node` in
`P2PConnection.peer_connect_helper()` instead of
`TestNode.add_p2p_connection()` and
`TestNode.add_outbound_p2p_connection()`.

This way tests can create an instance of `P2PConnection` and use
`P2PConnection.peer_connect_helper()` directly.

57529ac4db

in test/functional/test_framework/socks5.py:162 in f93fab4c58 outdated

157+                                data = s.recv(4096)
158+                                if data is None or len(data) == 0:
159+                                    done = True
160+                                    break
161+                                if s == self.conn:
162+                                    conn_to.sendall(data)

laanwj commented at 9:08 am on October 16, 2024:

i don’t think sendall can be guaranteed with non-blocking sockets, it will fail if it gets EAGAIN https://stackoverflow.com/questions/6240737/python-socket-sendall-function

vasild commented at 11:38 am on October 22, 2024:

Ok, sendall() doc says “On error, an exception is raised, and there is no way to determine how much data, if any, was successfully sent” so this is useless for us. The doc says as well that since Python 3.5 the method will retry if the syscall is interrupted, but mentions nothing about EAGAIN or EWOULDBLOCK.

So I followed https://docs.python.org/3/howto/sockets.html#using-a-socket and used the basic send() with a manual retry loop, like in that example.

Diving further into Python source code, sendall() calls sock_call_ex() which is retrying on EWOULDBLOCK and EAGAIN. This is not documented explicitly anywhere, so I decided to ignore it and go with the manual retry loop. Also, send() (in addition to sendall()) ends up with that retry, so maybe the manual retry loop will always loop just once.

in test/functional/test_framework/socks5.py:14 in b35e74d826 outdated

 9 import threading
10 import queue
11 import logging
12 
13+from .netutil import (
14+    format_addr_port

tdb3 commented at 3:31 pm on October 20, 2024:

non-blocking femto nit: If this file is touched again, could add a trailing comma to prevent the line from being changed if another import from netutil is needed.

vasild force-pushed on Oct 22, 2024

vasild commented at 11:20 am on October 22, 2024: contributor

f93fab4c58...57529ac4db: address suggestions

in test/functional/test_framework/socks5.py:184 in 57529ac4db

179+                dest = self.serv.conf.destinations_factory(requested_to_addr, port)
180+                if dest is not None:
181+                    logger.debug(f"Serving connection to {requested_to}, will redirect it to "
182+                                 f"{dest['actual_to_addr']}:{dest['actual_to_port']} instead")
183+                    with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to:
184+                        forward_sockets(self.conn, conn_to)

vasild commented at 11:51 am on October 22, 2024:

Following discussion with @tdb3, how do we know this code even works and is not totally broken? That is a reasonable question because this PR does not demonstrate that this works.

#29415 (which includes this PR) exercises the newly added functionality. The tests from #29415 that use this depend on the private broadcast functionality so can’t be included here.

tdb3 approved

tdb3 commented at 0:57 am on October 23, 2024: contributor

CR and test ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

Checked with a hacky little sanity test where a connection was made to the proxy on 127.0.0.1:10000 (set up to detour to 127.0.0.1:10001). A small echo server listened on 10001 (echoing back “testack”). The new detouring functionality of the proxy seemed to work as expected.

0build/test/functional/feature_framework_unit_tests.py

 0diff --git a/test/functional/feature_framework_unit_tests.py b/test/functional/feature_framework_unit_tests.py
 1index 14d83f8a70..9bbfcf987e 100755
 2--- a/test/functional/feature_framework_unit_tests.py
 3+++ b/test/functional/feature_framework_unit_tests.py
 4@@ -29,6 +29,7 @@ TEST_FRAMEWORK_MODULES = [
 5     "script",
 6     "script_util",
 7     "segwit_addr",
 8+    "socks5",
 9     "wallet_util",
10 ]
11 
12diff --git a/test/functional/test_framework/socks5.py b/test/functional/test_framework/socks5.py
13index 3387c8a1fe..e18d62ee3c 100644
14--- a/test/functional/test_framework/socks5.py
15+++ b/test/functional/test_framework/socks5.py
16@@ -7,6 +7,7 @@
17 import select
18 import socket
19 import threading
20+import unittest
21 import queue
22 import logging
23 
24@@ -230,3 +231,28 @@ class Socks5Server():
25         s.connect(self.conf.addr)
26         s.close()
27         self.thread.join()
28+
29+class TestFrameworkSocks5(unittest.TestCase):
30+    def test_destinations(self):
31+        socks_conf = Socks5Configuration()
32+        socks_conf.addr = ('127.0.0.1', 10000)
33+        socks_conf.unauth = True
34+        socks_conf.auth = False
35+        socks_conf.destinations_factory = lambda addr, port : {"actual_to_addr": '127.0.0.1', "actual_to_port": 10001}
36+        socks_serv = Socks5Server(socks_conf)
37+        socks_serv.start()
38+        # send initial greeting, receive response
39+        client_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
40+        client_sock.connect(('127.0.0.1', 10000))
41+        sendall(client_sock, bytearray([0x05, 0x01, 0x00]))
42+        response = recvall(client_sock, 2)
43+        assert response == bytearray([0x05, 0x00])
44+        # send connect (to 127.0.0.1:10000), receive response
45+        sendall(client_sock, bytearray([0x05, 0x01, 0x00, 0x03, 0x09, 0x31, 0x32, 0x37, 0x2e, 0x30, 0x2e, 0x30, 0x2e, 0x31, 0x27, 0x10]))
46+        response = recvall(client_sock, 10)
47+        assert response == bytearray([0x05, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00])
48+        # send "test" and observe response on 10001 (e.g. with echo server)
49+        sendall(client_sock, b'test')
50+        response = recvall(client_sock, 7)
51+        assert response == b'testack'
52+        socks_serv.stop()

 0import socket
 1
 2HOST = "127.0.0.1"
 3PORT = 10001
 4
 5with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
 6    s.bind((HOST, PORT))
 7    s.listen()
 8    conn, addr = s.accept()
 9    with conn:
10        while True:
11            data = conn.recv(1024)
12            if not data:
13                break
14            conn.sendall(data + b'ack')

DrahtBot requested review from pinheadmz on Oct 23, 2024

in test/functional/test_framework/socks5.py:186 in ebe42c00aa outdated

181+                    logger.debug(f"Serving connection to {requested_to}, will redirect it to "
182+                                 f"{dest['actual_to_addr']}:{dest['actual_to_port']} instead")
183+                    with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to:
184+                        forward_sockets(self.conn, conn_to)
185+                else:
186+                    logger.debug(f"Closing connection to {requested_to}: the destinations factory returned None")

mzumsande commented at 6:18 pm on October 24, 2024:

This log isn’t accurate if keep_alive is set, because then we’ll log that we close the connection but actually stay connected.

vasild commented at 11:08 am on November 6, 2024:

I think this is worth a followup. If you agree, I would open one, or just leave it as it is? What to reword it to? “Ending handling of connection to …”? Or log different messages based on self.serv.keep_alive?

mzumsande commented at 5:42 pm on November 6, 2024:

yes, happy to ack that follow-up. I think everything is fine, but my suggestion would be something like “Can’t serve the connection to…” because we already log “Serving connection to…” in the success case. And maybe add another log entry “Keeping connection alive” else clause in the finally block a few lines below to avoid conditional log messages.

vasild commented at 1:24 pm on November 7, 2024:

Done in https://github.com/bitcoin/bitcoin/pull/31239

in test/functional/test_framework/socks5.py:188 in ebe42c00aa outdated

183+                    with socket.create_connection((dest["actual_to_addr"], dest["actual_to_port"])) as conn_to:
184+                        forward_sockets(self.conn, conn_to)
185+                else:
186+                    logger.debug(f"Closing connection to {requested_to}: the destinations factory returned None")
187+            else:
188+                logger.debug(f"Closing connection to {requested_to}: no destinations factory")

mzumsande commented at 6:19 pm on October 24, 2024:

same as 2 lines above

mzumsande commented at 9:27 pm on October 24, 2024: contributor

Code review / tested ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

This is pretty cool, and could be used in other places than just for the private transaction relay (e.g. whenever we’d like to have an onion outbound peer in a functional test and exchange messages with it)

I tested this by extending feature_anchors.py to use a destinations_factory and checked that it completes the version handshake and behaves like a normal connection.

jonatack commented at 4:36 pm on October 25, 2024: member

Approach ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

achow101 commented at 7:19 pm on October 29, 2024: member

ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

DrahtBot requested review from jonatack on Oct 29, 2024

achow101 merged this on Oct 29, 2024

achow101 closed this on Oct 29, 2024

vasild deleted the branch on Nov 6, 2024

test: extend the SOCKS5 Python proxy to actually connect to a destination #29420

Code Coverage

Reviews

Conflicts