commit abab8898363de90a35085e62d02f66b9b1af9980 Author: Cecylia Bocovich cohosh@torproject.org Date: Thu Aug 12 17:29:05 2021 -0400

Parse X-Forwarded-For addresses from left to right

This parses X-Forwarded-For addresses from left to right since with the domain fronting at moat we may have more than one external proxy address. getClientIP is also updated to skip not only loopback addresses but any non-valid address (such as private addresses) that may have been in the header. --- bridgedb/distributors/common/http.py | 31 +++++++++++----------- bridgedb/distributors/moat/server.py | 36 +++++++++++++------------- bridgedb/metrics.py | 4 +-- bridgedb/parse/addr.py | 22 ---------------- bridgedb/test/test_distributors_common_http.py | 20 +++++++------- 5 files changed, 45 insertions(+), 68 deletions(-)

diff --git a/bridgedb/distributors/common/http.py b/bridgedb/distributors/common/http.py index 3bd7b41..755a4c8 100644 --- a/bridgedb/distributors/common/http.py +++ b/bridgedb/distributors/common/http.py @@ -21,7 +21,7 @@ import logging import os

from bridgedb.parse.addr import isIPAddress -from bridgedb.parse.addr import isLoopback +from bridgedb.parse.addr import isValidIP

#: The fully-qualified domain name for any and all web servers we run. @@ -55,7 +55,7 @@ def getFQDN(): """ return SERVER_PUBLIC_FQDN

-def getClientIP(request, useForwardedHeader=False, skipLoopback=False): +def getClientIP(request, useForwardedHeader=False, skipInvalid=False): """Get the client's IP address from the ``'X-Forwarded-For:'`` header, or from the :api:`request <twisted.web.server.Request>`.

@@ -63,8 +63,8 @@ def getClientIP(request, useForwardedHeader=False, skipLoopback=False): :param request: A ``Request`` for a :api:`twisted.web.resource.Resource`. :param bool useForwardedHeader: If ``True``, attempt to get the client's IP address from the ``'X-Forwarded-For:'`` header. - :param bool skipLoopback: If ``True``, and ``useForwardedHeader`` is - also ``True``, then skip any loopback addresses (127.0.0.1/8) when + :param bool skipInvalid: If ``True``, and ``useForwardedHeader`` is + also ``True``, then validate and skip any invalid addresses when parsing the X-Forwarded-For header. :rtype: ``None`` or :any:`str` :returns: The client's IP address, if it was obtainable. @@ -74,19 +74,18 @@ def getClientIP(request, useForwardedHeader=False, skipLoopback=False): if useForwardedHeader: header = request.getHeader("X-Forwarded-For") if header: - index = -1 - ip = header.split(",")[index].strip() - if skipLoopback: + values = header.split(",") + for ip in values: + ip = ip.strip() + if not skipInvalid: + break logging.info(("Parsing X-Forwarded-For again, ignoring " - "loopback addresses...")) - while isLoopback(ip): - index -= 1 - ip = header.split(",")[index].strip() - if not skipLoopback and isLoopback(ip): - logging.warn("Accepting loopback address: %s" % ip) - else: - if not isIPAddress(ip): - logging.warn("Got weird X-Forwarded-For value %r" % header) + "invalid addresses...")) + if isValidIP(ip): + break + if not isValidIP(ip): + logging.warn("Got weird X-Forwarded-For value %r" % header) + if skipInvalid: ip = None else: ip = request.getClientIP() diff --git a/bridgedb/distributors/moat/server.py b/bridgedb/distributors/moat/server.py index 7e83d94..fe03fc4 100644 --- a/bridgedb/distributors/moat/server.py +++ b/bridgedb/distributors/moat/server.py @@ -141,17 +141,17 @@ class JsonAPIResource(resource.Resource): """A resource which conforms to the `JSON API spec http://jsonapi.org/`__. """

- def __init__(self, useForwardedHeader=True, skipLoopback=False): + def __init__(self, useForwardedHeader=True, skipInvalid=False): """Create a JSON API resource, containing either error(s) or data.

:param bool useForwardedHeader: If ``True``, obtain the client's IP address from the ``X-Forwarded-For`` HTTP header. - :param bool skipLoopback: Skip loopback addresses when parsing the - X-Forwarded-For header. + :param bool skipInvalid: Skip invalid (e.g., loopback, private) addresses + when parsing the X-Forwarded-For header. """ resource.Resource.__init__(self) self.useForwardedHeader = useForwardedHeader - self.skipLoopback = skipLoopback + self.skipInvalid = skipInvalid

