commit d561974fc1bf81e0ef2ffee9a93f19454b954f9c Author: Damian Johnson atagar@torproject.org Date: Mon Jan 2 18:31:45 2017 -0800
Stop checking consensus field ordering
Tor's spec details the order that consensus fields should appear in, but this is a detail tor frequently gets wrong. The spec's been changed to loosen this requirement...
https://trac.torproject.org/projects/tor/ticket/21059 https://gitweb.torproject.org/torspec.git/commit/?id=578477a
As such Stem should no longer validate this. New misordered additions in the live tor consensus have been causing our validator to alarm but with this it's once again happy.
Turns out the order check also masked some issues with our missing fields test. Took a while to figure out but in-place modification of our 'attr' subtly broke it. Addressing this with a defensive shallow copy. --- stem/descriptor/networkstatus.py | 41 +++-------------------- test/mocking.py | 14 ++++---- test/unit/descriptor/networkstatus/document_v3.py | 29 +--------------- 3 files changed, 11 insertions(+), 73 deletions(-)
diff --git a/stem/descriptor/networkstatus.py b/stem/descriptor/networkstatus.py index 815e5f5..d104ec9 100644 --- a/stem/descriptor/networkstatus.py +++ b/stem/descriptor/networkstatus.py @@ -132,9 +132,6 @@ FOOTER_STATUS_DOCUMENT_FIELDS = ( ('directory-signature', True, True, True), )
-HEADER_FIELDS = [attr[0] for attr in HEADER_STATUS_DOCUMENT_FIELDS] -FOOTER_FIELDS = [attr[0] for attr in FOOTER_STATUS_DOCUMENT_FIELDS] - AUTH_START = 'dir-source' ROUTERS_START = 'r' FOOTER_START = 'directory-footer' @@ -962,12 +959,13 @@ class NetworkStatusDocumentV3(NetworkStatusDocument): def _header(self, document_file, validate): content = bytes.join(b'', _read_until_keywords((AUTH_START, ROUTERS_START, FOOTER_START), document_file)) entries = _get_descriptor_components(content, validate) + header_fields = [attr[0] for attr in HEADER_STATUS_DOCUMENT_FIELDS]
if validate: # all known header fields can only appear once except
for keyword, values in list(entries.items()): - if len(values) > 1 and keyword in HEADER_FIELDS and keyword != 'package' and keyword != 'shared-rand-commit': + if len(values) > 1 and keyword in header_fields and keyword != 'package' and keyword != 'shared-rand-commit': raise ValueError("Network status documents can only have a single '%s' line, got %i" % (keyword, len(values)))
if self._default_params: @@ -976,7 +974,6 @@ class NetworkStatusDocumentV3(NetworkStatusDocument): self._parse(entries, validate, parser_for_line = self.HEADER_PARSER_FOR_LINE)
_check_for_missing_and_disallowed_fields(self, entries, HEADER_STATUS_DOCUMENT_FIELDS) - _check_for_misordered_fields(entries, HEADER_FIELDS)
# default consensus_method and consensus_methods based on if we're a consensus or vote
@@ -990,13 +987,14 @@ class NetworkStatusDocumentV3(NetworkStatusDocument):
def _footer(self, document_file, validate): entries = _get_descriptor_components(document_file.read(), validate) + footer_fields = [attr[0] for attr in FOOTER_STATUS_DOCUMENT_FIELDS]
if validate: for keyword, values in list(entries.items()): # all known footer fields can only appear once except... # * 'directory-signature' in a consensus
- if len(values) > 1 and keyword in FOOTER_FIELDS: + if len(values) > 1 and keyword in footer_fields: if not (keyword == 'directory-signature' and self.is_consensus): raise ValueError("Network status documents can only have a single '%s' line, got %i" % (keyword, len(values)))
@@ -1015,7 +1013,6 @@ class NetworkStatusDocumentV3(NetworkStatusDocument): raise ValueError("Network status document's footer should start with a 'directory-signature' line prior to consensus-method 9")
_check_for_missing_and_disallowed_fields(self, entries, FOOTER_STATUS_DOCUMENT_FIELDS) - _check_for_misordered_fields(entries, FOOTER_FIELDS) else: self._footer_entries = entries self._entries.update(entries) @@ -1087,36 +1084,6 @@ def _check_for_missing_and_disallowed_fields(document, entries, fields): raise ValueError("Network status document has fields that shouldn't appear in this document type or version: %s" % ', '.join(disallowed_fields))
-def _check_for_misordered_fields(entries, expected): - """ - To be valid a network status document's fiends need to appear in a specific - order. Checks that known fields appear in that order (unrecognized fields - are ignored). - - :param dict entries: ordered keyword/value mappings of the header or footer - :param list expected: ordered list of expected fields (either - **HEADER_FIELDS** or **FOOTER_FIELDS**) - - :raises: **ValueError** if entries aren't properly ordered - """ - - # Earlier validation has ensured that our fields either belong to our - # document type or are unknown. Remove the unknown fields since they - # reflect a spec change and can appear anywhere in the document. - - actual = [field for field in entries.keys() if field in expected] - - # Narrow the expected to just what we have. If the lists then match then the - # order's valid. - - expected = [field for field in expected if field in actual] - - if actual != expected: - actual_label = ', '.join(actual) - expected_label = ', '.join(expected) - raise ValueError("The fields in a section of the document are misordered. It should be '%s' but was '%s'" % (expected_label, actual_label)) - - def _parse_int_mappings(keyword, value, validate): # Parse a series of 'key=value' entries, checking the following: # - values are integers diff --git a/test/mocking.py b/test/mocking.py index 5a86690..7ffb11c 100644 --- a/test/mocking.py +++ b/test/mocking.py @@ -323,9 +323,7 @@ def _get_descriptor_content(attr = None, exclude = (), header_template = (), foo """
header_content, footer_content = [], [] - - if attr is None: - attr = {} + attr = {} if attr is None else dict(attr)
attr = OrderedDict(attr) # shallow copy since we're destructive
@@ -564,8 +562,7 @@ def get_directory_authority(attr = None, exclude = (), is_vote = False, content :returns: DirectoryAuthority for the requested descriptor content """
- if attr is None: - attr = {} + attr = {} if attr is None else dict(attr)
if not is_vote: # entries from a consensus also have a mandatory 'vote-digest' field @@ -637,8 +634,7 @@ def get_network_status_document_v3(attr = None, exclude = (), authorities = None :returns: NetworkStatusDocumentV3 for the requested descriptor content """
- if attr is None: - attr = {} + attr = {} if attr is None else dict(attr)
# add defaults only found in a vote, consensus, or microdescriptor
@@ -658,7 +654,9 @@ def get_network_status_document_v3(attr = None, exclude = (), authorities = None }
for k, v in extra_defaults.items(): - if not (k in attr or (exclude and k in exclude)): + if exclude and k in exclude: + continue # explicitly excluding this field + elif k not in attr: attr[k] = v
desc_content = _get_descriptor_content(attr, exclude, NETWORK_STATUS_DOCUMENT_HEADER, NETWORK_STATUS_DOCUMENT_FOOTER) diff --git a/test/unit/descriptor/networkstatus/document_v3.py b/test/unit/descriptor/networkstatus/document_v3.py index aae12a6..4bbc2ef 100644 --- a/test/unit/descriptor/networkstatus/document_v3.py +++ b/test/unit/descriptor/networkstatus/document_v3.py @@ -32,7 +32,6 @@ from test.mocking import ( get_router_status_entry_micro_v3, get_directory_authority, get_network_status_document_v3, - CRYPTO_BLOB, DOC_SIG, NETWORK_STATUS_DOCUMENT_FOOTER, ) @@ -474,7 +473,7 @@ DnN5aFtYKiTc19qIC7Nmo+afPdDEf0MlJvEOP5EWl3w=
for entries in (HEADER_STATUS_DOCUMENT_FIELDS, FOOTER_STATUS_DOCUMENT_FIELDS): for field, in_votes, in_consensus, is_mandatory in entries: - if is_mandatory and ((is_consensus and in_consensus) or (is_vote and in_votes)): + if is_mandatory and field != 'vote-status' and ((is_consensus and in_consensus) or (is_vote and in_votes)): content = get_network_status_document_v3(attr, exclude = (field,), content = True) self.assertRaises(ValueError, NetworkStatusDocumentV3, content, True) NetworkStatusDocumentV3(content, False) # constructs without validation @@ -487,32 +486,6 @@ DnN5aFtYKiTc19qIC7Nmo+afPdDEf0MlJvEOP5EWl3w= document = get_network_status_document_v3({'pepperjack': 'is oh so tasty!'}) self.assertEqual(['pepperjack is oh so tasty!'], document.get_unrecognized_lines())
- def test_misordered_fields(self): - """ - Rearranges our descriptor fields. - """ - - for is_consensus in (True, False): - attr = {'vote-status': 'consensus'} if is_consensus else {'vote-status': 'vote'} - lines = get_network_status_document_v3(attr, content = True).split(b'\n') - - for index in range(len(lines) - 1): - # once we reach the authority entry or later we're done since swapping - # those won't be detected - - if is_consensus and lines[index].startswith(stem.util.str_tools._to_bytes(CRYPTO_BLOB[1:10])): - break - elif not is_consensus and lines[index].startswith(b'dir-source'): - break - - # swaps this line with the one after it - test_lines = list(lines) - test_lines[index], test_lines[index + 1] = test_lines[index + 1], test_lines[index] - - content = b'\n'.join(test_lines) - self.assertRaises(ValueError, NetworkStatusDocumentV3, content, True) - NetworkStatusDocumentV3(content, False) # constructs without validation - def test_duplicate_fields(self): """ Almost all fields can only appear once. Checking that duplicates cause