June 2018 - tor-commits - lists.torproject.org

[stem/master] Reword hardcoded VERSIONS cell comment
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 751e528794cb1fa40212fbe5994bab5bccabc51a Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 15:22:34 2018 -0700 Reword hardcoded VERSIONS cell comment Just pedantic twiddling on my part. I'd like to avoid citing tor-spec section numbers if possible since those might shift. --- stem/client/__init__.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index de952996..a12959f7 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -88,14 +88,14 @@ class Relay(object): else: raise - # per tor-spec.txt (section 3): - # "CIRCID_LEN is 2 for link protocol versions 1, 2, and 3. - # CIRCID_LEN is 4 for link protocol version 4 or higher. - # The first VERSIONS cell, and any cells sent before the - # first VERSIONS cell, always have CIRCID_LEN == 2 for backward - # compatibility." - - # thus we arbitrarily select link_protocol=2 to accomplish this + # To negotiate our link protocol the first VERSIONS cell is expected to use + # a circuit ID field size from protocol version 1-3 for backward + # compatibility... + # + # The first VERSIONS cell, and any cells sent before the + # first VERSIONS cell, always have CIRCID_LEN == 2 for backward + # compatibility. + conn.send(stem.client.cell.VersionsCell(link_protocols).pack(2)) response = conn.recv()

1 0

[stem/master] Better handle HTTP response codes when downloading descriptors
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 0b5110acdd1b61b85bdbafa9889c7cb0c383bcc3 Merge: b122a6fa 35493a8f Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 16:50:04 2018 -0700 Better handle HTTP response codes when downloading descriptors Good point from Dave that we shouldn't be so strict with the HTTP response codes we accept. In particular... * We should ignore the reason text (things other than 'OK' is fine). * Any 2xx response code is ok. * Add test coverage for these and non-2xx responses. stem/descriptor/remote.py | 4 ++-- test/unit/descriptor/remote.py | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-)

1 0