def getClientIP(self, request): """Get the client's IP address from the ``'X-Forwarded-For:'`` @@ -163,7 +163,7 @@ class JsonAPIResource(resource.Resource): :rtype: ``None`` or :any:`str` :returns: The client's IP address, if it was obtainable. """ - return getClientIP(request, self.useForwardedHeader, self.skipLoopback) + return getClientIP(request, self.useForwardedHeader, self.skipInvalid)

def formatDataForResponse(self, data, request): """Format a dictionary of ``data`` into JSON and add necessary response @@ -248,8 +248,8 @@ class CustomErrorHandlingResource(resource.Resource): class JsonAPIDataResource(JsonAPIResource): """A resource which returns some JSON API data."""

- def __init__(self, useForwardedHeader=True, skipLoopback=False): - JsonAPIResource.__init__(self, useForwardedHeader, skipLoopback) + def __init__(self, useForwardedHeader=True, skipInvalid=False): + JsonAPIResource.__init__(self, useForwardedHeader, skipInvalid)

def checkRequestHeaders(self, request): """The JSON API specification requires servers to respond with certain HTTP @@ -303,8 +303,8 @@ class CaptchaResource(JsonAPIDataResource): """A CAPTCHA."""

def __init__(self, hmacKey=None, publicKey=None, secretKey=None, - useForwardedHeader=True, skipLoopback=False): - JsonAPIDataResource.__init__(self, useForwardedHeader, skipLoopback) + useForwardedHeader=True, skipInvalid=False): + JsonAPIDataResource.__init__(self, useForwardedHeader, skipInvalid) self.hmacKey = hmacKey self.publicKey = publicKey self.secretKey = secretKey @@ -316,7 +316,7 @@ class CaptchaFetchResource(CaptchaResource): isLeaf = True

def __init__(self, hmacKey=None, publicKey=None, secretKey=None, - captchaDir="captchas", useForwardedHeader=True, skipLoopback=False): + captchaDir="captchas", useForwardedHeader=True, skipInvalid=False): """DOCDOC

:param bytes hmacKey: The master HMAC key, used for validating CAPTCHA @@ -334,8 +334,8 @@ class CaptchaFetchResource(CaptchaResource): :param str captchaDir: The directory where the cached CAPTCHA images :param bool useForwardedHeader: If ``True``, obtain the client's IP address from the ``X-Forwarded-For`` HTTP header. - :param bool skipLoopback: Skip loopback addresses when parsing the - X-Forwarded-For header. + :param bool skipInvalid: Skip invalid (e.g., loopback, private) addresses + when parsing the X-Forwarded-For header. """ CaptchaResource.__init__(self, hmacKey, publicKey, secretKey, useForwardedHeader) @@ -492,7 +492,7 @@ class CaptchaCheckResource(CaptchaResource):

def __init__(self, distributor, schedule, N=1, hmacKey=None, publicKey=None, secretKey=None, - useForwardedHeader=True, skipLoopback=False): + useForwardedHeader=True, skipInvalid=False): """Create a new resource for checking CAPTCHA solutions and returning bridges to a client.

@@ -505,8 +505,8 @@ class CaptchaCheckResource(CaptchaResource): :param int N: The number of bridges to hand out per query. :param bool useForwardedHeader: Whether or not we should use the the X-Forwarded-For header instead of the source IP address. - :param bool skipLoopback: Skip loopback addresses when parsing the - X-Forwarded-For header. + :param bool skipInvalid: Skip invalid (e.g., loopback, private) addresses + when parsing the X-Forwarded-For header. """ CaptchaResource.__init__(self, hmacKey, publicKey, secretKey, useForwardedHeader) @@ -809,7 +809,7 @@ def addMoatServer(config, distributor): captcha = None fwdHeaders = config.MOAT_USE_IP_FROM_FORWARDED_HEADER numBridges = config.MOAT_BRIDGES_PER_ANSWER - skipLoopback = config.MOAT_SKIP_LOOPBACK_ADDRESSES + skipInvalid = config.MOAT_SKIP_LOOPBACK_ADDRESSES

logging.info("Starting moat servers...")

