commit ad269657fe16f2603f207bdb2a979266c3b918e8 Author: Isis Lovecruft isis@torproject.org Date: Fri Dec 22 02:34:32 2017 +0000

Filter returned bridges to avoid multiples from the same /16 or /32.

* FIXES #24704 https://bugs.torproject.org/24704 --- bridgedb/Bridges.py | 52 ++++++++++++++++++++---- bridgedb/distributors/email/distributor.py | 2 +- bridgedb/distributors/https/distributor.py | 2 +- bridgedb/test/test_Bridges.py | 64 ++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/bridgedb/Bridges.py b/bridgedb/Bridges.py index 13bffd2..dfc6ac5 100644 --- a/bridgedb/Bridges.py +++ b/bridgedb/Bridges.py @@ -268,7 +268,39 @@ class BridgeRing(object): assert len(r) == N return r

- def getBridges(self, pos, N=1): + def filterDistinctSubnets(self, fingerprints): + """Given a chosen set of ``fingerprints`` of bridges to distribute, + filter the bridges such that they are in distinct subnets. + """ + logging.debug("Got %d possible bridges to filter" % len(fingerprints)) + + bridges = [] + subnets = [] + + for fingerprint in fingerprints: + bridge = self.bridges[fingerprint] + jump = False + for subnet in subnets: + if bridge.address in subnet: + jump = True + logging.debug( + ("Skipping distribution of bridge %s in a subnet which " + "contains another bridge we're already distributing") + % bridge) + break + if jump: + continue + + bridges.append(bridge) + if bridge.address.version == 4: + cidr = str(bridge.address) + "/16" + else: + cidr = str(bridge.address) + "/32" + subnets.append(ipaddr.IPNetwork(cidr)) + + return bridges + + def getBridges(self, pos, N=1, filterBySubnet=False): """Return **N** bridges appearing in this hashring after a position.

:param bytes pos: The position to jump to. Any bridges returned will @@ -285,19 +317,25 @@ class BridgeRing(object): count = len(subring) forced.extend(subring._getBridgeKeysAt(pos, count))

- keys = [ ] - for k in forced + self._getBridgeKeysAt(pos, N): + keys = [] + + # Oversample double the number we need, in case we need to + # filter them and some are within the same subnet. + for k in forced + self._getBridgeKeysAt(pos, N + N): if k not in keys: keys.append(k) else: logging.debug( "Got duplicate bridge %r in main hashring for position %r." % (logSafely(k.encode('hex')), pos.encode('hex'))) - keys = keys[:N] keys.sort()

- #Do not return bridges from the same /16 - bridges = [ self.bridges[k] for k in keys ] + if filterBySubnet: + bridges = self.filterDistinctSubnets(keys) + else: + bridges = [self.bridges[k] for k in keys] + + bridges = bridges[:N]

return bridges

@@ -551,7 +589,7 @@ class FilteredBridgeSplitter(object): For all sub-hashrings, the ``bridge`` will only be added iff it passes the filter functions for that sub-hashring.

- :type bridge: :class:`~bridgedb.Bridges.Bridge` + :type bridge: :class:`~bridgedb.bridges.Bridge` :param bridge: The bridge to add. """ # The bridge must be running to insert it: diff --git a/bridgedb/distributors/email/distributor.py b/bridgedb/distributors/email/distributor.py index b76c26c..fbf1a50 100644 --- a/bridgedb/distributors/email/distributor.py +++ b/bridgedb/distributors/email/distributor.py @@ -191,7 +191,7 @@ class EmailDistributor(Distributor): populate_from=self.hashring.bridges)

returnNum = self.bridgesPerResponse(ring) - result = ring.getBridges(pos, returnNum) + result = ring.getBridges(pos, returnNum, filterBySubnet=False)

db.setEmailTime(bridgeRequest.client, now) db.commit() diff --git a/bridgedb/distributors/https/distributor.py b/bridgedb/distributors/https/distributor.py index 5ff9d83..7bce8d9 100644 --- a/bridgedb/distributors/https/distributor.py +++ b/bridgedb/distributors/https/distributor.py @@ -342,6 +342,6 @@ class HTTPSDistributor(Distributor):

# Determine the appropriate number of bridges to give to the client: returnNum = self.bridgesPerResponse(ring) - answer = ring.getBridges(position, returnNum) + answer = ring.getBridges(position, returnNum, filterBySubnet=True)

return answer diff --git a/bridgedb/test/test_Bridges.py b/bridgedb/test/test_Bridges.py new file mode 100644 index 0000000..092b033 --- /dev/null +++ b/bridgedb/test/test_Bridges.py @@ -0,0 +1,64 @@ +# -*- coding: utf-8 -*- +#_____________________________________________________________________________ +# +# This file is part of BridgeDB, a Tor bridge distribution system. +# +# :authors: Isis Lovecruft 0xA3ADB67A2CDB8B35 isis@torproject.org +# :copyright: (c) 2017, Isis Lovecruft +# (c) 2017, The Tor Project, Inc. +# :license: see LICENSE for licensing information +#_____________________________________________________________________________ + +"""Unittests for :mod:`bridgedb.Bridges`.""" + +from __future__ import print_function + +import copy +import ipaddr +import logging + +from twisted.trial import unittest + +from bridgedb import Bridges +from bridgedb.test import util + +# For additional logger output for debugging, comment out the following: +#logging.disable(50) +# and then uncomment the following line: +Bridges.logging.getLogger().setLevel(10) + + +class BridgeRingTests(unittest.TestCase): + """Unittests for :class:`bridgedb.Bridges.BridgeRing`.""" + + def setUp(self): + self.ring = Bridges.BridgeRing('fake-hmac-key') + + def addBridgesFromSameSubnet(self): + bridges = copy.deepcopy(util.generateFakeBridges()) + subnet = "5.5.%d.%d" + i = 1 + j = 1 + + for bridge in bridges: + bridge.address = ipaddr.IPAddress(subnet % (i, j)) + self.ring.insert(bridge) + + if j == 255: + j = 1 + i += 1 + else: + j += 1 + + def test_filterDistinctSubnets(self): + """If there are bridges in the same subnet then they should be + filtered out of the results. + """ + self.addBridgesFromSameSubnet() + + chosen = self.ring.bridges.keys()[:10] + bridges = self.ring.filterDistinctSubnets(chosen) + + # Since they're all in the same /16, we should only get one + # bridge back: + self.assertEqual(len(bridges), 1)