fix REJECT target name

[quasd]

SUMMARY

Fix setting default target to reject. The target name is %%REJECT%% not REJECT.
https://firewalld.org/documentation/zone/options.html

After this pull request to way to set REJECT would be

  - name: "Set target to REJECT for public"
    ansible.posix.firewalld:
      zone: public
      permanent: yes
      target: '%%REJECT%%'
      state: enabled
    become: true

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

firewalld

ADDITIONAL INFORMATION

This snippet would fail due to there not being target called REJECT and using %%REJECT%% is not in allowed values for target

  - name: "Set target to REJECT for public"
    ansible.posix.firewalld:
      zone: public
      permanent: yes
      target: 'REJECT'
      state: enabled
    become: true

Ansible error

{
    "msg": "ERROR: Exception caught: org.fedoraproject.FirewallD1.Exception: INVALID_TARGET: REJECT Permanent operation",
    "invocation": {
        "module_args": {
            "zone": "public",
            "permanent": true,
            "target": "REJECT",
            "state": "enabled",
            "immediate": false,
            "timeout": 0,
            "icmp_block": null,
            "icmp_block_inversion": null,
            "service": null,
            "port": null,
            "port_forward": null,
            "rich_rule": null,
            "source": null,
            "interface": null,
            "masquerade": null,
            "offline": null
        }
    },
    "_ansible_no_log": false,
    "changed": false
}

syslog

Jun 29 09:49:39 <hostname retracted> ansible-ansible.posix.firewalld[9015]: Invoked with zone=public permanent=True state=enabled target=REJECT immediate=False timeout=0 icmp_[1850/7279] icmp_block_inversion=None service=None port=None port_forward=None rich_rule=None source=None interface=None masquerade=None offline=None                                                                 
Jun 29 09:49:39 <hostname retracted>  firewalld[915]: ERROR: INVALID_TARGET: REJECT

[Akasurde]

cc @aminvakil @saito-hideki @quidame Code LGTM, could you please take a look? Thanks in advance.

[quidame]

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 ?

[saito-hideki]

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:

• https://github.com/firewalld/firewalld/blob/stable-1.0/src/firewall/core/base.py#L28-L31

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!

[quidame]

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

[saito-hideki]

LGTM. I think this PR is reasonable to address the following issue:

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"
}

[quidame]

I suggest you add some short clarification in the changelog fragment. All the rest looks good to me.