Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix REJECT target name #215

Merged
merged 5 commits into from
Jul 14, 2021
Merged

fix REJECT target name #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
757b544
fix REJECT target name
e-hur Jun 29, 2021
fcb2a46
add fragment
Jun 29, 2021
310c68e
add tests for REJECT target
Jun 29, 2021
0492926
fix yaml syntax for reject choices
Jun 29, 2021
c596fea
Review comments
Akasurde Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/215_fix_REJECT_target_name.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- firewalld - fix setting zone target to ``%%REJECT%%`` (https://github.com/ansible-collections/ansible.posix/pull/215).
4 changes: 2 additions & 2 deletions plugins/modules/firewalld.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@
description:
- firewalld Zone target
- If state is set to C(absent), this will reset the target to default
choices: [ default, ACCEPT, DROP, REJECT ]
choices: [ default, ACCEPT, DROP, "%%REJECT%%" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @quasd, thanks for your contribution !

This replacement of REJECT by %%REJECT%% doesn't make sense to me, so I'd rather ask firewalld developpers why they have implemented things this way. All that can be discussed in this PR is their fault :)

Seriously, I'm not sure that staying close to firewalld semantic is the best way to follow for the module. ACCEPT, DROP and REJECT being the most common iptables targets, it makes sense to keep them as is.

If firewall-cmd accepts both REJECT and %%REJECT%%, does it make a difference in the results of firewalld-cmd ? If yes, then we have two reject targets, the naked one and the one with decoration. If no, then... "the simplest, the best" I would say. In other words, if a command called internally by the module needs %%REJECT%% rather than REJECT, it is the job of the module to add these percent signs around the user input, which should be REJECT I think.

If decorations around REJECT raise questions (and it seems they do, here), and the only answer to these questions is %%REJECT%% just means REJECT, so please drop the decorations from user interface.

Firewalld documentation says:

If the target is not specified, every packet not matching any rule will be rejected.

Also note that in iptables, DROP and ACCEPT can be used as rules targets as well as chain policies, while REJECT can't be used as a chain policy, but only as a rule target. That means that to get "every packet not matching any rule will be rejected" needs a final rule to do that, and is more difficult to manage than policies. That could explain why %%REJECT%% and not REJECT, and why %%REJECT%% and not %%DROP%%: the REJECT target behaves differently than the two others. Does it mean we have to write its name differently, as does firewalld ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi guys,
Sorry if I made misunderstood that but there seem to be two types of firewalld targets(Zone targets and Policy targets) like below:

ZONE_TARGETS = [ "ACCEPT", "%%REJECT%%", "DROP", DEFAULT_ZONE_TARGET, "default" ]
POLICY_TARGETS = [ "ACCEPT", "REJECT", "DROP", "CONTINUE" ]

As far as I know, the target option of the firewalld module is targeting Zone settings. And I have tried to launch the test playbook like below:

  tasks:
    - ansible.posix.firewalld:
        zone: external
        state: enabled
        permanent: yes
        target: REJECT
      become: yes

Currently, if users specify REJECT as the target value, the firewall module will be failed with the following traceback:

The full traceback is:
  File "/tmp/ansible_ansible.posix.firewalld_payload_h_561txs/ansible_ansible.posix.firewalld_payload.zip/ansible_collections/ansible/posix/plugins/module_utils/firewalld.py", line 112, in action_handler
    return action_func(*action_func_args)
  File "/tmp/ansible_ansible.posix.firewalld_payload_h_561txs/ansible_ansible.posix.firewalld_payload.zip/ansible_collections/ansible/posix/plugins/modules/firewalld.py", line 649, in set_enabled_permanent
  File "/tmp/ansible_ansible.posix.firewalld_payload_h_561txs/ansible_ansible.posix.firewalld_payload.zip/ansible_collections/ansible/posix/plugins/module_utils/firewalld.py", line 143, in update_fw_settings
    fw_zone.update(fw_settings)
  File "<decorator-gen-95>", line 2, in update
  File "/usr/lib/python3.6/site-packages/slip/dbus/polkit.py", line 121, in _enable_proxy
    return func(*p, **k)
  File "<decorator-gen-94>", line 2, in update
  File "/usr/lib/python3.6/site-packages/firewall/client.py", line 53, in handle_exceptions
    return func(*args, **kwargs)
  File "/usr/lib/python3.6/site-packages/firewall/client.py", line 525, in update
    self.fw_zone.update2(settings.getSettingsDbusDict())
  File "/usr/lib/python3.6/site-packages/slip/dbus/proxies.py", line 51, in __call__
    return dbus.proxies._ProxyMethod.__call__(self, *args, **kwargs)
  File "/usr/lib64/python3.6/site-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib64/python3.6/site-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
fatal: [server00]: FAILED! => {
    "changed": false,
    "invocation": {
        "module_args": {
            "icmp_block": null,
            "icmp_block_inversion": null,
            "immediate": false,
            "interface": null,
            "masquerade": null,
            "offline": null,
            "permanent": true,
            "port": null,
            "port_forward": null,
            "rich_rule": null,
            "service": null,
            "source": null,
            "state": "enabled",
            "target": "REJECT",
            "timeout": 0,
            "zone": "external"
        }
    },
    "msg": "ERROR: Exception caught: org.fedoraproject.FirewallD1.Exception: INVALID_TARGET: REJECT Permanent operation"
}

We may need to continue to discuss the underlying design. But personally, I think this PR is reasonable to address the above current issue.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @saito-hideki for investigating on this !
So, now it's clear that REJECT and %%REJECT%% don't have the same meaning.

type: str
version_added: 1.2.0
notes:
Expand Down Expand Up @@ -748,7 +748,7 @@ def main():
interface=dict(type='str'),
masquerade=dict(type='str'),
offline=dict(type='bool'),
target=dict(type='str', choices=['default', 'ACCEPT', 'DROP', 'REJECT']),
target=dict(type='str', choices=['default', 'ACCEPT', 'DROP', '%%REJECT%%']),
Akasurde marked this conversation as resolved.
Show resolved Hide resolved
Akasurde marked this conversation as resolved.
Show resolved Hide resolved
),
supports_check_mode=True,
required_by=dict(
Expand Down
52 changes: 52 additions & 0 deletions tests/integration/targets/firewalld/tasks/zone_target_test_cases.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,55 @@
assert:
that:
- result is not changed

- name: firewalld dmz zone target %%REJECT%%
firewalld:
zone: dmz
permanent: True
state: present
target: '%%REJECT%%'
register: result

- name: assert firewalld dmz zone target %%REJECT%% present worked
assert:
that:
- result is changed

- name: firewalld dmz zone target %%REJECT%% rerun (verify not changed)
firewalld:
zone: dmz
permanent: True
state: present
target: '%%REJECT%%'
register: result

- name: assert firewalld dmz zone target %%REJECT%% present worked (verify not changed)
assert:
that:
- result is not changed

- name: firewalld dmz zone target %%REJECT%% absent
firewalld:
zone: dmz
permanent: True
state: absent
target: '%%REJECT%%'
register: result

- name: assert firewalld dmz zone target %%REJECT%% absent worked
assert:
that:
- result is changed

- name: firewalld dmz zone target %%REJECT%% rerun (verify not changed)
firewalld:
zone: dmz
permanent: True
state: absent
target: '%%REJECT%%'
register: result

- name: assert firewalld dmz zone target %%REJECT%% present worked (verify not changed)
assert:
that:
- result is not changed