From 9cd26e539c4cfb012cef68398ff6b15aeeedff38 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 3 Aug 2018 14:56:31 +0000 Subject: [PATCH 01/28] Add abstract BaseRelayCell class It's intended to defined common Relay cell functionality, for encrypted and decrypted cells. By unpacking content no further than the payload, this abstraction should allow a lot of flexibility in handling Relay cells prior to decryption. Note that unpacking is not yet possible - subclasses must be defined with a VALUE. (Namely: RELAY and RELAY_EARLY) --- stem/client/cell.py | 36 ++++++++++++++++++++++++++++++++++++ test/unit/client/cell.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index 12fa994ca..a4fb1a676 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -14,6 +14,7 @@ |- CircuitCell - Circuit management. | |- CreateCell - Create a circuit. (section 5.1) | |- CreatedCell - Acknowledge create. (section 5.1) + | |- BaseRelayCell - End-to-end data; abstract. (section 6.1) | |- RelayCell - End-to-end data. (section 6.1) | |- DestroyCell - Stop using a circuit. (section 5.4) | |- CreateFastCell - Create a circuit, no PK. (section 5.1) @@ -83,6 +84,7 @@ class Cell(object): The following cell types explicitly don't have *unused* content: * PaddingCell (we consider all content part of payload) * VersionsCell (all content is unpacked and treated as a version specification) + * BaseRelayCell (we don't parse cell beyond header/body) * VPaddingCell (we consider all content part of payload) :var bytes unused: unused filler that padded the cell to the expected size @@ -320,6 +322,40 @@ def __init__(self): super(CreatedCell, self).__init__() # TODO: implement +class BaseRelayCell(CircuitCell): + """ + Cell whose subclasses are relayed over circuits. + + :var bytes payload: raw payload, quite possibly encrypted + """ + + NAME = 'INTERNAL_BASE_RELAY' # defined for error/other strings + IS_FIXED_SIZE = True # all relay cells are fixed-size + + # other attributes are deferred to subclasses, since this class cannot be directly unpacked + + def __init__(self, circ_id, payload): + if not payload: + raise ValueError('Relay cells require a payload') + if len(payload) != FIXED_PAYLOAD_LEN: + raise ValueError('Payload should be %i bytes, but was %i' % (FIXED_PAYLOAD_LEN, len(payload))) + + super(BaseRelayCell, self).__init__(circ_id, unused = b'') + self.payload = payload + + def pack(self, link_protocol): + # unlike everywhere else, we actually want to use the subclass type, NOT *this* class + return type(self)._pack(link_protocol, self.payload, circ_id = self.circ_id) + + @classmethod + def _unpack(cls, content, circ_id, link_protocol): + # unlike everywhere else, we actually want to use the subclass type, NOT *this* class + return cls(circ_id, content) + + def __hash__(self): + return stem.util._hash_attr(self, 'circ_id', 'payload', cache = True) + + class RelayCell(CircuitCell): """ Command concerning a relay circuit. diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index ce4926381..278e0b4c7 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -5,6 +5,7 @@ import datetime import hashlib import os +import struct import unittest from stem.client.datatype import ZERO, CertType, CloseReason, Address, Certificate @@ -14,6 +15,7 @@ FIXED_PAYLOAD_LEN, Cell, PaddingCell, + BaseRelayCell, RelayCell, DestroyCell, CreateFastCell, @@ -188,6 +190,35 @@ def test_padding_cell(self): self.assertEqual(b'', cell.unused) # always empty self.assertEqual(cell_bytes, cell.pack(link_protocol)) + def test_base_relay_cell(self): + arbitrary_circ_id = 123 + even_more_arbitrary_link_protocol = 1234 + + cell = BaseRelayCell(arbitrary_circ_id, RANDOM_PAYLOAD) + self.assertEqual(RANDOM_PAYLOAD, cell.payload) + self.assertEqual(arbitrary_circ_id, cell.circ_id) + self.assertEqual(True, cell.IS_FIXED_SIZE) + + # Cell.unpack not reachable - won't be tested + # but we can at least directly test _unpack, although it's a pretty simple method + cell_2 = BaseRelayCell._unpack(RANDOM_PAYLOAD, arbitrary_circ_id, even_more_arbitrary_link_protocol) + self.assertEqual(cell, cell_2) + + # pack not possible, but easily callable + self.assertRaises(struct.error, cell.pack, even_more_arbitrary_link_protocol) + + # check other values and inequality + for (circ_id, payload) in ((arbitrary_circ_id, ZERO * FIXED_PAYLOAD_LEN), (arbitrary_circ_id + 1, RANDOM_PAYLOAD)): + unequal_cell = BaseRelayCell(circ_id, payload) + self.assertEqual(payload, unequal_cell.payload) + self.assertNotEqual(cell, unequal_cell) + + # invalid constructions + self.assertRaisesWith(ValueError, 'Relay cells require a payload', BaseRelayCell, arbitrary_circ_id, None) + expected_message_format = 'Payload should be %i bytes, but was ' % FIXED_PAYLOAD_LEN + '%i' + for payload_len in (FIXED_PAYLOAD_LEN - 1, FIXED_PAYLOAD_LEN + 1): + self.assertRaisesWith(ValueError, expected_message_format % payload_len, BaseRelayCell, arbitrary_circ_id, ZERO * payload_len) + def test_relay_cell(self): for cell_bytes, (command, command_int, circ_id, stream_id, data, digest, unused, link_protocol) in RELAY_CELLS.items(): if not unused.strip(ZERO): From ba6d4c0a47b3fe62f55af3b7ab3d0e292c3dc74f Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Mon, 6 Aug 2018 21:45:26 +0000 Subject: [PATCH 02/28] Provide a nicer error message for packing negative numbers with unsigned types --- stem/client/datatype.py | 3 +++ test/unit/client/size.py | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/stem/client/datatype.py b/stem/client/datatype.py index 5ca4e820a..e19adfb19 100644 --- a/stem/client/datatype.py +++ b/stem/client/datatype.py @@ -349,6 +349,7 @@ def __init__(self, name, size, pack_format): self.name = name self.size = size self.format = pack_format + self.unsigned = pack_format.isupper() @staticmethod def pop(packed): @@ -357,6 +358,8 @@ def pop(packed): def pack(self, content): if not stem.util._is_int(content): raise ValueError('Size.pack encodes an integer, but was a %s' % type(content).__name__) + if self.unsigned and content < 0: + raise ValueError('A %s field cannot pack negative values, but %i was tried' % (self.name, content)) packed = struct.pack(self.format, content) diff --git a/test/unit/client/size.py b/test/unit/client/size.py index eebe36197..3d7d796f6 100644 --- a/test/unit/client/size.py +++ b/test/unit/client/size.py @@ -7,17 +7,22 @@ from stem.client.datatype import Size +SIGNED_CHAR = Size('SIGNED_CHAR', 1, '!b') + class TestSize(unittest.TestCase): def test_attributes(self): self.assertEqual('CHAR', Size.CHAR.name) self.assertEqual('!B', Size.CHAR.format) + self.assertEqual(True, Size.CHAR.unsigned) self.assertEqual(1, Size.CHAR.size) self.assertEqual(2, Size.SHORT.size) self.assertEqual(4, Size.LONG.size) self.assertEqual(8, Size.LONG_LONG.size) + self.assertEqual(False, SIGNED_CHAR.unsigned) + def test_pack(self): self.assertEqual(b'\x12', Size.CHAR.pack(18)) self.assertEqual(b'\x00\x12', Size.SHORT.pack(18)) @@ -26,9 +31,13 @@ def test_pack(self): self.assertRaisesWith(ValueError, 'Size.pack encodes an integer, but was a str', Size.CHAR.pack, 'hi') + self.assertRaisesWith(ValueError, 'A CHAR field cannot pack negative values, but -1 was tried', Size.CHAR.pack, -1) + bad_size = Size('BAD_SIZE', 1, '!H') self.assertRaisesRegexp(ValueError, re.escape("'\\x00\\x12' is the wrong size for a BAD_SIZE field"), bad_size.pack, 18) + self.assertEqual(b'\xFF', SIGNED_CHAR.pack(-1)) + def test_unpack(self): self.assertEqual(18, Size.CHAR.unpack(b'\x12')) self.assertEqual(18, Size.SHORT.unpack(b'\x00\x12')) From fcffb7fba89b410ba88dece16771677e7f9b6739 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Tue, 7 Aug 2018 18:32:31 +0000 Subject: [PATCH 03/28] Optimize error check for non-negative inputs stem.client presently makes typical use of non-negative inputs with unsigned types. --- stem/client/datatype.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stem/client/datatype.py b/stem/client/datatype.py index e19adfb19..1c7aec776 100644 --- a/stem/client/datatype.py +++ b/stem/client/datatype.py @@ -358,7 +358,7 @@ def pop(packed): def pack(self, content): if not stem.util._is_int(content): raise ValueError('Size.pack encodes an integer, but was a %s' % type(content).__name__) - if self.unsigned and content < 0: + if content < 0 and self.unsigned: raise ValueError('A %s field cannot pack negative values, but %i was tried' % (self.name, content)) packed = struct.pack(self.format, content) From aba5cd93d97cf904de7446ba877231c9bbe544fe Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Tue, 7 Aug 2018 18:56:24 +0000 Subject: [PATCH 04/28] Adjust test expectation for the different exception rising from Size --- test/unit/client/cell.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 278e0b4c7..a6ddf3eac 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -5,7 +5,6 @@ import datetime import hashlib import os -import struct import unittest from stem.client.datatype import ZERO, CertType, CloseReason, Address, Certificate @@ -205,7 +204,8 @@ def test_base_relay_cell(self): self.assertEqual(cell, cell_2) # pack not possible, but easily callable - self.assertRaises(struct.error, cell.pack, even_more_arbitrary_link_protocol) + # lots of things cause a ValueError, so this check isn't very specific, but the wording comes from Size and so isn't under the purview of this unit + self.assertRaises(ValueError, cell.pack, even_more_arbitrary_link_protocol) # check other values and inequality for (circ_id, payload) in ((arbitrary_circ_id, ZERO * FIXED_PAYLOAD_LEN), (arbitrary_circ_id + 1, RANDOM_PAYLOAD)): From 6033cd03297d567968dc988e1191f0a6c66caf87 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Tue, 7 Aug 2018 21:45:26 +0000 Subject: [PATCH 05/28] Add CANNOT_DIRECTLY_UNPACK facility; set True for existing RelayCell Since RELAY cells come across the wire encrypted, they cannot be directly unpacked - their payload must first be decrypted. Note that this breaks unpacking RelayCell in the interim. --- stem/client/cell.py | 5 +++-- test/unit/client/cell.py | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index a4fb1a676..dcebc544b 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -109,7 +109,7 @@ def by_name(name): """ for _, cls in inspect.getmembers(sys.modules[__name__]): - if name == getattr(cls, 'NAME', UNDEFINED): + if name == getattr(cls, 'NAME', UNDEFINED) and not getattr(cls, 'CANNOT_DIRECTLY_UNPACK', False): return cls raise ValueError("'%s' isn't a valid cell type" % name) @@ -125,7 +125,7 @@ def by_value(value): """ for _, cls in inspect.getmembers(sys.modules[__name__]): - if value == getattr(cls, 'VALUE', UNDEFINED): + if value == getattr(cls, 'VALUE', UNDEFINED) and not getattr(cls, 'CANNOT_DIRECTLY_UNPACK', False): return cls raise ValueError("'%s' isn't a valid cell value" % value) @@ -371,6 +371,7 @@ class RelayCell(CircuitCell): NAME = 'RELAY' VALUE = 3 IS_FIXED_SIZE = True + CANNOT_DIRECTLY_UNPACK = True def __init__(self, circ_id, command, data, digest = 0, stream_id = 0, recognized = 0, unused = b''): if 'HASH' in str(type(digest)): diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index a6ddf3eac..b8fe58df3 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -220,6 +220,8 @@ def test_base_relay_cell(self): self.assertRaisesWith(ValueError, expected_message_format % payload_len, BaseRelayCell, arbitrary_circ_id, ZERO * payload_len) def test_relay_cell(self): + self.assertEquals(True, RelayCell.CANNOT_DIRECTLY_UNPACK) + for cell_bytes, (command, command_int, circ_id, stream_id, data, digest, unused, link_protocol) in RELAY_CELLS.items(): if not unused.strip(ZERO): self.assertEqual(cell_bytes, RelayCell(circ_id, command, data, digest, stream_id).pack(link_protocol)) From 2175b7c65934cf380cfcc4198aa36baf06619d6b Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 8 Aug 2018 01:14:27 +0000 Subject: [PATCH 06/28] Add RawRelayCell and temp-fix RelayCell tests (interim) Notably, this along with CANNOT_DIRECTLY_UNPACK for RelayCell (prior change) allows Cell.pop() and Cell.unpack() to be executed directly on received bytes from a connection, and still process encrypted cells correctly into RELAY cells, to be decrypted later. RelayCell tests marked with TODO where temp-fix was applied. RelayCell implementation will gradually be improved/refactored into better supporting subclasses per command. --- stem/client/cell.py | 5 +++++ test/unit/client/cell.py | 34 ++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index dcebc544b..4f7aec794 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -356,6 +356,11 @@ def __hash__(self): return stem.util._hash_attr(self, 'circ_id', 'payload', cache = True) +class RawRelayCell(BaseRelayCell): + NAME = 'RELAY' + VALUE = 3 + + class RelayCell(CircuitCell): """ Command concerning a relay circuit. diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index b8fe58df3..3e091ee55 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -15,6 +15,7 @@ Cell, PaddingCell, BaseRelayCell, + RawRelayCell, RelayCell, DestroyCell, CreateFastCell, @@ -93,20 +94,24 @@ class TestCell(unittest.TestCase): def test_by_name(self): - cls = Cell.by_name('NETINFO') - self.assertEqual('NETINFO', cls.NAME) - self.assertEqual(8, cls.VALUE) - self.assertEqual(True, cls.IS_FIXED_SIZE) + for (expected_class, name, value, is_fixed_size) in ((NetinfoCell, 'NETINFO', 8, True), (RawRelayCell, 'RELAY', 3, True)): + cls = Cell.by_name(name) + self.assertEqual(expected_class, cls) + self.assertEqual(name, cls.NAME) + self.assertEqual(value, cls.VALUE) + self.assertEqual(is_fixed_size, cls.IS_FIXED_SIZE) self.assertRaises(ValueError, Cell.by_name, 'NOPE') self.assertRaises(ValueError, Cell.by_name, 85) self.assertRaises(ValueError, Cell.by_name, None) def test_by_value(self): - cls = Cell.by_value(8) - self.assertEqual('NETINFO', cls.NAME) - self.assertEqual(8, cls.VALUE) - self.assertEqual(True, cls.IS_FIXED_SIZE) + for (expected_class, name, value, is_fixed_size) in ((NetinfoCell, 'NETINFO', 8, True), (RawRelayCell, 'RELAY', 3, True)): + cls = Cell.by_value(value) + self.assertEqual(expected_class, cls) + self.assertEqual(name, cls.NAME) + self.assertEqual(value, cls.VALUE) + self.assertEqual(is_fixed_size, cls.IS_FIXED_SIZE) self.assertRaises(ValueError, Cell.by_value, 'NOPE') self.assertRaises(ValueError, Cell.by_value, 85) @@ -230,7 +235,12 @@ def test_relay_cell(self): self.assertEqual(cell_bytes, RelayCell(circ_id, command, data, digest, stream_id, unused = unused).pack(link_protocol)) self.assertEqual(cell_bytes, RelayCell(circ_id, command_int, data, digest, stream_id, unused = unused).pack(link_protocol)) - cell = Cell.pop(cell_bytes, link_protocol)[0] + # TODO - temporarily, we hack the interim tests by unpacking info via RawRelayCell + raw_cell = Cell.pop(cell_bytes, link_protocol)[0] + self.assertEqual(circ_id, raw_cell.circ_id) + self.assertEqual(cell_bytes[-FIXED_PAYLOAD_LEN:], raw_cell.payload) + + cell = RelayCell._unpack(raw_cell.payload, raw_cell.circ_id, link_protocol) self.assertEqual(circ_id, cell.circ_id) self.assertEqual(command, cell.command) self.assertEqual(command_int, cell.command_int) @@ -238,7 +248,9 @@ def test_relay_cell(self): self.assertEqual(digest, cell.digest) self.assertEqual(stream_id, cell.stream_id) self.assertEqual(unused, cell.unused) + self.assertEqual(cell_bytes, cell.pack(link_protocol)) + self.assertEqual(cell_bytes, raw_cell.pack(link_protocol)) digest = hashlib.sha1(b'hi') self.assertEqual(3257622417, RelayCell(5, 'RELAY_BEGIN_DIR', '', digest, 564346860).digest) @@ -258,7 +270,9 @@ def test_relay_cell(self): ZERO * 498, # data )) - self.assertRaisesWith(ValueError, 'RELAY cell said it had 65535 bytes of data, but only had 498', Cell.pop, mismatched_data_length_bytes, 2) + # TODO - temporarily, we hack the interim tests by unpacking info via RawRelayCell + raw_cell = Cell.pop(mismatched_data_length_bytes, 2)[0] + self.assertRaisesWith(ValueError, 'RELAY cell said it had 65535 bytes of data, but only had 498', RelayCell._unpack, raw_cell.payload, raw_cell.circ_id, 2) def test_destroy_cell(self): for cell_bytes, (circ_id, reason, reason_int, unused, link_protocol) in DESTROY_CELLS.items(): From 462c9867018bfc83f276d004929f7044067744be Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Thu, 9 Aug 2018 19:18:13 +0000 Subject: [PATCH 07/28] Document RawRelayCell in the module overview The line is long, so this change also extends the vertically aligned table format for other Cell classes, in a forward-looking manner. Arguably this could've been part of the previous change, but it made sense to break it out for readability purposes. --- stem/client/cell.py | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 4f7aec794..f18318dad 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -12,26 +12,27 @@ Cell - Base class for ORPort messages. |- CircuitCell - Circuit management. - | |- CreateCell - Create a circuit. (section 5.1) - | |- CreatedCell - Acknowledge create. (section 5.1) - | |- BaseRelayCell - End-to-end data; abstract. (section 6.1) - | |- RelayCell - End-to-end data. (section 6.1) - | |- DestroyCell - Stop using a circuit. (section 5.4) - | |- CreateFastCell - Create a circuit, no PK. (section 5.1) - | |- CreatedFastCell - Circuit created, no PK. (section 5.1) - | |- RelayEarlyCell - End-to-end data; limited. (section 5.6) - | |- Create2Cell - Extended CREATE cell. (section 5.1) - | +- Created2Cell - Extended CREATED cell. (section 5.1) + | |- CreateCell - Create a circuit. (section 5.1) + | |- CreatedCell - Acknowledge create. (section 5.1) + | |- BaseRelayCell - End-to-end data; abstract. (section 6.1) + | | +- RawRelayCell - End-to-end data. Payload not unpacked. (section 5.5.2.1, 5.5.3) + | |- RelayCell - End-to-end data. (section 6.1) + | |- DestroyCell - Stop using a circuit. (section 5.4) + | |- CreateFastCell - Create a circuit, no PK. (section 5.1) + | |- CreatedFastCell - Circuit created, no PK. (section 5.1) + | |- RelayEarlyCell - End-to-end data; limited. (section 5.6) + | |- Create2Cell - Extended CREATE cell. (section 5.1) + | +- Created2Cell - Extended CREATED cell. (section 5.1) | - |- PaddingCell - Padding negotiation. (section 7.2) - |- VersionsCell - Negotiate proto version. (section 4) - |- NetinfoCell - Time and address info. (section 4.5) - |- PaddingNegotiateCell - Padding negotiation. (section 7.2) - |- VPaddingCell - Variable-length padding. (section 7.2) - |- CertsCell - Relay certificates. (section 4.2) - |- AuthChallengeCell - Challenge value. (section 4.3) - |- AuthenticateCell - Client authentication. (section 4.5) - |- AuthorizeCell - Client authorization. (not yet used) + |- PaddingCell - Padding negotiation. (section 7.2) + |- VersionsCell - Negotiate proto version. (section 4) + |- NetinfoCell - Time and address info. (section 4.5) + |- PaddingNegotiateCell - Padding negotiation. (section 7.2) + |- VPaddingCell - Variable-length padding. (section 7.2) + |- CertsCell - Relay certificates. (section 4.2) + |- AuthChallengeCell - Challenge value. (section 4.3) + |- AuthenticateCell - Client authentication. (section 4.5) + |- AuthorizeCell - Client authorization. (not yet used) | |- pack - encodes cell into bytes |- unpack - decodes series of cells From b179161b907dab9befae0f59dbd2eb835376acf0 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 8 Aug 2018 03:06:38 +0000 Subject: [PATCH 08/28] Simplify encryption/decryption implementation in Circuit This is a proof-of-concept change to show the benefits of migrating finer details into the Cell abstraction. It still arguably violates the Cell abstraction by using RelayCell._unpack(), but that can be addressed in a later change. --- stem/client/__init__.py | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 24b6de385..71f6d3908 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -34,7 +34,7 @@ import stem.socket import stem.util.connection -from stem.client.datatype import ZERO, LinkProtocol, Address, Size, KDF, split +from stem.client.datatype import ZERO, LinkProtocol, Address, KDF __all__ = [ 'cell', @@ -237,45 +237,33 @@ def send(self, command, data = '', stream_id = 0): orig_digest = self.forward_digest.copy() orig_key = copy.copy(self.forward_key) - # Digests and such are computed using the RELAY cell payload. This - # doesn't include the initial circuit id and cell type fields. - # Circuit ids vary in length depending on the protocol version. - - header_size = self.relay.link_protocol.circ_id_size.size + 1 - try: + # Digests and such are computed using the RELAY cell payload. cell = stem.client.cell.RelayCell(self.id, command, data, 0, stream_id) - payload_without_digest = cell.pack(self.relay.link_protocol)[header_size:] + payload_without_digest = cell.pack(self.relay.link_protocol)[-stem.client.cell.FIXED_PAYLOAD_LEN:] self.forward_digest.update(payload_without_digest) cell = stem.client.cell.RelayCell(self.id, command, data, self.forward_digest, stream_id) - header, payload = split(cell.pack(self.relay.link_protocol), header_size) - encrypted_payload = header + self.forward_key.update(payload) + payload_with_digest = cell.pack(self.relay.link_protocol)[-stem.client.cell.FIXED_PAYLOAD_LEN:] + encrypted_payload = self.forward_key.update(payload_with_digest) + encrypted_cell = stem.client.cell.RawRelayCell(self.id, encrypted_payload) reply_cells = [] - self.relay._orport.send(encrypted_payload) + self.relay._orport.send(encrypted_cell.pack(self.relay.link_protocol)) reply = self.relay._orport.recv() - # Check that we got the correct number of bytes for a series of RELAY cells - - relay_cell_size = header_size + stem.client.cell.FIXED_PAYLOAD_LEN relay_cell_cmd = stem.client.cell.RelayCell.VALUE - if len(reply) % relay_cell_size != 0: - raise stem.ProtocolError('Circuit response should be a series of RELAY cells, but received an unexpected size for a response: %i' % len(reply)) - while reply: - circ_id, reply = self.relay.link_protocol.circ_id_size.pop(reply) - command, reply = Size.CHAR.pop(reply) - payload, reply = split(reply, stem.client.cell.FIXED_PAYLOAD_LEN) + raw_cell, reply = stem.client.cell.Cell.pop(reply, self.relay.link_protocol) - if command != relay_cell_cmd: - raise stem.ProtocolError('RELAY cell responses should be %i but was %i' % (relay_cell_cmd, command)) - elif circ_id != self.id: - raise stem.ProtocolError('Response should be for circuit id %i, not %i' % (self.id, circ_id)) + if raw_cell.VALUE != relay_cell_cmd: + raise stem.ProtocolError('RELAY cell responses should be %i but was %i' % (relay_cell_cmd, raw_cell.VALUE)) + elif raw_cell.circ_id != self.id: + raise stem.ProtocolError('Response should be for circuit id %i, not %i' % (self.id, raw_cell.circ_id)) - decrypted = self.backward_key.update(payload) - reply_cells.append(stem.client.cell.RelayCell._unpack(decrypted, self.id, self.relay.link_protocol)) + decrypted_payload = self.backward_key.update(raw_cell.payload) + reply_cells.append(stem.client.cell.RelayCell._unpack(decrypted_payload, self.id, self.relay.link_protocol)) return reply_cells except: From e048639c7cc6b4f1a25745c55af44b3705e80698 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 8 Aug 2018 19:13:15 +0000 Subject: [PATCH 09/28] Standardize some docstring keywords s/:parm /:param / s/:raise:/:raises:/ s/:return:/:returns:/ --- stem/client/cell.py | 14 +++++++------- stem/util/log.py | 2 +- stem/util/term.py | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index f18318dad..5ad577886 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -104,9 +104,9 @@ def by_name(name): """ Provides cell attributes by its name. - :parm str name: cell command to fetch + :param str name: cell command to fetch - :raise: **ValueError** if cell type is invalid + :raises: **ValueError** if cell type is invalid """ for _, cls in inspect.getmembers(sys.modules[__name__]): @@ -120,9 +120,9 @@ def by_value(value): """ Provides cell attributes by its value. - :parm int value: cell value to fetch + :param int value: cell value to fetch - :raise: **ValueError** if cell type is invalid + :raises: **ValueError** if cell type is invalid """ for _, cls in inspect.getmembers(sys.modules[__name__]): @@ -202,9 +202,9 @@ def _pack(cls, link_protocol, payload, unused = b'', circ_id = None): :param bytes payload: cell payload :param int circ_id: circuit id, if a CircuitCell - :return: **bytes** with the encoded payload + :returns: **bytes** with the encoded payload - :raise: **ValueError** if cell type invalid or payload makes cell too large + :raises: **ValueError** if cell type invalid or payload makes cell too large """ if issubclass(cls, CircuitCell): @@ -369,7 +369,7 @@ class RelayCell(CircuitCell): :var stem.client.RelayCommand command: command to be issued :var int command_int: integer value of our command :var bytes data: payload of the cell - :var int recognized: zero if cell is decrypted, non-zero otherwise + :var int recognized: zero if cell is decrypted, otherwise mostly non-zero (can rarely be zero) :var int digest: running digest held with the relay :var int stream_id: specific stream this concerns """ diff --git a/stem/util/log.py b/stem/util/log.py index 81fc88a6f..0b2de90a5 100644 --- a/stem/util/log.py +++ b/stem/util/log.py @@ -103,7 +103,7 @@ def get_logger(): """ Provides the stem logger. - :return: **logging.Logger** for stem + :returns: **logging.Logger** for stem """ return LOGGER diff --git a/stem/util/term.py b/stem/util/term.py index 3c6fe73a4..b4acd61fd 100644 --- a/stem/util/term.py +++ b/stem/util/term.py @@ -80,7 +80,7 @@ def encoding(*attrs): :data:`~stem.util.terminal.BgColor`, or :data:`~stem.util.terminal.Attr` to provide an ecoding for - :return: **str** of the ANSI escape sequence, **None** no attributes are + :returns: **str** of the ANSI escape sequence, **None** no attributes are recognized """ From d63dd758ebc993783baecf6a16fc8a39f48b84ab Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 10 Aug 2018 21:40:59 +0000 Subject: [PATCH 10/28] Refactor digest coercion into separate method This allows the logic to be reused. It may ultimately be better represented as a @property, but for now this should do fine. --- stem/client/cell.py | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 5ad577886..a7b573262 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -380,6 +380,32 @@ class RelayCell(CircuitCell): CANNOT_DIRECTLY_UNPACK = True def __init__(self, circ_id, command, data, digest = 0, stream_id = 0, recognized = 0, unused = b''): + digest = RelayCell._coerce_digest(digest) + + super(RelayCell, self).__init__(circ_id, unused) + self.command, self.command_int = RelayCommand.get(command) + self.recognized = recognized + 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: + raise ValueError('%s relay cells require a stream id' % self.command) + elif stream_id and self.command in STREAM_ID_DISALLOWED: + raise ValueError('%s relay cells concern the circuit itself and cannot have a stream id' % self.command) + + @staticmethod + def _coerce_digest(digest): + """ + Coerce any of HASH, str, int into the proper digest type for packing + + :param HASH,str,int digest: digest to be coerced + :returns: digest in type appropriate for packing + + :raises: **ValueError** if input digest type is unsupported + """ + if 'HASH' in str(type(digest)): # Unfortunately hashlib generates from a dynamic private class so # isinstance() isn't such a great option. With python2/python3 the @@ -395,18 +421,7 @@ def __init__(self, circ_id, command, data, digest = 0, stream_id = 0, recognized else: raise ValueError('RELAY cell digest must be a hash, string, or int but was a %s' % type(digest).__name__) - super(RelayCell, self).__init__(circ_id, unused) - self.command, self.command_int = RelayCommand.get(command) - self.recognized = recognized - 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: - raise ValueError('%s relay cells require a stream id' % self.command) - elif stream_id and self.command in STREAM_ID_DISALLOWED: - raise ValueError('%s relay cells concern the circuit itself and cannot have a stream id' % self.command) + return digest def pack(self, link_protocol): payload = bytearray() From 3f56c3b35143783e6c3c0717f19f45d8f8e21883 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 8 Aug 2018 17:31:29 +0000 Subject: [PATCH 11/28] Refactor payload parsing (unpacking) into separate method Eventually, having this separated out will help during decryption of cells in multi-hop circuits - each layer of decryption must attempt to interpret the payload in order to ascertain whether it has been fully decrypted. --- stem/client/cell.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index a7b573262..9194c29cb 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -436,6 +436,23 @@ def pack(self, link_protocol): @classmethod def _unpack(cls, content, circ_id, link_protocol): + command, recognized, stream_id, digest, data_len, data, unused = RelayCell._unpack_payload(content) + + if len(data) != data_len: + raise ValueError('%s cell said it had %i bytes of data, but only had %i' % (cls.NAME, data_len, len(data))) + + return RelayCell(circ_id, command, data, digest, stream_id, recognized, unused) + + @staticmethod + def _unpack_payload(content): + """ + Directly interpret the payload without any validation. + + :param bytes content: cell payload + + :returns: (command, recognized, stream_id, digest, data_len, data, unused) tuple + """ + command, content = Size.CHAR.pop(content) recognized, content = Size.SHORT.pop(content) # 'recognized' field stream_id, content = Size.SHORT.pop(content) @@ -443,10 +460,7 @@ def _unpack(cls, content, circ_id, link_protocol): data_len, content = Size.SHORT.pop(content) data, unused = split(content, data_len) - if len(data) != data_len: - raise ValueError('%s cell said it had %i bytes of data, but only had %i' % (cls.NAME, data_len, len(data))) - - return RelayCell(circ_id, command, data, digest, stream_id, recognized, unused) + return command, recognized, stream_id, digest, data_len, data, unused def __hash__(self): return stem.util._hash_attr(self, 'command_int', 'stream_id', 'digest', 'data', cache = True) From 05f083a0950228b3ce63f8f81bbee0480dfcc40c Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 8 Aug 2018 19:04:52 +0000 Subject: [PATCH 12/28] Refactor payload packing into separate method It's important to decouple packing the payload from packing the whole cell. This is probably not the final form this method will take, but in the interim, it should help reduce the magnitude of violating the Cell abstraction layer, for any payload encryption/decryption. --- stem/client/cell.py | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 9194c29cb..7c09d0e09 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -424,15 +424,9 @@ def _coerce_digest(digest): return digest def pack(self, link_protocol): - payload = bytearray() - payload += Size.CHAR.pack(self.command_int) - payload += Size.SHORT.pack(self.recognized) - payload += Size.SHORT.pack(self.stream_id) - payload += Size.LONG.pack(self.digest) - payload += Size.SHORT.pack(len(self.data)) - payload += self.data + payload = RelayCell._pack_payload(self.command_int, self.recognized, self.stream_id, self.digest, len(self.data), self.data) - return RelayCell._pack(link_protocol, bytes(payload), self.unused, self.circ_id) + return RelayCell._pack(link_protocol, payload, self.unused, self.circ_id) @classmethod def _unpack(cls, content, circ_id, link_protocol): @@ -462,6 +456,31 @@ def _unpack_payload(content): return command, recognized, stream_id, digest, data_len, data, unused + @staticmethod + def _pack_payload(command_int, recognized, stream_id, digest, data_len, data): + """ + Directly pack the payload without any validation beyond Size constraints. + + :param int command_int: integer value of our command + :param int recognized: zero if cell is decrypted, otherwise mostly non-zero (can rarely be zero) + :param int stream_id: specific stream this concerns + :param HASH,str,int digest: running digest held with the relay + :param int data_len: length of body data + :param bytes data: body data of the cell + + :returns: **bytes** with the packed payload + """ + + payload = bytearray() + payload += Size.CHAR.pack(command_int) + payload += Size.SHORT.pack(recognized) + payload += Size.SHORT.pack(stream_id) + payload += Size.LONG.pack(RelayCell._coerce_digest(digest)) + payload += Size.SHORT.pack(data_len) + payload += data + + return bytes(payload) + def __hash__(self): return stem.util._hash_attr(self, 'command_int', 'stream_id', 'digest', 'data', cache = True) From d427bfc1e9ae2f827796f455f5bba60d1b1f75fc Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Thu, 9 Aug 2018 20:28:15 +0000 Subject: [PATCH 13/28] Refactor 'unused' and padding packing into the RELAY cell payload This is more appropriate since the spec allows for (and recommends!) different behavior for these bytes than other padding bytes. However, presently the handling is identical. It is also important that the _pack_payload() method be able to produce a full-size payload, for simplifying encryption. --- stem/client/cell.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 7c09d0e09..42b955718 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -424,9 +424,9 @@ def _coerce_digest(digest): return digest def pack(self, link_protocol): - payload = RelayCell._pack_payload(self.command_int, self.recognized, self.stream_id, self.digest, len(self.data), self.data) + payload = RelayCell._pack_payload(self.command_int, self.recognized, self.stream_id, self.digest, len(self.data), self.data, self.unused) - return RelayCell._pack(link_protocol, payload, self.unused, self.circ_id) + return RelayCell._pack(link_protocol, payload, unused = b'', circ_id = self.circ_id) @classmethod def _unpack(cls, content, circ_id, link_protocol): @@ -457,7 +457,7 @@ def _unpack_payload(content): return command, recognized, stream_id, digest, data_len, data, unused @staticmethod - def _pack_payload(command_int, recognized, stream_id, digest, data_len, data): + def _pack_payload(command_int, recognized, stream_id, digest, data_len, data, unused = b'', pad_remainder = True): """ Directly pack the payload without any validation beyond Size constraints. @@ -467,6 +467,8 @@ def _pack_payload(command_int, recognized, stream_id, digest, data_len, data): :param HASH,str,int digest: running digest held with the relay :param int data_len: length of body data :param bytes data: body data of the cell + :param bytes unused: padding bytes to include after data + :param bool pad_remaining: pads up to payload size if **True** :returns: **bytes** with the packed payload """ @@ -478,6 +480,16 @@ def _pack_payload(command_int, recognized, stream_id, digest, data_len, data): payload += Size.LONG.pack(RelayCell._coerce_digest(digest)) payload += Size.SHORT.pack(data_len) payload += data + payload += unused + + if len(payload) > FIXED_PAYLOAD_LEN: + raise ValueError('Payload is too large (%i bytes), must not be more than %i.' % (len(payload), FIXED_PAYLOAD_LEN)) + + if pad_remainder: + # right now, it is acceptable to pad the remaining portion with ZEROs instead of random + # this is done due to threat model and simplifying some implementation + # however: in the future (TODO), this may become against the spec; see prop 289 + payload += ZERO * (FIXED_PAYLOAD_LEN - len(payload)) return bytes(payload) From a2bb3362ac4680a0bfdd553b87a4ccd8d5c645e8 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Thu, 9 Aug 2018 21:12:27 +0000 Subject: [PATCH 14/28] Further simplify digest calculation for encryption This is a continuation of proof-of-concept changes. Again, this is not the final form that encryption will take on. --- stem/client/__init__.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 71f6d3908..83c20365d 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -32,6 +32,7 @@ import stem import stem.client.cell import stem.socket +from stem.util import str_tools import stem.util.connection from stem.client.datatype import ZERO, LinkProtocol, Address, KDF @@ -239,12 +240,15 @@ def send(self, command, data = '', stream_id = 0): try: # Digests and such are computed using the RELAY cell payload. - cell = stem.client.cell.RelayCell(self.id, command, data, 0, stream_id) - payload_without_digest = cell.pack(self.relay.link_protocol)[-stem.client.cell.FIXED_PAYLOAD_LEN:] + + _, command_int = stem.client.datatype.RelayCommand.get(command) + # coerce to bytes, just in case + data = str_tools._to_bytes(data) + + payload_without_digest = stem.client.cell.RelayCell._pack_payload(command_int, 0, stream_id, 0, len(data), data) self.forward_digest.update(payload_without_digest) + payload_with_digest = stem.client.cell.RelayCell._pack_payload(command_int, 0, stream_id, self.forward_digest, len(data), data) - cell = stem.client.cell.RelayCell(self.id, command, data, self.forward_digest, stream_id) - payload_with_digest = cell.pack(self.relay.link_protocol)[-stem.client.cell.FIXED_PAYLOAD_LEN:] encrypted_payload = self.forward_key.update(payload_with_digest) encrypted_cell = stem.client.cell.RawRelayCell(self.id, encrypted_payload) From c342fb84bac39dc7359943cde32a697d28a506a6 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 11 Aug 2018 19:18:09 +0000 Subject: [PATCH 15/28] Fix docstring 'import path' for RelayCommand --- stem/client/__init__.py | 2 +- stem/client/cell.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 83c20365d..b0500c4a9 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -227,7 +227,7 @@ def send(self, command, data = '', stream_id = 0): """ Sends a message over the circuit. - :param stem.client.RelayCommand command: command to be issued + :param stem.client.datatype.RelayCommand command: command to be issued :param bytes data: message payload :param int stream_id: specific stream this concerns diff --git a/stem/client/cell.py b/stem/client/cell.py index 42b955718..381a73875 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -366,7 +366,7 @@ class RelayCell(CircuitCell): """ Command concerning a relay circuit. - :var stem.client.RelayCommand command: command to be issued + :var stem.client.datatype.RelayCommand command: command to be issued :var int command_int: integer value of our command :var bytes data: payload of the cell :var int recognized: zero if cell is decrypted, otherwise mostly non-zero (can rarely be zero) From 1a61269af72c9cca0afa43d7e93c2fa63fdae68c Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 10 Aug 2018 19:55:22 +0000 Subject: [PATCH 16/28] Add convenience method for packing payload of a RELAY cell There may be a bit too much going on here - i.e. these methods may not be anything but a bit redundant - but things will get trimmed down later when the API is better worked out. --- stem/client/cell.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 381a73875..39a729f1f 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -424,7 +424,7 @@ def _coerce_digest(digest): return digest def pack(self, link_protocol): - payload = RelayCell._pack_payload(self.command_int, self.recognized, self.stream_id, self.digest, len(self.data), self.data, self.unused) + payload = self.pack_payload() return RelayCell._pack(link_protocol, payload, unused = b'', circ_id = self.circ_id) @@ -456,6 +456,17 @@ def _unpack_payload(content): return command, recognized, stream_id, digest, data_len, data, unused + def pack_payload(self, **kwargs): + """ + Convenience method for running _pack_payload on self. + + :param bool pad_remaining: (optional, defaults to **True**) pads up to payload size if **True** + + :returns: **bytes** with the packed payload + """ + + return RelayCell._pack_payload(self.command_int, self.recognized, self.stream_id, self.digest, len(self.data), self.data, self.unused, **kwargs) + @staticmethod def _pack_payload(command_int, recognized, stream_id, digest, data_len, data, unused = b'', pad_remainder = True): """ From f0cdac302d07607cf256f43c557ea199b7f80d9e Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 10 Aug 2018 20:59:04 +0000 Subject: [PATCH 17/28] Add apply_digest instance method to RelayCell This method - currently unused - allows moving some of the digest-application logic into the Cell abstraction layer. It is a bit weird for it to have side-effects on a parameter (digest), but it's documented and works fairly logically. --- stem/client/cell.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index 39a729f1f..c1751ed60 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -456,6 +456,35 @@ def _unpack_payload(content): return command, recognized, stream_id, digest, data_len, data, unused + def apply_digest(self, digest, prep_cell = True): + """ + Calculates, updates, and applies the digest to the cell payload. + + :param HASH digest: running digest held with the relay + :param bool prep_cell: preps the cell payload according to the spec, if **True** (default) + if **False**, the digest will be calculated as-is, namely: + the 'recognized' field will not be set to 0, + the digest field will not be set to 0, + and any 'unused' padding will be taken as-is. + Use with caution. + + :sideeffect digest: this object will be updated via digest.update(payload) + :sideeffect self.recognized: this will be set to 0, if prep_cell is **True** + :sideeffect self.digest: this will be updated with the calculated digest + :sideeffect self.unused: this will be treated as padding and overwritten, if prep_cell is **True** + """ + + if prep_cell: + self.recognized = 0 + self.digest = 0 + self.unused = b'' + + payload_without_updated_digest = self.pack_payload() + digest.update(payload_without_updated_digest) + self.digest = RelayCell._coerce_digest(digest) + + return + def pack_payload(self, **kwargs): """ Convenience method for running _pack_payload on self. From 7e596921ce5af7645e8ecf5ce5624a7ae6ae6d22 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 10 Aug 2018 21:22:35 +0000 Subject: [PATCH 18/28] Further simplify digest application The abstraction layers are starting to look better separated. --- stem/client/__init__.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index b0500c4a9..74b6d2bfd 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -32,7 +32,6 @@ import stem import stem.client.cell import stem.socket -from stem.util import str_tools import stem.util.connection from stem.client.datatype import ZERO, LinkProtocol, Address, KDF @@ -239,15 +238,9 @@ def send(self, command, data = '', stream_id = 0): orig_key = copy.copy(self.forward_key) try: - # Digests and such are computed using the RELAY cell payload. - - _, command_int = stem.client.datatype.RelayCommand.get(command) - # coerce to bytes, just in case - data = str_tools._to_bytes(data) - - payload_without_digest = stem.client.cell.RelayCell._pack_payload(command_int, 0, stream_id, 0, len(data), data) - self.forward_digest.update(payload_without_digest) - payload_with_digest = stem.client.cell.RelayCell._pack_payload(command_int, 0, stream_id, self.forward_digest, len(data), data) + cell = stem.client.cell.RelayCell(self.id, command, data, stream_id = stream_id) + cell.apply_digest(self.forward_digest) + payload_with_digest = cell.pack_payload() encrypted_payload = self.forward_key.update(payload_with_digest) encrypted_cell = stem.client.cell.RawRelayCell(self.id, encrypted_payload) From b47ea8edacdff2d4a3644e95af9c40a6c3536a5d Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Wed, 15 Aug 2018 03:28:03 +0000 Subject: [PATCH 19/28] Refactor apply_digest to remove side effects Per discussion with Damian over IRC, instances of our Cell classes are intended to be immutable objects. So instead of modifying the RelayCell and having an additional side effect on the digest, this provides a modified copy of each, in a tuple. That makes apply_digest() a pure function, currently with referential transparency (i.e. also deterministic). In the future, it may utilize randomness for cell padding, which would make it non-deterministic. Overall this now more generally follows a functional-programming paradigm. --- stem/client/__init__.py | 4 ++-- stem/client/cell.py | 33 +++++++++++++++++++++------------ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 74b6d2bfd..b64956ff3 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -239,8 +239,8 @@ def send(self, command, data = '', stream_id = 0): try: cell = stem.client.cell.RelayCell(self.id, command, data, stream_id = stream_id) - cell.apply_digest(self.forward_digest) - payload_with_digest = cell.pack_payload() + cell_with_digest, self.forward_digest = cell.apply_digest(self.forward_digest) + payload_with_digest = cell_with_digest.pack_payload() encrypted_payload = self.forward_key.update(payload_with_digest) encrypted_cell = stem.client.cell.RawRelayCell(self.id, encrypted_payload) diff --git a/stem/client/cell.py b/stem/client/cell.py index c1751ed60..fec8bb1b6 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -458,7 +458,7 @@ def _unpack_payload(content): def apply_digest(self, digest, prep_cell = True): """ - Calculates, updates, and applies the digest to the cell payload. + Calculates, updates, and applies the digest to the cell payload, returning a new (cell, digest) tuple. :param HASH digest: running digest held with the relay :param bool prep_cell: preps the cell payload according to the spec, if **True** (default) @@ -468,22 +468,31 @@ def apply_digest(self, digest, prep_cell = True): and any 'unused' padding will be taken as-is. Use with caution. - :sideeffect digest: this object will be updated via digest.update(payload) - :sideeffect self.recognized: this will be set to 0, if prep_cell is **True** - :sideeffect self.digest: this will be updated with the calculated digest - :sideeffect self.unused: this will be treated as padding and overwritten, if prep_cell is **True** + :returns: (:class:`~stem.client.cell.RelayCell`, HASH) tuple updated as follows: + digest: updated via digest.update(payload) + RelayCell: a copy of self, with the following updates: + RelayCell.recognized: set to 0, if prep_cell is **True** + RelayCell.digest: updated with the calculated digest + RelayCell.unused: treated as padding and overwritten, if prep_cell is **True** """ if prep_cell: - self.recognized = 0 - self.digest = 0 - self.unused = b'' + new_cell_recognized = 0 + new_cell_digest = 0 + new_cell_unused = b'' + else: + new_cell_recognized = self.recognized + new_cell_digest = self.digest + new_cell_unused = self.unused + + new_digest = digest.copy() + new_cell = RelayCell(self.circ_id, self.command, self.data, digest = new_cell_digest, stream_id = self.stream_id, recognized = new_cell_recognized, unused = new_cell_unused) - payload_without_updated_digest = self.pack_payload() - digest.update(payload_without_updated_digest) - self.digest = RelayCell._coerce_digest(digest) + payload_without_updated_digest = new_cell.pack_payload() + new_digest.update(payload_without_updated_digest) + new_cell.digest = RelayCell._coerce_digest(new_digest) - return + return new_cell, new_digest def pack_payload(self, **kwargs): """ From 52a3632433fc972ac40071a505fb0a5c0df0a013 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 17 Aug 2018 20:51:00 +0000 Subject: [PATCH 20/28] Update stem.client.cell docstrings for style, consistency, and clarity Much of the updates here is to reduce the line length. --- stem/client/cell.py | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index fec8bb1b6..d1265c10a 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -369,7 +369,8 @@ class RelayCell(CircuitCell): :var stem.client.datatype.RelayCommand command: command to be issued :var int command_int: integer value of our command :var bytes data: payload of the cell - :var int recognized: zero if cell is decrypted, otherwise mostly non-zero (can rarely be zero) + :var int recognized: zero if cell is decrypted, otherwise mostly non-zero + (can rarely be zero) :var int digest: running digest held with the relay :var int stream_id: specific stream this concerns """ @@ -458,22 +459,26 @@ def _unpack_payload(content): def apply_digest(self, digest, prep_cell = True): """ - Calculates, updates, and applies the digest to the cell payload, returning a new (cell, digest) tuple. + Calculates, updates, and applies the digest to the cell payload, + returning a new (cell, digest) tuple. :param HASH digest: running digest held with the relay - :param bool prep_cell: preps the cell payload according to the spec, if **True** (default) + :param bool prep_cell: preps the cell payload according to the spec, if + **True** (default) if **False**, the digest will be calculated as-is, namely: - the 'recognized' field will not be set to 0, - the digest field will not be set to 0, - and any 'unused' padding will be taken as-is. - Use with caution. - - :returns: (:class:`~stem.client.cell.RelayCell`, HASH) tuple updated as follows: - digest: updated via digest.update(payload) - RelayCell: a copy of self, with the following updates: - RelayCell.recognized: set to 0, if prep_cell is **True** - RelayCell.digest: updated with the calculated digest - RelayCell.unused: treated as padding and overwritten, if prep_cell is **True** + * the 'recognized' field will not be set to 0, + * the digest field will not be set to 0, + * and any 'unused' padding will be taken as-is. + Use **False** with caution. + + :returns: (:class:`~stem.client.cell.RelayCell`, HASH) tuple of object + copies updated as follows: + * digest: updated via digest.update(payload) + * RelayCell: a copy of self, with the following updates: + * RelayCell.recognized: set to 0, if prep_cell is **True** + * RelayCell.digest: updated with the calculated digest + * RelayCell.unused: treated as padding and overwritten, if prep_cell + is **True** """ if prep_cell: @@ -496,9 +501,11 @@ def apply_digest(self, digest, prep_cell = True): def pack_payload(self, **kwargs): """ - Convenience method for running _pack_payload on self. + Convenience method for running + :func:`~stem.client.cell.RelayCell._pack_payload` on self. - :param bool pad_remaining: (optional, defaults to **True**) pads up to payload size if **True** + :param bool pad_remaining: (optional, defaults to **True**) pads up to + payload size if **True** :returns: **bytes** with the packed payload """ @@ -511,7 +518,8 @@ def _pack_payload(command_int, recognized, stream_id, digest, data_len, data, un Directly pack the payload without any validation beyond Size constraints. :param int command_int: integer value of our command - :param int recognized: zero if cell is decrypted, otherwise mostly non-zero (can rarely be zero) + :param int recognized: zero if cell is decrypted, otherwise mostly non-zero + (can rarely be zero) :param int stream_id: specific stream this concerns :param HASH,str,int digest: running digest held with the relay :param int data_len: length of body data From ec56c05ed59ee032432521a8217667b9d6698cea Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Fri, 17 Aug 2018 19:30:56 +0000 Subject: [PATCH 21/28] Refactor RELAY cell encryption into new encrypt instance method This further tunes the abstraction layers such that consumers don't need to care at all what part of the Cell is encrypted. They are still responsible for managing the digest/encryption key states, but don't need to know the finer details of the encryption. --- stem/client/__init__.py | 6 +----- stem/client/cell.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index b64956ff3..a228bebcf 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -239,11 +239,7 @@ def send(self, command, data = '', stream_id = 0): try: cell = stem.client.cell.RelayCell(self.id, command, data, stream_id = stream_id) - cell_with_digest, self.forward_digest = cell.apply_digest(self.forward_digest) - payload_with_digest = cell_with_digest.pack_payload() - - encrypted_payload = self.forward_key.update(payload_with_digest) - encrypted_cell = stem.client.cell.RawRelayCell(self.id, encrypted_payload) + encrypted_cell, self.forward_digest, self.forward_key = cell.encrypt(self.forward_digest, self.forward_key) reply_cells = [] self.relay._orport.send(encrypted_cell.pack(self.relay.link_protocol)) diff --git a/stem/client/cell.py b/stem/client/cell.py index d1265c10a..43dc076c9 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -39,6 +39,7 @@ +- pop - decodes cell with remainder """ +import copy import datetime import inspect import os @@ -499,6 +500,40 @@ def apply_digest(self, digest, prep_cell = True): return new_cell, new_digest + def encrypt(self, digest, encryptor, **kwargs): + """ + Preps a cell payload, including calculating digest, and encrypts it, + returning a new (RawRelayCell, digest, encryptor) tuple. + + The method name is technically a misnomer, as it also preps cell payload + and applies the digest, prior to encrypting. However, these operations + are defined per the spec as required for RELAY cells, and ... + (1) it is a natural mental extension to include them here; + (2) it would be a bit pointless to require method consumers to manually + call both, for pedantry. + + :param HASH digest: running digest held with the relay + :param cryptography.hazmat.primitives.ciphers.CipherContext encryptor: + running stream cipher encryptor held with the relay + + :param bool prep_cell: (optional, defaults to **True**) refer to + :func:`~stem.client.cell.RelayCell.apply_digest` + + :returns: (:class:`~stem.client.cell.RawRelayCell`, HASH, CipherContext) + tuple of object copies updated as follows: + * RawRelayCell: updated as specified in + :func:`~stem.client.cell.RelayCell.apply_digest`, then encrypted + * digest: updated via digest.update(payload) + * encryptor: updated via encryptor.update(payload_with_digest) + """ + + unencrypted_cell, new_digest = self.apply_digest(digest, **kwargs) + new_encryptor = copy.copy(encryptor) + encrypted_payload = new_encryptor.update(unencrypted_cell.pack_payload()) + encrypted_cell = RawRelayCell(unencrypted_cell.circ_id, encrypted_payload) + + return encrypted_cell, new_digest, new_encryptor + def pack_payload(self, **kwargs): """ Convenience method for running From b9877f3bddfa451342bb94ec20a5db424d5b41bc Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 18 Aug 2018 02:40:01 +0000 Subject: [PATCH 22/28] Add check_digest instance method to BaseRelayCell Similar to how apply_digest() is used by a RELAY cell sender, this collects the logic needed for a receiver to confirm that a cell is fully decrypted and that the cell's integrity is intact. This is new functionality. Prior to this change, stem.client had no facility to check the digest of a RELAY cell according to the spec. Not yet used. --- stem/client/cell.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index 43dc076c9..5235331e1 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -354,6 +354,44 @@ def _unpack(cls, content, circ_id, link_protocol): # unlike everywhere else, we actually want to use the subclass type, NOT *this* class return cls(circ_id, content) + def check_digest(self, digest): + """ + Calculates the running digest of the cell payload per the spec, returning + whether the cell's unpacked digest matched, along with the updated digest + if so. + + :param HASH digest: running digest held with the relay + + :returns: (digest_matches, digest) tuple of object copies updated as follows: + * digest_matches: **bool** indicating whether the digest matches + * digest: updated via digest.update(payload), if the digest matches; + otherwise a copy of the original + + :raises: **ValueError** if payload is the wrong size + """ + + command, recognized, stream_id, digest_from_cell, data_len, data, unused = RelayCell._unpack_payload(self.payload) + + # running digest is calculated using a zero'd digest field in the payload + prepared_payload = RelayCell._pack_payload(command, recognized, stream_id, 0, data_len, data, unused, pad_remainder = False) + + if len(prepared_payload) != FIXED_PAYLOAD_LEN: + # this should never fail + # if it did, it indicates a programming error either within stem.client.cell or a consumer + raise ValueError('Payload should be %i bytes, but was %i' % (FIXED_PAYLOAD_LEN, len(prepared_payload))) + + new_digest = digest.copy() + new_digest.update(prepared_payload) + + digest_matches = (RelayCell._coerce_digest(new_digest) == digest_from_cell) + + # only return the new_digest if the digest check passed + # even if not, return a copy of the original + # this allows a consumer to always assume the returned digest is a different object + digest_to_return = new_digest if digest_matches else digest.copy() + + return digest_matches, digest_to_return + def __hash__(self): return stem.util._hash_attr(self, 'circ_id', 'payload', cache = True) From caffd27f5b58c70506e0b89abe119421071239d4 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sun, 19 Aug 2018 03:48:05 +0000 Subject: [PATCH 23/28] Add check_recognized_field instance method to BaseRelayCell This is new functionality. (Nothing in stem.client currently attempts to check the 'recognized' field.) Not yet used. This is arguably a very inefficient method, and it might even obscure its purpose by existing (it's extremely simple), but it seemed better to break it out and deal with the foreseeable performance hit than to trend towards premature optimization. (It also is a bit counterintuitive for a value of 0 to mean True, so breaking this out makes that clearer.) There are many ways this could be optimized, and - especially since so much of this unit is still in flux - it doesn't make sense yet to do so. --- stem/client/cell.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index 5235331e1..250d400a7 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -354,6 +354,22 @@ def _unpack(cls, content, circ_id, link_protocol): # unlike everywhere else, we actually want to use the subclass type, NOT *this* class return cls(circ_id, content) + def check_recognized_field(self): + """ + Checks the 'recognized' field of the cell payload, which indicates whether + it is **probably** fully decrypted. + + :returns: **bool** indicating whether the 'recognized' field indicates + likely decryption. Per the spec: + * **False** guarantees the cell *not* to be fully decrypted. + * **True** does *not* guarantee the cell to be fully decrypted, and it + must be checked further. See also + :func:`~stem.client.cell.BaseRelayCell.check_digest` + """ + + _, recognized_from_cell, _, _, _, _, _ = RelayCell._unpack_payload(self.payload) + return recognized_from_cell == 0 + def check_digest(self, digest): """ Calculates the running digest of the cell payload per the spec, returning From 0798b518bb13028d8044d28a8146a6066f30c4d2 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sun, 19 Aug 2018 04:32:52 +0000 Subject: [PATCH 24/28] Add interpret_cell instance method to BaseRelayCell This method facilitates separating decryption / recognition of a cell from the interpretation of a cell. Many places in the spec indicate that cells should be dropped (ignored) under certain conditions, but that such cells should still be accounted for within the running decryption / digest. Furthermore, it may be useful in some cases to convert between raw cells and interpreted cells, without including the other steps of encryption / decryption. This additionally allows removing one of the interim TODOs in the RELAY cell unit tests. Arguably the tests need some rework with all these changes, but that can come a bit later. Not yet used, beyond the aforementioned unit tests. This method will probably evolve a decent amount, since the unit is still in a state of high flux. --- stem/client/cell.py | 35 +++++++++++++++++++++++++++++++++++ test/unit/client/cell.py | 4 ++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/stem/client/cell.py b/stem/client/cell.py index 250d400a7..a96b2f7bc 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -408,6 +408,41 @@ def check_digest(self, digest): return digest_matches, digest_to_return + def interpret_cell(self): + """ + Interprets the cell payload, returning a new + :class:`~stem.client.cell.RelayCell` class or subclass according to its + contents. + + This method should only be used on fully decrypted cells, but that + responsibility is relegated to the caller. + + Furthermore, this interpretation may cause an exception for a NYI relay + command, a malformed cell, or some other reason. + + :returns: :class:`~stem.client.cell.RelayCell` class or subclass + """ + + # TODO: this mapping is quite hardcoded right now, but probably needs to be + # completely reworked once the Cell class hierarchy is better fleshed out. + # + # (It doesn't really make sense to have anything beyond this hack in the + # interim.) + # + # At that time, it would probably be modeled after Cell.by_value(), albeit + # specialized for the multiple types of RELAY / RELAY_EARLY cells. + + relay_cells_by_value = { + RawRelayCell.VALUE: RelayCell, + RelayEarlyCell.VALUE: RelayEarlyCell, + } + new_cls = relay_cells_by_value[self.VALUE] + + dummy_link_protocol = None + new_cell = new_cls._unpack(self.payload, self.circ_id, dummy_link_protocol) + + return new_cell + def __hash__(self): return stem.util._hash_attr(self, 'circ_id', 'payload', cache = True) diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py index 3e091ee55..b4c0bd94b 100644 --- a/test/unit/client/cell.py +++ b/test/unit/client/cell.py @@ -235,12 +235,12 @@ def test_relay_cell(self): self.assertEqual(cell_bytes, RelayCell(circ_id, command, data, digest, stream_id, unused = unused).pack(link_protocol)) self.assertEqual(cell_bytes, RelayCell(circ_id, command_int, data, digest, stream_id, unused = unused).pack(link_protocol)) - # TODO - temporarily, we hack the interim tests by unpacking info via RawRelayCell + # first unpack via RawRelayCell, then interpret into RelayCell raw_cell = Cell.pop(cell_bytes, link_protocol)[0] self.assertEqual(circ_id, raw_cell.circ_id) self.assertEqual(cell_bytes[-FIXED_PAYLOAD_LEN:], raw_cell.payload) - cell = RelayCell._unpack(raw_cell.payload, raw_cell.circ_id, link_protocol) + cell = raw_cell.interpret_cell() self.assertEqual(circ_id, cell.circ_id) self.assertEqual(command, cell.command) self.assertEqual(command_int, cell.command_int) From ae7feeac6d2f6adb632dd145e7534ab86314baca Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 18 Aug 2018 03:58:13 +0000 Subject: [PATCH 25/28] Add decrypt instance method to BaseRelayCell Akin to encrypt(), this takes care of decryption and all the ancillary functionality related to it. It composes the functionality of the building blocks added in previous changes, namely: * check_recognized_field() * check_digest() * interpret_cell() - but not automatically done by default A consumer of this, especially in a multi-hop circuit, will only need to manage the digest/decryptor state (in the case of a cell that cannot be decrypted+recognized... fallback) and pay attention to the 'fully_decrypted' result. This should make a consumer's implementation pretty readable, something like: # relay_1 := nearest relay in circuit decryption_states = [ (relay_1, digest_1, decryptor_1), (relay_2, digest_2, decryptor_2), (relay_3, digest_3, decryptor_3), ] new_decryption_states = copy.deepcopy(decryption_states) from_relay = None for i in range(len(new_decryption_states)): relay, digest, decryptor = new_decryption_states[i] cell, fully_decrypted, digest, decryptor = cell.decrypt(digest, decryptor) new_decryption_states[i] = (relay, digest, decryptor) if fully_decrypted: from_relay = relay break if from_relay: decryption_states = new_decryption_states else: # handle this, since the cell couldn't be decrypted # probably raise Something() pass cell_and_hop = (cell, from_relay) # naming things is hard # do something with the decrypted cell # ^ probably feeding it into the queue/handler for the (circuit_id, relay, stream_id) --- stem/client/cell.py | 65 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/stem/client/cell.py b/stem/client/cell.py index a96b2f7bc..48dbcf369 100644 --- a/stem/client/cell.py +++ b/stem/client/cell.py @@ -443,6 +443,71 @@ def interpret_cell(self): return new_cell + def decrypt(self, digest, decryptor, interpret = False): + """ + Decrypts a cell and checks whether it is fully decrypted, + returning a new (Cell, fully_decrypted, digest, decryptor) tuple. + Optionally also interprets the cell (not generally recommended). + + The method name is technically a misnomer, as it also checks whether the + cell has been fully decrypted (after decrypting), updating the digest if so. + However, these operations are defined per the spec as required for RELAY + cells, and ... + (1) it is a natural mental extension to include them here; + (2) it would be a bit pointless to require method consumers to manually + do all of that, for pedantry. + + :param HASH digest: running digest held with the relay + :param cryptography.hazmat.primitives.ciphers.CipherContext decryptor: + running stream cipher decryptor held with the relay + + :param bool interpret: (optional, defaults to **False**) Use **True** with + caution. The spec indicates that a fully decrypted cell should be + accounted for in digest and decryptor, independent of cell validity. Using + **True**, while convenient, may cause an exception for a NYI relay + command, a malformed cell, or some other reason. This option should only + be used when the consumer will consider the circuit to have a fatal error + in such cases, and catches/handles the exception accordingly (e.g. sending + a DestroyCell). + + :returns: (:class:`~stem.client.cell.Cell`, bool, HASH, CipherContext) tuple + of object copies updated as follows: + * Cell: either :class:`~stem.client.cell.RawRelayCell` with a decrypted + payload or :class:`~stem.client.cell.RelayCell` class or subclass, if + **interpret** is **True** and the cell was fully decrypted + * fully_decrypted: **bool** indicating whether the cell is fully + decrypted + * digest: updated via digest.update(payload), if the cell was fully + decrypted; otherwise a copy of the original + * decryptor: updated via decryptor.update(payload) + """ + + new_decryptor = copy.copy(decryptor) + + # actually decrypt + decrypted_payload = new_decryptor.update(self.payload) + new_cell = self.__class__(self.circ_id, decrypted_payload) + + # do post-decryption checks to ascertain whether cell is fully decrypted + if new_cell.check_recognized_field(): + digest_matches, new_digest = new_cell.check_digest(digest) + fully_decrypted = digest_matches + else: + new_digest = None + fully_decrypted = False + + # only return the new_digest if the digest check meant that the cell has been fully decrypted + # + # furthermore, even if the digest was not updated, return a copy + # this allows a consumer to always assume the returned digest is a different object + digest_to_return = new_digest if fully_decrypted else digest.copy() + + if interpret and fully_decrypted: + # this might raise an exception; oh well, we did warn about that + new_cell = new_cell.interpret_cell() + + return new_cell, fully_decrypted, digest_to_return, new_decryptor + def __hash__(self): return stem.util._hash_attr(self, 'circ_id', 'payload', cache = True) From a9128d22f235e248691e5e33f34b18d8cd0c18e3 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 18 Aug 2018 04:12:35 +0000 Subject: [PATCH 26/28] Now use the decrypt method in our Circuit class This also now actually checks the 'recognized' field and digest, unlike before. Keep in mind that this implementation is really meant for illustrative purposes, in the interim. (There are some limitations to it.) --- stem/client/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index a228bebcf..33ac0d8d7 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -255,8 +255,11 @@ def send(self, command, data = '', stream_id = 0): elif raw_cell.circ_id != self.id: raise stem.ProtocolError('Response should be for circuit id %i, not %i' % (self.id, raw_cell.circ_id)) - decrypted_payload = self.backward_key.update(raw_cell.payload) - reply_cells.append(stem.client.cell.RelayCell._unpack(decrypted_payload, self.id, self.relay.link_protocol)) + decrypted_cell, fully_decrypted, self.backward_digest, self.backward_key = raw_cell.decrypt(self.backward_digest, self.backward_key, interpret = True) + if not fully_decrypted: + raise stem.ProtocolError('Response for circuit id %i was not fully decrypted, when expected to be' % self.id) + + reply_cells.append(decrypted_cell) return reply_cells except: From ce161c647101bbd9a43effd0952fbb5b453c6941 Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 18 Aug 2018 04:14:43 +0000 Subject: [PATCH 27/28] Actually make copies of the backward digest/key, too And rename the other 'orig_' vars to 'orig_forward_' vars, for clarity. --- stem/client/__init__.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 33ac0d8d7..91e8c4ca5 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -234,8 +234,10 @@ def send(self, command, data = '', stream_id = 0): """ with self.relay._orport_lock: - orig_digest = self.forward_digest.copy() - orig_key = copy.copy(self.forward_key) + orig_forward_digest = self.forward_digest.copy() + orig_forward_key = copy.copy(self.forward_key) + orig_backward_digest = self.backward_digest.copy() + orig_backward_key = copy.copy(self.backward_key) try: cell = stem.client.cell.RelayCell(self.id, command, data, stream_id = stream_id) @@ -263,8 +265,10 @@ def send(self, command, data = '', stream_id = 0): return reply_cells except: - self.forward_digest = orig_digest - self.forward_key = orig_key + self.forward_digest = orig_forward_digest + self.forward_key = orig_forward_key + self.backward_digest = orig_backward_digest + self.backward_key = orig_backward_key raise def close(self): From 0be343dfdc9cde3481b502d7826ffbe848831a6b Mon Sep 17 00:00:00 2001 From: Dave Rolek Date: Sat, 18 Aug 2018 04:26:42 +0000 Subject: [PATCH 28/28] Rearrange the digest/key copies to be a bit more spec-compliant This arrangement is better, as it better keeps track of the digest/key on a cell-by-cell basis, but it's still not spec-compliant in a number of ways, such as its potential to drop cells or get corrupted backward digest/key state in the case of an exception. That's ok - this is just an interim implementation, and this change is just to make things a bit better for illustrative purposes. --- stem/client/__init__.py | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/stem/client/__init__.py b/stem/client/__init__.py index 91e8c4ca5..8c96b8c29 100644 --- a/stem/client/__init__.py +++ b/stem/client/__init__.py @@ -236,20 +236,27 @@ def send(self, command, data = '', stream_id = 0): with self.relay._orport_lock: orig_forward_digest = self.forward_digest.copy() orig_forward_key = copy.copy(self.forward_key) - orig_backward_digest = self.backward_digest.copy() - orig_backward_key = copy.copy(self.backward_key) try: cell = stem.client.cell.RelayCell(self.id, command, data, stream_id = stream_id) encrypted_cell, self.forward_digest, self.forward_key = cell.encrypt(self.forward_digest, self.forward_key) - reply_cells = [] self.relay._orport.send(encrypted_cell.pack(self.relay.link_protocol)) - reply = self.relay._orport.recv() + except: + self.forward_digest = orig_forward_digest + self.forward_key = orig_forward_key + raise + + reply = self.relay._orport.recv() + reply_cells = [] - relay_cell_cmd = stem.client.cell.RelayCell.VALUE + relay_cell_cmd = stem.client.cell.RelayCell.VALUE - while reply: + while reply: + orig_backward_digest = self.backward_digest.copy() + orig_backward_key = copy.copy(self.backward_key) + + try: raw_cell, reply = stem.client.cell.Cell.pop(reply, self.relay.link_protocol) if raw_cell.VALUE != relay_cell_cmd: @@ -260,16 +267,14 @@ def send(self, command, data = '', stream_id = 0): decrypted_cell, fully_decrypted, self.backward_digest, self.backward_key = raw_cell.decrypt(self.backward_digest, self.backward_key, interpret = True) if not fully_decrypted: raise stem.ProtocolError('Response for circuit id %i was not fully decrypted, when expected to be' % self.id) + except: + self.backward_digest = orig_backward_digest + self.backward_key = orig_backward_key + raise - reply_cells.append(decrypted_cell) + reply_cells.append(decrypted_cell) - return reply_cells - except: - self.forward_digest = orig_forward_digest - self.forward_key = orig_forward_key - self.backward_digest = orig_backward_digest - self.backward_key = orig_backward_key - raise + return reply_cells def close(self): with self.relay._orport_lock: