Opened 5 years ago

Closed 5 years ago

#12872 closed enhancement (fixed)

Know within which country a bridge is located

Reported by: sysrqb Owned by: isis
Priority: Immediate Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: bridgedb-dist, easy, bridgedb-0.3.0, isis2014Q3Q4, isisExB
Cc: isis, sysrqb Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This has many uses, but in particular this will allow us to do #12843. I suggest we not bother rewriting large portions of the code to satisfy this and simply add another attribute to the Bridge class specifying its country. As soon as we have this information we can do more smart things with it (and those can be other smarter tickets).

For simplicity, we can only rely on the the ORPort netblock. This is what the BA checks. In the future, when ORPorts are disableable, we'll need to rely on the addresses in the transport lines, but that can be dealt with later, I think.

Child Tickets

Attachments (2)

0001-Added-countryCode-attribute-refactored-GeoIP-logic-i.patch (7.7 KB) - added by pagea 5 years ago.
Bridges now have a countryCode attribute. In addition, I moved all GeoIP logic into its own module in bridgedb.geo.
0002-Fixed-some-defects-isis-found-in-the-previous-patch.patch (4.3 KB) - added by pagea 5 years ago.
Made an amendment patch after isis reviewed my changes over IRC. Mostly doing more thorough error checking and fixing some small whitespace issues.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by isis

Cc: isis sysrqb added
Keywords: bridgedb-dist added
Priority: normalblocker
Status: newaccepted

Updating priority to "blocker" because this is preventing #12843.

FWIW, most of the functionality should already be in place to do this, but it needs some ♥ and definitely some tests. I'm also marking this ticket as "easy" because this is 99% done.

To fix this, you'd want to look at the bridgedb.HTTPServer.WebResourceBridges.getBridgeRequestAnswer() method, which, incidentally, has many XXXs that I put in it because I believe the entire thing is... lacking. Particularly, you want these lines where the country code is obtained:

        # XXX This can also be a separate function
        # XXX if the ip is None, this throws an exception
        if geoip:
            countryCode = geoip.country_code_by_addr(ip)
            if countryCode:
                logging.debug("Client request from GeoIP CC: %s" % countryCode)


And, of course, no complaints if some of those XXXs can be removed. :)

comment:2 Changed 5 years ago by isis

Keywords: easy added

comment:3 Changed 5 years ago by isis

Sorry, I misread this and thought "getting the client's geoip data".

I've already started doing this in the branch for #9380, and so this is blocking on that. (I'm not going to rewrite the bridgedb.Bridges.Bridge class twice.)

comment:4 Changed 5 years ago by isis

Parent ID: #12843

Changed 5 years ago by pagea

Bridges now have a countryCode attribute. In addition, I moved all GeoIP logic into its own module in bridgedb.geo.

comment:5 Changed 5 years ago by pagea

Review & constructive criticism on the patch is welcome; this patch is my first contribution to this project and I fully expect to have to make some changes to make it suitable for merging.

comment:6 in reply to:  5 ; Changed 5 years ago by isis

Replying to pagea:

Review & constructive criticism on the patch is welcome; this patch is my first contribution to this project and I fully expect to have to make some changes to make it suitable for merging.


Thanks a lot! For future reference, I reviewed pagea's patch, reporting potential issues as I found them on IRC. Here's the relevant things I pointed out on IRC:

  • In L62 and L60 in geo.py, you probably want to double-check that geoip and geoipv6 are not None, otherwise L65 will produce an AttributeError.
  • If ip in L65 in geo.py is an ipaddr.IPAddress, then I believe that you'll need to pass str(ip), rather than the actual class as in db.country_code_by_addr(ip), so use db.country_code_by_addr(str(ip)) instead… or actually ip.compressed or ip.exploded is probably safer. (I don't remember if ip.__str__() == ip.compressed.)
  • In geo.py, logSafely() is imported from bridgedb.safelogging, but not used.

Just for your future information, if you're logging an IP address, or an email address (or anything which looks sufficiently like one), and it is surrounded by whitespace characters, then BridgeDB's logger will automatically sanitise it if SAFELOGGING is enabled, without you needing to call logSafely().

  • In L672 in HTTPServer.py, you might want to check that ip is not None, or an empty str, or whatever other things which twisted.web.http.Request.getClientIP() might decide to return to you. You can also use bridgedb.parse.addr.isIPAddress(ip, compressed=False) as a way to ensure that the IP address is valid and that it's definitely an instance of ipaddr.IPAddress.
  • Tiny whitespace nitpicks: don't remove the second newline from L801 in HTTPServer.py, and put four more spaces before the word "reported" on L123 in HTTPServer.py.

But overall it looks excellent :D

comment:7 Changed 5 years ago by isis

Status: acceptedneeds_revision

Changed 5 years ago by pagea

Made an amendment patch after isis reviewed my changes over IRC. Mostly doing more thorough error checking and fixing some small whitespace issues.

comment:8 in reply to:  6 ; Changed 5 years ago by pagea

Replying to isis:

  • If ip in L65 in geo.py is an ipaddr.IPAddress, then I believe that you'll need to pass str(ip), rather than the actual class as in db.country_code_by_addr(ip), so use db.country_code_by_addr(str(ip)) instead… or actually ip.compressed or ip.exploded is probably safer. (I don't remember if ip.__str__() == ip.compressed.)

