From 5089a4d53b353a71cc910bb2ba96cc450405b97c Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 10 Jul 2017 17:03:43 +0100 Subject: [PATCH 1/6] retrieve sms ip whitelist from credentials on paas --- app/authentication/auth.py | 2 +- app/cloudfoundry_config.py | 1 + app/config.py | 2 +- tests/app/authentication/test_authentication.py | 8 ++++---- tests/app/test_cloudfoundry_config.py | 10 +++++++++- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index b1b269b5c..3743b42e8 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -49,7 +49,7 @@ def restrict_ip_sms(): # X-Forwarded-For looks like "203.0.113.195, 70.41.3.18, 150.172.238.178" ip_list = request.headers.get("X-Forwarded-For") ip = ip_list.split(',')[0].strip() - current_app.logger.info("Inbound sms ip list {}".format(ip_list)) + current_app.logger.info("Inbound sms ip route list {}".format(ip_list)) if ip in current_app.config.get('ALLOW_IP_INBOUND_SMS'): current_app.logger.info("Inbound sms ip addresses {} passed ".format(ip)) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 9d918928b..1868924ab 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -43,6 +43,7 @@ def extract_notify_config(notify_config): os.environ['SECRET_KEY'] = notify_config['credentials']['secret_key'] os.environ['DANGEROUS_SALT'] = notify_config['credentials']['dangerous_salt'] os.environ['PERFORMANCE_PLATFORM_TOKEN'] = notify_config['credentials'].get('performance_platform_token', '') + os.environ['SMS_INBOUND_WHITELIST'] = notify_config['credentials']['allow_ip_inbound_sms'] def extract_notify_aws_config(aws_config): diff --git a/app/config.py b/app/config.py index 03c577bfc..4dc1639b2 100644 --- a/app/config.py +++ b/app/config.py @@ -262,7 +262,7 @@ class Config(object): FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - ALLOW_IP_INBOUND_SMS = [] + ALLOW_IP_INBOUND_SMS = os.environ.get('SMS_INBOUND_WHITELIST', []) ###################### diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 32bb47327..e03e7558f 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -313,8 +313,8 @@ def __create_token(service_id): def restrict_ip_sms_app(): app = flask.Flask(__name__) app.config['TESTING'] = True - app.config['ALLOW_IP_INBOUND_SMS'] = ['134.213.243.188'] - + app.config['ALLOW_IP_INBOUND_SMS'] = ['111.111.111.111', '100.100.100.100'] + # app.config['ALLOW_IP_INBOUND_SMS'] = os.environ['SMS_INBOUND_WHITELIST'] blueprint = flask.Blueprint('restrict_ip_sms_app', __name__) @blueprint.route('/') @@ -332,7 +332,7 @@ def test_allow_valid_ips(restrict_ip_sms_app): response = restrict_ip_sms_app.get( path='/', headers=[ - ('X-Forwarded-For', '134.213.243.188, 222.222.222.222, 127.0.0.1'), + ('X-Forwarded-For', '111.111.111.111, 222.222.222.222, 127.0.0.1'), ] ) @@ -345,7 +345,7 @@ def test_reject_invalid_ips(restrict_ip_sms_app): restrict_ip_sms_app.get( path='/', headers=[ - ('X-Forwarded-For', '222.222.222.222, 111.111.111.111, 127.0.0.1') + ('X-Forwarded-For', '222.222.222.222, 333.333.333.333, 127.0.0.1') ] ) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index acd57796e..204c7262e 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -16,7 +16,8 @@ def notify_config(): 'admin_client_secret': 'admin client secret', 'secret_key': 'secret key', 'dangerous_salt': 'dangerous salt', - 'performance_platform_token': 'performance platform token' + 'performance_platform_token': 'performance platform token', + 'allow_ip_inbound_sms': ['111.111.111.111', '100.100.100.100'] } } @@ -197,3 +198,10 @@ def test_redis_config(): assert os.environ['REDIS_ENABLED'] == '1' assert os.environ['REDIS_URL'] == 'redis url' + + +@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') +def test_sms_config(): + extract_cloudfoundry_config() + + assert os.environ['SMS_INBOUND_WHITELIST'] == ['111.111.111.111', '100.100.100.100'] From 50d01d18c56c703faa78e94e3ecba5a41d8e2d6d Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 10 Jul 2017 17:25:38 +0100 Subject: [PATCH 2/6] reading ip address 3rd from the back rather than the first one --- app/authentication/auth.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 3743b42e8..31f2764d9 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -47,9 +47,12 @@ def restrict_ip_sms(): ip = '' if request.headers.get("X-Forwarded-For"): # X-Forwarded-For looks like "203.0.113.195, 70.41.3.18, 150.172.238.178" - ip_list = request.headers.get("X-Forwarded-For") - ip = ip_list.split(',')[0].strip() - current_app.logger.info("Inbound sms ip route list {}".format(ip_list)) + ip_route = request.headers.get("X-Forwarded-For") + # ip = ip_list.split(',')[0].strip() + ip_list = ip_route.split(',') + if len(ip_list) >= 3: + ip = ip_list[len(ip_list) - 3] + current_app.logger.info("Inbound sms ip route list {}".format(ip_route)) if ip in current_app.config.get('ALLOW_IP_INBOUND_SMS'): current_app.logger.info("Inbound sms ip addresses {} passed ".format(ip)) From 226ae5784be3e08381c0e48d90143c9fe6717ddc Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 10 Jul 2017 17:33:13 +0100 Subject: [PATCH 3/6] reading ip address 3rd from the back rather than the first one --- app/authentication/auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 31f2764d9..40eccc3b8 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -48,7 +48,6 @@ def restrict_ip_sms(): if request.headers.get("X-Forwarded-For"): # X-Forwarded-For looks like "203.0.113.195, 70.41.3.18, 150.172.238.178" ip_route = request.headers.get("X-Forwarded-For") - # ip = ip_list.split(',')[0].strip() ip_list = ip_route.split(',') if len(ip_list) >= 3: ip = ip_list[len(ip_list) - 3] From 6b650c6d96936db0e873e42c94f3c53a2383ad8b Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 11 Jul 2017 09:33:56 +0100 Subject: [PATCH 4/6] changed name of test fixture --- tests/app/test_cloudfoundry_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index 204c7262e..1c1bf3271 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -201,7 +201,7 @@ def test_redis_config(): @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_sms_config(): +def test_sms_inbound_config(): extract_cloudfoundry_config() assert os.environ['SMS_INBOUND_WHITELIST'] == ['111.111.111.111', '100.100.100.100'] From 5d571891871386bab3098bcd2c17adf1038ea913 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 11 Jul 2017 09:50:09 +0100 Subject: [PATCH 5/6] changed name of test fixture --- app/authentication/auth.py | 2 +- app/config.py | 2 +- tests/app/authentication/test_authentication.py | 16 ++++++++++++++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 40eccc3b8..83d096296 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -53,7 +53,7 @@ def restrict_ip_sms(): ip = ip_list[len(ip_list) - 3] current_app.logger.info("Inbound sms ip route list {}".format(ip_route)) - if ip in current_app.config.get('ALLOW_IP_INBOUND_SMS'): + if ip in current_app.config.get('SMS_INBOUND_WHITELIST'): current_app.logger.info("Inbound sms ip addresses {} passed ".format(ip)) return else: diff --git a/app/config.py b/app/config.py index 4dc1639b2..94950155f 100644 --- a/app/config.py +++ b/app/config.py @@ -262,7 +262,7 @@ class Config(object): FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - ALLOW_IP_INBOUND_SMS = os.environ.get('SMS_INBOUND_WHITELIST', []) + SMS_INBOUND_WHITELIST = os.environ.get('SMS_INBOUND_WHITELIST', []) ###################### diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index e03e7558f..e423ab9a2 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -313,8 +313,7 @@ def __create_token(service_id): def restrict_ip_sms_app(): app = flask.Flask(__name__) app.config['TESTING'] = True - app.config['ALLOW_IP_INBOUND_SMS'] = ['111.111.111.111', '100.100.100.100'] - # app.config['ALLOW_IP_INBOUND_SMS'] = os.environ['SMS_INBOUND_WHITELIST'] + app.config['SMS_INBOUND_WHITELIST'] = ['111.111.111.111', '100.100.100.100'] blueprint = flask.Blueprint('restrict_ip_sms_app', __name__) @blueprint.route('/') @@ -350,3 +349,16 @@ def test_reject_invalid_ips(restrict_ip_sms_app): ) assert exc_info.value.short_message == 'Unknown source IP address from the SMS provider' + + +@pytest.mark.xfail(reason='Currently not blocking invalid IPs', strict=True) +def test_illegitimate_ips(restrict_ip_sms_app): + with pytest.raises(AuthError) as exc_info: + restrict_ip_sms_app.get( + path='/', + headers=[ + ('X-Forwarded-For', '111.111.111.111, 999.999.999.999, 333.333.333.333, 127.0.0.1') + ] + ) + + assert exc_info.value.short_message == 'Unknown source IP address from the SMS provider' From d3db4a6a01e0fb2f08ca0db10624f77562439a5f Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 11 Jul 2017 09:59:41 +0100 Subject: [PATCH 6/6] Added a unit test to validate traffic from more than 3 hops --- app/authentication/auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 83d096296..8242795b2 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -47,6 +47,7 @@ def restrict_ip_sms(): ip = '' if request.headers.get("X-Forwarded-For"): # X-Forwarded-For looks like "203.0.113.195, 70.41.3.18, 150.172.238.178" + # Counting backwards and look at the IP at the 3rd last hop - hence, hop(end-3) ip_route = request.headers.get("X-Forwarded-For") ip_list = ip_route.split(',') if len(ip_list) >= 3: