-
Notifications
You must be signed in to change notification settings - Fork 38.1k
tor: respect non-onion -onlynet= for outgoing Tor connections #22651
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
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
#!/usr/bin/env python3 | ||
# Copyright (c) 2021-2021 The Bitcoin Core developers | ||
# Distributed under the MIT software license, see the accompanying | ||
# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
""" | ||
Test -onlynet configuration option. | ||
""" | ||
|
||
from test_framework.basic_server import ( | ||
BasicServer, | ||
tor_control, | ||
) | ||
from test_framework.test_framework import BitcoinTestFramework | ||
from test_framework.util import ( | ||
assert_equal, | ||
) | ||
|
||
|
||
class OnlynetTest(BitcoinTestFramework): | ||
def set_test_params(self): | ||
self.tor_control_server = BasicServer(bind=('127.0.0.1', 0), | ||
response_generator=tor_control) | ||
port = self.tor_control_server.listen_port | ||
self.extra_args = [ | ||
vasild marked this conversation as resolved.
Show resolved
Hide resolved
|
||
['-onlynet=ipv4', f'-torcontrol=127.0.0.1:{port}', '-listenonion=1'], | ||
['-onlynet=ipv4', f'-torcontrol=127.0.0.1:{port}', '-listenonion=1', '-onion=127.0.0.1:9050'], | ||
['-onlynet=ipv4', f'-torcontrol=127.0.0.1:{port}', '-listenonion=1', '-proxy=127.0.0.1:9050'], | ||
] | ||
vasild marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.num_nodes = len(self.extra_args) | ||
|
||
def onion_is_reachable(self, node): | ||
for net in self.nodes[node].getnetworkinfo()['networks']: | ||
if net['name'] == 'onion': | ||
return net['reachable'] | ||
return False | ||
|
||
def run_test(self): | ||
# Node 0 would fail the check without 5384c98993fed5480719e1c3380c0c66263daa7e. | ||
# Nodes 1 and 2 pass with or without that commit, they are here just to ensure | ||
# behavior does not change unintentionally. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe use a docstring and describe the commit, feel free to ignore - # Node 0 would fail the check without c591379eb03381145c85ee5760ce829be337a749.
- # Nodes 1 and 2 pass with or without that commit, they are here just to ensure
- # behavior does not change unintentionally.
+ """
+ - The test of node 0 would fail without commit c591379eb0,
+ "tor: respect non-onion -onlynet= for outgoing Tor connections."
+
+ - The tests of nodes 1 and 2 pass with or without that commit;
+ they ensure that behavior does not change unintentionally.
+ """ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving it as is. |
||
for node, expected_reachable in [[0, False], [1, True], [2, True]]: | ||
self.log.info(f'Test node {node} {self.extra_args[node]}') | ||
assert_equal(self.onion_is_reachable(node), expected_reachable) | ||
|
||
|
||
if __name__ == '__main__': | ||
OnlynetTest().main() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
#!/usr/bin/env python3 | ||
# Copyright (c) 2021-2021 The Bitcoin Core developers | ||
# Distributed under the MIT software license, see the accompanying | ||
# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
""" | ||
A basic TCP server that responds to requests based on a given/custom criteria. | ||
""" | ||
|
||
import socket | ||
import threading | ||
|
||
|
||
class BasicServer: | ||
def __init__(self, *, bind, response_generator): | ||
""" | ||
:param bind: Listen address-port tuple, for example ("127.0.0.1", 80). | ||
If the port is 0 then a random available one will be chosen and it | ||
vasild marked this conversation as resolved.
Show resolved
Hide resolved
|
||
can be found later in the listen_port property. | ||
:param response_generator: A callback function, supplied with the | ||
request. It returns a reply that is to be send to the client either as | ||
a string or as an array which is joined into a single string. If it | ||
returns None, then the connection is closed by the server. | ||
""" | ||
# create_server() is only in Python 3.8. Replace the below 4 lines once only >=3.8 is supported. | ||
# https://docs.python.org/3/library/socket.html#socket.create_server | ||
# self.listen_socket = socket.create_server(address=bind, reuse_port=True) | ||
self.listen_socket = socket.socket(socket.AF_INET) | ||
self.listen_socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
self.listen_socket.bind(bind) | ||
self.listen_socket.listen() | ||
|
||
self.listen_port = self.listen_socket.getsockname()[1] | ||
|
||
self.response_generator = response_generator | ||
self.accept_loop_thread = threading.Thread(target=self.accept_loop, name="accept loop", daemon=True) | ||
self.accept_loop_thread.start() | ||
|
||
def accept_loop(self): | ||
try: | ||
while True: | ||
(accepted_socket, _) = self.listen_socket.accept() | ||
t = threading.Thread(target=self.process_incoming, args=(accepted_socket,), name="process incoming", daemon=True) | ||
t.start() | ||
except: | ||
# The accept() method raises when listen_socket is closed from the destructor. | ||
pass | ||
|
||
def process_incoming(self, accepted_socket): | ||
while True: | ||
request = accepted_socket.makefile().readline() | ||
response = self.response_generator(request=request.rstrip()) | ||
response_separator = '\r\n' | ||
if response is None: | ||
# End of communication. | ||
accepted_socket.close() | ||
break | ||
elif isinstance(response, list): | ||
response_str = response_separator.join(response) | ||
else: | ||
response_str = response | ||
response_str += response_separator | ||
|
||
totalsent = 0 | ||
while totalsent < len(response_str): | ||
sent = accepted_socket.send(bytes(response_str[totalsent:], 'utf-8')) | ||
if sent == 0: | ||
accepted_socket.close() | ||
raise RuntimeError("connection broken: socket.send() returned 0") | ||
totalsent += sent | ||
|
||
def __del__(self): | ||
self.listen_socket.close() | ||
self.accept_loop_thread.join() | ||
|
||
|
||
def tor_control(*, request): | ||
""" | ||
https://gitweb.torproject.org/torspec.git/tree/control-spec.txt | ||
""" | ||
if request == 'PROTOCOLINFO 1': | ||
return ['250-PROTOCOLINFO 1', | ||
'250-AUTH METHODS=NULL', | ||
'250 OK' | ||
] | ||
elif request == 'AUTHENTICATE': | ||
return '250 OK' | ||
elif request.startswith('ADD_ONION '): | ||
return ['250-ServiceID=2g5qfdkn2vvcbqhzcyvyiitg4ceukybxklraxjnu7atlhd22gdwywaid', | ||
'250-PrivateKey=123456', | ||
'250 OK', | ||
] | ||
else: | ||
return None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
You suggested this one earlier and I deliberately skipped it but did not explain why.
Here we are testing whether the result of
GetArg()
equals to its second argument, so it is better to keep that obvious, e.g.GetArg(..., X) == X
.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.
Because
==
will generate/call string comparison code, instead of just checking string length usingempty()
. Better just use specialized member functions instead of some kind "generic" comparison.Another example is using
clear()
vs= ""
: https://www.youtube.com/watch?v=3X9qK7HWxjkThere 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 readability trumps micro-optimization here, leaving it as is.
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 not micro-optimization, but avoiding unnecessary possible premature pessimization, see: https://stackoverflow.com/questions/15875252/premature-optimization-and-premature-pessimization-related-to-c-coding-standar/32269630#32269630
It's not optimization as it's not something more complex, harder to write or to read, not something uncommon.