ip is a string extracted from IPAddress.exploded, so country_code_by_addr(ip) works as it should. Besides that, I believe that I implemented all of the changes that you suggested.

But overall it looks excellent :D

Thanks!

comment:9 Changed 5 years ago by isis

Type: defectenhancement

comment:10 Changed 5 years ago by pagea

Status: needs_revisionneeds_review

comment:11 Changed 5 years ago by isis

Keywords: bridgedb-0.2.5 added

comment:12 in reply to:  8 Changed 5 years ago by isis

Replying to pagea:

Besides that, I believe that I implemented all of the changes that you suggested.

This looks great! Thank you! I'll be merging these two patches for bridgedb-0.2.5.

comment:13 Changed 5 years ago by isis

Keywords: Isis2014Q3Q4 added

comment:14 Changed 5 years ago by isis

Keywords: isis2014Q3Q4 added; Isis2014Q3Q4 removed

comment:15 Changed 5 years ago by isis

Keywords: isisExB added

comment:16 Changed 5 years ago by isis

Keywords: bridgedb-0.3.0 added; bridgedb-0.2.5 removed
Resolution: fixed
Status: needs_reviewclosed

Both patches are merged! Thanks, pagea!

I had to change a few minor things because of the changes for #9380… Because the bridgedb.Bridges.Bridge class no longer exists now, these changes:

diff --git a/lib/bridgedb/Bridges.py b/lib/bridgedb/Bridges.py
index 2439130..4b80f12 100644
--- a/lib/bridgedb/Bridges.py
+++ b/lib/bridgedb/Bridges.py
@@ -22,6 +22,7 @@ import random                  
 import bridgedb.Storage              
 import bridgedb.Bucket             
+import bridgedb.geo                              
 from bridgedb.crypto import getHMACFunc
 from bridgedb.parse import addr
@@ -118,6 +119,8 @@ class Bridge(object):
         given in the bridge's descriptor, corresponding to desc_digest.
     :attr bool verified: Did we receive the descriptor for this bridge that
         was specified in the networkstatus?
+    :attr str countryCode: The two-letter country code of this bridge as
+    reported by GeoIP.
     """
     def __init__(self, nickname, ip, orport, fingerprint=None, id_digest=None,
                  or_addresses=None, transports=None):
@@ -135,6 +138,7 @@ class Bridge(object):
         self.desc_digest = None
         self.ei_digest = None
         self.verified = False
+        self.countryCode = None
      
         if id_digest is not None:
             assert fingerprint is None

@@ -149,6 +153,13 @@ class Bridge(object):
         else:  
             raise TypeError("Bridge with no ID")

+        # Determine the country code of this bridge.
+        self.countryCode = bridgedb.geo.getCountryCode(ip)         
+        if self.countryCode is None:
+            logging.debug("  Bridge: Country code not detected. IP: %s" % ip)
+        else:  
+            logging.debug("  Bridge: Country code found: %s" % self.countryCode)
+
     def setDescriptorDigest(self, digest):
         """Set the descriptor digest, specified in the NS."""
         self.desc_digest = digest

have been refactored into the bridgedb.brides.BridgeAddressBase class, so that the GeoIP country code can be obtained for both a Bridge's main ORAddress, as well as any of its Pluggable Transport addresses:

diff --git a/lib/bridgedb/bridges.py b/lib/bridgedb/bridges.py
index 10e1ffc..b791730 100644
--- a/lib/bridgedb/bridges.py
+++ b/lib/bridgedb/bridges.py
@@ -25,6 +25,7 @@ from Crypto.Util.number import long_to_bytes
 
 import bridgedb.Storage
 
+from bridgedb import geo
 from bridgedb import safelog
 from bridgedb import bridgerequest
 from bridgedb.crypto import removePKCS1Padding
@@ -141,11 +142,15 @@ class BridgeAddressBase(object):
     :type address: ``ipaddr.IPv4Address`` or ``ipaddr.IPv6Address``
     :ivar address: The IP address of :class:`Bridge` or one of its
         :class:`PluggableTransport`s.
+
+    :type country: str
+    :ivar country: The two-letter GeoIP country code of the :ivar:`address`.
     """
 
     def __init__(self):
         self._fingerprint = None
         self._address = None
+        self._country = None
 
     @property
     def fingerprint(self):
@@ -199,6 +204,18 @@ class BridgeAddressBase(object):
         """Reset this Bridge's address to ``None``."""
         self._address = None
 
+    @property
+    def country(self):
+        """Get the two-letter GeoIP country code for the :ivar:`address`.
+
+        :rtype: str or ``None``
+        :returns: If :ivar:`address` is set, this returns a two-letter country
+            code for the geolocated region that :ivar:`address` is within;
+            otherwise, returns ``None``.
+        """
+        if self.address:
+            return geo.getCountryCode(self.address)
+
 
 class PluggableTransport(BridgeAddressBase):
     """A single instance of a Pluggable Transport (PT) offered by a

Additionally, I made a couple tiny fixes and added unittests for all the relevant code.

Note: See TracTickets for help on using tickets.