From 18d0a891db6e794d3ddabf2ff45f4acf80283094 Mon Sep 17 00:00:00 2001 From: David Ames Date: Tue, 1 May 2018 10:14:54 -0700 Subject: [PATCH] Allow GRE traffic in converged architecture In a converged architecture with storage and compute on the same host, UFW can get in the way of tunneled traffic interpreting it as INVALID. UFW makes solving this more difficult than it needs to be. See http://northernmost.org/blog/gre-tunnels-and-ufw/index.html for context. This change updates /etc/ufw/before.rules to add GRE as an allowed input. Also, guarantee ufw is installed for LP #1763716 Please review and merge charm-helpers first: https://github.com/juju/charm-helpers/pull/170 Change-Id: I789854c33e3af12f7412633dbf7c921beb0ed2b5 Closes-Bug: #1757564 Closes-Bug: #1763716 --- .gitignore | 2 + charmhelpers/contrib/network/ufw.py | 23 +++++++++ config.yaml | 7 +++ hooks/swift_storage_hooks.py | 59 +++++++++++++++++++++- lib/swift_storage_utils.py | 6 +++ unit_tests/test_swift_storage_relations.py | 32 ++++++++++++ 6 files changed, 128 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 3af73ec..e9ea010 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,5 @@ bin tags *.sw[nop] *.pyc +tests/cirros-* +func-results.json diff --git a/charmhelpers/contrib/network/ufw.py b/charmhelpers/contrib/network/ufw.py index 5cff71b..5db622f 100644 --- a/charmhelpers/contrib/network/ufw.py +++ b/charmhelpers/contrib/network/ufw.py @@ -151,6 +151,29 @@ def enable(soft_fail=False): return True +def reload(): + """ + Reload ufw + + :returns: True if ufw is successfully enabled + """ + output = subprocess.check_output(['ufw', 'reload'], + universal_newlines=True, + env={'LANG': 'en_US', + 'PATH': os.environ['PATH']}) + + m = re.findall('^Firewall reloaded\n', + output, re.M) + hookenv.log(output, level='DEBUG') + + if len(m) == 0: + hookenv.log("ufw couldn't be reloaded", level='WARN') + return False + else: + hookenv.log("ufw reloaded", level='INFO') + return True + + def disable(): """ Disable ufw diff --git a/config.yaml b/config.yaml index 90d504d..54843eb 100644 --- a/config.yaml +++ b/config.yaml @@ -171,6 +171,13 @@ options: Sample rate determines what percentage of the metric points a client should send to the server. Only takes effect if statsd-host is set. + enable-firewall: + type: boolean + default: True + description: | + By default the swift-storage charm will use the UFW firewall to + protect storage daemons. This option allows the administrator to + disable this feature. allow-ufw-ip6-softfail: description: | When this option is set to True the charm will disable the IPv6 diff --git a/hooks/swift_storage_hooks.py b/hooks/swift_storage_hooks.py index 2743c25..49dceea 100755 --- a/hooks/swift_storage_hooks.py +++ b/hooks/swift_storage_hooks.py @@ -15,7 +15,9 @@ # limitations under the License. import os +import shutil import sys +import tempfile from lib.swift_storage_utils import ( PACKAGES, @@ -87,6 +89,49 @@ CONFIGS = register_configs() NAGIOS_PLUGINS = '/usr/local/lib/nagios/plugins' SUDOERS_D = '/etc/sudoers.d' STORAGE_MOUNT_PATH = '/srv/node' +UFW_DIR = '/etc/ufw' + + +def add_ufw_gre_rule(ufw_rules_path): + """Add allow gre rule to UFW + + Make a copy of existing UFW before rules, insert our new rule and replace + existing rules with updated version. + """ + rule = '-A ufw-before-input -p 47 -j ACCEPT' + rule_exists = False + with tempfile.NamedTemporaryFile() as tmpfile: + # Close pre-opened file so that we can replace it with a copy of our + # config file. + tmpfile.file.close() + dst = tmpfile.name + # Copy over ufw rules file + shutil.copyfile(ufw_rules_path, dst) + with open(dst, 'r') as fd: + lines = fd.readlines() + + # Check whether the line we are adding already exists + for line in lines: + if rule in line: + rule_exists = True + break + + added = False + if not rule_exists: + # Insert our rule as the first rule. + with open(dst, 'w') as fd: + for line in lines: + if not added and line.startswith('-A'): + fd.write('# Allow GRE Traffic (added by swift-storage ' + 'charm)\n') + fd.write('{}\n'.format(rule)) + fd.write(line) + added = True + else: + fd.write(line) + + # Replace existing config with updated one. + shutil.copyfile(dst, ufw_rules_path) def initialize_ufw(): @@ -96,6 +141,11 @@ def initialize_ufw(): :return: None """ + + if not config('enable-firewall'): + log("Firewall has been administratively disabled", "DEBUG") + return + # this charm will monitor exclusively the ports used, using 'allow' as # default policy enables sharing the machine with other services ufw.default_policy('allow', 'incoming') @@ -108,6 +158,10 @@ def initialize_ufw(): # Enable ufw.enable(soft_fail=config('allow-ufw-ip6-softfail')) + # Allow GRE traffic + add_ufw_gre_rule(os.path.join(UFW_DIR, 'before.rules')) + ufw.reload() + @hooks.hook('install.real') @harden() @@ -128,7 +182,10 @@ def install(): @pause_aware_restart_on_change(RESTART_MAP) @harden() def config_changed(): - initialize_ufw() + if config('enable-firewall'): + initialize_ufw() + else: + ufw.disable() if config('prefer-ipv6'): status_set('maintenance', 'Configuring ipv6') assert_charm_supports_ipv6() diff --git a/lib/swift_storage_utils.py b/lib/swift_storage_utils.py index f9a34d8..b1b3394 100644 --- a/lib/swift_storage_utils.py +++ b/lib/swift_storage_utils.py @@ -87,6 +87,7 @@ from charmhelpers.core.decorators import ( PACKAGES = [ 'swift', 'swift-account', 'swift-container', 'swift-object', 'xfsprogs', 'gdisk', 'lvm2', 'python-jinja2', 'python-psutil', + 'ufw', ] VERSION_PACKAGE = 'swift-account' @@ -621,6 +622,11 @@ def setup_ufw(): :side effect: calls several external functions :return: None """ + + if not config('enable-firewall'): + log("Firewall has been administratively disabled", "DEBUG") + return + ports = [config('object-server-port'), config('container-server-port'), config('account-server-port')] diff --git a/unit_tests/test_swift_storage_relations.py b/unit_tests/test_swift_storage_relations.py index 91a92d7..ba7ee3b 100644 --- a/unit_tests/test_swift_storage_relations.py +++ b/unit_tests/test_swift_storage_relations.py @@ -16,6 +16,7 @@ from mock import patch import os import json import uuid +import tempfile from test_utils import CharmTestCase, patch_open @@ -53,6 +54,7 @@ TO_PATCH = [ 'fetch_swift_rings', 'save_script_rc', 'setup_rsync', + 'rsync', 'setup_storage', 'register_configs', 'update_nrpe_config', @@ -67,6 +69,22 @@ TO_PATCH = [ ] +UFW_DUMMY_RULES = """ +# Don't delete these required lines, otherwise there will be errors +*filter +:ufw-before-input - [0:0] +:ufw-before-output - [0:0] +:ufw-before-forward - [0:0] +:ufw-not-local - [0:0] +# End required lines + + +# allow all on loopback +-A ufw-before-input -i lo -j ACCEPT +-A ufw-before-output -o lo -j ACCEPT +""" + + class SwiftStorageRelationsTests(CharmTestCase): def setUp(self): @@ -76,10 +94,12 @@ class SwiftStorageRelationsTests(CharmTestCase): self.relation_get.side_effect = self.test_relation.get self.get_relation_ip.return_value = '10.10.10.2' + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_prunepath(self): hooks.config_changed() self.add_to_updatedb_prunepath.assert_called_with("/srv/node") + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_install_hook(self): self.test_config.set('openstack-origin', 'cloud:precise-havana') hooks.install() @@ -92,6 +112,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertTrue(self.setup_storage.called) self.assertTrue(self.execd_preinstall.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_config_changed_no_upgrade_available(self): self.openstack_upgrade_available.return_value = False self.relations_of_type.return_value = False @@ -102,6 +123,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertTrue(self.CONFIGS.write_all.called) self.assertTrue(self.setup_rsync.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_config_changed_upgrade_available(self): self.openstack_upgrade_available.return_value = True self.relations_of_type.return_value = False @@ -111,6 +133,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertTrue(self.do_openstack_upgrade.called) self.assertTrue(self.CONFIGS.write_all.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_config_changed_with_openstack_upgrade_action(self): self.openstack_upgrade_available.return_value = True self.test_config.set('action-managed-upgrade', True) @@ -121,6 +144,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertFalse(self.do_openstack_upgrade.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) def test_config_changed_nrpe_master(self): self.openstack_upgrade_available.return_value = False self.relations_of_type.return_value = True @@ -131,6 +155,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertTrue(self.setup_rsync.called) self.assertTrue(self.update_nrpe_config.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) @patch.object(hooks, 'assert_charm_supports_ipv6') def test_config_changed_ipv6(self, mock_assert_charm_supports_ipv6): self.test_config.set('prefer-ipv6', True) @@ -142,6 +167,7 @@ class SwiftStorageRelationsTests(CharmTestCase): self.assertTrue(self.CONFIGS.write_all.called) self.assertTrue(self.setup_rsync.called) + @patch.object(hooks, 'add_ufw_gre_rule', lambda *args: None) @patch.object(hooks, 'ensure_devs_tracked') def test_upgrade_charm(self, mock_ensure_devs_tracked): self.filter_installed_packages.return_value = [ @@ -323,3 +349,9 @@ class SwiftStorageRelationsTests(CharmTestCase): def test_main_hook_missing(self, _argv): hooks.main() self.assertTrue(self.log.called) + + def test_add_ufw_gre_rule(self): + with tempfile.NamedTemporaryFile() as tmpfile: + tmpfile.file.write(UFW_DUMMY_RULES) + tmpfile.file.close() + hooks.add_ufw_gre_rule(tmpfile.name)