[stem/master] Accept all 2xx statuses when downloading descriptors
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 35493a8fa57b4900edbde78dc5d2092dc046cd68 Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 16:31:32 2018 -0700 Accept all 2xx statuses when downloading descriptors Also expanding our test coverage for this section a bit. --- stem/descriptor/remote.py | 4 ++-- test/unit/descriptor/remote.py | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index b266409a..158a010d 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -854,8 +854,8 @@ def _download_from_orport(endpoint, compression, resource): first_line, data = response.split(b'\r\n', 1) header_data, data = data.split(b'\r\n\r\n', 1) - if not first_line.startswith(b'HTTP/1.0 200 '): - raise stem.ProtocolError("Response should begin with HTTP success, but was '%s'" % first_line) + if not first_line.startswith(b'HTTP/1.0 2'): + raise stem.ProtocolError("Response should begin with HTTP success, but was '%s'" % str_tools._to_unicode(first_line)) headers = {} diff --git a/test/unit/descriptor/remote.py b/test/unit/descriptor/remote.py index 9afa624b..75bda0a0 100644 --- a/test/unit/descriptor/remote.py +++ b/test/unit/descriptor/remote.py @@ -8,6 +8,7 @@ import socket import time import unittest +import stem import stem.descriptor.remote import stem.prereq import stem.util.str_tools @@ -80,8 +81,11 @@ HEADER = '\r\n'.join([ ]) -def _orport_mock(data, encoding = 'identity'): - data = b'HTTP/1.0 200 This is fine\r\n' + stem.util.str_tools._to_bytes(HEADER % encoding) + b'\r\n\r\n' + data +def _orport_mock(data, encoding = 'identity', response_code_header = None): + if response_code_header is None: + response_code_header = b'HTTP/1.0 200 OK\r\n' + + data = response_code_header + stem.util.str_tools._to_bytes(HEADER % encoding) + b'\r\n\r\n' + data cells = [] for hunk in [data[i:i + 50] for i in range(0, len(data), 50)]: @@ -134,6 +138,33 @@ class TestDescriptorDownloader(unittest.TestCase): self.assertEqual('moria1', list(reply)[0].nickname) self.assertEqual(5, len(reply.reply_headers)) + def test_orport_response_code_headers(self): + """ + When successful Tor provides a '200 OK' status, but we should accept other 2xx + response codes, reason text, and recognize HTTP errors. + """ + + response_code_headers = ( + b'HTTP/1.0 200 OK\r\n', + b'HTTP/1.0 205 OK\r\n', + b'HTTP/1.0 200 This is also alright\r\n', + ) + + for header in response_code_headers: + with patch('stem.client.Relay.connect', _orport_mock(TEST_DESCRIPTOR, response_code_header = header)): + stem.descriptor.remote.their_server_descriptor( + endpoints = [stem.ORPort('12.34.56.78', 1100)], + validate = True, + ).run() + + with patch('stem.client.Relay.connect', _orport_mock(TEST_DESCRIPTOR, response_code_header = b'HTTP/1.0 500 Kaboom\r\n')): + request = stem.descriptor.remote.their_server_descriptor( + endpoints = [stem.ORPort('12.34.56.78', 1100)], + validate = True, + ) + + self.assertRaisesRegexp(stem.ProtocolError, "Response should begin with HTTP success, but was 'HTTP/1.0 500 Kaboom'", request.run) + @patch(URL_OPEN, _dirport_mock(TEST_DESCRIPTOR)) def test_using_dirport(self): """

1 0

[stem/master] Add slight clarification to variable naming
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit c533577383508dac2b21eea8ddd613f0f24c775f Author: Dave Rolek <dmr-x(a)riseup.net> Date: Mon Jun 11 01:49:33 2018 +0000 Add slight clarification to variable naming --- stem/descriptor/remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index e7baee65..9547b59a 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -853,7 +853,7 @@ def _download_from_orport(endpoint, compression, resource): circ.send(RelayCommand.BEGIN_DIR, stream_id = 1) response = b''.join([cell.data for cell in circ.send(RelayCommand.DATA, request, stream_id = 1)]) first_line, data = response.split(b'\r\n', 1) - header_data, data = data.split(b'\r\n\r\n', 1) + header_data, body_data = data.split(b'\r\n\r\n', 1) if not first_line.startswith(b'HTTP/1.0 2'): raise stem.ProtocolError("Response should begin with HTTP success, but was '%s'" % str_tools._to_unicode(first_line)) @@ -867,7 +867,7 @@ def _download_from_orport(endpoint, compression, resource): key, value = line.split(': ', 1) headers[key] = value - return _decompress(data, headers.get('Content-Encoding')), headers + return _decompress(body_data, headers.get('Content-Encoding')), headers def _download_from_dirport(url, compression, timeout):

1 0

[stem/master] Remove redundant (and mildly confusing) prefix module prefix.prefix
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit a3f4430b62cb907a3b4a9cef0e96adcbbfe1a009 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Mon Jun 11 02:21:06 2018 +0000 Remove redundant (and mildly confusing) prefix module prefix.prefix This expands a bit beyond stem.client consumers, but it's better to address all instances than to turn a blind eye due to scope purism. --- test/integ/descriptor/remote.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integ/descriptor/remote.py b/test/integ/descriptor/remote.py index c4e4f699..65800677 100644 --- a/test/integ/descriptor/remote.py +++ b/test/integ/descriptor/remote.py @@ -27,7 +27,7 @@ class TestDescriptorDownloader(unittest.TestCase): ).run())[0] self.assertEqual('moria1', desc.nickname) - self.assertTrue(isinstance(desc, stem.descriptor.stem.descriptor.server_descriptor.ServerDescriptor)) + self.assertTrue(isinstance(desc, stem.descriptor.server_descriptor.ServerDescriptor)) @test.require.only_run_once @test.require.online @@ -40,7 +40,7 @@ class TestDescriptorDownloader(unittest.TestCase): ).run())[0] self.assertEqual('moria1', desc.nickname) - self.assertTrue(isinstance(desc, stem.descriptor.stem.descriptor.server_descriptor.ServerDescriptor)) + self.assertTrue(isinstance(desc, stem.descriptor.server_descriptor.ServerDescriptor)) @test.require.only_run_once @test.require.online @@ -158,7 +158,7 @@ class TestDescriptorDownloader(unittest.TestCase): single_query_results = list(single_query) self.assertEqual(1, len(single_query_results)) self.assertEqual('moria1', single_query_results[0].nickname) - self.assertTrue(isinstance(single_query_results[0], stem.descriptor.stem.descriptor.server_descriptor.ServerDescriptor)) + self.assertTrue(isinstance(single_query_results[0], stem.descriptor.server_descriptor.ServerDescriptor)) self.assertEqual(2, len(list(multiple_query)))

1 0

[stem/master] Adjust import ordering
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 4c9e78ae50016af9668cf5b2f33045aa359ba69a Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Jun 23 16:37:07 2018 -0700 Adjust import ordering Minor thing, but throughout our codebase we usually import in alphabetical order with the following groupings... * builtins * stem imports * 'from' imports * try/catch imports Guess this is from PEP8? It's been so long I don't remember any more. :P --- stem/descriptor/remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index 94220a9c..e7baee65 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -100,12 +100,12 @@ import zlib import stem import stem.client -from stem.client.datatype import RelayCommand import stem.descriptor import stem.directory import stem.prereq import stem.util.enum +from stem.client.datatype import RelayCommand from stem.util import log, str_tools try:

1 0

[stem/master] Stylistic edit: reorder attribute assignments
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 4ccd6ce80e7a87ca162f4504b89a52bd9a538502 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Sun Jun 10 19:48:19 2018 +0000 Stylistic edit: reorder attribute assignments This is mostly a personal thing - having these in the order that the RELAY cell payload is defined helps avoid incidental misreading. --- stem/client/cell.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 1e382db8..57ccc1ab 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -343,10 +343,10 @@ class RelayCell(CircuitCell): super(RelayCell, self).__init__(circ_id, unused) self.command, self.command_int = RelayCommand.get(command) - self.data = str_tools._to_bytes(data) self.recognized = recognized - self.digest = digest self.stream_id = stream_id + self.digest = digest + self.data = str_tools._to_bytes(data) if digest == 0: if not stream_id and self.command in STREAM_ID_REQUIRED:

1 0

[stem/master] Replace RelayCommand strings with use of the enum
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit acb0b03e325819fd4aaae1d82c8dd7d148f51041 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Sun Jun 10 22:56:47 2018 +0000 Replace RelayCommand strings with use of the enum --- stem/descriptor/remote.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/stem/descriptor/remote.py b/stem/descriptor/remote.py index 158a010d..94220a9c 100644 --- a/stem/descriptor/remote.py +++ b/stem/descriptor/remote.py @@ -100,6 +100,7 @@ import zlib import stem import stem.client +from stem.client.datatype import RelayCommand import stem.descriptor import stem.directory import stem.prereq @@ -849,8 +850,8 @@ def _download_from_orport(endpoint, compression, resource): 'User-Agent: %s' % stem.USER_AGENT, )) + '\r\n\r\n' - circ.send('RELAY_BEGIN_DIR', stream_id = 1) - response = b''.join([cell.data for cell in circ.send('RELAY_DATA', request, stream_id = 1)]) + circ.send(RelayCommand.BEGIN_DIR, stream_id = 1) + response = b''.join([cell.data for cell in circ.send(RelayCommand.DATA, request, stream_id = 1)]) first_line, data = response.split(b'\r\n', 1) header_data, data = data.split(b'\r\n\r\n', 1)

1 0

[stem/master] Decorate the ORPort downloading test as requiring the cryptography module
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit df82798dff980fff9ae9b32bfa07f7d7ddb1c27a Author: Dave Rolek <dmr-x(a)riseup.net> Date: Mon Jun 11 03:01:23 2018 +0000 Decorate the ORPort downloading test as requiring the cryptography module (All circuit construction requires the cryptography module.) --- test/integ/descriptor/remote.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integ/descriptor/remote.py b/test/integ/descriptor/remote.py index 65800677..41fded2c 100644 --- a/test/integ/descriptor/remote.py +++ b/test/integ/descriptor/remote.py @@ -18,6 +18,7 @@ import test.require class TestDescriptorDownloader(unittest.TestCase): @test.require.only_run_once @test.require.online + @test.require.cryptography def test_downloading_via_orport(self): moria1 = stem.directory.Authority.from_cache()['moria1']

1 0

[stem/master] Fix bug that ignored VERSIONS cell link_protocol when packing
by atagar＠torproject.org 23 Jun '18

23 Jun '18

commit 10a7bdbc93121ab333685de25a95932a52c6b4e9 Author: Dave Rolek <dmr-x(a)riseup.net> Date: Thu May 31 02:38:39 2018 +0000 Fix bug that ignored VERSIONS cell link_protocol when packing The link_protocol was hardcoded to 2 within pack() for VERSIONS but the spec explicitly states "To be interpreted correctly, later VERSIONS cells MUST have a CIRCID_LEN matching the version negotiated with the first VERSIONS cell." (section 4.1). So while the use of VERSIONS cells after the negotiation is not expected, stem.client still needs to pack them correctly in case it is leveraged to test this property in any tor implementation. The use of link_protocol 2 to set the CIRCID_LEN appropriately has moved instead to stem.client.Relay, for sending the first VERSIONS cell. --- stem/client/__init__.py | 10 +++++++++- stem/client/cell.py | 7 ++----- test/unit/client/cell.py | 13 +++++++------ 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index c0099182..de952996 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -88,7 +88,15 @@ class Relay(object): else: raise - conn.send(stem.client.cell.VersionsCell(link_protocols).pack()) + # per tor-spec.txt (section 3): + # "CIRCID_LEN is 2 for link protocol versions 1, 2, and 3. + # CIRCID_LEN is 4 for link protocol version 4 or higher. + # The first VERSIONS cell, and any cells sent before the + # first VERSIONS cell, always have CIRCID_LEN == 2 for backward + # compatibility." + + # thus we arbitrarily select link_protocol=2 to accomplish this + conn.send(stem.client.cell.VersionsCell(link_protocols).pack(2)) response = conn.recv() # Link negotiation ends right away if we lack a common protocol diff --git a/stem/client/cell.py b/stem/client/cell.py index a7019f35..1e382db8 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -505,12 +505,9 @@ class VersionsCell(Cell): super(VersionsCell, self).__init__() self.versions = versions - def pack(self, link_protocol = None): - # Used for link version negotiation so we don't have that yet. This is fine - # since VERSION cells avoid most version dependent attributes. - + def pack(self, link_protocol): payload = b''.join([Size.SHORT.pack(v) for v in self.versions]) - return VersionsCell._pack(2, payload) + return VersionsCell._pack(link_protocol, payload) @classmethod def _unpack(cls, content, circ_id, link_protocol): diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 16d01293..402a1d4e 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -55,9 +55,10 @@ CREATED_FAST_CELLS = { } VERSIONS_CELLS = { - b'\x00\x00\x07\x00\x00': [], - b'\x00\x00\x07\x00\x02\x00\x01': [1], - b'\x00\x00\x07\x00\x06\x00\x01\x00\x02\x00\x03': [1, 2, 3], + b'\x00\x00\x07\x00\x00': ([], 2), + b'\x00\x00\x07\x00\x02\x00\x01': ([1], 2), + b'\x00\x00\x07\x00\x06\x00\x01\x00\x02\x00\x03': ([1, 2, 3], 2), + b'\x00\x00\x00\x00\x07\x00\x08\x00\x01\x00\x02\x00\x03\x00\x04': ([1, 2, 3, 4], 4), } NETINFO_CELLS = { @@ -250,9 +251,9 @@ class TestCell(unittest.TestCase): self.assertRaisesRegexp(ValueError, 'Key material should be 20 bytes, but was 3', CreateFastCell, 5, 'boo') def test_versions_cell(self): - for cell_bytes, versions in VERSIONS_CELLS.items(): - self.assertEqual(cell_bytes, VersionsCell(versions).pack()) - self.assertEqual(versions, Cell.pop(cell_bytes, 2)[0].versions) + for cell_bytes, (versions, link_protocol) in VERSIONS_CELLS.items(): + self.assertEqual(cell_bytes, VersionsCell(versions).pack(link_protocol)) + self.assertEqual(versions, Cell.pop(cell_bytes, link_protocol)[0].versions) def test_netinfo_cell(self): for cell_bytes, (timestamp, receiver_address, sender_addresses) in NETINFO_CELLS.items():

1 0