@@ -836,10 +836,10 @@ def addMoatServer(config, distributor): moat = CustomErrorHandlingResource() fetch = CaptchaFetchResource(hmacKey, publicKey, secretKey, config.GIMP_CAPTCHA_DIR, - fwdHeaders, skipLoopback) + fwdHeaders, skipInvalid) check = CaptchaCheckResource(distributor, sched, numBridges, hmacKey, publicKey, secretKey, - fwdHeaders, skipLoopback) + fwdHeaders, skipInvalid)

moat.putChild(b"fetch", fetch) moat.putChild(b"check", check) diff --git a/bridgedb/metrics.py b/bridgedb/metrics.py index 14073ee..7751e94 100644 --- a/bridgedb/metrics.py +++ b/bridgedb/metrics.py @@ -470,7 +470,7 @@ class HTTPSMetrics(Metrics): # two-letter country code. ipAddr = getClientIP(request, useForwardedHeader=True, - skipLoopback=False) + skipInvalid=True) self.updateSubnetCounter(ipAddr) countryCode = resolveCountryCode(ipAddr)

@@ -578,7 +578,7 @@ class MoatMetrics(Metrics):

ipAddr = getClientIP(request, useForwardedHeader=True, - skipLoopback=False) + skipInvalid=True) countryCode = resolveCountryCode(ipAddr)

try: diff --git a/bridgedb/parse/addr.py b/bridgedb/parse/addr.py index 1d59c9a..4576b05 100644 --- a/bridgedb/parse/addr.py +++ b/bridgedb/parse/addr.py @@ -436,28 +436,6 @@ def isValidIP(ip): return False return True

-def isLoopback(ip): - """Determine if ``ip`` is a loopback or localhost address (127.0.0.1/8). - - :type: ``str`` or ``ipaddr.IPAddress`` - :param ip: An IP address. - :rtype: bool - :returns: ``True`` if the IP was a loopback or localhost address; ``False`` - otherwise. - """ - try: - if isinstance(ip, str): - ip = ipaddr.IPAddress(ip) - - if ip.is_loopback: - return True - else: - return False - except Exception as err: - logging.debug("Error attempting to parse IP address: %s", ip) - - return False - def normalizeEmail(emailaddr, domainmap, domainrules, ignorePlus=True): """Normalise an email address according to the processing rules for its canonical originating domain. diff --git a/bridgedb/test/test_distributors_common_http.py b/bridgedb/test/test_distributors_common_http.py index ff3da41..a3333ed 100644 --- a/bridgedb/test/test_distributors_common_http.py +++ b/bridgedb/test/test_distributors_common_http.py @@ -88,26 +88,26 @@ class GetClientIPTests(unittest.TestCase): """ request = self.createRequestWithIPs() request.headers.update({'x-forwarded-for': 'pineapple'}) - clientIP = server.getClientIP(request, useForwardedHeader=True) + clientIP = server.getClientIP(request, useForwardedHeader=True, skipInvalid=True) self.assertEqual(clientIP, None)

- def test_getClientIP_XForwardedFor_skip_loopback(self): + def test_getClientIP_XForwardedFor_skip_invalid(self): request = self.createRequestWithIPs() request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.1'}) - clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=True) + clientIP = server.getClientIP(request, useForwardedHeader=True, skipInvalid=True) self.assertEqual(clientIP, '3.3.3.3')

- def test_getClientIP_XForwardedFor_skip_loopback_multiple(self): + def test_getClientIP_XForwardedFor_skip_invalid_multiple(self): request = self.createRequestWithIPs() - request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.6, 127.0.0.1'}) - clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=True) + request.headers.update({'x-forwarded-for': '127.0.0.1, 192.168.3.1, 3.3.3.3, 127.0.0.6'}) + clientIP = server.getClientIP(request, useForwardedHeader=True, skipInvalid=True) self.assertEqual(clientIP, '3.3.3.3')

- def test_getClientIP_XForwardedFor_no_skip_loopback(self): + def test_getClientIP_XForwardedFor_no_skip_invalid(self): request = self.createRequestWithIPs() - request.headers.update({'x-forwarded-for': '3.3.3.3, 127.0.0.1'}) - clientIP = server.getClientIP(request, useForwardedHeader=True, skipLoopback=False) - self.assertEqual(clientIP, '127.0.0.1') + request.headers.update({'x-forwarded-for': '192.168.0.1, 3.3.3.3, 127.0.0.1'}) + clientIP = server.getClientIP(request, useForwardedHeader=True, skipInvalid=False) + self.assertEqual(clientIP, '192.168.0.1')

def test_getClientIP_fromRequest(self): """getClientIP() should return the IP address from the request instance