From a0b8b85fa5ab512f0ece4c660aba754fc85edc9e Mon Sep 17 00:00:00 2001 From: Hans Jerry Illikainen Date: Mon, 2 Dec 2019 07:01:44 +0000 Subject: [PATCH] ufw: add support for interface_in and interface_out (#65382) * ufw: escalate privileges in integration tests A few of the integration tests for the UFW module forgot to `become`. This is problematic if the test suite is executed as a non-privileged user. This commit amends that by adding `become` when appropriate. * ufw: add unit tests for direction and interface Extend the unit tests for the UFW module to test the `direction` and `interface` parameters. This will help in the implementation of a fix for issue #63903. * ufw: add support for interface_in and interface_out The UFW module has support for specifying `direction` and `interface` for UFW rules. Rules with these parameters are built such that per-interface filtering only apply to a single direction based on the value of `direction`. Not being able to specify multiple interfaces complicates things for `routed` rules where one might want to apply filtering only for a specific combination of `in` and `out` interfaces. This commit introduces two new parameters to the UFW module: `interface_in` and `interface_out`. These rules are mutually exclusive with the old `direction` and `interface` parameter because of the ambiguity of having e.g.: direction: XXX interface: foo interface_XXX: bar Fixes #63903 --- changelogs/fragments/63903-ufw.yaml | 2 + lib/ansible/modules/system/ufw.py | 35 ++++- test/integration/targets/ufw/tasks/main.yml | 3 + .../targets/ufw/tasks/tests/interface.yml | 81 +++++++++++ test/units/modules/system/test_ufw.py | 133 ++++++++++++++++++ 5 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 changelogs/fragments/63903-ufw.yaml create mode 100644 test/integration/targets/ufw/tasks/tests/interface.yml diff --git a/changelogs/fragments/63903-ufw.yaml b/changelogs/fragments/63903-ufw.yaml new file mode 100644 index 0000000000..975ecc129d --- /dev/null +++ b/changelogs/fragments/63903-ufw.yaml @@ -0,0 +1,2 @@ +minor_changes: + - ufw - accept ``interface_in`` and ``interface_out`` as parameters. diff --git a/lib/ansible/modules/system/ufw.py b/lib/ansible/modules/system/ufw.py index 4fe7e8ef50..6452f7c910 100644 --- a/lib/ansible/modules/system/ufw.py +++ b/lib/ansible/modules/system/ufw.py @@ -46,7 +46,8 @@ options: aliases: [ policy ] direction: description: - - Select direction for a rule or default policy command. + - Select direction for a rule or default policy command. Mutually + exclusive with I(interface_in) and I(interface_out). type: str choices: [ in, incoming, out, outgoing, routed ] logging: @@ -127,9 +128,29 @@ options: type: bool interface: description: - - Specify interface for rule. + - Specify interface for the rule. The direction (in or out) used + for the interface depends on the value of I(direction). See + I(interface_in) and I(interface_out) for routed rules that needs + to supply both an input and output interface. Mutually + exclusive with I(interface_in) and I(interface_out). type: str aliases: [ if ] + interface_in: + description: + - Specify input interface for the rule. This is mutually + exclusive with I(direction) and I(interface). However, it is + compatible with I(interface_out) for routed rules. + type: str + aliases: [ if_in ] + version_added: "2.10" + interface_out: + description: + - Specify output interface for the rule. This is mutually + exclusive with I(direction) and I(interface). However, it is + compatible with I(interface_in) for routed rules. + type: str + aliases: [ if_out ] + version_added: "2.10" route: description: - Apply the rule to routed/forwarded packets. @@ -315,6 +336,8 @@ def main(): insert_relative_to=dict(choices=['zero', 'first-ipv4', 'last-ipv4', 'first-ipv6', 'last-ipv6'], default='zero'), rule=dict(type='str', choices=['allow', 'deny', 'limit', 'reject']), interface=dict(type='str', aliases=['if']), + interface_in=dict(type='str', aliases=['if_in']), + interface_out=dict(type='str', aliases=['if_out']), log=dict(type='bool', default=False), from_ip=dict(type='str', default='any', aliases=['from', 'src']), from_port=dict(type='str'), @@ -327,6 +350,9 @@ def main(): supports_check_mode=True, mutually_exclusive=[ ['name', 'proto', 'logging'], + # Mutual exclusivity with `interface` implied by `required_by`. + ['direction', 'interface_in'], + ['direction', 'interface_out'], ], required_one_of=([command_keys]), required_by=dict( @@ -484,6 +510,9 @@ def main(): elif command == 'rule': if params['direction'] not in ['in', 'out', None]: module.fail_json(msg='For rules, direction must be one of "in" and "out", or direction must not be specified.') + if not params['route'] and params['interface_in'] and params['interface_out']: + module.fail_json(msg='Only route rules can combine ' + 'interface_in and interface_out') # Rules are constructed according to the long format # # ufw [--dry-run] [route] [delete] [insert NUM] allow|deny|reject|limit [in|out on INTERFACE] [log|log-all] \ @@ -520,6 +549,8 @@ def main(): cmd.append([value]) cmd.append([params['direction'], "%s" % params['direction']]) cmd.append([params['interface'], "on %s" % params['interface']]) + cmd.append([params['interface_in'], "in on %s" % params['interface_in']]) + cmd.append([params['interface_out'], "out on %s" % params['interface_out']]) cmd.append([module.boolean(params['log']), 'log']) for (key, template) in [('from_ip', "from %s"), ('from_port', "port %s"), diff --git a/test/integration/targets/ufw/tasks/main.yml b/test/integration/targets/ufw/tasks/main.yml index f7975de933..28198cd600 100644 --- a/test/integration/targets/ufw/tasks/main.yml +++ b/test/integration/targets/ufw/tasks/main.yml @@ -7,8 +7,10 @@ - name: Install iptables (SuSE only) package: name: iptables + become: yes when: ansible_os_family == 'Suse' - name: Install ufw + become: yes package: name: ufw @@ -17,6 +19,7 @@ - include_tasks: run-test.yml with_fileglob: - "tests/*.yml" + become: yes # Cleanup always: diff --git a/test/integration/targets/ufw/tasks/tests/interface.yml b/test/integration/targets/ufw/tasks/tests/interface.yml new file mode 100644 index 0000000000..776a72f879 --- /dev/null +++ b/test/integration/targets/ufw/tasks/tests/interface.yml @@ -0,0 +1,81 @@ +- name: Enable + ufw: + state: enabled + +- name: Route with interface in and out + ufw: + rule: allow + route: yes + interface_in: foo + interface_out: bar + proto: tcp + from_ip: 1.1.1.1 + to_ip: 8.8.8.8 + from_port: 1111 + to_port: 2222 + +- name: Route with interface in + ufw: + rule: allow + route: yes + interface_in: foo + proto: tcp + from_ip: 1.1.1.1 + from_port: 1111 + +- name: Route with interface out + ufw: + rule: allow + route: yes + interface_out: bar + proto: tcp + from_ip: 1.1.1.1 + from_port: 1111 + +- name: Non-route with interface in + ufw: + rule: allow + interface_in: foo + proto: tcp + from_ip: 1.1.1.1 + from_port: 3333 + +- name: Non-route with interface out + ufw: + rule: allow + interface_out: bar + proto: tcp + from_ip: 1.1.1.1 + from_port: 4444 + +- name: Check result + shell: ufw status |grep -E '(ALLOW|DENY|REJECT|LIMIT)' |sed -E 's/[ \t]+/ /g' + register: ufw_status + +- assert: + that: + - '"8.8.8.8 2222/tcp on bar ALLOW FWD 1.1.1.1 1111/tcp on foo " in stdout' + - '"Anywhere ALLOW FWD 1.1.1.1 1111/tcp on foo " in stdout' + - '"Anywhere on bar ALLOW FWD 1.1.1.1 1111/tcp " in stdout' + - '"Anywhere on foo ALLOW 1.1.1.1 3333/tcp " in stdout' + - '"Anywhere ALLOW OUT 1.1.1.1 4444/tcp on bar " in stdout' + vars: + stdout: '{{ ufw_status.stdout_lines }}' + +- name: Non-route with interface_in and interface_out + ufw: + rule: allow + interface_in: foo + interface_out: bar + proto: tcp + from_ip: 1.1.1.1 + from_port: 1111 + to_ip: 8.8.8.8 + to_port: 2222 + ignore_errors: yes + register: ufw_non_route_iface + +- assert: + that: + - ufw_non_route_iface is failed + - '"Only route rules" in ufw_non_route_iface.msg' diff --git a/test/units/modules/system/test_ufw.py b/test/units/modules/system/test_ufw.py index 1cfbac2782..b169e94e67 100644 --- a/test/units/modules/system/test_ufw.py +++ b/test/units/modules/system/test_ufw.py @@ -52,6 +52,11 @@ dry_mode_cmd_with_port_700 = { "ufw --dry-run allow from any to any port 7000 proto tcp": skippg_adding_existing_rules, "ufw --dry-run delete allow from any to any port 7000 proto tcp": "", "ufw --dry-run delete allow from any to any port 7001 proto tcp": user_rules_with_port_7000, + "ufw --dry-run route allow in on foo out on bar from 1.1.1.1 port 7000 to 8.8.8.8 port 7001 proto tcp": "", + "ufw --dry-run allow in on foo from any to any port 7003 proto tcp": "", + "ufw --dry-run allow in on foo from 1.1.1.1 port 7002 to 8.8.8.8 port 7003 proto tcp": "", + "ufw --dry-run allow out on foo from any to any port 7004 proto tcp": "", + "ufw --dry-run allow out on foo from 1.1.1.1 port 7003 to 8.8.8.8 port 7004 proto tcp": "", grep_config_cli: user_rules_with_port_7000 } @@ -169,6 +174,134 @@ class TestUFW(unittest.TestCase): result = self.__getResult(do_nothing_func_port_7000) self.assertFalse(result.exception.args[0]['changed']) + def test_check_mode_add_detailed_route(self): + set_module_args({ + 'rule': 'allow', + 'route': 'yes', + 'interface_in': 'foo', + 'interface_out': 'bar', + 'proto': 'tcp', + 'from_ip': '1.1.1.1', + 'to_ip': '8.8.8.8', + 'from_port': '7000', + 'to_port': '7001', + '_ansible_check_mode': True + }) + + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_ambiguous_route(self): + set_module_args({ + 'rule': 'allow', + 'route': 'yes', + 'interface_in': 'foo', + 'interface_out': 'bar', + 'direction': 'in', + 'interface': 'baz', + '_ansible_check_mode': True + }) + + with self.assertRaises(AnsibleFailJson) as result: + self.__getResult(do_nothing_func_port_7000) + + exc = result.exception.args[0] + self.assertTrue(exc['failed']) + self.assertIn('mutually exclusive', exc['msg']) + + def test_check_mode_add_interface_in(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'port': '7003', + 'interface_in': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_interface_out(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'port': '7004', + 'interface_out': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_non_route_interface_both(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'port': '7004', + 'interface_in': 'foo', + 'interface_out': 'bar', + '_ansible_check_mode': True + }) + + with self.assertRaises(AnsibleFailJson) as result: + self.__getResult(do_nothing_func_port_7000) + + exc = result.exception.args[0] + self.assertTrue(exc['failed']) + self.assertIn('combine', exc['msg']) + + def test_check_mode_add_direction_in(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'port': '7003', + 'direction': 'in', + 'interface': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_direction_in_with_ip(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'from_ip': '1.1.1.1', + 'from_port': '7002', + 'to_ip': '8.8.8.8', + 'to_port': '7003', + 'direction': 'in', + 'interface': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_direction_out(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'port': '7004', + 'direction': 'out', + 'interface': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + + def test_check_mode_add_direction_out_with_ip(self): + set_module_args({ + 'rule': 'allow', + 'proto': 'tcp', + 'from_ip': '1.1.1.1', + 'from_port': '7003', + 'to_ip': '8.8.8.8', + 'to_port': '7004', + 'direction': 'out', + 'interface': 'foo', + '_ansible_check_mode': True + }) + result = self.__getResult(do_nothing_func_port_7000) + self.assertTrue(result.exception.args[0]['changed']) + def test_check_mode_delete_existing_rules(self): set_module_args({