URL not properly URI encoded in cyberark_account

shorbachuk commented 1 year ago

Summary

cyberark_account url is not properly url encoded when state=absent and safe is None.

I want to delete an account but I do not have the same name. The CyberArk API does not require a safe name.

Steps to Reproduce

      - name: Remove account   
        cyberark.pas.cyberark_account:
          identified_by: "address,username"
          address: "{{ inventory_hostname_short }}"
          safe:
          username: "admin"
          cyberark_session: "{{ cyberark_session }}"
          state: absent

Expected Results

Account is removed

Actual Results

FAILED! => {"changed": false, "module_stderr": "Traceback (most recent call last):\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible_collections/cyberark/pas/plugins/modules/cyberark_account.py\\", line 1102, in get_account\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible/module_utils/urls.py\\", line 1384, in open_url\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible/module_utils/urls.py\\", line 1294, in open\\n  File \\"/usr/lib64/python3.8/urllib/request.py\\", line 222, in urlopen\\n    return opener.open(url, data, timeout)\\n  File \\"/usr/lib64/python3.8/urllib/request.py\\", line 525, in open\\n    response = self._open(req, data)\\n  File \\"/usr/lib64/python3.8/urllib/request.py\\", line 542, in _open\\n    result = self._call_chain(self.handle_open, protocol, protocol +\\n  File \\"/usr/lib64/python3.8/urllib/request.py\\", line 502, in _call_chain\\n    result = func(*args)\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible/module_utils/urls.py\\", line 443, in https_open\\n  File \\"/usr/lib64/python3.8/urllib/request.py\\", line 1354, in do_open\\n    h.request(req.get_method(), req.selector, req.data, headers,\\n  File \\"/usr/lib64/python3.8/http/client.py\\", line 1256, in request\\n    self._send_request(method, url, body, headers, encode_chunked)\\n  File \\"/usr/lib64/python3.8/http/client.py\\", line 1267, in _send_request\\n    self.putrequest(method, url, **skips)\\n  File \\"/usr/lib64/python3.8/http/client.py\\", line 1101, in putrequest\\n    self._validate_path(url)\\n  File \\"/usr/lib64/python3.8/http/client.py\\", line 1201, in _validate_path\\n    raise InvalidURL(f\\"URL can't contain control characters. {url!r} \\"\\nhttp.client.InvalidURL: URL can't contain control characters. '/PasswordVault/api/accounts?search=REDACTED.redacted.com admin' (found at least ' ')\\n\\nDuring handling of the above exception, another exception occurred:\\n\\nTraceback (most recent call last):\\n  File \\"/home/runner/.ansible/tmp/ansible-tmp-1691432082.671086-58-126268203065216/AnsiballZ_cyberark_account.py\\", line 102, in <module>\\n    _ansiballz_main()\\n  File \\"/home/runner/.ansible/tmp/ansible-tmp-1691432082.671086-58-126268203065216/AnsiballZ_cyberark_account.py\\", line 94, in _ansiballz_main\\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\\n  File \\"/home/runner/.ansible/tmp/ansible-tmp-1691432082.671086-58-126268203065216/AnsiballZ_cyberark_account.py\\", line 40, in invoke_module\\n    runpy.run_module(mod_name='ansible_collections.cyberark.pas.plugins.modules.cyberark_account', init_globals=None, run_name='__main__', alter_sys=True)\\n  File \\"/usr/lib64/python3.8/runpy.py\\", line 207, in run_module\\n    return _run_module_code(code, init_globals, run_name, mod_spec)\\n  File \\"/usr/lib64/python3.8/runpy.py\\", line 97, in _run_module_code\\n    _run_code(code, mod_globals, init_globals,\\n  File \\"/usr/lib64/python3.8/runpy.py\\", line 87, in _run_code\\n    exec(code, run_globals)\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible_collections/cyberark/pas/plugins/modules/cyberark_account.py\\", line 1309, in <module>\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible_collections/cyberark/pas/plugins/modules/cyberark_account.py\\", line 1267, in main\\n  File \\"/tmp/ansible_cyberark.pas.cyberark_account_payload_l3glafvu/ansible_cyberark.pas.cyberark_account_payload.zip/ansible_collections/cyberark/pas/plugins/modules/cyberark_account.py\\", line 1165, in get_account\\nAttributeError: 'InvalidURL' object has no attribute 'code'\\n", "module_stdout": "", "msg": "MODULE FAILURE\\nSee stdout/stderr for the exact error", "rc": 1}

Reproducible

[x] Always
[ ] Sometimes
[ ] Non-Reproducible

Version/Tag number

1.0.19

Environment setup

Ansible 2.13

Additional Information

It seems the search_string is only properly encoded when the safe_filter is Not None

I believe Line 1087 should be

    end_point = ("/PasswordVault/api/accounts?search=%s") % (quote(search_string.lstrip()))

compostCY commented 1 year ago

Please note this is working as designed in this release - safe is a required attribute needed to pass in: safe: description:

The safe in the Vault where the privileged account is to be located required: true type: str

as stated in the doc: https://github.com/cyberark/ansible-security-automation-collection/blob/master/docs/cyberark_account.md

We may consider changing the behavior in the future release.

thanks.

shorbachuk commented 1 year ago

required=true requires the safe to be present but it does not require non-null as per my example. Not asking to change the required=true behavior just asking to fix the malformed uri that results

compostCY commented 1 year ago

Thanks for the clarification. You are absolutely right. We will add this to handle the malformed uri.

compostCY commented 1 year ago

Hello Shorbachuk: Upon further review with our internal team, it seems to be an open issue with the Ansible platform itself when required is used. The discussion is here: https://github.com/ansible/ansible/issues/69190

if required, why allow someone to sent a none/empty value. This is definitely a debatable issue.

Will keep an open eye on this issue going forward.

shorbachuk commented 1 year ago

What is there to monitor? That issue is closed. Also confirms the behavior is as I said. I’m a cyberark customer and would like this fixed as it’s clearly a bug

why allow empty safe? Because the api doesn’t require a safe when filtering

cyberark / ansible-security-automation-collection